CLOUDP-131647: Update to stateful set ignored existing volumes (#1064)

* CLOUDP-131647: Update to stateful set ignored existing volumes

* Release notes, review fixes

Author: lsierant

.gitignore

@@ -99,3 +99,4 @@ diagnostics
 
 Pipfile
 Pipfile.lock
+.community-operator-dev

controllers/mongodb_tls_test.go

@@ -8,7 +8,7 @@ import (
 
 	mdbv1 "github.com/mongodb/mongodb-kubernetes-operator/api/v1"
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/automationconfig"
-	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/client"
+	kubeClient "github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/client"
 	mdbClient "github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/client"
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/configmap"
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/secret"
@@ -21,7 +21,7 @@ import (
 
 func TestStatefulSet_IsCorrectlyConfiguredWithTLS(t *testing.T) {
 	mdb := newTestReplicaSetWithTLS()
-	mgr := client.NewManager(&mdb)
+	mgr := kubeClient.NewManager(&mdb)
 
 	client := mdbClient.NewClient(mgr.GetClient())
 	err := createTLSSecret(client, mdb, "CERT", "KEY", "")
@@ -37,14 +37,17 @@ func TestStatefulSet_IsCorrectlyConfiguredWithTLS(t *testing.T) {
 	err = mgr.GetClient().Get(context.TODO(), types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, &sts)
 	assert.NoError(t, err)
 
-	// Assert that all TLS volumes have been added.
+	assertStatefulsetVolumesAndVolumeMounts(t, sts, mdb.TLSOperatorCASecretNamespacedName().Name, mdb.TLSOperatorSecretNamespacedName().Name)
+}
+
+func assertStatefulsetVolumesAndVolumeMounts(t *testing.T, sts appsv1.StatefulSet, expectedTLSCASecretName string, expectedTLSOperatorSecretName string) {
 	assert.Len(t, sts.Spec.Template.Spec.Volumes, 7)
 	permission := int32(416)
 	assert.Contains(t, sts.Spec.Template.Spec.Volumes, corev1.Volume{
 		Name: "tls-ca",
 		VolumeSource: corev1.VolumeSource{
 			Secret: &corev1.SecretVolumeSource{
-				SecretName:  mdb.TLSOperatorCASecretNamespacedName().Name,
+				SecretName:  expectedTLSCASecretName,
 				DefaultMode: &permission,
 			},
 		},
@@ -53,7 +56,7 @@ func TestStatefulSet_IsCorrectlyConfiguredWithTLS(t *testing.T) {
 		Name: "tls-secret",
 		VolumeSource: corev1.VolumeSource{
 			Secret: &corev1.SecretVolumeSource{
-				SecretName:  mdb.TLSOperatorSecretNamespacedName().Name,
+				SecretName:  expectedTLSOperatorSecretName,
 				DefaultMode: &permission,
 			},
 		},
@@ -81,9 +84,57 @@ func TestStatefulSet_IsCorrectlyConfiguredWithTLS(t *testing.T) {
 	assert.Contains(t, mongodbContainer.VolumeMounts, tlsCAVolumeMount)
 }
 
+func TestStatefulSet_IsCorrectlyConfiguredWithTLSAfterChangingExistingVolumes(t *testing.T) {
+	mdb := newTestReplicaSetWithTLS()
+	mgr := kubeClient.NewManager(&mdb)
+
+	cli := mdbClient.NewClient(mgr.GetClient())
+	err := createTLSSecret(cli, mdb, "CERT", "KEY", "")
+	assert.NoError(t, err)
+
+	tlsCAVolumeSecretName := mdb.TLSOperatorCASecretNamespacedName().Name
+	changedTLSCAVolumeSecretName := tlsCAVolumeSecretName + "-old"
+
+	err = createTLSSecretWithNamespaceAndName(cli, mdb.Namespace, changedTLSCAVolumeSecretName, "CERT", "KEY", "")
+	assert.NoError(t, err)
+
+	err = createTLSConfigMap(cli, mdb)
+	assert.NoError(t, err)
+
+	r := NewReconciler(mgr)
+	res, err := r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
+	assertReconciliationSuccessful(t, res, err)
+
+	sts := appsv1.StatefulSet{}
+	err = mgr.GetClient().Get(context.TODO(), types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, &sts)
+	assert.NoError(t, err)
+
+	assertStatefulsetVolumesAndVolumeMounts(t, sts, tlsCAVolumeSecretName, mdb.TLSOperatorSecretNamespacedName().Name)
+
+	// updating sts tls-ca volume directly to simulate changing of underlying volume's secret
+	for i := range sts.Spec.Template.Spec.Volumes {
+		if sts.Spec.Template.Spec.Volumes[i].Name == "tls-ca" {
+			sts.Spec.Template.Spec.Volumes[i].VolumeSource.Secret.SecretName = changedTLSCAVolumeSecretName
+		}
+	}
+
+	err = mgr.GetClient().Update(context.TODO(), &sts)
+	assert.NoError(t, err)
+
+	assertStatefulsetVolumesAndVolumeMounts(t, sts, changedTLSCAVolumeSecretName, mdb.TLSOperatorSecretNamespacedName().Name)
+
+	res, err = r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: mdb.Namespace, Name: mdb.Name}})
+	assertReconciliationSuccessful(t, res, err)
+
+	sts = appsv1.StatefulSet{}
+	err = mgr.GetClient().Get(context.TODO(), types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, &sts)
+	assert.NoError(t, err)
+	assertStatefulsetVolumesAndVolumeMounts(t, sts, tlsCAVolumeSecretName, mdb.TLSOperatorSecretNamespacedName().Name)
+}
+
 func TestAutomationConfig_IsCorrectlyConfiguredWithTLS(t *testing.T) {
 	createAC := func(mdb mdbv1.MongoDBCommunity) automationconfig.AutomationConfig {
-		client := mdbClient.NewClient(client.NewManager(&mdb).GetClient())
+		client := mdbClient.NewClient(kubeClient.NewManager(&mdb).GetClient())
 		err := createTLSSecret(client, mdb, "CERT", "KEY", "")
 		assert.NoError(t, err)
 		err = createTLSConfigMap(client, mdb)
@@ -154,13 +205,13 @@ func TestAutomationConfig_IsCorrectlyConfiguredWithTLS(t *testing.T) {
 func TestTLSOperatorSecret(t *testing.T) {
 	t.Run("Secret is created if it doesn't exist", func(t *testing.T) {
 		mdb := newTestReplicaSetWithTLS()
-		c := mdbClient.NewClient(client.NewManager(&mdb).GetClient())
+		c := mdbClient.NewClient(kubeClient.NewManager(&mdb).GetClient())
 		err := createTLSSecret(c, mdb, "CERT", "KEY", "")
 		assert.NoError(t, err)
 		err = createTLSConfigMap(c, mdb)
 		assert.NoError(t, err)
 
-		r := NewReconciler(client.NewManagerWithClient(c))
+		r := NewReconciler(kubeClient.NewManagerWithClient(c))
 
 		err = r.ensureTLSResources(mdb)
 		assert.NoError(t, err)
@@ -175,7 +226,7 @@ func TestTLSOperatorSecret(t *testing.T) {
 
 	t.Run("Secret is updated if it already exists", func(t *testing.T) {
 		mdb := newTestReplicaSetWithTLS()
-		k8sclient := mdbClient.NewClient(client.NewManager(&mdb).GetClient())
+		k8sclient := mdbClient.NewClient(kubeClient.NewManager(&mdb).GetClient())
 		err := createTLSSecret(k8sclient, mdb, "CERT", "KEY", "")
 		assert.NoError(t, err)
 		err = createTLSConfigMap(k8sclient, mdb)
@@ -190,7 +241,7 @@ func TestTLSOperatorSecret(t *testing.T) {
 		err = k8sclient.CreateSecret(s)
 		assert.NoError(t, err)
 
-		r := NewReconciler(client.NewManagerWithClient(k8sclient))
+		r := NewReconciler(kubeClient.NewManagerWithClient(k8sclient))
 
 		err = r.ensureTLSResources(mdb)
 		assert.NoError(t, err)
@@ -226,13 +277,13 @@ func TestCombineCertificateAndKey(t *testing.T) {
 func TestPemSupport(t *testing.T) {
 	t.Run("Success if only pem is provided", func(t *testing.T) {
 		mdb := newTestReplicaSetWithTLS()
-		c := mdbClient.NewClient(client.NewManager(&mdb).GetClient())
+		c := mdbClient.NewClient(kubeClient.NewManager(&mdb).GetClient())
 		err := createTLSSecret(c, mdb, "", "", "CERT\nKEY")
 		assert.NoError(t, err)
 		err = createTLSConfigMap(c, mdb)
 		assert.NoError(t, err)
 
-		r := NewReconciler(client.NewManagerWithClient(c))
+		r := NewReconciler(kubeClient.NewManagerWithClient(c))
 
 		err = r.ensureTLSResources(mdb)
 		assert.NoError(t, err)
@@ -246,13 +297,13 @@ func TestPemSupport(t *testing.T) {
 	})
 	t.Run("Success if pem is equal to cert+key", func(t *testing.T) {
 		mdb := newTestReplicaSetWithTLS()
-		c := mdbClient.NewClient(client.NewManager(&mdb).GetClient())
+		c := mdbClient.NewClient(kubeClient.NewManager(&mdb).GetClient())
 		err := createTLSSecret(c, mdb, "CERT", "KEY", "CERT\nKEY")
 		assert.NoError(t, err)
 		err = createTLSConfigMap(c, mdb)
 		assert.NoError(t, err)
 
-		r := NewReconciler(client.NewManagerWithClient(c))
+		r := NewReconciler(kubeClient.NewManagerWithClient(c))
 
 		err = r.ensureTLSResources(mdb)
 		assert.NoError(t, err)
@@ -266,13 +317,13 @@ func TestPemSupport(t *testing.T) {
 	})
 	t.Run("Failure if pem is different from cert+key", func(t *testing.T) {
 		mdb := newTestReplicaSetWithTLS()
-		c := mdbClient.NewClient(client.NewManager(&mdb).GetClient())
+		c := mdbClient.NewClient(kubeClient.NewManager(&mdb).GetClient())
 		err := createTLSSecret(c, mdb, "CERT1", "KEY1", "CERT\nKEY")
 		assert.NoError(t, err)
 		err = createTLSConfigMap(c, mdb)
 		assert.NoError(t, err)
 
-		r := NewReconciler(client.NewManagerWithClient(c))
+		r := NewReconciler(kubeClient.NewManagerWithClient(c))
 
 		err = r.ensureTLSResources(mdb)
 		assert.Error(t, err)
@@ -295,10 +346,10 @@ func createTLSConfigMap(c k8sClient.Client, mdb mdbv1.MongoDBCommunity) error {
 	return c.Create(context.TODO(), &configMap)
 }
 
-func createTLSSecret(c k8sClient.Client, mdb mdbv1.MongoDBCommunity, crt string, key string, pem string) error {
+func createTLSSecretWithNamespaceAndName(c k8sClient.Client, namespace string, name string, crt string, key string, pem string) error {
 	sBuilder := secret.Builder().
-		SetName(mdb.Spec.Security.TLS.CertificateKeySecret.Name).
-		SetNamespace(mdb.Namespace)
+		SetName(name).
+		SetNamespace(namespace)
 
 	if crt != "" {
 		sBuilder.SetField(tlsSecretCertName, crt)
@@ -314,6 +365,10 @@ func createTLSSecret(c k8sClient.Client, mdb mdbv1.MongoDBCommunity, crt string,
 	return c.Create(context.TODO(), &s)
 }
 
+func createTLSSecret(c k8sClient.Client, mdb mdbv1.MongoDBCommunity, crt string, key string, pem string) error {
+	return createTLSSecretWithNamespaceAndName(c, mdb.Namespace, mdb.Spec.Security.TLS.CertificateKeySecret.Name, crt, key, pem)
+}
+
 func createUserPasswordSecret(c k8sClient.Client, mdb mdbv1.MongoDBCommunity, userPasswordSecretName string, password string) error {
 	sBuilder := secret.Builder().
 		SetName(userPasswordSecretName).

docs/RELEASE_NOTES.md

@@ -1,3 +1,18 @@
+# MongoDB Kubernetes Operator 0.7.5
+
+## Upgrade breaking change notice
+Versions 0.7.3, 0.7.4 have an issue that breaks deployment of MongoDB replica set when:
+* TLS is enabled
+* Replica set was deployed using the operator with version <=0.7.2
+
+If above conditions are met, it is strongly advised to upgrade the MongoDB Kubernetes Operator to version 0.7.5 or higher.
+
+## Kubernetes Operator
+
+- Bug fixes
+  - Fixed ignoring changes to existing volumes in the StatefulSet, i.e. changes of the volumes' underlying secret. This could cause that TLS enabled MongoDB deployment was not able to locate TLS certificates when upgrading the operator to versions 0.7.3 or 0.7.4.   
+
+
 # MongoDB Kubernetes Operator 0.7.4
 
 ## Kubernetes Operator

pkg/kube/podtemplatespec/podspec_template.go

@@ -2,6 +2,7 @@ package podtemplatespec
 
 import (
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/kube/container"
+	"github.com/mongodb/mongodb-kubernetes-operator/pkg/util/merge"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -107,30 +108,25 @@ func WithServiceAccount(serviceAccountName string) Modification {
 	}
 }
 
-// WithVolumes appends the given volumes to the existing volume, it ensures not
-// to override existing volumes
+// WithVolumes ensures given volume is present in the PodTemplateSpec. It merges the volumes with existing ones.
 func WithVolumes(volumes []corev1.Volume) Modification {
 	return func(template *corev1.PodTemplateSpec) {
-		present := make(map[string]struct{})
-		for _, v := range template.Spec.Volumes {
-			present[v.Name] = struct{}{}
-		}
-		for _, v := range volumes {
-			if _, ok := present[v.Name]; !ok {
-				template.Spec.Volumes = append(template.Spec.Volumes, v)
-			}
+		for _, volume := range volumes {
+			WithVolume(volume)(template)
 		}
 	}
 }
 
-// WithVolume ensures the given volume exists
+// WithVolume ensures given volume is present in the PodTemplateSpec. It merges the volume if it already exists.
 func WithVolume(volume corev1.Volume) Modification {
 	return func(template *corev1.PodTemplateSpec) {
-		for _, v := range template.Spec.Volumes {
-			if v.Name == volume.Name {
+		for i := range template.Spec.Volumes {
+			if template.Spec.Volumes[i].Name == volume.Name {
+				template.Spec.Volumes[i] = merge.Volume(template.Spec.Volumes[i], volume)
 				return
 			}
 		}
+
 		template.Spec.Volumes = append(template.Spec.Volumes, volume)
 	}
 }

pkg/kube/podtemplatespec/podspec_template_test.go

@@ -369,9 +369,9 @@ func TestAddVolumes(t *testing.T) {
 
 	p := New(volumeModification, volumesModification)
 	assert.Len(t, p.Spec.Volumes, 2)
-	assert.Equal(t, p.Spec.Volumes[0].Name, "new-volume")
-	assert.Equal(t, p.Spec.Volumes[1].Name, "new-volume-2")
-
+	assert.Equal(t, "new-volume", p.Spec.Volumes[0].Name)
+	assert.Equal(t, "new-volume-2", p.Spec.Volumes[1].Name)
+	assert.Equal(t, "new-host-path", p.Spec.Volumes[0].VolumeSource.HostPath.Path)
 }
 
 func int64Ref(i int64) *int64 {

test/e2e/replica_set_operator_upgrade/replica_set_operator_upgrade_test.go

@@ -41,19 +41,76 @@ func TestReplicaSetOperatorUpgrade(t *testing.T) {
 	t.Run("Basic tests", mongodbtests.BasicFunctionality(&mdb, true))
 	t.Run("Keyfile authentication is configured", tester.HasKeyfileAuth(3))
 	t.Run("Test Basic Connectivity", tester.ConnectivitySucceeds())
-	t.Run("Test SRV Connectivity", tester.ConnectivitySucceeds(WithURI(mdb.MongoSRVURI("")), WithoutTls(), WithReplicaSet((mdb.Name))))
+	t.Run("Test SRV Connectivity", tester.ConnectivitySucceeds(WithURI(mdb.MongoSRVURI("")), WithoutTls(), WithReplicaSet(mdb.Name)))
 	t.Run("Test Basic Connectivity with generated connection string secret",
 		tester.ConnectivitySucceeds(WithURI(mongodbtests.GetConnectionStringForUser(mdb, scramUser))))
 	t.Run("Test SRV Connectivity with generated connection string secret",
 		tester.ConnectivitySucceeds(WithURI(mongodbtests.GetSrvConnectionStringForUser(mdb, scramUser))))
 	t.Run("Ensure Authentication", tester.EnsureAuthenticationIsConfigured(3))
 	t.Run("AutomationConfig has the correct version", mongodbtests.AutomationConfigVersionHasTheExpectedVersion(&mdb, 1))
 
-	// upgrade the uperator to master
+	// upgrade the operator to master
 	config := setup.LoadTestConfigFromEnv()
 	err = setup.DeployOperator(config, "mdb", false, false)
 	assert.NoError(t, err)
 
 	// Perform the basic tests
 	t.Run("Basic tests", mongodbtests.BasicFunctionality(&mdb, true))
 }
+
+// TestReplicaSetOperatorUpgradeFrom0_7_2 is intended to be run locally not in CI.
+// It simulates deploying cluster using community operator 0.7.2 and then upgrading it using newer version.
+func TestReplicaSetOperatorUpgradeFrom0_7_2(t *testing.T) {
+	t.Skip("Supporting this test in CI requires installing also CRDs from release v0.7.2")
+	resourceName := "mdb-upg"
+	testConfig := setup.LoadTestConfigFromEnv()
+
+	// deploy operator and other components as it was at version 0.7.2
+	testConfig.OperatorImage = "quay.io/mongodb/mongodb-kubernetes-operator:0.7.2"
+	testConfig.VersionUpgradeHookImage = "quay.io/mongodb/mongodb-kubernetes-operator-version-upgrade-post-start-hook:1.0.3"
+	testConfig.ReadinessProbeImage = "quay.io/mongodb/mongodb-kubernetes-readinessprobe:1.0.6"
+	testConfig.AgentImage = "quay.io/mongodb/mongodb-agent:11.0.5.6963-1"
+
+	ctx := setup.SetupWithTestConfig(t, testConfig, true, resourceName)
+	defer ctx.Teardown()
+
+	mdb, user := e2eutil.NewTestMongoDB(ctx, resourceName, "")
+	scramUser := mdb.GetScramUsers()[0]
+	mdb.Spec.Security.TLS = e2eutil.NewTestTLSConfig(false)
+
+	_, err := setup.GeneratePasswordForUser(ctx, user, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tester, err := FromResource(t, mdb)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	runTests := func(t *testing.T) {
+		t.Run("Create MongoDB Resource", mongodbtests.CreateMongoDBResource(&mdb, ctx))
+		t.Run("Basic tests", mongodbtests.BasicFunctionality(&mdb, true))
+		t.Run("AutomationConfig has the correct version", mongodbtests.AutomationConfigVersionHasTheExpectedVersion(&mdb, 1))
+		t.Run("Keyfile authentication is configured", tester.HasKeyfileAuth(3))
+		t.Run("Has TLS Mode", tester.HasTlsMode("requireSSL", 60, WithTls(mdb)))
+		t.Run("Test Basic Connectivity", tester.ConnectivitySucceeds())
+		t.Run("Test SRV Connectivity", tester.ConnectivitySucceeds(WithURI(mdb.MongoSRVURI("")), WithoutTls(), WithReplicaSet(mdb.Name)))
+		t.Run("Test Basic Connectivity with generated connection string secret",
+			tester.ConnectivitySucceeds(WithURI(mongodbtests.GetConnectionStringForUser(mdb, scramUser))))
+		t.Run("Test SRV Connectivity with generated connection string secret",
+			tester.ConnectivitySucceeds(WithURI(mongodbtests.GetSrvConnectionStringForUser(mdb, scramUser))))
+		t.Run("Ensure Authentication", tester.EnsureAuthenticationIsConfigured(3))
+	}
+
+	runTests(t)
+
+	// When running against local operator we could stop here,
+	// rescale helm operator deployment to zero and run local operator then.
+
+	testConfig = setup.LoadTestConfigFromEnv()
+	err = setup.DeployOperator(testConfig, resourceName, true, false)
+	assert.NoError(t, err)
+
+	runTests(t)
+}