CLOUDP-130486 fix operator crash due to empty modes array (#1116)

* fix: log validation warning if modes array empty * fix: autoAuthMechanism defaults if modes array empty * fix: add unit test for GetScramOptions * update: make warning message more descriptive
mongodb · Nov 3, 2022 · 4e1ab36 · 4e1ab36
1 parent cece58a
commit 4e1ab36
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 17 deletions.
diff --git a/api/v1/mongodbcommunity_types.go b/api/v1/mongodbcommunity_types.go
@@ -602,23 +602,27 @@ func (m MongoDBCommunity) GetOwnerReferences() []metav1.OwnerReference {
 // GetScramOptions returns a set of Options that are used to configure scram
 // authentication.
 func (m MongoDBCommunity) GetScramOptions() scram.Options {
-
 	ignoreUnknownUsers := true
 	if m.Spec.Security.Authentication.IgnoreUnknownUsers != nil {
 		ignoreUnknownUsers = *m.Spec.Security.Authentication.IgnoreUnknownUsers
 	}
 
 	authModes := m.Spec.Security.Authentication.Modes
 	defaultAuthMechanism := ConvertAuthModeToAuthMechanism(defaultMode)
-	autoAuthMechanism := ConvertAuthModeToAuthMechanism(authModes[0])
-
+	var autoAuthMechanism string
 	authMechanisms := make([]string, len(authModes))
 
-	for i, authMode := range authModes {
-		if authMech := ConvertAuthModeToAuthMechanism(authMode); authMech != "" {
-			authMechanisms[i] = authMech
-			if authMech == defaultAuthMechanism {
-				autoAuthMechanism = defaultAuthMechanism
+	if len(authModes) == 0 {
+		autoAuthMechanism = defaultAuthMechanism
+	} else {
+		autoAuthMechanism = ConvertAuthModeToAuthMechanism(authModes[0])
+
+		for i, authMode := range authModes {
+			if authMech := ConvertAuthModeToAuthMechanism(authMode); authMech != "" {
+				authMechanisms[i] = authMech
+				if authMech == defaultAuthMechanism {
+					autoAuthMechanism = defaultAuthMechanism
+				}
 			}
 		}
 	}

diff --git a/api/v1/mongodbcommunity_types_test.go b/api/v1/mongodbcommunity_types_test.go
@@ -66,6 +66,17 @@ func TestMongodConfigurationWithNestedMapsAfterUnmarshalling(t *testing.T) {
 	}, mc.Object)
 }
 
+func TestGetScramOptions(t *testing.T) {
+	t.Run("Default AutoAuthMechanism set if modes array empty", func(t *testing.T) {
+		mdb := newModesArray(nil, "empty-modes-array", "my-namespace")
+
+		options := mdb.GetScramOptions()
+
+		assert.EqualValues(t, defaultMode, options.AutoAuthMechanism)
+		assert.EqualValues(t, []string{}, options.AutoAuthMechanisms)
+	})
+}
+
 func TestGetScramCredentialsSecretName(t *testing.T) {
 	testusers := []struct {
 		in  MongoDBUser
@@ -192,3 +203,21 @@ func newReplicaSet(members int, name, namespace string) MongoDBCommunity {
 		},
 	}
 }
+
+func newModesArray(modes []AuthMode, name, namespace string) MongoDBCommunity {
+	return MongoDBCommunity{
+		TypeMeta: metav1.TypeMeta{},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+		Spec: MongoDBCommunitySpec{
+			Security: Security{
+				Authentication: Authentication{
+					Modes:              modes,
+					IgnoreUnknownUsers: nil,
+				},
+			},
+		},
+	}
+}
diff --git a/controllers/replica_set_controller.go b/controllers/replica_set_controller.go
@@ -589,7 +589,7 @@ func (r ReplicaSetReconciler) validateSpec(mdb mdbv1.MongoDBCommunity) error {
 	lastSuccessfulConfigurationSaved, ok := mdb.Annotations[lastSuccessfulConfiguration]
 	if !ok {
 		// First version of Spec
-		return validation.ValidateInitalSpec(mdb)
+		return validation.ValidateInitalSpec(mdb, r.log)
 	}
 
 	lastSpec := mdbv1.MongoDBCommunitySpec{}
@@ -598,7 +598,7 @@ func (r ReplicaSetReconciler) validateSpec(mdb mdbv1.MongoDBCommunity) error {
 		return err
 	}
 
-	return validation.ValidateUpdate(mdb, lastSpec)
+	return validation.ValidateUpdate(mdb, lastSpec, r.log)
 }
 
 func getCustomRolesModification(mdb mdbv1.MongoDBCommunity) (automationconfig.Modification, error) {

diff --git a/controllers/validation/validation.go b/controllers/validation/validation.go
@@ -7,23 +7,24 @@ import (
 
 	mdbv1 "github.com/mongodb/mongodb-kubernetes-operator/api/v1"
 	"github.com/mongodb/mongodb-kubernetes-operator/pkg/authentication/scram"
+	"go.uber.org/zap"
 )
 
 // ValidateInitalSpec checks if the resource's initial Spec is valid.
-func ValidateInitalSpec(mdb mdbv1.MongoDBCommunity) error {
-	return validateSpec(mdb)
+func ValidateInitalSpec(mdb mdbv1.MongoDBCommunity, log *zap.SugaredLogger) error {
+	return validateSpec(mdb, log)
 }
 
 // ValidateUpdate validates that the new Spec, corresponding to the existing one, is still valid.
-func ValidateUpdate(mdb mdbv1.MongoDBCommunity, oldSpec mdbv1.MongoDBCommunitySpec) error {
+func ValidateUpdate(mdb mdbv1.MongoDBCommunity, oldSpec mdbv1.MongoDBCommunitySpec, log *zap.SugaredLogger) error {
 	if oldSpec.Security.TLS.Enabled && !mdb.Spec.Security.TLS.Enabled {
 		return errors.New("TLS can't be set to disabled after it has been enabled")
 	}
-	return validateSpec(mdb)
+	return validateSpec(mdb, log)
 }
 
 // validateSpec validates the specs of the given resource definition.
-func validateSpec(mdb mdbv1.MongoDBCommunity) error {
+func validateSpec(mdb mdbv1.MongoDBCommunity, log *zap.SugaredLogger) error {
 	if err := validateUsers(mdb); err != nil {
 		return err
 	}
@@ -32,7 +33,7 @@ func validateSpec(mdb mdbv1.MongoDBCommunity) error {
 		return err
 	}
 
-	if err := validateAuthModeSpec(mdb); err != nil {
+	if err := validateAuthModeSpec(mdb, log); err != nil {
 		return err
 	}
 
@@ -102,9 +103,14 @@ func validateArbiterSpec(mdb mdbv1.MongoDBCommunity) error {
 }
 
 // validateAuthModeSpec checks that the list of modes does not contain duplicates.
-func validateAuthModeSpec(mdb mdbv1.MongoDBCommunity) error {
+func validateAuthModeSpec(mdb mdbv1.MongoDBCommunity, log *zap.SugaredLogger) error {
 	allModes := mdb.Spec.Security.Authentication.Modes
 
+	// Issue warning if Modes array is empty
+	if len(allModes) == 0 {
+		log.Warnf("An empty Modes array has been provided. The default mode (SCRAM-SHA-256) will be used.")
+	}
+
 	// Check that no auth is defined more than once
 	mapModes := make(map[mdbv1.AuthMode]struct{})
 	for i, mode := range allModes {