Skip to content

fake bump kmstokms test #712

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

Draft
gangwgr wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fake bump kmstokms test #712

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
3 changes: 3 additions & 0 deletions _output/golang-versions
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
1.25
1.25
1.25
3 changes: 3 additions & 0 deletions _output/named-golang-versions
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.ci-operator.yaml: 1.25
go.mod: 1.25
Dockerfile: 1.25
2 changes: 2 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,5 @@ require (

// This replace is required for we use the OCP fork of Ginkgo.
replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20260303184444-1cc650aa0565

replace github.com/openshift/library-go => github.com/ardaguclu/library-go v0.0.0-20260618055608-feb27829a74c
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ github.com/NYTimes/gziphandler v1.1.1 h1:ZUDjpQae29j0ryrS0u/B8HZfJBtBQHjqw2rQ2cq
github.com/NYTimes/gziphandler v1.1.1/go.mod h1:n/CVRwUEOgIxrgPvAQhUUr9oeUtvrhMomdKFjzJNB0c=
github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI=
github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g=
github.com/ardaguclu/library-go v0.0.0-20260618055608-feb27829a74c h1:wCyA9uDne5jqE8Lv7p6Gk2tHyTfpJ+7weCCnHPZcr4I=
github.com/ardaguclu/library-go v0.0.0-20260618055608-feb27829a74c/go.mod h1:/HBhy6jm/igWI3Y1vYFwFG3ZCcXmnNsKUT6VBpPyM9A=
github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM=
Expand Down Expand Up @@ -174,8 +176,6 @@ github.com/openshift/build-machinery-go v0.0.0-20251023084048-5d77c1a5e5af h1:Ui
github.com/openshift/build-machinery-go v0.0.0-20251023084048-5d77c1a5e5af/go.mod h1:8jcm8UPtg2mCAsxfqKil1xrmRMI3a+XU2TZ9fF8A7TE=
github.com/openshift/client-go v0.0.0-20260512113608-deb4dc54551a h1:EKx2XhOKehd1C5ptY7IrLl4WV35E8kP0pRPnG5BUZXk=
github.com/openshift/client-go v0.0.0-20260512113608-deb4dc54551a/go.mod h1:V933kvY/cb/Un7UCEOhXHUySNX327u7Epe8g9KNqg2Q=
github.com/openshift/library-go v0.0.0-20260615113748-bc9d4056464b h1:fmTTzoHLhRf4QsMKw4cEbnvUeUcgcaNesh5mueWnaVs=
github.com/openshift/library-go v0.0.0-20260615113748-bc9d4056464b/go.mod h1:/HBhy6jm/igWI3Y1vYFwFG3ZCcXmnNsKUT6VBpPyM9A=
github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20260303184444-1cc650aa0565 h1:3/q8qM4HbFa+Een8wgzpwO8W6mO7Po+MwY6uxiXi/ac=
github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20260303184444-1cc650aa0565/go.mod h1:ArE1D/XhNXBXCBkKOLkbsb2c81dQHCRcF5zwn/ykDRo=
github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde/go.mod h1:nZgzbfBr3hhjoZnS66nKrHmduYNpc34ny7RK4z5/HM0=
Expand Down
78 changes: 74 additions & 4 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 @@ -19,12 +19,20 @@ import (
)

var _ = g.Describe("[sig-openshift-apiserver] cluster-openshift-apiserver-operator", func() {
g.It("TestKMSEncryptionOnOff [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) {
testKMSEncryptionOnOff(ctx, g.GinkgoTB())
//g.It("TestKMSEncryptionOnOff [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) {
// testKMSEncryptionOnOff(ctx, g.GinkgoTB())
//})

//g.It("TestKMSEncryptionProvidersMigration [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) {
// testKMSEncryptionProvidersMigration(ctx, g.GinkgoTB())
//})

g.It("TestKMSToKMSMigration [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) {
testKMSToKMSMigration(ctx, g.GinkgoTB())
})

g.It("TestKMSEncryptionProvidersMigration [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) {
testKMSEncryptionProvidersMigration(ctx, g.GinkgoTB())
g.It("TestKMSToKMSOnOff [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) {
testKMSToKMSOnOff(ctx, g.GinkgoTB())
})
})

Expand Down Expand Up @@ -108,3 +116,65 @@ func testKMSEncryptionProvidersMigration(ctx context.Context, t testing.TB) {
}),
})
}

