CLOUDP-69289: Fix custom StatefulSet spec merging of Env Vars (#140)

Author: chatton

agent/Dockerfile

@@ -21,11 +21,13 @@ RUN mkdir -p agent \
     && rm -r mongodb-mms-automation-agent-*
 
 RUN mkdir -p /var/lib/mongodb-mms-automation/probes/ \
-#    && curl --retry 3 https://readinessprobe.s3-us-west-1.amazonaws.com/readinessprobe -o /var/lib/mongodb-mms-automation/probes/readinessprobe \
     && curl --retry 3 https://readinessprobe-test.s3-us-west-1.amazonaws.com/readiness -o /var/lib/mongodb-mms-automation/probes/readinessprobe \
     && chmod +x /var/lib/mongodb-mms-automation/probes/readinessprobe \
     && mkdir -p /var/log/mongodb-mms-automation/ \
-    && chmod -R +wr /var/log/mongodb-mms-automation/
+    && chmod -R +wr /var/log/mongodb-mms-automation/ \
+    # ensure that the agent user can write the logs in OpenShift
+    && touch /var/log/mongodb-mms-automation/readiness.log \
+    && chmod ugo+rw /var/log/mongodb-mms-automation/readiness.log
 
 # Install MongoDB tools. The agent will automatically search the folder and find the binaries.
 RUN curl --fail --retry 3 --silent https://downloads.mongodb.org/tools/db/mongodb-database-tools-ubuntu1604-x86_64-${tools_version}.tgz -o mongodb-tools.tgz \

deploy/crds/mongodb.com_mongodb_crd.yaml

@@ -91,6 +91,10 @@ spec:
                   - enabled
                   type: object
               type: object
+            statefulSet:
+              description: StatefulSetConfiguration holds the optional custom StatefulSet
+                that should be merged into the operator created one.
+              type: object
             type:
               description: Type defines which type of MongoDB deployment the resource
                 should create

deploy/crds/mongodb.com_v1_mongodb_openshift_cr.yaml

@@ -0,0 +1,21 @@
+apiVersion: mongodb.com/v1
+kind: MongoDB
+metadata:
+  name: example-openshift-mongodb
+spec:
+  members: 3
+  type: ReplicaSet
+  version: "4.2.6"
+  statefulSet:
+    spec:
+      template:
+        spec:
+          containers:
+            - name: "mongodb-agent"
+              env:
+                - name: MANAGED_SECURITY_CONTEXT
+                  value: "true"
+            - name: "mongod"
+              env:
+                - name: MANAGED_SECURITY_CONTEXT
+                  value: "true"

deploy/operator.yaml

@@ -34,3 +34,4 @@ spec:
               value: quay.io/mongodb/mongodb-agent:10.15.1.6468-1
             - name: VERSION_UPGRADE_HOOK_IMAGE
               value: quay.io/mongodb/mongodb-kubernetes-operator-version-upgrade-post-start-hook:1.0.2
+

pkg/apis/mongodb/v1/mongodb_types.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	appsv1 "k8s.io/api/apps/v1"
+
 	"k8s.io/apimachinery/pkg/runtime"
 
 	"k8s.io/apimachinery/pkg/types"
@@ -47,10 +48,10 @@ type MongoDBSpec struct {
 
 	// Users specifies the MongoDB users that should be configured in your deployment
 	// +required
-	Users []MongoDBUser `json:"users"`
+	Users []MongoDBUser `json:"-"` // `json:"users"`
 
 	// +optional
-	StatefulSetConfiguration StatefulSetConfiguration `json:"statefulset,omitempty"`
+	StatefulSetConfiguration StatefulSetConfiguration `json:"statefulSet,omitempty"`
 
 	// AdditionalMongodConfig is additional configuration that can be passed to
 	// each data-bearing mongod at runtime. Uses the same structure as the mongod
@@ -62,7 +63,8 @@ type MongoDBSpec struct {
 // StatefulSetConfiguration holds the optional custom StatefulSet
 // that should be merged into the operator created one.
 type StatefulSetConfiguration struct {
-	Spec appsv1.StatefulSetSpec `json:"spec"`
+	// The StatefulSet override options for underlying StatefulSet
+	Spec appsv1.StatefulSetSpec `json:"spec"` // TODO: this pollutes the crd generation
 }
 
 // MongodConfiguration holds the optional mongod configuration
@@ -130,7 +132,7 @@ type Role struct {
 
 type Security struct {
 	// +optional
-	Authentication Authentication `json:"authentication"`
+	Authentication Authentication `json:"-"` //`json:"authentication"`
 	// TLS configuration for both client-server and server-server communication
 	// +optional
 	TLS TLS `json:"tls"`

pkg/controller/mongodb/replica_set_controller.go

@@ -484,7 +484,8 @@ func getMongodConfigModification(mdb mdbv1.MongoDB) automationconfig.Modificatio
 	return func(ac *automationconfig.AutomationConfig) {
 		for i := range ac.Processes {
 			// Mergo requires both objects to have the same type
-			mergo.Merge(&ac.Processes[i].Args26, objx.New(mdb.Spec.AdditionalMongodConfig.Object), mergo.WithOverride)
+			// TODO: proper error handling
+			_ = mergo.Merge(&ac.Processes[i].Args26, objx.New(mdb.Spec.AdditionalMongodConfig.Object), mergo.WithOverride)
 		}
 	}
 }

pkg/kube/container/container_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/mongodb/mongodb-kubernetes-operator/pkg/util/envvar"
+
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/resourcerequirements"
 
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/lifecycle"
@@ -132,7 +134,7 @@ func TestMergeEnvs(t *testing.T) {
 		},
 	}
 
-	merged := mergeEnvs(existing, desired)
+	merged := envvar.MergeWithOverride(existing, desired)
 
 	t.Run("EnvVars should be sorted", func(t *testing.T) {
 		assert.Equal(t, "A_env", merged[0].Name)

pkg/kube/container/containers.go

@@ -4,6 +4,8 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/mongodb/mongodb-kubernetes-operator/pkg/util/envvar"
+
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/lifecycle"
 	corev1 "k8s.io/api/core/v1"
 )
@@ -103,31 +105,10 @@ func WithLifecycle(lifeCycleMod lifecycle.Modification) Modification {
 // WithEnvs ensures all of the provided envs exist in the container
 func WithEnvs(envs ...corev1.EnvVar) Modification {
 	return func(container *corev1.Container) {
-		container.Env = mergeEnvs(container.Env, envs)
+		container.Env = envvar.MergeWithOverride(container.Env, envs)
 	}
 }
 
-func mergeEnvs(existing, desired []corev1.EnvVar) []corev1.EnvVar {
-	envMap := make(map[string]corev1.EnvVar)
-	for _, env := range existing {
-		envMap[env.Name] = env
-	}
-
-	for _, env := range desired {
-		envMap[env.Name] = env
-	}
-
-	var mergedEnv []corev1.EnvVar
-	for _, env := range envMap {
-		mergedEnv = append(mergedEnv, env)
-	}
-
-	sort.SliceStable(mergedEnv, func(i, j int) bool {
-		return mergedEnv[i].Name < mergedEnv[j].Name
-	})
-	return mergedEnv
-}
-
 // WithVolumeMounts sets the VolumeMounts
 func WithVolumeMounts(volumeMounts []corev1.VolumeMount) Modification {
 	volumesMountsCopy := make([]corev1.VolumeMount, len(volumeMounts))

pkg/kube/podtemplatespec/podspec_template.go

@@ -3,6 +3,7 @@ package podtemplatespec
 import (
 	"github.com/imdario/mergo"
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/container"
+	"github.com/mongodb/mongodb-kubernetes-operator/pkg/util/envvar"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -297,11 +298,14 @@ func mergeContainers(defaultContainers, customContainers []corev1.Container) ([]
 	for _, defaultContainer := range defaultContainers {
 		if customContainer, ok := customMap[defaultContainer.Name]; ok {
 			// The container is present in both maps, so we need to merge
-			// Merge mounts
+			// MergeWithOverride mounts
 			mergedMounts, err := mergeVolumeMounts(defaultContainer.VolumeMounts, customContainer.VolumeMounts)
 			if err != nil {
 				return nil, err
 			}
+
+			mergedEnvs := envvar.MergeWithOverride(defaultContainer.Env, customContainer.Env)
+
 			if err := mergo.Merge(&defaultContainer, customContainer, mergo.WithOverride); err != nil { //nolint
 				return nil, err
 			}
@@ -310,6 +314,7 @@ func mergeContainers(defaultContainers, customContainers []corev1.Container) ([]
 			// to the defaulted limits
 			defaultContainer.Resources = customContainer.Resources
 			defaultContainer.VolumeMounts = mergedMounts
+			defaultContainer.Env = mergedEnvs
 		}
 		// The default container was not modified by the override, so just add it
 		mergedContainers = append(mergedContainers, defaultContainer)

pkg/kube/podtemplatespec/podspec_template_test.go

@@ -182,6 +182,41 @@ func TestMultipleMerges(t *testing.T) {
 	}
 }
 
+func TestMergeEnvironmentVariables(t *testing.T) {
+	otherDefaultContainer := getDefaultContainer()
+	otherDefaultContainer.Env = append(otherDefaultContainer.Env, corev1.EnvVar{
+		Name:  "name1",
+		Value: "val1",
+	})
+
+	overrideOtherDefaultContainer := getDefaultContainer()
+	overrideOtherDefaultContainer.Env = append(overrideOtherDefaultContainer.Env, corev1.EnvVar{
+		Name:  "name2",
+		Value: "val2",
+	})
+	overrideOtherDefaultContainer.Env = append(overrideOtherDefaultContainer.Env, corev1.EnvVar{
+		Name:  "name1",
+		Value: "changedValue",
+	})
+
+	defaultSpec := getDefaultPodSpec()
+	defaultSpec.Spec.Containers = []corev1.Container{otherDefaultContainer}
+
+	customSpec := getCustomPodSpec()
+	customSpec.Spec.Containers = []corev1.Container{overrideOtherDefaultContainer}
+
+	mergedSpec, err := MergePodTemplateSpecs(defaultSpec, customSpec)
+	assert.NoError(t, err)
+
+	mergedContainer := mergedSpec.Spec.Containers[0]
+
+	assert.Len(t, mergedContainer.Env, 2)
+	assert.Equal(t, mergedContainer.Env[0].Name, "name1")
+	assert.Equal(t, mergedContainer.Env[0].Value, "changedValue")
+	assert.Equal(t, mergedContainer.Env[1].Name, "name2")
+	assert.Equal(t, mergedContainer.Env[1].Value, "val2")
+}
+
 func TestMergeContainer(t *testing.T) {
 	vol0 := corev1.VolumeMount{Name: "container-0.volume-mount-0"}
 	sideCarVol := corev1.VolumeMount{Name: "container-1.volume-mount-0"}