CNTRLPLANE-3237: Introduce kms to kms migration#2192
Open
ardaguclu wants to merge 1 commit intoopenshift:masterfrom
Open
CNTRLPLANE-3237: Introduce kms to kms migration#2192ardaguclu wants to merge 1 commit intoopenshift:masterfrom
ardaguclu wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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 "5.0.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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughKey controller and KMS helpers detect KMS provider-config changes that require key migration, create new encryption keys with internal reason "kms-provider-changed", and propagate per-key KMS provider configs into encryption config secrets; tests and e2e flows updated to cover migration vs non-migration deltas. ChangesKMS provider + key lifecycle
Sequence DiagramsequenceDiagram
autonumber
participant Controller as Controller
participant KMSHelpers as KMS Helpers
participant SecretStore as Secret Store
participant API as API Server
Controller->>API: Read current APIServerEncryption (KMS config)
Controller->>SecretStore: Read latest encryption keys
Controller->>KMSHelpers: Compare latest key provider vs current KMS config
KMSHelpers-->>Controller: NeedsNewKey? (true/false)
alt NeedsNewKey == true
Controller->>SecretStore: Create new encryption-key secret (internal-reason: kms-provider-changed)
SecretStore-->>Controller: Confirm creation
Controller->>API: Update encryption-config referencing new key
else NeedsNewKey == false
Controller-->>Controller: No key rotation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ardaguclu 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 |
Member
Author
|
Due to the test function updates, #2163 is prerequisite for this PR (there is no dependency in terms of core functionality) |
openshift-ci
Bot
added
the
do-not-merge/hold
Member
Author
|
/uncc @dgrisonnet |
openshift-ci
Bot
requested review from
bertinatto
and removed request for
dgrisonnet
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/operator/encryption/controllers/key_controller_test.go (1)
622-774: ⚡ Quick winConsider one regression case for “existing KMS secret missing provider config”.
Given the new KMS comparison path, a test with an older/malformed KMS key secret lacking provider config would protect against panic regressions and assert fallback behavior (recreate key with
kms-provider-changed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/key_controller_test.go` around lines 622 - 774, Add a regression test that simulates an existing KMS key Secret missing the provider config to ensure the comparison path doesn't panic and the controller recreates the key with the "secrets-kms-provider-changed" internal-reason; copy one of the KMS table tests (e.g., the "creates a new KMS key when VaultAddress changes" case), but construct initialObjects using encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSConfig or a similar helper modified to produce a Secret that lacks the "encryption.apiserver.operator.openshift.io-kms-provider-config" data key (or contains malformed JSON), set apiServerObjects using simpleAPIServer with dummyKMSConfig changes, and assert in validateFunc that a create:secrets action occurs and the created Secret has annotation encryption.apiserver.operator.openshift.io/internal-reason == "secrets-kms-provider-changed" and contains a valid provider config (no panic).pkg/operator/encryption/kms/helpers_test.go (1)
33-117: ⚡ Quick winAdd coverage for nil and provider-type change branches.
Current table covers Vault field deltas well, but it misses
latest/current == nilandTypemismatch paths, which are central guard rails for this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/helpers_test.go` around lines 33 - 117, Add two table-driven test cases to the existing tests slice in helpers_test.go to cover nil and provider-type-change branches: one case where latest is nil and current is a non-nil *configv1.KMSConfig (expect true/indicating change) and one case where latest is non-nil and current is nil (expect true), plus a case where both are non-nil but their Type fields differ (e.g., latest.Type = "Vault" and current.Type = "KMSPlugin") expecting true; reference the same KMSConfig/Vault structures used in the file and ensure the cases follow the same shape (name, latest, current, expected) as the other table entries so the test invokes the same comparison helper being exercised by the existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 367-373: The code dereferences latestKey.KMSConfig.Provider when
currentMode == state.KMS; add a nil/empty guard for latestKey.KMSConfig and
latestKey.KMSConfig.Provider before calling
kms.NeedsNewKey(latestKey.KMSConfig.Provider, currentApiServerEncryption.KMS);
if either is nil/empty, treat it as a provider change and return the same
migration result (e.g., return latestKeyID, "kms-provider-changed", true);
otherwise call kms.NeedsNewKey as before. Ensure the check surrounds the call to
kms.NeedsNewKey in key_controller.go so you never dereference a nil KMSConfig or
Provider.
In `@pkg/operator/encryption/kms/helpers.go`:
- Around line 16-25: The NeedsNewKey function currently dereferences latest and
current without nil checks; update it to first guard against nil inputs (e.g.,
if latest == nil || current == nil return true) before comparing Type, and when
handling provider-specific logic (NeedsNewKey's Vault branch) ensure
latest.Vault and current.Vault are non-nil before accessing VaultAddress,
VaultNamespace, TransitMount, TransitKey — if either Vault pointer is nil, treat
that as a change (return true) otherwise compare the individual fields. This
ensures nil-safety for NeedsNewKey and avoids controller panic.
In `@test/e2e-encryption/encryption_test.go`:
- Around line 514-519: Replace the fixed time.Sleep(5 * time.Second) with a
bounded poll that asserts "no new key" by repeatedly calling waitForKeys (or
querying the key count via the same mechanism used by waitForKeys) until the
count remains stable for a short stabilization window or a global timeout is
reached; after the fakeApiServerClient.Patch call (and after require.NoError),
implement polling that samples the key count, waits a small interval, rechecks,
and only proceeds to waitForConditionStatus("Encrypted",
operatorv1.ConditionTrue) when the key count has not increased during the
stabilization period or the timeout elapses, failing the test on timeout.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller_test.go`:
- Around line 622-774: Add a regression test that simulates an existing KMS key
Secret missing the provider config to ensure the comparison path doesn't panic
and the controller recreates the key with the "secrets-kms-provider-changed"
internal-reason; copy one of the KMS table tests (e.g., the "creates a new KMS
key when VaultAddress changes" case), but construct initialObjects using
encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSConfig or a similar
helper modified to produce a Secret that lacks the
"encryption.apiserver.operator.openshift.io-kms-provider-config" data key (or
contains malformed JSON), set apiServerObjects using simpleAPIServer with
dummyKMSConfig changes, and assert in validateFunc that a create:secrets action
occurs and the created Secret has annotation
encryption.apiserver.operator.openshift.io/internal-reason ==
"secrets-kms-provider-changed" and contains a valid provider config (no panic).
In `@pkg/operator/encryption/kms/helpers_test.go`:
- Around line 33-117: Add two table-driven test cases to the existing tests
slice in helpers_test.go to cover nil and provider-type-change branches: one
case where latest is nil and current is a non-nil *configv1.KMSConfig (expect
true/indicating change) and one case where latest is non-nil and current is nil
(expect true), plus a case where both are non-nil but their Type fields differ
(e.g., latest.Type = "Vault" and current.Type = "KMSPlugin") expecting true;
reference the same KMSConfig/Vault structures used in the file and ensure the
cases follow the same shape (name, latest, current, expected) as the other table
entries so the test invokes the same comparison helper being exercised by the
existing tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 58265021-9d56-4fab-bdb5-25c2e172ce2c
📒 Files selected for processing (6)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.go
Comment on lines
+16
to
+25
| func NeedsNewKey(latest, current *configv1.KMSConfig) bool { | ||
| if latest.Type != current.Type { | ||
| // TODO: Integrate this with pre-flight checker | ||
| return true | ||
| } | ||
| if latest.Type == configv1.VaultKMSProvider { | ||
| if latest.Vault.VaultAddress != current.Vault.VaultAddress || | ||
| latest.Vault.VaultNamespace != current.Vault.VaultNamespace || | ||
| latest.Vault.TransitMount != current.Vault.TransitMount || | ||
| latest.Vault.TransitKey != current.Vault.TransitKey { |
There was a problem hiding this comment.
Add nil-safety to NeedsNewKey before dereferencing provider fields.
At Line 17, latest/current are dereferenced unconditionally. A nil input will panic the controller path.
Suggested fix
func NeedsNewKey(latest, current *configv1.KMSConfig) bool {
+ if latest == nil || current == nil {
+ return latest != current
+ }
if latest.Type != current.Type {
// TODO: Integrate this with pre-flight checker
return true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NeedsNewKey(latest, current *configv1.KMSConfig) bool { | |
| if latest.Type != current.Type { | |
| // TODO: Integrate this with pre-flight checker | |
| return true | |
| } | |
| if latest.Type == configv1.VaultKMSProvider { | |
| if latest.Vault.VaultAddress != current.Vault.VaultAddress || | |
| latest.Vault.VaultNamespace != current.Vault.VaultNamespace || | |
| latest.Vault.TransitMount != current.Vault.TransitMount || | |
| latest.Vault.TransitKey != current.Vault.TransitKey { | |
| func NeedsNewKey(latest, current *configv1.KMSConfig) bool { | |
| if latest == nil || current == nil { | |
| return latest != current | |
| } | |
| if latest.Type != current.Type { | |
| // TODO: Integrate this with pre-flight checker | |
| return true | |
| } | |
| if latest.Type == configv1.VaultKMSProvider { | |
| if latest.Vault.VaultAddress != current.Vault.VaultAddress || | |
| latest.Vault.VaultNamespace != current.Vault.VaultNamespace || | |
| latest.Vault.TransitMount != current.Vault.TransitMount || | |
| latest.Vault.TransitKey != current.Vault.TransitKey { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/kms/helpers.go` around lines 16 - 25, The NeedsNewKey
function currently dereferences latest and current without nil checks; update it
to first guard against nil inputs (e.g., if latest == nil || current == nil
return true) before comparing Type, and when handling provider-specific logic
(NeedsNewKey's Vault branch) ensure latest.Vault and current.Vault are non-nil
before accessing VaultAddress, VaultNamespace, TransitMount, TransitKey — if
either Vault pointer is nil, treat that as a change (return true) otherwise
compare the individual fields. This ensures nil-safety for NeedsNewKey and
avoids controller panic.
Member
Author
There was a problem hiding this comment.
KMSConfig can not be nil if encryption mode is KMS
There was a problem hiding this comment.
@ardaguclu, understood — since NeedsNewKey is only invoked on the KMS code path and KMSConfig is guaranteed to be non-nil in that context, the nil guard is unnecessary. Thanks for the clarification!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
Comment on lines
+514
to
+519
| t.Logf("KMS non-migration change: only KMSPluginImage changes (no new key expected)") | ||
| _, err = fakeApiServerClient.Patch(ctx, "cluster", types.MergePatchType, []byte(`{"spec":{"encryption":{"type":"KMS","kms":{"type":"Vault","vault":{"kmsPluginImage":"registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000","vaultAddress":"https://vault-new.example.com","authentication":{"type":"AppRole","appRole":{"secret":{"name":"vault-approle-secret"}}},"transitKey":"test-transit-key"}}}}}`), metav1.PatchOptions{}) | ||
| require.NoError(t, err) | ||
| time.Sleep(5 * time.Second) | ||
| waitForKeys(12) | ||
| waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) |
There was a problem hiding this comment.
Replace fixed sleep with a bounded poll for “no new key” assertion.
Line 517’s time.Sleep(5 * time.Second) makes this check timing-sensitive. Prefer polling key count stability within a timeout window.
Suggested direction
- time.Sleep(5 * time.Second)
- waitForKeys(12)
+ err = wait.PollUntilContextTimeout(ctx, 200*time.Millisecond, 10*time.Second, true, func(ctx context.Context) (bool, error) {
+ l, err := kubeClient.CoreV1().Secrets("openshift-config-managed").List(ctx, metav1.ListOptions{LabelSelector: keySecretsLabel})
+ if err != nil {
+ return false, err
+ }
+ return len(l.Items) == 12, nil
+ })
+ require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Logf("KMS non-migration change: only KMSPluginImage changes (no new key expected)") | |
| _, err = fakeApiServerClient.Patch(ctx, "cluster", types.MergePatchType, []byte(`{"spec":{"encryption":{"type":"KMS","kms":{"type":"Vault","vault":{"kmsPluginImage":"registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000","vaultAddress":"https://vault-new.example.com","authentication":{"type":"AppRole","appRole":{"secret":{"name":"vault-approle-secret"}}},"transitKey":"test-transit-key"}}}}}`), metav1.PatchOptions{}) | |
| require.NoError(t, err) | |
| time.Sleep(5 * time.Second) | |
| waitForKeys(12) | |
| waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) | |
| t.Logf("KMS non-migration change: only KMSPluginImage changes (no new key expected)") | |
| _, err = fakeApiServerClient.Patch(ctx, "cluster", types.MergePatchType, []byte(`{"spec":{"encryption":{"type":"KMS","kms":{"type":"Vault","vault":{"kmsPluginImage":"registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000","vaultAddress":"https://vault-new.example.com","authentication":{"type":"AppRole","appRole":{"secret":{"name":"vault-approle-secret"}}},"transitKey":"test-transit-key"}}}}}`), metav1.PatchOptions{}) | |
| require.NoError(t, err) | |
| err = wait.PollUntilContextTimeout(ctx, 200*time.Millisecond, 10*time.Second, true, func(ctx context.Context) (bool, error) { | |
| l, err := kubeClient.CoreV1().Secrets("openshift-config-managed").List(ctx, metav1.ListOptions{LabelSelector: keySecretsLabel}) | |
| if err != nil { | |
| return false, err | |
| } | |
| return len(l.Items) == 12, nil | |
| }) | |
| require.NoError(t, err) | |
| waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e-encryption/encryption_test.go` around lines 514 - 519, Replace the
fixed time.Sleep(5 * time.Second) with a bounded poll that asserts "no new key"
by repeatedly calling waitForKeys (or querying the key count via the same
mechanism used by waitForKeys) until the count remains stable for a short
stabilization window or a global timeout is reached; after the
fakeApiServerClient.Patch call (and after require.NoError), implement polling
that samples the key count, waits a small interval, rechecks, and only proceeds
to waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) when the key
count has not increased during the stabilization period or the timeout elapses,
failing the test on timeout.
ardaguclu
force-pushed
the
kms-to-kms
branch
2 times, most recently
from
4fdbb32 to
3c4eb34
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-encryption/encryption_test.go (1)
555-557: ⚡ Quick winUse
encoding.DecodeKMSConfiginstead of raw JSON unmarshal.Line 556 bypasses the shared decode path used elsewhere in this test. Reusing
encoding.DecodeKMSConfigkeeps this assertion aligned with the actual serialization contract and avoids drift if encoding behavior changes.Suggested change
- var providerConfig13 configv1.KMSConfig - require.NoError(t, json.Unmarshal(kmsProviderConfigData13, &providerConfig13)) - require.Equal(t, "https://vault-new.example.com", providerConfig13.Vault.VaultAddress) - require.Equal(t, "test-transit-key", providerConfig13.Vault.TransitKey) + providerConfig13, err := encoding.DecodeKMSConfig(kmsProviderConfigData13) + require.NoError(t, err) + require.Equal(t, "https://vault-new.example.com", providerConfig13.Vault.VaultAddress) + require.Equal(t, "test-transit-key", providerConfig13.Vault.TransitKey)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-encryption/encryption_test.go` around lines 555 - 557, Replace the direct json.Unmarshal call on kmsProviderConfigData13 with the shared decoder: call encoding.DecodeKMSConfig(kmsProviderConfigData13, &providerConfig13) (or the appropriate signature) instead of json.Unmarshal; keep the variable providerConfig13 and the subsequent assertion require.Equal(t, "https://vault-new.example.com", providerConfig13.Vault.VaultAddress) unchanged so the test uses the common serialization/decoding path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e-encryption/encryption_test.go`:
- Around line 555-557: Replace the direct json.Unmarshal call on
kmsProviderConfigData13 with the shared decoder: call
encoding.DecodeKMSConfig(kmsProviderConfigData13, &providerConfig13) (or the
appropriate signature) instead of json.Unmarshal; keep the variable
providerConfig13 and the subsequent assertion require.Equal(t,
"https://vault-new.example.com", providerConfig13.Vault.VaultAddress) unchanged
so the test uses the common serialization/decoding path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 367b45f1-563c-4184-897d-fd59eebb3636
📒 Files selected for processing (11)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptiondata/secret.gopkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/state/types.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/encryption/kms/helpers.go
- pkg/operator/encryption/kms/helpers_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-encryption/encryption_test.go (1)
558-560: ⚡ Quick winUse
encoding.DecodeKMSConfiginstead of rawjson.Unmarshalfor provider payloads.At Line 559, decoding via the same helper used elsewhere keeps this test resilient if provider serialization details evolve.
Proposed change
- var providerConfig13 configv1.KMSConfig - require.NoError(t, json.Unmarshal(kmsProviderConfigData13, &providerConfig13)) - require.Equal(t, "https://vault-new.example.com", providerConfig13.Vault.VaultAddress) - require.Equal(t, "test-transit-key", providerConfig13.Vault.TransitKey) + providerConfig13, err := encoding.DecodeKMSConfig(kmsProviderConfigData13) + require.NoError(t, err) + require.Equal(t, "https://vault-new.example.com", providerConfig13.Vault.VaultAddress) + require.Equal(t, "test-transit-key", providerConfig13.Vault.TransitKey)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-encryption/encryption_test.go` around lines 558 - 560, Replace the raw json.Unmarshal call with the shared helper encoding.DecodeKMSConfig to decode provider payloads: instead of calling json.Unmarshal(kmsProviderConfigData13, &providerConfig13) use encoding.DecodeKMSConfig(kmsProviderConfigData13, &providerConfig13) and assert no error (require.NoError on the DecodeKMSConfig call); keep the subsequent require.Equal on providerConfig13.Vault.VaultAddress unchanged. This updates the test to use the same decoding path as other tests and stays resilient to provider serialization changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e-encryption/encryption_test.go`:
- Around line 558-560: Replace the raw json.Unmarshal call with the shared
helper encoding.DecodeKMSConfig to decode provider payloads: instead of calling
json.Unmarshal(kmsProviderConfigData13, &providerConfig13) use
encoding.DecodeKMSConfig(kmsProviderConfigData13, &providerConfig13) and assert
no error (require.NoError on the DecodeKMSConfig call); keep the subsequent
require.Equal on providerConfig13.Vault.VaultAddress unchanged. This updates the
test to use the same decoding path as other tests and stays resilient to
provider serialization changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc4000d9-f95c-4399-b8d7-d62f35f54adb
📒 Files selected for processing (11)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptiondata/secret.gopkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/state/types.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.go
✅ Files skipped from review due to trivial changes (5)
- pkg/operator/encryption/state/types.go
- pkg/operator/encryption/encryptiondata/config.go
- pkg/operator/encryption/controllers/key_controller.go
- pkg/operator/encryption/secrets/secrets_test.go
- pkg/operator/encryption/kms/helpers_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/key_controller_test.go (1)
670-673: 💤 Low valueMinor: Error message says "encode" but operation is "decode".
The error message at line 672 says "failed to encode" but
DecodeKMSConfigis being called. Same issue at line 715.Suggested fix
kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"] providerConfig, err := encoding.DecodeKMSConfig(kmsProviderConfigData) if err != nil { - ts.Fatalf("failed to encode KMS config: %v", err) + ts.Fatalf("failed to decode KMS config: %v", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/key_controller_test.go` around lines 670 - 673, The test uses encoding.DecodeKMSConfig but the ts.Fatalf message incorrectly says "failed to encode KMS config"; update the error messages for the DecodeKMSConfig calls (the ts.Fatalf around providerConfig, err := encoding.DecodeKMSConfig(...)) to say "failed to decode KMS config: %v" so the log matches the operation (repeat the same fix for the second DecodeKMSConfig occurrence in the file).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller_test.go`:
- Around line 670-673: The test uses encoding.DecodeKMSConfig but the ts.Fatalf
message incorrectly says "failed to encode KMS config"; update the error
messages for the DecodeKMSConfig calls (the ts.Fatalf around providerConfig, err
:= encoding.DecodeKMSConfig(...)) to say "failed to decode KMS config: %v" so
the log matches the operation (repeat the same fix for the second
DecodeKMSConfig occurrence in the file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e105136c-78f0-4611-8355-8d4e8ab5c4c0
📒 Files selected for processing (5)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gotest/e2e-encryption/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/encryption/kms/helpers.go
- pkg/operator/encryption/kms/helpers_test.go
Member
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
Member
Author
Contributor
|
@ardaguclu: all tests passed! 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. |
p0lyn0mial
reviewed
May 7, 2026
| // However, we need to create a new key if migration-triggering fields | ||
| // in the KMS provider configuration have changed. | ||
| if kms.NeedsNewKey(latestKey.KMSConfig.Provider, currentApiServerEncryption.KMS) { | ||
| return latestKeyID, "kms-provider-changed", true |
Contributor
There was a problem hiding this comment.
how could we e2e test this change ? :)
Member
Author
There was a problem hiding this comment.
We'll update kms plugin deployment in library-go with the fake Vault kms plugin. We'll have actual Vault kms plugin coming from stepregistry. We'll move from one to another :)
Member
Author
There was a problem hiding this comment.
ah, there is simpler way: We have already run multiple kms plugins (km-1.sock, kms-2.sock, etc.). We'll just migrate from one to another.
p0lyn0mial
reviewed
May 7, 2026
| // in a way that requires creating a new encryption key and migrating storage. | ||
| // Only fields that affect the Key Encryption Key (KEK) trigger migration. | ||
| // Fields like KMSPluginImage, TLS, and Authentication are non-migration fields. | ||
| func NeedsNewKey(latest, current *configv1.KMSConfig) bool { |
Contributor
There was a problem hiding this comment.
i think this could be internal to the key_controller.
won't be used by the preflight-checker.
the key_controller will drive the preflight-checker
Member
Author
There was a problem hiding this comment.
That makes sense. So NeedsNewKey will return true, key_controller interacts with pre-flight checker.
p0lyn0mial
reviewed
May 7, 2026
| // in a way that requires creating a new encryption key and migrating storage. | ||
| // Only fields that affect the Key Encryption Key (KEK) trigger migration. | ||
| // Fields like KMSPluginImage, TLS, and Authentication are non-migration fields. | ||
| func NeedsNewKey(latest, current *configv1.KMSConfig) bool { |
Contributor
There was a problem hiding this comment.
are we missing a nil check for both params?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR introduces KMS to KMS migration described in https://github.com/openshift/enhancements/blob/master/enhancements/kube-apiserver/kms-encryption-foundations.md. KMS to KMS migration is triggered only when predefined set of fields are updated.
This PR paves the way for pre-flight checker. So that preflight-checker can be integrated to key controller.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests