fix: fix auto inject sidecar vars and update test

Author: kaixi-wang

helm/fiftyone-teams-app/templates/delegated-operator-job-configmap.yaml

@@ -45,14 +45,13 @@ data:
   {{- $mergedExtraContainers := $jobConfig.extraContainers | default $baseTpl.extraContainers }}
   {{- $mergedNativeSidecarContainers := $jobConfig.nativeSidecarContainers | default $baseTpl.nativeSidecarContainers }}
 
-  {{- /* telemetry-sidecar needs a shared emptyDir; values can't reliably keep it through fleet regenerators, so reconcile it here. */ -}}
-  {{- $needsTelemetrySocket := false }}
+  {{- $hasExecutorSidecar := false }}
   {{- range $sidecar := $mergedNativeSidecarContainers }}
-    {{- range $vm := ($sidecar.volumeMounts | default list) }}
-      {{- if eq $vm.name "telemetry-socket" }}{{- $needsTelemetrySocket = true }}{{- end }}
+    {{- range $env := ($sidecar.env | default list) }}
+      {{- if and (eq $env.name "EXECUTOR_SIDECAR") (eq (toString $env.value) "true") }}{{- $hasExecutorSidecar = true }}{{- end }}
     {{- end }}
   {{- end }}
-  {{- if $needsTelemetrySocket }}
+  {{- if $hasExecutorSidecar }}
     {{- $hasVolume := false }}
     {{- range $v := ($mergedVolumes | default list) }}
       {{- if eq $v.name "telemetry-socket" }}{{- $hasVolume = true }}{{- end }}
@@ -154,18 +153,26 @@ data:
             {{- end }}
           {{- with $mergedNativeSidecarContainers }}
           initContainers:
-            {{- /* TARGET_CONTAINER tells the telemetry sidecar's log tailer which container to follow; the executor is hardcoded so we know the value. */ -}}
             {{- range $sidecar := . }}
+            {{- $isExecutor := false }}
             {{- $hasTargetContainer := false }}
             {{- range $env := ($sidecar.env | default list) }}
-              {{- if eq $env.name "TARGET_CONTAINER" }}
-                {{- $hasTargetContainer = true }}
-              {{- end }}
+              {{- if eq $env.name "TARGET_CONTAINER" }}{{- $hasTargetContainer = true }}{{- end }}
+              {{- if and (eq $env.name "EXECUTOR_SIDECAR") (eq (toString $env.value) "true") }}{{- $isExecutor = true }}{{- end }}
             {{- end }}
             {{- $rendered := deepCopy $sidecar }}
             {{- if not $hasTargetContainer }}
               {{- $_ := set $rendered "env" (prepend ($sidecar.env | default list) (dict "name" "TARGET_CONTAINER" "value" "executor")) }}
             {{- end }}
+            {{- if $isExecutor }}
+              {{- $hasSocketMount := false }}
+              {{- range $vm := ($sidecar.volumeMounts | default list) }}
+                {{- if eq $vm.name "telemetry-socket" }}{{- $hasSocketMount = true }}{{- end }}
+              {{- end }}
+              {{- if not $hasSocketMount }}
+                {{- $_ := set $rendered "volumeMounts" (append ($sidecar.volumeMounts | default list) (dict "name" "telemetry-socket" "mountPath" "/tmp/telemetry")) }}
+              {{- end }}
+            {{- end }}
             {{- list $rendered | toYaml | nindent 12 }}
             {{- end }}
           {{- end }}

tests/unit/helm/delegated-operator-job-configmap_test.go

@@ -657,3 +657,141 @@ func (s *doK8sConfigMapTemplateTest) loadTestFile(filename, chartVersion string)
 	result = strings.TrimSpace(result)
 	return strings.TrimSpace(result)
 }
