Skip to content

NO-JIRA: Add test for KMS to KMS migration #938

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.

Sign up for GitHub

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

Merged
openshift-merge-bot merged 1 commit into from
Jul 2, 2026
+67 −7
Merged

NO-JIRA: Add test for KMS to KMS migration #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion cmd/cluster-authentication-operator-tests-ext/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,16 @@ func prepareOperatorTestsRegistry() (*oteextension.Registry, error) {
Name: "openshift/cluster-authentication-operator/encryption-kms",
Parallelism: 1,
Qualifiers: []string{
`name.contains("KMSEncryption")`,
`name.contains("KMSEncryption") && !name.contains("[Suite:encryption-kms-2]")`,
},
})

extension.AddSuite(oteextension.Suite{
Name: "openshift/cluster-authentication-operator/encryption-kms-2",
Parents: []string{"openshift/kms"},
Parallelism: 1,
Qualifiers: []string{
`name.contains("[Suite:encryption-kms-2]")`,
},
})

Expand Down
12 changes: 6 additions & 6 deletions test/e2e-encryption-kms/encryption_kms.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ var _ = g.Describe("[sig-auth] cluster-authentication-operator", func() {
g.It("TestKMSEncryptionProvidersMigration [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) {
testKMSEncryptionProvidersMigration(ctx, g.GinkgoTB())
})

})

// testKMSEncryptionOnOff tests KMS encryption on/off cycle.
Expand Down Expand Up @@ -58,12 +59,11 @@ func testKMSEncryptionOnOff(ctx context.Context, t testing.TB) {

// testKMSEncryptionProvidersMigration tests migration between KMS and AES encryption providers.
// This test:
// 1. Deploys the real Vault KMS plugin
// 2. Creates a test OAuth access token (TokenOfLife)
// 3. Randomly picks one AES encryption provider (AESGCM or AESCBC)
// 4. Shuffles the selected AES provider with KMS to create a randomized migration order
// 5. Migrates between the providers in the shuffled order
// 6. Verifies token is correctly encrypted after each migration
// 1. Creates a test OAuth access token (TokenOfLife)
// 2. Randomly picks one AES encryption provider (AESGCM or AESCBC)
// 3. Shuffles the selected AES provider with KMS to create a randomized migration order
// 4. Migrates between the providers in the shuffled order
// 5. Verifies token is correctly encrypted after each migration
func testKMSEncryptionProvidersMigration(ctx context.Context, t testing.TB) {
library.TestEncryptionProvidersMigration(ctx, t, library.ProvidersMigrationScenario{
BasicScenario: library.BasicScenario{
Expand Down
51 changes: 51 additions & 0 deletions test/e2e-encryption-kms/encryption_kms_2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package e2e_encryption_kms

import (
"context"
"testing"

g "github.com/onsi/ginkgo/v2"
"k8s.io/apimachinery/pkg/runtime"

operatorencryption "github.com/openshift/cluster-authentication-operator/test/library/encryption"
library "github.com/openshift/library-go/test/library/encryption"
librarykms "github.com/openshift/library-go/test/library/encryption/kms"
)

var _ = g.Describe("[sig-auth] cluster-authentication-operator", func() {
g.It("TestKMSEncryptionKMSToKMSMigration [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m][Suite:encryption-kms-2]", func(ctx context.Context) {
testKMSEncryptionKMSToKMSMigration(ctx, g.GinkgoTB())
})
})

// 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
Comment on lines +21 to +27

@coderabbitai coderabbitai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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 order

As 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.

Suggested change
// 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

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))
},
Comment on lines +28 to +41

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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

AssertResourceEncryptedFunc: operatorencryption.AssertTokenOfLifeEncrypted,
AssertResourceNotEncryptedFunc: operatorencryption.AssertTokenOfLifeNotEncrypted,
ResourceFunc: func(t testing.TB, _ string) runtime.Object { return operatorencryption.TokenOfLife(t) },
ResourceName: "TokenOfLife",
EncryptionProviders: library.ShuffleEncryptionProviders([]library.EncryptionProvider{
librarykms.DefaultVaultEncryptionProvider(ctx, t),
librarykms.SecondaryVaultEncryptionProvider(ctx, t),
}),
})
}