fix: Merge env vars correctly (#2096)

Author: julienmancuso

deploy/cloud/operator/internal/controller/dynamocomponentdeployment_controller.go

@@ -1214,46 +1214,14 @@ func (r *DynamoComponentDeploymentReconciler) generatePodTemplateSpec(ctx contex
 
 	containerPort := commonconsts.DynamoServicePort
 
-	var envs []corev1.EnvVar
-	envsSeen := make(map[string]struct{})
-
 	resourceAnnotations := opt.dynamoComponentDeployment.Spec.Annotations
-	specEnvs := opt.dynamoComponentDeployment.Spec.Envs
 
 	if resourceAnnotations == nil {
 		resourceAnnotations = make(map[string]string)
 	}
 
 	isDebugModeEnabled := checkIfIsDebugModeEnabled(resourceAnnotations)
 
-	if specEnvs != nil {
-		envs = make([]corev1.EnvVar, 0, len(specEnvs)+1)
-
-		for _, env := range specEnvs {
-			if _, ok := envsSeen[env.Name]; ok {
-				continue
-			}
-			if env.Name == commonconsts.EnvDynamoServicePort {
-				// nolint: gosec
-				containerPort, err = strconv.Atoi(env.Value)
-				if err != nil {
-					return nil, errors.Wrapf(err, "invalid port value %s", env.Value)
-				}
-			}
-			envsSeen[env.Name] = struct{}{}
-			envVar := corev1.EnvVar{
-				Name: env.Name,
-			}
-			if env.Value != "" {
-				envVar.Value = env.Value
-			}
-			if env.ValueFrom != nil {
-				envVar.ValueFrom = env.ValueFrom
-			}
-			envs = append(envs, envVar)
-		}
-	}
-
 	defaultEnvs := []corev1.EnvVar{
 		{
 			Name:  commonconsts.EnvDynamoServicePort,
@@ -1275,11 +1243,7 @@ func (r *DynamoComponentDeploymentReconciler) generatePodTemplateSpec(ctx contex
 		})
 	}
 
-	for _, env := range defaultEnvs {
-		if _, ok := envsSeen[env.Name]; !ok {
-			envs = append(envs, env)
-		}
-	}
+	envs := dynamo.MergeEnvs(opt.dynamoComponentDeployment.Spec.Envs, defaultEnvs)
 
 	var livenessProbe *corev1.Probe
 	if opt.dynamoComponentDeployment.Spec.LivenessProbe != nil {
@@ -1470,6 +1434,8 @@ func (r *DynamoComponentDeploymentReconciler) generatePodTemplateSpec(ctx contex
 				err = errors.Wrapf(err, "failed to merge extraPodSpecMainContainer into container")
 				return nil, err
 			}
+			// finally merge the envs from extraPodSpecMainContainer into container
+			container.Env = dynamo.MergeEnvs(container.Env, extraPodSpecMainContainer.Env)
 		}
 	}
 

deploy/cloud/operator/internal/controller/dynamocomponentdeployment_controller_test.go

@@ -824,6 +824,12 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing.
 							DynamoComponent: "test-lws-component",
 							DynamoTag:       "test-tag",
 							DynamoComponentDeploymentSharedSpec: v1alpha1.DynamoComponentDeploymentSharedSpec{
+								Envs: []corev1.EnvVar{
+									{
+										Name:  "TEST_ENV_FROM_DYNAMO_COMPONENT_DEPLOYMENT_SPEC",
+										Value: "test_value_from_dynamo_component_deployment_spec",
+									},
+								},
 								ServiceName:     "test-lws-deploy-service",
 								DynamoNamespace: &[]string{"default"}[0],
 								Annotations: map[string]string{
@@ -845,6 +851,12 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing.
 										Args: []string{
 											"some dynamo command",
 										},
+										Env: []corev1.EnvVar{
+											{
+												Name:  "TEST_ENV_FROM_EXTRA_POD_SPEC",
+												Value: "test_value_from_extra_pod_spec",
+											},
+										},
 									},
 								},
 							},
@@ -896,7 +908,7 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing.
 										Image:   "test-image:latest",
 										Command: []string{"sh", "-c"},
 										Args:    []string{"ray start --head --port=6379 && some dynamo command"},
