[CLOUDP-349630] Don't generate scramshacreds if MongoDBUser's password is not changed

controllers/om/automation_config.go

@@ -394,6 +394,13 @@ type ScramShaCreds struct {
 	StoredKey      string `json:"storedKey"`
 }
 
+func (s ScramShaCreds) Equals(i ScramShaCreds) bool {
+	return s.IterationCount == i.IterationCount &&
+		s.Salt == i.Salt &&
+		s.ServerKey == i.ServerKey &&
+		s.StoredKey == i.StoredKey
+}
+
 func (u *MongoDBUser) AddRole(role *Role) {
 	u.Roles = append(u.Roles, role)
 }

controllers/operator/authentication/scramsha_credentials.go

@@ -28,10 +28,57 @@ const (
 	rfc5802MandatedSaltSize = 4
 )
 
+func isPasswordChanged(user *om.MongoDBUser, password string, acUser *om.MongoDBUser) (bool, error) {
+	// if something unexpected happens return true to make sure that the scramShaCreds will be generated again.
+	if acUser == nil || acUser.ScramSha256Creds == nil || acUser.ScramSha1Creds == nil {
+		return true, nil
+	}
+
+	if acUser.ScramSha256Creds.Salt != "" && acUser.ScramSha1Creds.Salt != "" {
+		sha256Salt, err := base64.StdEncoding.DecodeString(acUser.ScramSha256Creds.Salt)
+		if err != nil {
+			return true, err
+		}
+		sha1Salt, err := base64.StdEncoding.DecodeString(acUser.ScramSha1Creds.Salt)
+		if err != nil {
+			return true, nil
+		}
+		// generate scramshacreds with (new) password but with old salt to verify if given password
+		// is actually changed
+		newScramSha256Creds, err := computeScramShaCreds(user.Username, password, sha256Salt, ScramSha256)
+		if err != nil {
+			return false, xerrors.Errorf("error generating scramSha256 creds to verify with already present user on automation config %w", err)
+		}
+
+		newScramSha1Creds, err := computeScramShaCreds(user.Username, password, sha1Salt, MongoDBCR)
+		if err != nil {
+			return false, xerrors.Errorf("error generating scramSha1 creds to verify with already present user on automation config %w", err)
+		}
+		return !newScramSha256Creds.Equals(*acUser.ScramSha256Creds) || !newScramSha1Creds.Equals(*acUser.ScramSha1Creds), nil
+	}
+
+	return true, nil
+}
+
 // ConfigureScramCredentials creates both SCRAM-SHA-1 and SCRAM-SHA-256 credentials. This ensures
 // that changes to the authentication settings on the MongoDB resources won't leave MongoDBUsers without
 // the correct credentials.
