Change the way we figure out container to read the artifacts from (#2310

)

* Change how we figure out container to get logs from

While running the kanister function, to read the artifact that the phase has
produced we get the logs of the kanister function pod (let's say `KubeTask`).
If there is just one container in the KubeTask pod in that case things would
work but if there are multiple contianers in the pod (let's say service mesh
injected one), in that case the logic that we have to figure out the container
name was not efficient. If was relying on the first container of the pod.

This commit changes that logic to read the container name from `podOptions` or
fallback to the default container name. This should work in all the cases, if
container name is not specified while creating `podOptions` we will use default
container name everywhere otherwise we will use the container name that is spec
ified in the podOptions.

* Remove duplicate code that was overreding container name

* Export the util fun `containerNameFromPodOpts`

* Figure out container name from podOptions while creating repo server pod

When we create pod for a repo server CR, we try to figure out the `podOverride`
which added (I am not sure why) a container with hardcoded name. This commit
changes that and we figure out the container name from podOptions now.

* Rename `ContainerNameFromPodOpts` to `ContainerNameFromPodOptsOrDefault`

* Move `ContainerNameFromPodOpts` from `pod_controller.go` to `pod.go`

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

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.ContainerName != "" {
+		return po.ContainerName
+	}
+
+	return defaultContainerName
+}
+
 // 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

@@ -211,26 +211,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
-}
-
 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))
 }
 
 func (p *podController) GetCommandExecutor() (PodCommandExecutor, error) {
@@ -246,7 +232,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 = pce
@@ -267,7 +253,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 = pfw

pkg/kube/pod_test.go

@@ -881,3 +881,27 @@ 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)
+}