Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
API-1843: FeatureGate(d) KMS encryption #2035
base: master
Are you sure you want to change the base?
API-1843: FeatureGate(d) KMS encryption #2035
Changes from all commits
c559147
470263a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These
+listType=atomic
for exisiting fields in APISeverSpec are required to make theverify-crd-schema
pass, which fails the test saying lack of SSA tags in the generated CRD. Alternative to adding these is to override the specific test but this retains the API violations as-is today.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.
We used to have some validation on the valid characters here, where did it go?
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.
Had removed it based on suggestions from @benluddy
#2035 (comment)
Let's re-check if we want to keep that validation rule.
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 would at least check the character set is lowercase alphanumeric segments separate by hyphens, even if it is not as strict as what you had. We know they use the region as part of the domain name, so it must be a valid domain segment, so below should be accurate.
And the maxLength can be 50, per AWS' own docs, based on Ben's link.
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.
Added
+ // +kubebuilder:validation:XValidation:rule="self.matches('^[a-z0-9]+(-[a-z0-9]+)*$')",message="region must be a valid AWS region, consisting of lowercase characters, digits and hyphens (-) only."
Also, added an integration test case to look for that error message when it is violated.
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.
btw, on that AWS region on a similar context
found from elsewhere that
"aws-global"
seems to also be a somewhat valid region for AWS Go SDK v2 trying to connect over AWS STS credentials with some legacy behaviourxref: https://github.com/cert-manager/cert-manager/blob/537e71ee639a41887e93b0fd151bf063c4730536/pkg/issuer/acme/dns/route53/route53.go#L103
however the said regex, covers this case too so nvm!
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.
xref #2124
Here's an example of our API validation rejecting a value that would have been accepted by an AWS API. Just sharing to support my opinion that it's worth erring on the side of being lax with these. I would be a little surprised if these values were validated consistently across all services within AWS.
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.
+1. I'm all for including documented regexes or, perhaps, relatively simple ones, but this is at least the second time I can recall that we (installer team) have had to revise regexes we attempted to reverse engineer the requirements. In the first case we inadvertently excluded disk encryption keys with capital letters, which are allowed in Azure.
Where possible, I prefer validating against the cloud api rather than regexes, but that may just be a luxury we have in the installer.