// testKMSToKMSMigration tests KMS-to-KMS migration (primary → secondary → primary → identity).
func testKMSToKMSMigration(ctx context.Context, t testing.TB) {
cs := operatorencryption.GetClients(t)

ns := fmt.Sprintf("test-kms-to-kms-migration-%d", rand.IntN(4))
_, err := cs.KubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{})
Comment on lines +124 to +125

@coderabbitai coderabbitai Bot Jun 16, 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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Increase namespace entropy to avoid test collisions.

Line 124 and Line 155 use rand.IntN(4), which gives only 4 possible namespace names; this is prone to AlreadyExists flakes across reruns/shards.

💡 Suggested fix 
- ns := fmt.Sprintf("test-kms-to-kms-migration-%d", rand.IntN(4))
+ ns := fmt.Sprintf("test-kms-to-kms-migration-%d", rand.Uint64())

- ns := fmt.Sprintf("test-kms-to-kms-onoff-%d", rand.IntN(4))
+ ns := fmt.Sprintf("test-kms-to-kms-onoff-%d", rand.Uint64())

Also applies to: 155-156

🤖 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 124 - 125, The
namespace creation in test/e2e-encryption-kms/encryption_kms.go at lines 124-125
(anchor location) uses rand.IntN(4) which provides only 4 possible namespace
names, causing test collisions on reruns and shards. The same issue exists at
lines 155-156 (sibling location). Replace rand.IntN(4) with rand.IntN(100000) at
both locations in the fmt.Sprintf calls that generate the namespace names to
significantly increase entropy and prevent AlreadyExists flakes.

require.NoError(t, err)
defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})

@coderabbitai coderabbitai Bot Jun 16, 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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate the file and check its content around the specified lines
wc -l test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 136

🏁 Script executed:

# Read the file around lines 127 and 158 to verify the issue
sed -n '120,165p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3107

🏁 Script executed:

# Check the imports in the file to see if require is available
head -30 test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 1260

🏁 Script executed:

# Check for other Delete() calls that might have similar issues
grep -n "\.Delete(" test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 436

🏁 Script executed:

# Also search for similar patterns with deferred operations that might ignore errors
grep -n "defer.*\\.Delete\\|defer.*\\.Delete(" test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 436

Fix ignored errors in deferred namespace cleanup across all instances.

Lines 57, 94, 127, and 158 all ignore errors from Namespaces().Delete() in deferred calls. Cleanup errors can cause leaked namespaces and cascade into subsequent e2e test failures. Per Go coding guidelines, error returns must never be ignored.

🛠️ Suggested fix 
- defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
+ defer func() {
+ 	err := cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
+ 	require.NoError(t, err)
+ }()

Apply this pattern to all four occurrences (lines 57, 94, 127, and 158).

