From 0d5b820b6ecf1f57fad905ac11b2e5ee5a5474e0 Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Wed, 20 Sep 2023 11:46:25 +0200 Subject: [PATCH] Revert "Revert "Change the way we figure out container to read the artifacts from"" (#2316) * Revert "Revert "Change the way we figure out container to read the artifacts from (#2310)" (#2314)" This reverts commit dad529b0b9a35d9cf8be9b6435ac2b2cfc2f6954. * Make `ContainerNameFromPodOptsOrDefault` nil safe --- pkg/controllers/repositoryserver/handler.go | 16 +++++++----- pkg/controllers/repositoryserver/utils.go | 7 +++--- pkg/kube/pod.go | 20 +++++++++------ pkg/kube/pod_controller.go | 22 +++-------------- pkg/kube/pod_test.go | 27 +++++++++++++++++++++ 5 files changed, 56 insertions(+), 36 deletions(-) diff --git a/pkg/controllers/repositoryserver/handler.go b/pkg/controllers/repositoryserver/handler.go index 60eacd5ed7..347831dc44 100644 --- a/pkg/controllers/repositoryserver/handler.go +++ b/pkg/controllers/repositoryserver/handler.go @@ -214,18 +214,19 @@ func (h *RepoServerHandler) updateServiceNameInPodLabels(pod *corev1.Pod, svc *c } func (h *RepoServerHandler) createPod(ctx context.Context, repoServerNamespace string, svc *corev1.Service) (*corev1.Pod, []corev1.EnvVar, error) { - podOverride, err := h.preparePodOverride(ctx) - + vols, err := getVolumes(ctx, h.KubeCli, h.RepositoryServerSecrets.storage, repoServerNamespace) if err != nil { return nil, nil, err } - vols, err := getVolumes(ctx, h.KubeCli, h.RepositoryServerSecrets.storage, repoServerNamespace) + podOptions := getPodOptions(repoServerNamespace, svc, vols) + + podOverride, err := h.preparePodOverride(ctx, podOptions) if err != nil { return nil, nil, err } + podOptions.PodOverride = podOverride - podOptions := getPodOptions(repoServerNamespace, podOverride, svc, vols) pod, envVars, err := h.setCredDataFromSecretInPod(ctx, podOptions) if err != nil { return nil, nil, err @@ -273,14 +274,17 @@ func (h *RepoServerHandler) setCredDataFromSecretInPod(ctx context.Context, podO return pod, envVars, nil } -func (h *RepoServerHandler) preparePodOverride(ctx context.Context) (map[string]interface{}, error) { +func (h *RepoServerHandler) preparePodOverride(ctx context.Context, po *kube.PodOptions) (map[string]interface{}, error) { namespace := h.RepositoryServer.GetNamespace() podOverride, err := getPodOverride(ctx, h.Reconciler, namespace) if err != nil { return nil, err } if err := addTLSCertConfigurationInPodOverride( - &podOverride, h.RepositoryServerSecrets.serverTLS.Name); err != nil { + &podOverride, + h.RepositoryServerSecrets.serverTLS.Name, + po, + ); err != nil { return nil, errors.Wrap(err, "Failed to attach TLS Certificate configuration") } return podOverride, nil diff --git a/pkg/controllers/repositoryserver/utils.go b/pkg/controllers/repositoryserver/utils.go index a993e46545..513142d11f 100644 --- a/pkg/controllers/repositoryserver/utils.go +++ b/pkg/controllers/repositoryserver/utils.go @@ -156,7 +156,7 @@ func volumeSpecForName(podSpec corev1.PodSpec, podOverride map[string]interface{ } } -func addTLSCertConfigurationInPodOverride(podOverride *map[string]interface{}, tlsCertSecretName string) error { +func addTLSCertConfigurationInPodOverride(podOverride *map[string]interface{}, tlsCertSecretName string, po *kube.PodOptions) error { podSpecBytes, err := json.Marshal(*podOverride) if err != nil { return errors.Wrap(err, "Failed to marshal Pod Override") @@ -178,7 +178,7 @@ func addTLSCertConfigurationInPodOverride(podOverride *map[string]interface{}, t if len(podOverrideSpec.Containers) == 0 { podOverrideSpec.Containers = append(podOverrideSpec.Containers, corev1.Container{ - Name: "container", + Name: kube.ContainerNameFromPodOptsOrDefault(po), }) } @@ -199,7 +199,7 @@ func addTLSCertConfigurationInPodOverride(podOverride *map[string]interface{}, t return nil } -func getPodOptions(namespace string, podOverride map[string]interface{}, svc *corev1.Service, vols map[string]string) *kube.PodOptions { +func getPodOptions(namespace string, svc *corev1.Service, vols map[string]string) *kube.PodOptions { uidguid := int64(0) nonRootBool := false return &kube.PodOptions{ @@ -208,7 +208,6 @@ func getPodOptions(namespace string, podOverride map[string]interface{}, svc *co Image: consts.GetKanisterToolsImage(), ContainerName: repoServerPodContainerName, Command: []string{"bash", "-c", "tail -f /dev/null"}, - PodOverride: podOverride, Labels: map[string]string{repoServerServiceNameKey: svc.Name}, PodSecurityContext: &corev1.PodSecurityContext{ RunAsUser: &uidguid, diff --git a/pkg/kube/pod.go b/pkg/kube/pod.go index 7dbcc12d96..641b42ea71 100644 --- a/pkg/kube/pod.go +++ b/pkg/kube/pod.go @@ -113,7 +113,7 @@ func GetPodObjectFromPodOptions(cli kubernetes.Interface, opts *PodOptions) (*v1 defaultSpecs := v1.PodSpec{ Containers: []v1.Container{ { - Name: defaultContainerName, + Name: ContainerNameFromPodOptsOrDefault(opts), Image: opts.Image, Command: opts.Command, ImagePullPolicy: v1.PullPolicy(v1.PullIfNotPresent), @@ -143,7 +143,7 @@ func GetPodObjectFromPodOptions(cli kubernetes.Interface, opts *PodOptions) (*v1 // Always put the main container the first sort.Slice(patchedSpecs.Containers, func(i, j int) bool { - return patchedSpecs.Containers[i].Name == defaultContainerName + return patchedSpecs.Containers[i].Name == ContainerNameFromPodOptsOrDefault(opts) }) pod := &v1.Pod{ @@ -161,11 +161,6 @@ func GetPodObjectFromPodOptions(cli kubernetes.Interface, opts *PodOptions) (*v1 pod.Name = opts.Name } - // Override default container name if applicable - if opts.ContainerName != "" { - pod.Spec.Containers[0].Name = opts.ContainerName - } - // Add Annotations and Labels, if specified if opts.Annotations != nil { pod.ObjectMeta.Annotations = opts.Annotations @@ -199,6 +194,17 @@ func GetPodObjectFromPodOptions(cli kubernetes.Interface, opts *PodOptions) (*v1 return pod, nil } +// ContainerNameFromPodOptsOrDefault returns the container name if it's set in +// the passed `podOptions` value. If not, it's returns the default container +// name. This should be used whenever we create pods for Kanister functions. +func ContainerNameFromPodOptsOrDefault(po *PodOptions) string { + if po == nil || po.ContainerName == "" { + return defaultContainerName + } + + return po.ContainerName +} + // CreatePod creates a pod with a single container based on the specified image func CreatePod(ctx context.Context, cli kubernetes.Interface, opts *PodOptions) (*v1.Pod, error) { pod, err := GetPodObjectFromPodOptions(cli, opts) diff --git a/pkg/kube/pod_controller.go b/pkg/kube/pod_controller.go index 202fa4f9bd..ac2caeee31 100644 --- a/pkg/kube/pod_controller.go +++ b/pkg/kube/pod_controller.go @@ -215,28 +215,12 @@ func (p *podController) StopPod(ctx context.Context, stopTimeout time.Duration, return nil } -// getContainerName returns container name, which should be passed to -// operations that require it. -// If the container name was specified in podOptions, it will be used. -// Otherwise, the first container name from specs will be taken as best effort -// (when pods are created with sidecars, sidecar containers are placed after -// main container). -func (p *podController) getContainerName() string { - if p.podOptions.ContainerName != "" { - return p.podOptions.ContainerName - } - - return p.pod.Spec.Containers[0].Name -} - -// StreamPodLogs returns io.ReadCloser which could be used to receive logs from pod -// Container will be decided based on the result of getContainerName function. func (p *podController) StreamPodLogs(ctx context.Context) (io.ReadCloser, error) { if p.podName == "" { return nil, ErrPodControllerPodNotStarted } - return StreamPodLogs(ctx, p.cli, p.pod.Namespace, p.pod.Name, p.getContainerName()) + return StreamPodLogs(ctx, p.cli, p.pod.Namespace, p.pod.Name, ContainerNameFromPodOptsOrDefault(p.podOptions)) } // GetCommandExecutor returns PodCommandExecutor instance which is configured to execute commands within pod controlled @@ -256,7 +240,7 @@ func (p *podController) GetCommandExecutor() (PodCommandExecutor, error) { cli: p.cli, namespace: p.pod.Namespace, podName: p.podName, - containerName: p.getContainerName(), + containerName: ContainerNameFromPodOptsOrDefault(p.podOptions), } pce.pcep = &podCommandExecutorProcessor{ @@ -282,7 +266,7 @@ func (p *podController) GetFileWriter() (PodFileWriter, error) { cli: p.cli, namespace: p.podOptions.Namespace, podName: p.podName, - containerName: p.getContainerName(), + containerName: ContainerNameFromPodOptsOrDefault(p.podOptions), } pfw.fileWriterProcessor = &podFileWriterProcessor{ diff --git a/pkg/kube/pod_test.go b/pkg/kube/pod_test.go index 1b9205ff23..db5edbacd8 100644 --- a/pkg/kube/pod_test.go +++ b/pkg/kube/pod_test.go @@ -881,3 +881,30 @@ func (s *PodSuite) TestSetLifecycleHook(c *C) { c.Assert(err, IsNil) c.Assert(pod.Spec.Containers[0].Lifecycle, DeepEquals, lch) } + +func (s *PodControllerTestSuite) TestContainerNameFromPodOptsOrDefault(c *C) { + for _, tc := range []struct { + podOptsContainerName string + expectedContainerName string + }{ + { + podOptsContainerName: "conone", + expectedContainerName: "conone", + }, + { + podOptsContainerName: "", + expectedContainerName: defaultContainerName, + }, + } { + name := ContainerNameFromPodOptsOrDefault(&PodOptions{ + ContainerName: tc.podOptsContainerName, + }) + c.Assert(name, Equals, tc.expectedContainerName) + } + + name := ContainerNameFromPodOptsOrDefault(&PodOptions{}) + c.Assert(name, Equals, defaultContainerName) + + name = ContainerNameFromPodOptsOrDefault(nil) + c.Assert(name, Equals, defaultContainerName) +}