WIP: Backport KMS TechPreview v1 API changes to 4.21#2737
WIP: Backport KMS TechPreview v1 API changes to 4.21#2737bertinatto wants to merge 3 commits intoopenshift:release-4.21from
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @bertinatto! Some important instructions when contributing to openshift/api: |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/L
Review Summary by QodoBackport KMS TechPreview v1 API changes to 4.21
WalkthroughsDescription• Add new KMSEncryption feature gate for v1 API • Remove KMS configuration from TechPreviewNoUpgrade CRD • Update KMSEncryptionProvider to DevPreviewNoUpgrade only • Update feature gate manifests across all deployment profiles Diagramflowchart LR
A["KMSEncryptionProvider<br/>DevPreviewNoUpgrade"] --> B["Feature Gate<br/>Updates"]
C["New KMSEncryption<br/>Feature Gate"] --> B
D["Remove KMS Config<br/>from TechPreviewNoUpgrade"] --> B
B --> E["Updated Manifests<br/>All Profiles"]
File Changes1. features/features.go
|
Code Review by Qodo
1. KMSEncryption tests missing
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| { | ||
| "name": "KMSEncryption" | ||
| }, | ||
| { | ||
| "name": "KMSEncryptionProvider" | ||
| }, |
There was a problem hiding this comment.
2. Kmsencryption enabled in default 🐞 Bug ✓ Correctness
The Default payload FeatureGate manifests enable KMSEncryption, but the feature gate registry only enables it for DevPreviewNoUpgrade and TechPreviewNoUpgrade. This mismatch is likely to fail hack/verify-payload-featuregates.sh (generated manifests won’t match) and/or unintentionally turn on the gate for Default clusters.
Agent Prompt
### Issue description
`KMSEncryption` is listed as **enabled** in the **Default** payload FeatureGate manifests, but the feature-gate registry enables it only for `DevPreviewNoUpgrade` and `TechPreviewNoUpgrade`. This creates a drift that is expected to fail `hack/verify-payload-featuregates.sh` (regen + diff) and/or unintentionally enable the gate for Default clusters.
### Issue Context
- Feature gate registry is in `features/features.go`.
- Payload featuregate manifests are expected to be generated from `features.AllFeatureSets()` (see `hack/verify-payload-featuregates.sh`).
### Fix Focus Areas
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml[125-142]
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml[130-146]
- features/features.go[653-667]
- hack/verify-payload-featuregates.sh[5-10]
### Expected fix
Option A (recommended): remove `KMSEncryption` from the Default payload manifests and regenerate via `hack/update-payload-featuregates.sh`.
Option B: if Default is intended, add `configv1.Default` to `FeatureGateKMSEncryption.enableIn(...)` and regenerate manifests + update docs accordingly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - identity | ||
| - aescbc | ||
| - aesgcm | ||
| - KMS |
There was a problem hiding this comment.
1. kmsencryption tests missing 📘 Rule violation ⛯ Reliability
This PR introduces the KMSEncryption FeatureGate and changes the APIServer TechPreviewNoUpgrade CRD schema to no longer allow type: KMS, but the existing validation tests still only target KMSEncryptionProvider. Without adding/updating tests for the new gate and updated schema, these API changes risk going unvalidated and regressing.
Agent Prompt
## Issue description
The PR changes the APIServer encryption schema behavior (TechPreviewNoUpgrade CRD no longer allows `type: KMS`) and introduces a new FeatureGate (`KMSEncryption`), but the validation tests under `config/v1/tests/apiservers.config.openshift.io/` still only cover `KMSEncryptionProvider`.
## Issue Context
Per compliance, API/CRD changes must be accompanied by corresponding integration/validation tests under the API group's `tests/` directory.
## Fix Focus Areas
- features/features.go[653-667]
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml[157-183]
- config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml[1-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
bertinatto
force-pushed
the
kms-tp-v1-4.21
branch
2 times, most recently
from
c0f5796 to
e8e8195
Compare
bertinatto
force-pushed
the
kms-tp-v1-4.21
branch
from
e8e8195 to
a2258d5
Compare
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/1682"). | ||
| enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade). | ||
| enableIn(configv1.DevPreviewNoUpgrade). |
There was a problem hiding this comment.
Do we want to do this in 4.21 as well? Or should we just introduce KMSEncryption?
|
@bertinatto: 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. |
3 participants
Manual cherry-pick of #2669.