API-1843: FeatureGate(d) KMS encryption #2035
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @swghosh! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XS
openshift-ci-robot
added
the
jira/valid-reference
|
@swghosh: This pull request references API-1843 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
size/L
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
swghosh
changed the title
openshift-ci
bot
added
size/M
|
/cc @dgrisonnet @tkashem |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
added
size/XXL
swghosh
changed the title
swghosh
force-pushed
the
kms-01
branch
2 times, most recently
from
793d8ff to
f40ab94
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: swghosh, tkashem The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Show resolved
Hide resolved
|
New changes are detected. LGTM label has been removed. |
| @@ -52,6 +52,7 @@ type APIServerSpec struct { | |||
| // server from JavaScript applications. | |||
| // The values are regular expressions that correspond to the Golang regular expression language. | |||
| // +optional | |||
| // +listType=atomic | |||
There was a problem hiding this comment.
These +listType=atomic for exisiting fields in APISeverSpec are required to make the verify-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.
|
/retest |
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Outdated
Show resolved
Hide resolved
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Outdated
Show resolved
Hide resolved
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Outdated
Show resolved
Hide resolved
|
updated the integration test suite to be more verbose and specific about the error checks. |
| // | ||
| // +kubebuilder:validation:MaxLength=64 | ||
| // +kubebuilder:validation:MinLength=1 | ||
| Region string `json:"region"` |
There was a problem hiding this comment.
We used to have some validation on the valid characters here, where did it go?
There was a problem hiding this comment.
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.
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.
[a-z0-9]+(-[a-z0-9]+)*
And the maxLength can be 50, per AWS' own docs, based on Ben's link.
There was a problem hiding this comment.
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.
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 behaviour
xref: 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.
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.
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.
+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.
|
/retest |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
swghosh
force-pushed
the
kms-01
branch
3 times, most recently
from
f53d6be to
b949ddc
Compare
|
API is looking good, need to get the gate merged and go from there Are the team happy with the shape of this API? How has feedback on the EP been? |
Signed-off-by: Swarup Ghosh <[email protected]>
Signed-off-by: Swarup Ghosh <[email protected]>
|
@swghosh: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/payload 4.18 ci blocking |
|
@swghosh: trigger 4 job(s) of type blocking for the ci release of OCP 4.18
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9dbe3bb0-be17-11ef-999e-1bab93b98ddb-0 trigger 13 job(s) of type blocking for the nightly release of OCP 4.18
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9dbe3bb0-be17-11ef-999e-1bab93b98ddb-1 trigger 4 job(s) of type blocking for the ci release of OCP 4.19
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9dbe3bb0-be17-11ef-999e-1bab93b98ddb-2 trigger 13 job(s) of type blocking for the nightly release of OCP 4.19
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9dbe3bb0-be17-11ef-999e-1bab93b98ddb-3 |
This feature is primarily targeted for SelfManagedHA OpenShift TechPreview.
Feature Gate PR: #2071