Merge pull request #816 from 3scale/backport-811-MGDAPI-5152-duplicat…

…e-env [Backport #811 MGDAPI-5152] remove duplicate env
3scale · Apr 17, 2023 · 08c8cdc · 08c8cdc
2 parents 8af6320 + 13b04c5
commit 08c8cdc
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 2 deletions.
diff --git a/pkg/3scale/amp/component/system.go b/pkg/3scale/amp/component/system.go
@@ -239,7 +239,6 @@ func (system *System) buildSystemBaseEnv() []v1.EnvVar {
 
 	if system.Options.S3FileStorageOptions != nil {
 		result = append(result,
-			helper.EnvVarFromConfigMap("FILE_UPLOAD_STORAGE", "system-environment", "FILE_UPLOAD_STORAGE"),
 			helper.EnvVarFromSecret(AwsAccessKeyID, system.Options.S3FileStorageOptions.ConfigurationSecretName, AwsAccessKeyID),
 			helper.EnvVarFromSecret(AwsSecretAccessKey, system.Options.S3FileStorageOptions.ConfigurationSecretName, AwsSecretAccessKey),
 			helper.EnvVarFromSecret(AwsBucket, system.Options.S3FileStorageOptions.ConfigurationSecretName, AwsBucket),

diff --git a/pkg/3scale/amp/operator/system_reconciler.go b/pkg/3scale/amp/operator/system_reconciler.go
@@ -89,6 +89,7 @@ func (r *SystemReconciler) Reconcile() (reconcile.Result, error) {
 		reconcilers.DeploymentConfigTolerationsMutator,
 		reconcilers.DeploymentConfigPodTemplateLabelsMutator,
 		r.systemAppDCResourceMutator,
+		reconcilers.DeploymentConfigRemoveDuplicateEnvVarMutator,
 	}
 
 	if r.apiManager.Spec.System.AppSpec.Replicas != nil {
@@ -107,6 +108,7 @@ func (r *SystemReconciler) Reconcile() (reconcile.Result, error) {
 		reconcilers.DeploymentConfigAffinityMutator,
 		reconcilers.DeploymentConfigTolerationsMutator,
 		reconcilers.DeploymentConfigPodTemplateLabelsMutator,
+		reconcilers.DeploymentConfigRemoveDuplicateEnvVarMutator,
 	}
 
 	if r.apiManager.Spec.System.SidekiqSpec.Replicas != nil {

diff --git a/pkg/helper/envvarutils.go b/pkg/helper/envvarutils.go
@@ -107,3 +107,18 @@ func EnsureEnvVar(desired v1.EnvVar, existingEnvVars *[]v1.EnvVar) bool {
 
 	return update
 }
+
+// RemoveDuplicateEnvVars removes duplicate env vars by name from a slice
+func RemoveDuplicateEnvVars(envVars []v1.EnvVar) []v1.EnvVar {
+	set := map[string]v1.EnvVar{}
+	var result []v1.EnvVar
+
+	for idx := range envVars {
+		if _, ok := set[envVars[idx].Name]; !ok {
+			set[envVars[idx].Name] = envVars[idx]
+			result = append(result, envVars[idx])
+		}
+	}
+
+	return result
+}
diff --git a/pkg/reconcilers/deploymentconfig.go b/pkg/reconcilers/deploymentconfig.go
@@ -7,7 +7,6 @@ import (
 
 	"github.com/3scale/3scale-operator/pkg/common"
 	"github.com/3scale/3scale-operator/pkg/helper"
-
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	appsv1 "github.com/openshift/api/apps/v1"
@@ -196,3 +195,17 @@ func DeploymentConfigPodTemplateLabelsMutator(desired, existing *appsv1.Deployme
 
 	return updated, nil
 }
+
+// DeploymentConfigRemoveDuplicateEnvVarMutator ensures pod env vars are not duplicated
+func DeploymentConfigRemoveDuplicateEnvVarMutator(_, existing *appsv1.DeploymentConfig) (bool, error) {
+	updated := false
+	for idx := range existing.Spec.Template.Spec.Containers {
+		prunedEnvs := helper.RemoveDuplicateEnvVars(existing.Spec.Template.Spec.Containers[idx].Env)
+		if !reflect.DeepEqual(existing.Spec.Template.Spec.Containers[idx].Env, prunedEnvs) {
+			existing.Spec.Template.Spec.Containers[idx].Env = prunedEnvs
+			updated = true
+		}
+	}
+
+	return updated, nil
+}
diff --git a/pkg/reconcilers/deploymentconfig_test.go b/pkg/reconcilers/deploymentconfig_test.go
@@ -535,3 +535,65 @@ func TestDeploymentConfigPodTemplateLabelsMutator(t *testing.T) {
 		})
 	}
 }
+
+func TestDeploymentConfigRemoveDuplicateEnvVarMutator(t *testing.T) {
+	dcFactory := func(envs []corev1.EnvVar) *appsv1.DeploymentConfig {
+		return &appsv1.DeploymentConfig{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "DeploymentConfig",
+				APIVersion: "apps.openshift.io/v1",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "myDC",
+				Namespace: "myNS",
+			},
+			Spec: appsv1.DeploymentConfigSpec{
+				Template: &corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name: "container1",
+								Env:  envs,
+							},
+							{
+								Name: "container2",
+								Env:  envs,
+							},
+						},
+					},
+				},
+			},
+		}
+	}
+
+	envsA := []corev1.EnvVar{{Name: "a", Value: "1"}, {Name: "b", Value: "2"}}
+	envsB := []corev1.EnvVar{{Name: "a", Value: "1"}, {Name: "a", Value: "1"}, {Name: "b", Value: "2"}}
+
+	cases := []struct {
+		testName        string
+		existingEnvs    []corev1.EnvVar
+		expectedResult  bool
+		expectedNewEnvs []corev1.EnvVar
+	}{
+		{"NothingToReconcile", envsA, false, envsA},
+		{"EnvsReconciled", envsB, true, envsA},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.testName, func(subT *testing.T) {
+			existing := dcFactory(tc.existingEnvs)
+			update, err := DeploymentConfigRemoveDuplicateEnvVarMutator(nil, existing)
+			if err != nil {
+				subT.Fatal(err)
+			}
+			if update != tc.expectedResult {
+				subT.Fatalf("result failed, expected: %t, got: %t", tc.expectedResult, update)
+			}
+			for idx := range existing.Spec.Template.Spec.Containers {
+				if !reflect.DeepEqual(existing.Spec.Template.Spec.Containers[idx].Env, tc.expectedNewEnvs) {
+					subT.Fatal(cmp.Diff(existing.Spec.Template.Spec.Containers[idx].Env, tc.expectedNewEnvs))
+				}
+			}
+		})
+	}
+}