-func ConfigureScramCredentials(user *om.MongoDBUser, password string) error {
+func ConfigureScramCredentials(user *om.MongoDBUser, password string, ac *om.AutomationConfig) error {
+	// there are chances that the reconciliation is happening again for this user resource and we wouldn't
+	// want to generate scram creds again if the password of the user has not changed.
+	_, acUser := ac.Auth.GetUser(user.Username, user.Database)
+	changed, err := isPasswordChanged(user, password, acUser)
+	if err != nil {
+		return err
+	}
+	if !changed {
+		// since the scram creds generated using the old salt are same with the scram creds stored in automation config
+		// there is no need to generate new salt/creds.
+		user.ScramSha256Creds = acUser.ScramSha256Creds
+		user.ScramSha1Creds = acUser.ScramSha1Creds
+		return nil
+	}
+
 	scram256Salt, err := generateSalt(sha256.New)
 	if err != nil {
 		return xerrors.Errorf("error generating scramSha256 salt: %w", err)

controllers/operator/authentication/scramsha_credentials_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/mongodb/mongodb-kubernetes/controllers/om"
 )
 
 func TestScramSha1SecretsMatch(t *testing.T) {
@@ -23,3 +25,41 @@ func assertSecretsMatch(t *testing.T, hash func() hash.Hash, passwordHash string
 	assert.Equal(t, computedStoredKey, storedKey)
 	assert.Equal(t, computedServerKey, serverKey)
 }
+
+var testAutomationConfig = &om.AutomationConfig{
+	Auth: &om.Auth{
+		Users: make([]*om.MongoDBUser, 0),
+	},
+}
+
+func Test_isPasswordChanged(t *testing.T) {
+	userPassword := "secretpassword"
+	userNewPassword := "newsecretpassword"
+
+	mongoUser := om.MongoDBUser{
+		Username: "new-user",
+		Database: "admin",
+	}
+
+	// will generate scram creds for the user mongoUser and set it in its fields
+	ConfigureScramCredentials(&mongoUser, userPassword, testAutomationConfig)
+
+	testAutomationConfig.Auth.Users = append(testAutomationConfig.Auth.Users, &om.MongoDBUser{
+		Username:         mongoUser.Username,
+		Database:         mongoUser.Database,
+		ScramSha256Creds: mongoUser.ScramSha256Creds,
+		ScramSha1Creds:   mongoUser.ScramSha1Creds,
+	})
+
+	// now that the scram creds are set in the automation config, let's say the reconciliation happens again
+	// with the same user and same password, isPasswordChanged should return false
+	_, u := testAutomationConfig.Auth.GetUser(mongoUser.Username, mongoUser.Database)
+	op, err := isPasswordChanged(&mongoUser, userPassword, u)
+	assert.Nil(t, err)
+	assert.False(t, op)
+
+	// if reconciliation happens again with diff password, isPasswordChanged should return true
+	op, err = isPasswordChanged(&mongoUser, userNewPassword, u)
+	assert.Nil(t, err)
+	assert.True(t, op)
+}

controllers/operator/mongodbuser_controller.go

@@ -333,7 +333,7 @@ func AddMongoDBUserController(ctx context.Context, mgr manager.Manager, memberCl
 // toOmUser converts a MongoDBUser specification and optional password into an
 // automation config MongoDB user. If the user has no password then a blank
 // password should be provided.
-func toOmUser(spec userv1.MongoDBUserSpec, password string) (om.MongoDBUser, error) {
+func toOmUser(spec userv1.MongoDBUserSpec, password string, ac *om.AutomationConfig) (om.MongoDBUser, error) {
 	user := om.MongoDBUser{
 		Database:                   spec.Database,
 		Username:                   spec.Username,
@@ -344,7 +344,7 @@ func toOmUser(spec userv1.MongoDBUserSpec, password string) (om.MongoDBUser, err
 
 	// only specify password if we're dealing with non-x509 users
 	if spec.Database != authentication.ExternalDB {
-		if err := authentication.ConfigureScramCredentials(&user, password); err != nil {
+		if err := authentication.ConfigureScramCredentials(&user, password, ac); err != nil {
 			return om.MongoDBUser{}, xerrors.Errorf("error generating SCRAM credentials: %w", err)
 		}
 	}
@@ -385,7 +385,7 @@ func (r *MongoDBUserReconciler) handleScramShaUser(ctx context.Context, user *us
 			auth.RemoveUser(user.Status.Username, user.Status.Database)
 		}
 
-		desiredUser, err := toOmUser(user.Spec, password)
+		desiredUser, err := toOmUser(user.Spec, password, ac)
 		if err != nil {
 			return err
 		}
@@ -421,18 +421,18 @@ func (r *MongoDBUserReconciler) handleScramShaUser(ctx context.Context, user *us
 }
 
 func (r *MongoDBUserReconciler) handleExternalAuthUser(ctx context.Context, user *userv1.MongoDBUser, conn om.Connection, log *zap.SugaredLogger) (reconcile.Result, error) {
-	desiredUser, err := toOmUser(user.Spec, "")
-	if err != nil {
-		return r.updateStatus(ctx, user, workflow.Failed(xerrors.Errorf("error updating user %w", err)), log)
-	}
-
 	shouldRetry := false
 	updateFunction := func(ac *om.AutomationConfig) error {
 		if !externalAuthMechanismsAvailable(ac.Auth.DeploymentAuthMechanisms) {
 			shouldRetry = true
 			return xerrors.Errorf("no external authentication mechanisms (LDAP or x509) have been configured")
 		}
 
+		desiredUser, err := toOmUser(user.Spec, "", ac)
+		if err != nil {
+			return xerrors.Errorf("errorr updating user %w", err)
+		}
+
 		auth := ac.Auth
 		if user.ChangedIdentifier() {
 			auth.RemoveUser(user.Status.Username, user.Status.Database)
@@ -442,7 +442,7 @@ func (r *MongoDBUserReconciler) handleExternalAuthUser(ctx context.Context, user
 		return nil
 	}
 
-	err = conn.ReadUpdateAutomationConfig(updateFunction, log)
+	err := conn.ReadUpdateAutomationConfig(updateFunction, log)
 	if err != nil {
 		if shouldRetry {
 			return r.updateStatus(ctx, user, workflow.Pending("%s", err.Error()).WithRetry(10), log)

controllers/operator/mongodbuser_controller_test.go

@@ -2,6 +2,7 @@ package operator
 
 import (
 	"context"
+	"encoding/json"
 	"testing"
 	"time"
 
@@ -71,6 +72,69 @@ func TestUserIsAdded_ToAutomationConfig_OnSuccessfulReconciliation(t *testing.T)
 	assert.Equal(t, len(user.Spec.Roles), len(createdUser.Roles))
 }
 
+// Not making this (DeepCopy) a method of type AutomationConfig because I want to be
+// explicit that this method is just for test code.
+func DeepCopy(original *om.AutomationConfig) (*om.AutomationConfig, error) {
+	if original == nil {
+		return nil, nil
+	}
+
+	b, err := json.Marshal(original)
+	if err != nil {
+		return nil, err
+	}
+
+	var newAC om.AutomationConfig
+	err = json.Unmarshal(b, &newAC)
+	if err != nil {
+		return nil, err
+	}
+	return &newAC, nil
+}
+
+func TestNoChange_InAC_After_Same_User_Reconciliation(t *testing.T) {
+	ctx := context.Background()
+	user := DefaultMongoDBUserBuilder().SetMongoDBResourceName("my-rs").Build()
+	reconciler, client, omConnectionFactory := userReconcilerWithAuthMode(ctx, user, util.AutomationConfigScramSha256Option)
+
+	// initialize resources required for the tests
+	_ = client.Create(ctx, DefaultReplicaSetBuilder().EnableAuth().AgentAuthMode("SCRAM").
+		SetName("my-rs").Build())
+	createUserControllerConfigMap(ctx, client)
+	createPasswordSecret(ctx, client, user.Spec.PasswordSecretKeyRef, "password")
+
+	actual, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: kube.ObjectKey(user.Namespace, user.Name)})
+
+	okReconcileResult, _ := workflow.OK().ReconcileResult()
+
+	assert.Nil(t, err, "there should be no error on successful reconciliation")
+	assert.Equal(t, okReconcileResult, actual, "there should be a successful reconciliation if the password is a valid reference")
+
+	ac, _ := omConnectionFactory.GetConnection().ReadAutomationConfig()
+	// since underlying implmentation of ReadAutomationConfig just has reference to automation config
+	// it's better to deep copy this version of AC so that it can be used to compare later.
+	originalAC, err := DeepCopy(ac)
+	assert.Nil(t, err)
+
+	// the automation config should have been updated during reconciliation
+	assert.Len(t, ac.Auth.Users, 1, "the MongoDBUser should have been added to the AutomationConfig")
+
+	_, createdUser := ac.Auth.GetUser("my-user", "admin")
+	assert.Equal(t, user.Spec.Username, createdUser.Username)
+	assert.Equal(t, user.Spec.Database, createdUser.Database)
+	assert.Equal(t, len(user.Spec.Roles), len(createdUser.Roles))
+
+	// reconcile the same user again and make sure the automation config is not updated, if the user's password is not changed
+	// we don't generate scram creds and because of that we wouldn't see changes in automatino config
+	reconcileResult, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: kube.ObjectKey(user.Namespace, user.Name)})
+	assert.Nil(t, err)
+	assert.Equal(t, okReconcileResult, reconcileResult)
+
+	acAfterSecondReconcile, _ := omConnectionFactory.GetConnection().ReadAutomationConfig()
+	// verify that the automation cnofig has not been changed because we ran reconciliation second time with the same user
+	assert.True(t, acAfterSecondReconcile.EqualsWithoutDeployment(*originalAC), "Automation config before the second reconciliation and after the second reconciliation should be same")
+}
+
 func TestReconciliationSucceed_OnAddingUser_FromADifferentNamespace(t *testing.T) {
 	ctx := context.Background()
 	user := DefaultMongoDBUserBuilder().SetMongoDBResourceName("my-rs").Build()