+
+func (s *doK8sConfigMapTemplateTest) TestNativeSidecarTelemetryAutoInject() {
+	testCases := []struct {
+		name     string
+		values   map[string]string
+		expected func(subT gruntworkTesting.TestingT, job batchv1.Job)
+	}{
+		{
+			"AutoInjectsTelemetrySocketWiringWhenSidecarHasExecutorEnv",
+			map[string]string{
+				"delegatedOperatorJobTemplates.jobs.testJob.unused":                               "nil",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].name":          "telemetry-sidecar",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].image":         "telemetry-sidecar:latest",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].restartPolicy": "Always",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].env[0].name":   "EXECUTOR_SIDECAR",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].env[0].value":  "true",
+			},
+			func(subT gruntworkTesting.TestingT, job batchv1.Job) {
+				podSpec := job.Spec.Template.Spec
+
+				s.Require().NotNil(podSpec.ShareProcessNamespace, "shareProcessNamespace must be set when nativeSidecarContainers present")
+				s.True(*podSpec.ShareProcessNamespace, "shareProcessNamespace must be true")
+
+				s.Require().Len(podSpec.InitContainers, 1, "Should be one initContainer (the sidecar)")
+				sidecar := podSpec.InitContainers[0]
+				s.True(envContains(sidecar.Env, "TARGET_CONTAINER", "executor"), "Sidecar must have auto-injected TARGET_CONTAINER=executor")
+				s.True(volumeMountContains(sidecar.VolumeMounts, "telemetry-socket", "/tmp/telemetry"), "Sidecar must have auto-injected telemetry-socket volumeMount")
+
+				executor := findContainer(podSpec.Containers, "executor")
+				s.Require().NotNil(executor, "executor container must exist")
+				s.True(volumeMountContains(executor.VolumeMounts, "telemetry-socket", "/tmp/telemetry"), "Executor must have auto-injected telemetry-socket volumeMount")
+
+				s.True(volumeContains(podSpec.Volumes, "telemetry-socket"), "Pod must have auto-injected telemetry-socket emptyDir volume")
+			},
+		},
+		{
+			"PreservesExplicitTargetContainerOverride",
+			map[string]string{
+				"delegatedOperatorJobTemplates.jobs.testJob.unused":                               "nil",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].name":          "telemetry-sidecar",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].image":         "telemetry-sidecar:latest",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].restartPolicy": "Always",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].env[0].name":   "EXECUTOR_SIDECAR",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].env[0].value":  "true",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].env[1].name":   "TARGET_CONTAINER",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].env[1].value":  "custom-executor",
+			},
+			func(subT gruntworkTesting.TestingT, job batchv1.Job) {
+				sidecar := job.Spec.Template.Spec.InitContainers[0]
+				s.True(envContains(sidecar.Env, "TARGET_CONTAINER", "custom-executor"), "User-set TARGET_CONTAINER must win")
+				s.False(envContains(sidecar.Env, "TARGET_CONTAINER", "executor"), "Auto-inject must not also append TARGET_CONTAINER=executor")
+			},
+		},
+		{
+			"DoesNotInjectTelemetryWiringForNonExecutorSidecar",
+			map[string]string{
+				"delegatedOperatorJobTemplates.jobs.testJob.unused":                               "nil",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].name":          "istio-proxy",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].image":         "istio-proxy:latest",
+				"delegatedOperatorJobTemplates.template.nativeSidecarContainers[0].restartPolicy": "Always",
+			},
+			func(subT gruntworkTesting.TestingT, job batchv1.Job) {
+				podSpec := job.Spec.Template.Spec
+				s.False(volumeContains(podSpec.Volumes, "telemetry-socket"), "Non-executor sidecar must not auto-inject telemetry-socket volume")
+				sidecar := podSpec.InitContainers[0]
+				s.False(volumeMountContains(sidecar.VolumeMounts, "telemetry-socket", "/tmp/telemetry"), "Non-executor sidecar must not auto-inject volumeMount")
+				executor := findContainer(podSpec.Containers, "executor")
+				s.Require().NotNil(executor)
+				s.False(volumeMountContains(executor.VolumeMounts, "telemetry-socket", "/tmp/telemetry"), "Executor must not get telemetry-socket mount when no executor sidecar")
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		testCase := testCase
+		s.Run(testCase.name, func() {
+			subT := s.T()
+			subT.Parallel()
+
+			options := &helm.Options{SetValues: testCase.values}
+			output := helm.RenderTemplate(subT, options, s.chartPath, s.releaseName, s.templates)
+
+			var configMap corev1.ConfigMap
+			helm.UnmarshalK8SYaml(subT, output, &configMap)
+
+			s.Require().NotEmpty(configMap.Data["testJob.yaml"], "testJob.yaml should be rendered in configmap data")
+
+			jinjaArgs := map[string]interface{}{
+				"_id":      strings.ToLower(random.UniqueId()),
+				"_command": "fiftyone",
+				"_args":    []string{"delegated", "launch"},
+			}
+			actualJobYaml, err := convertJinjaToYAML(configMap.Data["testJob.yaml"], jinjaArgs)
+			s.NoError(err)
+
+			var job batchv1.Job
+			helm.UnmarshalK8SYaml(subT, actualJobYaml, &job)
+
+			testCase.expected(subT, job)
+		})
+	}
+}
+
+func envContains(envs []corev1.EnvVar, name, value string) bool {
+	for _, e := range envs {
+		if e.Name == name && e.Value == value {
+			return true
+		}
+	}
+	return false
+}
+
+func volumeMountContains(vms []corev1.VolumeMount, name, mountPath string) bool {
+	for _, vm := range vms {
+		if vm.Name == name && vm.MountPath == mountPath {
+			return true
+		}
+	}
+	return false
+}
+
+func volumeContains(vols []corev1.Volume, name string) bool {
+	for _, v := range vols {
+		if v.Name == name {
+			return true
+		}
+	}
+	return false
+}
+
+func findContainer(containers []corev1.Container, name string) *corev1.Container {
+	for i, c := range containers {
+		if c.Name == name {
+			return &containers[i]
+		}
+	}
+	return nil
+}