📝 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
defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
defer func() {
err := cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
require.NoError(t, 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 `@test/e2e-encryption-kms/encryption_kms.go` at line 127, The deferred
`Namespaces().Delete()` calls at lines 57, 94, 127, and 158 in the
encryption_kms.go file all ignore the error return value, which can cause leaked
namespaces and cascade test failures. Modify each of the four deferred cleanup
calls to capture the error returned from `Delete()` and log or handle it
appropriately rather than ignoring it. Apply the same error handling pattern
consistently across all four locations where the deferred namespace deletion
occurs.

Source: Coding guidelines


library.TestKMSToKMSMigration(ctx, t, library.KMSToKMSMigrationScenario{
BasicScenario: library.BasicScenario{
Namespace: operatorclient.GlobalMachineSpecifiedConfigNamespace,
LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + operatorclient.TargetNamespace,
EncryptionConfigSecretName: fmt.Sprintf("encryption-config-%s", operatorclient.TargetNamespace),
EncryptionConfigSecretNamespace: operatorclient.GlobalMachineSpecifiedConfigNamespace,
OperatorNamespace: operatorclient.OperatorNamespace,
TargetGRs: operatorencryption.DefaultTargetGRs,
AssertFunc: operatorencryption.AssertRoutes,
},
CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object {
return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
},
Comment on lines +140 to +141

@coderabbitai coderabbitai Bot Jun 16, 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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd -t f encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 132

🏁 Script executed:

wc -l test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 136

🏁 Script executed:

sed -n '130,180p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3466

🏁 Script executed:

sed -n '100,145p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3141

🏁 Script executed:

sed -n '155,180p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 1880

🏁 Script executed:

sed -n '70,130p' test/e2e-encryption-kms/encryption_kms.go | head -70

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3774

Replace context.TODO() with the ctx parameter passed to the enclosing test functions.

Lines 140 and 171 use context.TODO() within closures where ctx is accessible via closure capture from the surrounding function scope. This prevents proper cancellation and timeout propagation as required by Go security guidelines.

Suggested fixes

Line 140:

- return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
+ return operatorencryption.CreateAndStoreRouteOfLife(ctx, t, operatorencryption.GetClients(t), ns)

Line 171:

- return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
+ return operatorencryption.CreateAndStoreRouteOfLife(ctx, t, operatorencryption.GetClients(t), ns)
📝 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
return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
},
return operatorencryption.CreateAndStoreRouteOfLife(ctx, t, operatorencryption.GetClients(t), ns)
},
🤖 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 140 - 141, Replace
all instances of context.TODO() with the ctx parameter that is accessible via
closure capture from the enclosing test function scope. In the
operatorencryption.CreateAndStoreRouteOfLife call and any other similar calls
within the test setup closures, use the ctx parameter passed to the test
function instead of context.TODO() to ensure proper cancellation and timeout
propagation as required by Go security guidelines.

Source: Coding guidelines

AssertResourceEncryptedFunc: operatorencryption.AssertRouteOfLifeEncrypted,
AssertResourceNotEncryptedFunc: operatorencryption.AssertRouteOfLifeNotEncrypted,
ResourceFunc: func(t testing.TB, _ string) runtime.Object { return operatorencryption.RouteOfLife(t, ns) },
ResourceName: "RouteOfLife",
PrimaryProvider: librarykms.DefaultVaultEncryptionProvider(ctx, t),
SecondaryProvider: librarykms.SecondaryVaultEncryptionProvider(ctx, t),
})
}

// testKMSToKMSOnOff tests KMS on/off cycle with two distinct KMS providers.
func testKMSToKMSOnOff(ctx context.Context, t testing.TB) {
cs := operatorencryption.GetClients(t)

ns := fmt.Sprintf("test-kms-to-kms-onoff-%d", rand.IntN(4))
_, err := cs.KubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{})
require.NoError(t, err)
defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})

library.TestKMSToKMSOnOff(ctx, t, library.KMSToKMSMigrationScenario{
BasicScenario: library.BasicScenario{
Namespace: operatorclient.GlobalMachineSpecifiedConfigNamespace,
LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + operatorclient.TargetNamespace,
EncryptionConfigSecretName: fmt.Sprintf("encryption-config-%s", operatorclient.TargetNamespace),
EncryptionConfigSecretNamespace: operatorclient.GlobalMachineSpecifiedConfigNamespace,
OperatorNamespace: operatorclient.OperatorNamespace,
TargetGRs: operatorencryption.DefaultTargetGRs,
AssertFunc: operatorencryption.AssertRoutes,
},
CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object {
return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
},
AssertResourceEncryptedFunc: operatorencryption.AssertRouteOfLifeEncrypted,
AssertResourceNotEncryptedFunc: operatorencryption.AssertRouteOfLifeNotEncrypted,
ResourceFunc: func(t testing.TB, _ string) runtime.Object { return operatorencryption.RouteOfLife(t, ns) },
ResourceName: "RouteOfLife",
PrimaryProvider: librarykms.DefaultVaultEncryptionProvider(ctx, t),
SecondaryProvider: librarykms.SecondaryVaultEncryptionProvider(ctx, t),
})
}
76 changes: 61 additions & 15 deletions vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading