From e045dfd4552a8827ac0f7a72c8098e482a29b0de Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Fri, 31 Mar 2023 14:38:06 +0100 Subject: [PATCH 1/4] MGDAPI-5152 - remove duplicate FILE_UPLOAD_STORAGE env var --- pkg/3scale/amp/component/system.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/3scale/amp/component/system.go b/pkg/3scale/amp/component/system.go index 5eb3e062a..4e2936c1b 100644 --- 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), From efbef27fc94af94da66b07371266d037d27a9677 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 11 Apr 2023 18:13:48 +0100 Subject: [PATCH 2/4] MGDAPI-5152 - upgrade: env var sync mutator for system and sidekiq --- pkg/3scale/amp/operator/system_reconciler.go | 2 + pkg/helper/envvarutils.go | 15 +++++ pkg/reconcilers/deploymentconfig.go | 15 ++++- pkg/reconcilers/deploymentconfig_test.go | 64 ++++++++++++++++++++ 4 files changed, 95 insertions(+), 1 deletion(-) diff --git a/pkg/3scale/amp/operator/system_reconciler.go b/pkg/3scale/amp/operator/system_reconciler.go index 4c16a7109..4b2b04704 100644 --- 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 index aa6065383..06f36cad1 100644 --- 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 index 80afbe82e..d052eb94c 100644 --- 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 not duplicated +func DeploymentConfigRemoveDuplicateEnvVarMutator(desired, 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 index be25aff88..c95d90814 100644 --- a/pkg/reconcilers/deploymentconfig_test.go +++ b/pkg/reconcilers/deploymentconfig_test.go @@ -535,3 +535,67 @@ func TestDeploymentConfigPodTemplateLabelsMutator(t *testing.T) { }) } } + +func TestDeploymentConfigEnvVarSyncMutator(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 + desiredEnvs []corev1.EnvVar + expectedResult bool + expectedNewEnvs []corev1.EnvVar + }{ + {"NothingToReconcile", envsA, envsA, false, envsA}, + {"EnvsReconciled", envsB, envsA, true, envsA}, + } + + for _, tc := range cases { + t.Run(tc.testName, func(subT *testing.T) { + existing := dcFactory(tc.existingEnvs) + desired := dcFactory(tc.desiredEnvs) + update, err := DeploymentConfigRemoveDuplicateEnvVarMutator(desired, 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)) + } + } + }) + } +} From 6366df4d3bd81da9d655954558156e5ace747949 Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 13 Apr 2023 16:23:09 +0100 Subject: [PATCH 3/4] MGDAPI-5152 - fix: typos from refactor --- pkg/reconcilers/deploymentconfig.go | 4 ++-- pkg/reconcilers/deploymentconfig_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconcilers/deploymentconfig.go b/pkg/reconcilers/deploymentconfig.go index d052eb94c..ea76db489 100644 --- a/pkg/reconcilers/deploymentconfig.go +++ b/pkg/reconcilers/deploymentconfig.go @@ -196,8 +196,8 @@ func DeploymentConfigPodTemplateLabelsMutator(desired, existing *appsv1.Deployme return updated, nil } -// DeploymentConfigRemoveDuplicateEnvVarMutator ensures pod env vars not duplicated -func DeploymentConfigRemoveDuplicateEnvVarMutator(desired, existing *appsv1.DeploymentConfig) (bool, error) { +// 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) diff --git a/pkg/reconcilers/deploymentconfig_test.go b/pkg/reconcilers/deploymentconfig_test.go index c95d90814..85dd8c6ff 100644 --- a/pkg/reconcilers/deploymentconfig_test.go +++ b/pkg/reconcilers/deploymentconfig_test.go @@ -536,7 +536,7 @@ func TestDeploymentConfigPodTemplateLabelsMutator(t *testing.T) { } } -func TestDeploymentConfigEnvVarSyncMutator(t *testing.T) { +func TestDeploymentConfigRemoveDuplicateEnvVarMutator(t *testing.T) { dcFactory := func(envs []corev1.EnvVar) *appsv1.DeploymentConfig { return &appsv1.DeploymentConfig{ TypeMeta: metav1.TypeMeta{ From 13b04c536706d718a801b7360637588d5a887683 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 14 Apr 2023 12:22:10 +0100 Subject: [PATCH 4/4] MGDAPI-5152 - refactor: remove redundant fields from TestDeploymentConfigRemoveDuplicateEnvVarMutator --- pkg/reconcilers/deploymentconfig_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/reconcilers/deploymentconfig_test.go b/pkg/reconcilers/deploymentconfig_test.go index 85dd8c6ff..6c5c6440d 100644 --- a/pkg/reconcilers/deploymentconfig_test.go +++ b/pkg/reconcilers/deploymentconfig_test.go @@ -572,19 +572,17 @@ func TestDeploymentConfigRemoveDuplicateEnvVarMutator(t *testing.T) { cases := []struct { testName string existingEnvs []corev1.EnvVar - desiredEnvs []corev1.EnvVar expectedResult bool expectedNewEnvs []corev1.EnvVar }{ - {"NothingToReconcile", envsA, envsA, false, envsA}, - {"EnvsReconciled", envsB, envsA, true, envsA}, + {"NothingToReconcile", envsA, false, envsA}, + {"EnvsReconciled", envsB, true, envsA}, } for _, tc := range cases { t.Run(tc.testName, func(subT *testing.T) { existing := dcFactory(tc.existingEnvs) - desired := dcFactory(tc.desiredEnvs) - update, err := DeploymentConfigRemoveDuplicateEnvVarMutator(desired, existing) + update, err := DeploymentConfigRemoveDuplicateEnvVarMutator(nil, existing) if err != nil { subT.Fatal(err) }