-
Notifications
You must be signed in to change notification settings - Fork 93
[GEP-26] Add support for Workload Identity credentials to etcd backups #1151
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
[GEP-26] Add support for Workload Identity credentials to etcd backups #1151
Conversation
dimityrmirchev
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.
Thanks!
|
|
||
| if !slices.Contains(allowedTokenURLs, rawTokenURL) { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("credentialsConfig").Child(keyTokenURL), cfg[keyTokenURL], "should be one of the allowed URLs: "+strings.Join(allowedTokenURLs, ", "))) | ||
| allErrs = append(allErrs, field.NotSupported(fldPath.Child("credentialsConfig").Child(keyTokenURL), cfg[keyTokenURL], allowedTokenURLs)) |
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 think Forbidden should be the proper word to describe a value that can be allowed otherwise. This is also what the documentation suggests.
| allErrs = append(allErrs, field.NotSupported(fldPath.Child("credentialsConfig").Child(keyTokenURL), cfg[keyTokenURL], allowedTokenURLs)) | |
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("credentialsConfig").Child(keyTokenURL), cfg[keyTokenURL], allowedTokenURLs)) |
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 do not think so, looking the docs for https://pkg.go.dev/k8s.io/[email protected]/pkg/util/validation/field#Forbidden and https://pkg.go.dev/k8s.io/[email protected]/pkg/util/validation/field#NotSupported NotSupported is the correct one as we have list of supported values, forbidden means that in certain conditions the value is acceptable, but in the particular case it is not allowed.
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.
forbidden means that in certain conditions the value is acceptable
Exactly, if the admission is configured to allow the value then this value will be accepted. In the documentation it even mentions as an example a security policy, which is exactly the case here. We allow only known token URLs.
Not supported as mentioned in the documentation "This is used to report unknown values for enumerated fields (e.g. a list of valid values)" would be used to indicate a value is not supported, e.g. you input a "mode" and the modes can be any of ["low", "medium", "high"] but not "ultra" as per the application and not configuration.
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 with ea48800
Co-authored-by: Dimitar Mirchev <[email protected]>
W.r.t. what kind of error to be token_url validation
dimityrmirchev
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.
Thank you for addressing my comments!
/lgtm
kon-angelo
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
How to categorize this PR?
/area security ipcei documentation
/kind enhancement
/platform gcp
What this PR does / why we need it:
[GEP-26] Add support for Workload Identity credentials to etcd backups
Which issue(s) this PR fixes:
Part of gardener/gardener#9586
Special notes for your reviewer:
etcd-backup-restore will add support for WorkloadIdentity credentials with gardener/etcd-backup-restore#908
If anybody wants to try this change locally with gardener with extensions, the following patch to gardener is required
spoiler for patch:
cc @dimityrmirchev
Release note: