-
Notifications
You must be signed in to change notification settings - Fork 72
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
CLOUDP-205313: patch dbuser update to support external DB #2378
Conversation
c0b7db8
to
cf5987d
Compare
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.
One minor ask if possible, otherwise I can approve
} | ||
|
||
func (opts *UpdateOpts) validateAuthDB() error { | ||
if opts.authDB == "" { |
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.
[q] what does the API return if you don't set this value?
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.
It maintains the previous behavior (setting the DatabaseName from the convert.getAuthDB(out)
method)
The intent was to avoid breaking the existing behaviour for programmatic users who won't have the flag set
mongodb-atlas-cli/internal/convert/database_user.go
Lines 116 to 141 in 811c672
// GetAuthDB determines the authentication database based on the type of user. | |
// LDAP, X509 and AWSIAM should all use $external. | |
// SCRAM-SHA should use admin. | |
func GetAuthDB(user *atlasv2.CloudDatabaseUser) string { | |
// base documentation https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/database_user | |
_, isX509 := adminX509Type[pointer.GetOrDefault(user.X509Type, "")] | |
_, isIAM := awsIAMType[pointer.GetOrDefault(user.AwsIAMType, "")] | |
// just USER is external | |
isLDAP := user.LdapAuthType != nil && *user.LdapAuthType == userLdapAuthType | |
if isX509 || isIAM || isLDAP { | |
return ExternalAuthDB | |
} | |
return defaultUserDatabase | |
} | |
var adminX509Type = map[string]struct{}{ | |
"MANAGED": {}, | |
"CUSTOMER": {}, | |
} | |
var awsIAMType = map[string]struct{}{ | |
"USER": {}, | |
"ROLE": {}, | |
} |
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.
okay, works for me
@@ -118,6 +131,7 @@ func UpdateBuilder() *cobra.Command { | |||
|
|||
cmd.Flags().StringVarP(&opts.username, flag.Username, flag.UsernameShort, "", usage.DBUsername) | |||
cmd.Flags().StringVarP(&opts.password, flag.Password, flag.PasswordShort, "", usage.DBUserPassword) | |||
cmd.Flags().StringVar(&opts.authDB, flag.AuthDB, "", usage.AtlasAuthDB) |
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.
should we set admin as default here?
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.
Same consideration as above
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
Proposed changes
Jira ticket: CLOUDP-205313
Before fix:
After fix:
Closes #2338
Checklist
make fmt
and formatted my codeFurther comments