-
Notifications
You must be signed in to change notification settings - Fork 30
[CLOUDP-349630] Don't generate scramshacreds if MongoDBUser's password is not changed
#628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
MCK 1.6.1 Release NotesBug Fixes
|
lsierant
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Please look into existing e2e tests which are checking changing passwords and consider adding there a simulated MongoDBUser reconciler to ensure no automation config version is being bumped in the result.
| return r.updateStatus(ctx, user, workflow.OK(), log) | ||
| } | ||
|
|
||
| func (r *MongoDBUserReconciler) handleExternalAuthUser(ctx context.Context, user *userv1.MongoDBUser, conn om.Connection, log *zap.SugaredLogger) (reconcile.Result, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lucian-tosa,
Before I merge this, I would like to highlight that because we are changing signature of toOmUser it's also affecting handleExternalAuthUser. Even though we were mainly planning to handle the scram sha authentication as part of this change.
Do you think it's an acceptable change even for external users? If yes, I will go ahead and merge the PR.
Summary
When the reconciliation happens for the
MongoDBUserresource (because of controller restart or because of any other reason), forSCRAMauthentication mechanism, the scram sha creds are generated and then they are set in the automation config which results into another version getting created for Automation Config.This workflow can be improved to generated the scram sha creds only when the password of the user is changed. If we do that we will not be changing the user's scram creds and the automation config will not be updated.
To achieve this we are checking if the scram sha creds that are in automation config for a user, match with the scram sha creds generated with the stored salt and new password. If the scram sha creds match it means that the password for the user is not changed and we don't have to generate the new scram sha creds, otherwise new scram sha creds will be generated.
Proof of Work
Unit test:
I am yet to do some more tests to actually be confident that the automation config is not changed when reconciliation happens.
Checklist
skip-changeloglabel if not needed