-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Dont allow cloud tenants to update certain cluster networking config fields #28634
Conversation
|
||
func (a *ServerWithRoles) validateCloudNetworkConfigUpdate(newConfig, oldConfig types.ClusterNetworkingConfig) error { | ||
if a.hasBuiltinRole(types.RoleAdmin) { | ||
return nil |
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.
I'd argue that not having permission should return an error rather than nil
. How can I know if the config is updated if this function returns nil
and do nothing?
} | ||
|
||
if !modules.GetModules().Features().Cloud { | ||
return nil |
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.
The same as above. IMO this should be an 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.
I'm not sure I understand, for non cloud instances, its okay for non admin users to to update the config. This is the same way ValidateResource in modules.go works
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.
My bad, this is validate not set 🤦
But in this case, if you're an admin then we don't need to validate the values? Shouldn't this and if a.hasBuiltinRole(types.RoleAdmin) {
check be done last not first?
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.
As far as I understand from the issue (#18829 (comment)) cluster_networking_config is going to be a dynamic resource and so we'll want admin users to be able to update it without checking the rules, I think
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 looks correct to me too
bf576e6
to
0b81a64
Compare
} | ||
|
||
if !modules.GetModules().Features().Cloud { | ||
return nil |
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 looks correct to me too
f4b95d8
to
4040404
Compare
…fields (#41197) * Dont allow cloud tenants to update certain cluster networking config fields I'm bringing #28634 back from the dead, looks like it got nuked from orbit in a bad merge conflict resolution in #27873 Dont allow cloud tenants to update certain cluster networking fields * share validate impl * add godoc comment --------- Co-authored-by: Alex McGrath <alex.mcgrath@goteleport.com>
…onfig fields (#41247) * Dont allow cloud tenants to update certain cluster networking config fields I'm bringing #28634 back from the dead, looks like it got nuked from orbit in a bad merge conflict resolution in #27873 Dont allow cloud tenants to update certain cluster networking fields * share validate impl * add godoc comment --------- Co-authored-by: Alex McGrath <alex.mcgrath@goteleport.com>
Fix for #18829
This change allows cloud tenants to editing all fields of the cluster networking config except for those specified in #18829 --
The fields can be changed if the user editing them has the Admin role type