Revert "Revert "Change the way we figure out container to read the ar…

…tifacts from"" (#2316)

* Revert "Revert "Change the way we figure out container to read the artifacts from (#2310)" (#2314)"

This reverts commit dad529b.

* Make `ContainerNameFromPodOptsOrDefault` nil safe

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

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,

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)

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{

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)
+}