-
Notifications
You must be signed in to change notification settings - Fork 45
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
fix: fix the use of a hardcoded admin user #653
Conversation
@@ -104,6 +112,11 @@ func NewClient(ctx context.Context, config ControllerConfiguration) (*Client, er | |||
defaultJAASCheck = true | |||
} | |||
|
|||
user := config.Username | |||
if config.ClientID != "" { | |||
user = fmt.Sprintf("%s%s", config.ClientID, serviceAccountSuffix) |
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.
A small thing but in the future we might want to force the user to add the "@serviceaccount" suffix themselves. So you could consider only adding the suffix if it's not already present. Lots of "ifs" so feel free to ignore this.
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.
done
a258a7a
to
7e2527d
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.
lgtm
/build |
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.
This change works for now, however I wonder if juju 4 will break it: https://github.com/juju/juju/blob/f200bb872ee96b9b40c65b400171ad249e612627/apiserver/facades/client/keymanager/keymanager.go#L167-L177
Please add more description to the commit message around the above and sshkeys.
I did qa with a user with add-cloud
permissions and had no problem on juju 3.5/edge.
35d7274
to
575152c
Compare
The terraform provider code used a hardcoded `admin` user in a number of place, which caused issues if a different username (or client credentails) was used. With this change we use the username (or clientID) as specified in the provider. This PR also changes the username that is passed in arguments to SSH key management calls. Juju 3.6 does not associate ssh keys with users - ssh keys are global per model - so the username is ignored. For consistency we pass in the username that is used to log in to the controller because in Juju 4 ssh keys will be associated with users.
575152c
to
4a5e92b
Compare
/merge |
Description
The terraform provider code used a hardcoded
admin
user in a number of place, which caused issuesif a different username (or client credentails) was used. With this change we use the username (or
clientID) as specified in the provider.
3.6 does not associate ssh keys with users - ssh keys are global per model - so the username
is ignored. For consistency we pass in the username that is used to log in to the
controller because in Juju 4 ssh keys will be associated with users.
Type of change
QA steps
Run JAAS integration tests.