CLOUDP-382956: fix: use spec.db as authSource in MongoDBUser connection string secret#932
CLOUDP-382956: fix: use spec.db as authSource in MongoDBUser connection string secret#932filipcirtog wants to merge 10 commits intomasterfrom
Conversation
MCK 1.7.1 Release NotesBug Fixes
Other Changes
|
There was a problem hiding this comment.
Pull request overview
This PR ensures the MongoDBUser-generated connection string secret sets authSource based on the user’s spec.db (rather than being implicitly admin), and adds coverage for both admin and non-admin database users.
Changes:
- Pass the MongoDBUser
spec.dbthrough connection string generation soauthSourcematches the user’s database. - Extend e2e connectivity tests (replica set + sharded cluster) to validate
authSourcein generated secrets, including a non-admin DB user. - Add a unit test to assert
authSourceis set fromspec.dbin the connection string secret.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/mongodb-kubernetes-tests/tests/authentication/sharded_cluster_scram_sha_256_connectivity.py | Adds assertions for authSource in user secret connection strings; adds non-admin DB user coverage. |
| docker/mongodb-kubernetes-tests/tests/authentication/replica_set_scram_sha_256_connectivity.py | Adds non-admin DB user creation + secret assertions; updates expected user counts accordingly. |
| docker/mongodb-kubernetes-tests/tests/authentication/fixtures/scram-sha-user-non-admin-db.yaml | New MongoDBUser fixture for a user in a non-admin database. |
| controllers/operator/mongodbuser_controller_test.go | Adds unit test verifying authSource follows MongoDBUser spec.db. |
| controllers/operator/mongodbuser_controller.go | Uses user spec.db when building connection strings for the connection string secret. |
| controllers/operator/mongodbopsmanager_controller.go | Updates BuildConnectionString interface usage to new signature (passes empty authSource). |
| controllers/operator/connectionstring/connectionstring.go | Extends ConnectionStringBuilder interface signature to include authSource. |
| api/v1/mdbmulti/mongodb_multi_types.go | Extends BuildConnectionString signature and applies authSource override. |
| api/v1/mdb/mongodb_types.go | Extends BuildConnectionString signature and applies authSource override. |
| api/v1/mdb/mongodb_types_test.go | Updates tests for the new BuildConnectionString signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // authSource is only meaningful when an authMechanism is set. | ||
| if authSource == "" || authMechanism == "" { |
There was a problem hiding this comment.
The new authSource cleanup uses the derived authSource/authMechanism (from authSourceAndMechanism) rather than the final merged query params. If a caller provides authMechanism via connection params (e.g., non-SCRAM mechanisms) along with authSource, this code will still delete authSource, leaving an inconsistent URI. Consider basing the deletion on the merged connectionParams["authMechanism"] (or removing this block) so caller-supplied mechanisms behave correctly.
| if authSource == "" || authMechanism == "" { | |
| mergedAuthMechanism, hasAuthMechanism := connectionParams["authMechanism"] | |
| if !hasAuthMechanism || mergedAuthMechanism == "" { |
There was a problem hiding this comment.
In practice I don't think we ever set authMechanism via the connectionParams map currently, but that might change in the future, so I suppose it doesn't hurt to check the map after merging, instead of relying on just the result of authSourceAndMechanism.
| _ = client.Create(ctx, DefaultReplicaSetBuilder().EnableSCRAM().AgentAuthMode("SCRAM").SetName("my-rs").Build()) | ||
| createUserControllerConfigMap(ctx, client) | ||
| createPasswordSecret(ctx, client, user.Spec.PasswordSecretKeyRef, "password") |
There was a problem hiding this comment.
This test ignores the error returned by client.Create(...). If the replica set object fails to create, the test could proceed and fail later in a less clear way. Please assert require.NoError(t, client.Create(...)) (or similar) here for clearer failures.
fealebenpae
left a comment
fealebenpae
left a comment
There was a problem hiding this comment.
Looks good so far! I second Copilot's comment about resolving authMechanism from the merged connectionParams map.
I would like to see an assertion in the e2e test that validates we can actually connect and authenticate with the connection string generated in the secret.
| } | ||
|
|
||
| // authSource is only meaningful when an authMechanism is set. | ||
| if authSource == "" || authMechanism == "" { |
There was a problem hiding this comment.
In practice I don't think we ever set authMechanism via the connectionParams map currently, but that might change in the future, so I suppose it doesn't hurt to check the map after merging, instead of relying on just the result of authSourceAndMechanism.
Summary
The
authSourceparameter in the connection string secret generated for a MongoDBUser was always set to admin, regardless of which database the user was created in. This meant that users created in non-admin databases could not authenticate using the generated connection string.Proof of Work
The
spec.dbfield of the MongoDBUser is now passed asauthSourcewhen building the connection string, overriding the default admin. The override is only applied when anauthMechanismis also present, so connection strings for resources without authentication configured are not affected.Checklist
skip-changeloglabel if not needed