-
Notifications
You must be signed in to change notification settings - Fork 584
OCPBUGS-68343: Introduce KMSEncryption feature gate #2669
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
base: master
Are you sure you want to change the base?
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@ardaguclu: This pull request references CNTRLPLANE-2241 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
|
Hello @ardaguclu! Some important instructions when contributing to openshift/api: |
|
@ardaguclu: This pull request references CNTRLPLANE-2241 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
📝 WalkthroughWalkthroughA new feature gate, KMSEncryption, was added and registered while KMSEncryptionProvider’s enablement was narrowed. Feature-gate entries were added or adjusted across payload manifests and the generated feature-gated CRD manifest. The APIServer CRD schema had the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
p0lyn0mial
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
config/v1/types_apiserver.go
Outdated
|
|
||
| // APIServerEncryption is used to encrypt sensitive resources on the cluster. | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryptionProvider,rule="has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms)",message="kms config is required when encryption type is KMS, and forbidden otherwise" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryptionProvider,rule="self.type != 'KMS' ? !has(self.kms) : true",message="kms config is forbidden when encryption type is not KMS" |
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.
From what I understand, this field will be deprecated in the future anyway.
For KMS v1, we don’t need it at all.
To make the backport easier, we would like to relax the validation only.
Considering that there hasn’t been any implementation added for this field, the changes look good to me.
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.
Agreed. Thanks for review.
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.
Do we expect in the future to have or require this KMS field?
The field is tech preview, so we don't need to deprecate it, we could remove it if we think that it won't be needed in the long term at all.
If we do need it in the long term, is there a future where it is acceptable that the KMS field is an optional union member? Or are you expecting in this case that the KMS field would be required when type is KMS?
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.
It is hard to answer these questions for sure, since we actively discuss the design. But in my opinion, it is likely that we would mark this field (or another field like kmsv2 by tombstoning kmsConfig) as required.
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 like to make sure, that if we don't need this field, it is not promoted to the default feature set until it is implemented
Which gate are you intending to promote here? Can the required parts be under one gate and the union member/union validation be under a separate gate?
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.
also, with this new approach, we would not need to change or backport any changes to openshift/client-go, right?
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.
There's no client-go changes for this no, I think this LGTM now, we can merge this, promote the new KMSEncryption gate and then come back to the other gate when we do the fuller implementation later
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.
promote the new KMSEncryption gate
With respect to this, what should I do?
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.
Normally, we would require 5 E2E tests reporting into component readiness gated on this feature gate [OCPFeatureGate:KMSEncryption], are you able to build some tests for this part of the feature?
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.
Currently we are actively working on deploying kms plugin on OCP for our e2e tests (openshift/release#73750). But it is not ready yet.
Can we backport this PR to 4.21 before e2e tests?. We'll need to vendor library-go with openshift/api, after that apiserver operators need to be vendored with library-go and openshift/api to run E2E tests.
|
Scheduling tests matching the |
|
I'm cancelling this lgtm to not trigger CI jobs, until API reviewers review this PR |
8368b38 to
fe4267b
Compare
|
@ardaguclu: This pull request references CNTRLPLANE-2241 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
|
/lgtm @p0lyn0mial @ardaguclu provided you are both happy, I'll leave the |
|
Scheduling tests matching the |
| } | ||
|
|
||
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum="";identity;aescbc;aesgcm | ||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=KMSEncryptionProvider,enum="";identity;aescbc;aesgcm;KMS |
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.
won't this flag also be on for TP ? If yes, will it allow for setting an empty KMSConfig ?
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.
@JoelSpeed on TP enabled cluster, won't this old feature gate be enabled too?. Won't this create conflicting behavior? i.e. KMSEncryption allows simply KMS enum but KMSEncryptionProvider does not allow it.
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.
@JoelSpeed is there a way to write an integration / unit test that would enable these two FG ?
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.
or each feature gate needs to be explicitly enabled on the cluster. So that we would just assume that user will enable KMSEncryption but not KMSEncryptionProvider
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.
Currently yes, on a TP cluster you would see the old/existing behaviour. If you promoted the feature KMSEncyption to default, you would not be able to set the KMS field because it wouldn't exist.
If you are concerned about the interactions between them/testing both, you can either disable KMSEncryptionProvider from TechPreview and move it back to just DevPreview, or we can set up tests that require both gates, and just one gate, or, one gate that not the other. In the test files where you specify the gates, prefix with - to negate the inclusion
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.
Won't this create conflicting behavior? i.e. KMSEncryption allows simply KMS enum but KMSEncryptionProvider does not allow it.
You'd currently observe just the behaviour of the KMSEncryptionProvider feature gate, as it's a superset of the new gate.
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, we are both fine to moving KMSEncryptionProvider to DevPreview.
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've moved KMSEncryptionProvider FG to DevPreview.
| - KMSEncryptionProvider | ||
| - -KMSEncryption |
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 was wrong, you didn't need line 6 here, please add this file back without L6
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 back.
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.
CI is in good shape. Would you please have a look at when you have a chance?
45ee221 to
ec37c6e
Compare
|
@ardaguclu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
@ardaguclu: This pull request references Jira Issue OCPBUGS-68343, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
/jira refresh |
|
@ardaguclu: This pull request references Jira Issue OCPBUGS-68343, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
|
/lgtm |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema, ci/prow/verify-crdify DetailsIn 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 kubernetes-sigs/prow repository. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, p0lyn0mial The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
User description
As we discussed in openshift/enhancements#1900. We need to allow nil KMSConfig, when encryption type is set to KMS.
This PR introduces new
KMSEncryptionfeature gate to exclude KMSConfig which is only enabled whenKMSEncryptionProviderfeature gate.PR Type
Enhancement
Description
Relax KMS validation rule to allow nil KMSConfig when encryption type is KMS
Update validation message to only forbid kms config for non-KMS encryption types
Add test case verifying KMS type with nil kms config is accepted
Update all CRD manifests and generated files with new validation rule
Diagram Walkthrough
File Walkthrough
1 files
Update KMS validation rule to allow nil config1 files
Add test for KMS type with nil kms config7 files
Update KMS validation in CustomNoUpgrade CRD manifestUpdate KMS validation in DevPreviewNoUpgrade CRD manifestUpdate KMS validation in TechPreviewNoUpgrade CRD manifestUpdate KMS validation in featuregated CRD manifestUpdate KMS validation in payload CustomNoUpgrade CRDUpdate KMS validation in payload DevPreviewNoUpgrade CRDUpdate KMS validation in payload TechPreviewNoUpgrade CRD1 files
Update OpenAPI schema with validation and documentation changes