-										Env:     []corev1.EnvVar{{Name: "DYNAMO_PORT", Value: fmt.Sprintf("%d", commonconsts.DynamoServicePort)}},
+										Env:     []corev1.EnvVar{{Name: "DYNAMO_PORT", Value: fmt.Sprintf("%d", commonconsts.DynamoServicePort)}, {Name: "TEST_ENV_FROM_DYNAMO_COMPONENT_DEPLOYMENT_SPEC", Value: "test_value_from_dynamo_component_deployment_spec"}, {Name: "TEST_ENV_FROM_EXTRA_POD_SPEC", Value: "test_value_from_extra_pod_spec"}},
 										VolumeMounts: []corev1.VolumeMount{
 											{
 												Name: "shared-memory", MountPath: "/dev/shm",
@@ -948,7 +960,7 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing.
 										Image:        "test-image:latest",
 										Command:      []string{"sh", "-c"},
 										Args:         []string{"ray start --address=$(LWS_LEADER_ADDRESS):6379 --block"},
-										Env:          []corev1.EnvVar{{Name: "DYNAMO_PORT", Value: fmt.Sprintf("%d", commonconsts.DynamoServicePort)}},
+										Env:          []corev1.EnvVar{{Name: "DYNAMO_PORT", Value: fmt.Sprintf("%d", commonconsts.DynamoServicePort)}, {Name: "TEST_ENV_FROM_DYNAMO_COMPONENT_DEPLOYMENT_SPEC", Value: "test_value_from_dynamo_component_deployment_spec"}, {Name: "TEST_ENV_FROM_EXTRA_POD_SPEC", Value: "test_value_from_extra_pod_spec"}},
 										VolumeMounts: []corev1.VolumeMount{{Name: "shared-memory", MountPath: "/dev/shm"}},
 										Ports: []corev1.ContainerPort{{Protocol: corev1.ProtocolTCP, Name: commonconsts.DynamoServicePortName, ContainerPort: commonconsts.DynamoServicePort}, {
 											Protocol: corev1.ProtocolTCP, Name: commonconsts.DynamoHealthPortName, ContainerPort: commonconsts.DynamoHealthPort,

deploy/cloud/operator/internal/dynamo/graph.go

@@ -176,7 +176,7 @@ func GenerateDynamoComponentsDeployments(ctx context.Context, parentDynamoGraphD
 		}
 		// merge the envs from the parent deployment with the envs from the service
 		if len(parentDynamoGraphDeployment.Spec.Envs) > 0 {
-			deployment.Spec.Envs = mergeEnvs(parentDynamoGraphDeployment.Spec.Envs, deployment.Spec.Envs)
+			deployment.Spec.Envs = MergeEnvs(parentDynamoGraphDeployment.Spec.Envs, deployment.Spec.Envs)
 		}
 		err := updateDynDeploymentConfig(deployment, commonconsts.DynamoServicePort)
 		if err != nil {
@@ -279,7 +279,7 @@ func overrideWithDynDeploymentConfig(ctx context.Context, dynamoDeploymentCompon
 	return nil
 }
 
-func mergeEnvs(common, specific []corev1.EnvVar) []corev1.EnvVar {
+func MergeEnvs(common, specific []corev1.EnvVar) []corev1.EnvVar {
 	envMap := make(map[string]corev1.EnvVar)
 
 	// Add all common environment variables.
@@ -362,7 +362,7 @@ func GenerateGrovePodGangSet(ctx context.Context, dynamoDeployment *v1alpha1.Dyn
 		}
 		// merge the envs from the parent deployment with the envs from the service
 		if len(dynamoDeployment.Spec.Envs) > 0 {
-			container.Env = mergeEnvs(dynamoDeployment.Spec.Envs, container.Env)
+			container.Env = MergeEnvs(dynamoDeployment.Spec.Envs, container.Env)
 		}
 		container.Env = append(container.Env, corev1.EnvVar{
 			Name:  commonconsts.EnvDynamoServicePort,

deploy/cloud/operator/internal/dynamo/graph_test.go

@@ -1106,7 +1106,7 @@ func Test_mergeEnvs(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := mergeEnvs(tt.args.common, tt.args.specific)
+			got := MergeEnvs(tt.args.common, tt.args.specific)
 			sort.Slice(got, func(i, j int) bool {
 				return got[i].Name < got[j].Name
 			})