NO-JIRA: Add test for KMS to KMS migration#938
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request explicitly references no jira issue. 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. |
WalkthroughAdds a new e2e test suite for migrating token encryption between two distinct Vault KMS providers, registers it as a separate serial test suite ( ChangesKMS-to-KMS migration test
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test as testKMSEncryptionKMSToKMSMigration
participant Library as library.TestEncryptionProvidersMigration
participant PrimaryKMS as Primary Vault KMS
participant SecondaryKMS as Secondary Vault KMS
Test->>Test: create TokenOfLife resource
Test->>Test: shuffle provider order
Test->>Library: migrate to PrimaryKMS
Library->>PrimaryKMS: apply encryption config
Library->>Test: assert token encrypted
Test->>Library: migrate to SecondaryKMS
Library->>SecondaryKMS: apply encryption config
Library->>Test: assert token encrypted
Test->>Library: switch to identity/off mode
Library->>Test: assert token unencrypted
🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 63-68: The helper comment for testKMSEncryptionKMSToKMSMigration
only describes the default-to-secondary path, but the provider order is
randomized in the test flow, so it must reflect both possible migration
directions. Update the comment near testKMSEncryptionKMSToKMSMigration to
describe that the test may run default -> secondary or secondary -> default
after the shuffle, while keeping the step summary accurate and contextual.
- Around line 80-82: The CreateResourceFunc closure is ignoring the
already-available ctx by calling CreateAndStoreTokenOfLife with context.TODO(),
which drops cancellation and timeout propagation. Update the CreateResourceFunc
in the encryption_kms test to pass the captured ctx through to
operatorencryption.CreateAndStoreTokenOfLife, keeping the existing
operatorencryption.GetClients(t) call intact. This change should be made
wherever the token creation closure is defined so the context flows consistently
through the test helper.
🪄 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: 47dacfab-7878-4d07-8087-aa0def91fc83
📒 Files selected for processing (1)
test/e2e-encryption-kms/encryption_kms.go
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | ||
| return operatorencryption.CreateAndStoreTokenOfLife(context.TODO(), t, operatorencryption.GetClients(t)) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate ctx instead of hardcoding context.TODO().
This closure already has ctx in scope, so using context.TODO() here drops cancellation/timeout propagation for the token creation call.
Suggested fix
CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object {
- return operatorencryption.CreateAndStoreTokenOfLife(context.TODO(), t, operatorencryption.GetClients(t))
+ return operatorencryption.CreateAndStoreTokenOfLife(ctx, t, operatorencryption.GetClients(t))
},As per coding guidelines, "Propagate context.Context... Never discard a context or substitute context.Background()/context.TODO() when a context is already available." As per path instructions, **/*.go: "context.Context for cancellation and timeouts".
📝 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.
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | |
| return operatorencryption.CreateAndStoreTokenOfLife(context.TODO(), t, operatorencryption.GetClients(t)) | |
| }, | |
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | |
| return operatorencryption.CreateAndStoreTokenOfLife(ctx, t, operatorencryption.GetClients(t)) | |
| }, |
🤖 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-kms/encryption_kms.go` around lines 80 - 82, The
CreateResourceFunc closure is ignoring the already-available ctx by calling
CreateAndStoreTokenOfLife with context.TODO(), which drops cancellation and
timeout propagation. Update the CreateResourceFunc in the encryption_kms test to
pass the captured ctx through to operatorencryption.CreateAndStoreTokenOfLife,
keeping the existing operatorencryption.GetClients(t) call intact. This change
should be made wherever the token creation closure is defined so the context
flows consistently through the test helper.
Sources: Coding guidelines, Path instructions
ardaguclu
force-pushed
the
kms-to-kms-e2e
branch
from
9d77502 to
eb9e044
Compare
openshift-ci
Bot
removed
the
do-not-merge/hold
openshift-ci-robot
added
the
verified
|
@ardaguclu: This PR has been marked as verified by 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
ardaguclu
force-pushed
the
kms-to-kms-e2e
branch
from
eb9e044 to
c8a93ef
Compare
openshift-ci-robot
removed
the
verified
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@ardaguclu: The 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. |
ardaguclu
force-pushed
the
kms-to-kms-e2e
branch
from
c8a93ef to
7b6bae7
Compare
openshift-ci-robot
removed
the
verified
|
@ardaguclu: This pull request explicitly references no jira issue. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/e2e-encryption-kms/encryption_kms_to_kms.go`:
- Around line 21-27: The helper comment for testKMSEncryptionKMSToKMSMigration
is outdated and incorrectly mentions “KMS and AES providers” even though the
test only shuffles the default and secondary Vault KMS providers. Update the
comment near testKMSEncryptionKMSToKMSMigration to accurately describe the two
KMS providers and keep the step list consistent with the actual migration flow,
without changing the test logic.
🪄 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: 8390ff7c-e305-4235-a6a9-c622183a39ea
📒 Files selected for processing (3)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-encryption-kms/encryption_kms.gotest/e2e-encryption-kms/encryption_kms_to_kms.go
✅ Files skipped from review due to trivial changes (1)
- cmd/cluster-authentication-operator-tests-ext/main.go
| // testKMSEncryptionKMSToKMSMigration tests migration between two distinct KMS providers | ||
| // (default Vault instance and secondary Vault instance). | ||
| // This test: | ||
| // 1. Shuffles the KMS and AES providers to create a randomized migration order | ||
| // 2. Migrates between the providers in the shuffled order | ||
| // 3. Verifies token is correctly encrypted after each migration | ||
| // 4. Switches to identity (off) to verify the resource is re-written unencrypted |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the helper comment to describe two KMS providers.
Lines 24-25 still say “KMS and AES providers”, but this scenario only shuffles the default and secondary Vault KMS providers, so the comment now misstates the test flow.
Suggested fix
-// 1. Shuffles the KMS and AES providers to create a randomized migration order
+// 1. Shuffles the two KMS providers to create a randomized migration orderAs per coding guidelines, "Comments should be meaningful and add context and/or explain choices that cannot be expressed through clear code."
📝 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.
| // testKMSEncryptionKMSToKMSMigration tests migration between two distinct KMS providers | |
| // (default Vault instance and secondary Vault instance). | |
| // This test: | |
| // 1. Shuffles the KMS and AES providers to create a randomized migration order | |
| // 2. Migrates between the providers in the shuffled order | |
| // 3. Verifies token is correctly encrypted after each migration | |
| // 4. Switches to identity (off) to verify the resource is re-written unencrypted | |
| // testKMSEncryptionKMSToKMSMigration tests migration between two distinct KMS providers | |
| // (default Vault instance and secondary Vault instance). | |
| // This test: | |
| // 1. Shuffles the two KMS providers to create a randomized migration order | |
| // 2. Migrates between the providers in the shuffled order | |
| // 3. Verifies token is correctly encrypted after each migration | |
| // 4. Switches to identity (off) to verify the resource is re-written unencrypted |
🤖 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-kms/encryption_kms_to_kms.go` around lines 21 - 27, The
helper comment for testKMSEncryptionKMSToKMSMigration is outdated and
incorrectly mentions “KMS and AES providers” even though the test only shuffles
the default and secondary Vault KMS providers. Update the comment near
testKMSEncryptionKMSToKMSMigration to accurately describe the two KMS providers
and keep the step list consistent with the actual migration flow, without
changing the test logic.
Source: Coding guidelines
|
/verified bypass |
|
@ardaguclu: The 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. |
openshift-ci-robot
added
the
verified
openshift-ci-robot
removed
the
verified
|
@ardaguclu: This pull request explicitly references no jira issue. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/e2e-encryption-kms/encryption_kms_2.go`:
- Around line 28-41: `testKMSEncryptionKMSToKMSMigration` is discarding its
available context inside `CreateResourceFunc` by calling `context.TODO()`
instead of reusing `ctx`. Update the closure to pass the captured `ctx` through
to `operatorencryption.CreateAndStoreTokenOfLife`, keeping context propagation
consistent with the rest of the function and avoiding a new background context.
🪄 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: 77e1e2e0-a006-4b81-b8b1-af1d7ce435a2
📒 Files selected for processing (3)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-encryption-kms/encryption_kms.gotest/e2e-encryption-kms/encryption_kms_2.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e-encryption-kms/encryption_kms.go
| func testKMSEncryptionKMSToKMSMigration(ctx context.Context, t testing.TB) { | ||
| library.TestEncryptionProvidersMigration(ctx, t, library.ProvidersMigrationScenario{ | ||
| BasicScenario: library.BasicScenario{ | ||
| Namespace: "openshift-config-managed", | ||
| LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", | ||
| EncryptionConfigSecretName: "encryption-config-openshift-oauth-apiserver", | ||
| EncryptionConfigSecretNamespace: "openshift-config-managed", | ||
| OperatorNamespace: "openshift-authentication-operator", | ||
| TargetGRs: operatorencryption.DefaultTargetGRs, | ||
| AssertFunc: operatorencryption.AssertTokens, | ||
| }, | ||
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | ||
| return operatorencryption.CreateAndStoreTokenOfLife(context.TODO(), t, operatorencryption.GetClients(t)) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Discarding the passed-in ctx in favor of context.TODO().
testKMSEncryptionKMSToKMSMigration receives ctx (Line 28) and correctly forwards it to the KMS provider constructors (Line 47-48), but CreateResourceFunc discards it and calls context.TODO() instead, even though ctx is captured in the closure's scope.
🔧 Proposed fix
CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object {
- return operatorencryption.CreateAndStoreTokenOfLife(context.TODO(), t, operatorencryption.GetClients(t))
+ return operatorencryption.CreateAndStoreTokenOfLife(ctx, t, operatorencryption.GetClients(t))
},As per coding guidelines, "When a function accepts or has access to a context.Context, pass it through to downstream calls that accept one. Never discard a context or substitute context.Background()/context.TODO() when a context is already available."
📝 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.
| func testKMSEncryptionKMSToKMSMigration(ctx context.Context, t testing.TB) { | |
| library.TestEncryptionProvidersMigration(ctx, t, library.ProvidersMigrationScenario{ | |
| BasicScenario: library.BasicScenario{ | |
| Namespace: "openshift-config-managed", | |
| LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", | |
| EncryptionConfigSecretName: "encryption-config-openshift-oauth-apiserver", | |
| EncryptionConfigSecretNamespace: "openshift-config-managed", | |
| OperatorNamespace: "openshift-authentication-operator", | |
| TargetGRs: operatorencryption.DefaultTargetGRs, | |
| AssertFunc: operatorencryption.AssertTokens, | |
| }, | |
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | |
| return operatorencryption.CreateAndStoreTokenOfLife(context.TODO(), t, operatorencryption.GetClients(t)) | |
| }, | |
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | |
| return operatorencryption.CreateAndStoreTokenOfLife(ctx, t, operatorencryption.GetClients(t)) | |
| }, |
🤖 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-kms/encryption_kms_2.go` around lines 28 - 41,
`testKMSEncryptionKMSToKMSMigration` is discarding its available context inside
`CreateResourceFunc` by calling `context.TODO()` instead of reusing `ctx`.
Update the closure to pass the captured `ctx` through to
`operatorencryption.CreateAndStoreTokenOfLife`, keeping context propagation
consistent with the rest of the function and avoiding a new background context.
Source: Coding guidelines
|
/retest |
|
/test e2e-aws-operator-encryption-kms-2 |
|
/retest |
openshift-ci-robot
added
the
verified
|
@ardaguclu: This PR has been marked as verified by 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
added
the
approved
|
/retest |
|
@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. |
4 participants
This PR adds new test for kms to kms migration.
Summary by CodeRabbit
New Features
Tests