NO-JIRA: add more rotation tests scenarios - #2149
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tjungblu: 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. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
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:
WalkthroughAdds two cluster-operator test helpers, redirects ChangesEncryption rotation tests and operator helpers
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant rotationScenario
participant operatorencryption
participant KubeAPIServerOperator
TestRunner->>rotationScenario: start rotation test
rotationScenario->>operatorencryption: UnsupportedConfigFunc -> UpdateUnsupportedConfig(raw)
operatorencryption->>KubeAPIServerOperator: Get Operator / Update Spec.UnsupportedConfigOverrides.Raw
rotationScenario->>operatorencryption: GetOperatorConditionsFunc -> GetClusterOperatorConditions()
operatorencryption->>KubeAPIServerOperator: Get Operator -> return Status.Conditions
rotationScenario->>TestRunner: complete with pass/fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@go.mod`:
- Line 140: The go.mod currently contains a replace directive pointing
github.com/openshift/library-go to a personal fork
(github.com/tjungblu/library-go) — remove that replace directive and restore
usage of the official module (github.com/openshift/library-go) by either
reverting the replace line or replacing it with the appropriate upstream
version/tag once PR `#2218` is merged (or choose a stable upstream version if the
PR won’t be accepted); after updating go.mod, run dependency cleanup (e.g., go
mod tidy) to refresh go.sum and ensure the module graph is consistent.
🪄 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: 620b9dd7-48ee-4982-865e-a199272eedf3
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/migration_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modtest/e2e-encryption-rotation/encryption_rotation_test.gotest/library/encryption/helpers.go
|
|
||
| replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20251001123353-fd5b1fb35db1 | ||
|
|
||
| replace github.com/openshift/library-go => github.com/tjungblu/library-go v0.0.0-20260512113548-2e4a77cb3139 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Has https://github.com/openshift/library-go/pull/2218 merged, and what upstream commit/tag contains those changes so this replace can be removed?
💡 Result:
No evidence found that openshift/library-go#2218 has merged or exists. Searches across GitHub, OpenShift repos, and web did not return the specific PR page, merge commit, or references to it being merged [1][2][3]. The library-go repo was last pushed on 2026-03-26 [1], but no mention of PR 2218 in recent bumps like #2076 [3] or others [4][5][6]. Without the PR merged into master, there is no upstream commit or tag containing those changes, so any replace directive cannot be removed yet. Check the PR status directly on GitHub or if the number is correct.
Citations:
- 1: https://github.com/openshift/library-go
- 2: OCPBUGS-84536: Bump library-go #2131
- 3: OCPBUGS-65626: update library-go to latest #2026
- 4: OCPBUGS-70323: bump library-go #1986
- 5: OCPBUGS-60045: Bump library-go to latest master #1928
- 6: openshift/cluster-storage-operator@14c5f71
Remove the personal-fork module replace before merging.
Line 140 pins github.com/openshift/library-go to a personal fork (github.com/tjungblu/library-go), which weakens dependency provenance and can hurt reproducibility if that fork or commit moves or is removed. The referenced upstream PR (#2218) does not appear to have been merged yet. Switch back to an official github.com/openshift/library-go version once the upstream PR is merged, or identify an alternative stable upstream version to use instead.
🤖 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 `@go.mod` at line 140, The go.mod currently contains a replace directive
pointing github.com/openshift/library-go to a personal fork
(github.com/tjungblu/library-go) — remove that replace directive and restore
usage of the official module (github.com/openshift/library-go) by either
reverting the replace line or replacing it with the appropriate upstream
version/tag once PR `#2218` is merged (or choose a stable upstream version if the
PR won’t be accepted); after updating go.mod, run dependency cleanup (e.g., go
mod tidy) to refresh go.sum and ensure the module graph is consistent.
070c3a4 to
0d620d6
Compare
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
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-rotation/encryption_rotation_test.go`:
- Line 35: The test is passing the wrong type to the EncryptionProvider field —
replace uses of configv1.EncryptionType(*provider) with
configv1.APIServerEncryption(*provider) so the field receives the expected type;
update both occurrences where EncryptionProvider is set (the earlier two
instances that mirror the correct pattern used later) to use
configv1.APIServerEncryption to match the working example in the file.
🪄 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: 28136918-5eb8-4290-9043-b947ce3bcc5a
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/test/library/encryption/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/kms/k8s_mock_kms_plugin_deployer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modtest/e2e-encryption-rotation/encryption_rotation_test.gotest/library/encryption/helpers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- test/library/encryption/helpers.go
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
from library-go openshift/library-go#2218 Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
|
@tjungblu: 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
go.mod (1)
140-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the personal-fork
library-goreplace before merge.Line 140 redirects to
github.com/tjungblu/library-go, which reduces supply-chain provenance and release reproducibility for this repo. Please switch back to the officialgithub.com/openshift/library-gosource once the upstream change is available.Has https://github.com/openshift/library-go/pull/2218 merged, and what official github.com/openshift/library-go commit/tag should be used in go.mod instead of a fork replace?🤖 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 `@go.mod` at line 140, The go.mod currently contains a replace directive pointing to a personal fork (replace github.com/openshift/library-go => github.com/tjungblu/library-go ...); remove that forked replace before merge and restore the official module path (github.com/openshift/library-go) by either deleting the replace line or updating it to the official upstream commit/tag once the upstream PR (e.g. openshift/library-go#2218) has merged; locate the replace entry in go.mod and replace or remove it so the module resolution uses the canonical github.com/openshift/library-go source and update the go.sum via `go mod tidy` afterwards.
🤖 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-rotation/encryption_rotation_test.go`:
- Around line 41-60: This test (TestEncryptionRotationDuringFirstMigration)
calls GetClusterOperatorConditions via
library.TestEncryptionRotationDuringFirstMigration and must be guarded from
running on MicroShift; add the standard MicroShift protection by either tagging
the test with the API-specific tag/[Skipped:MicroShift] or performing a runtime
skip at the top of TestEncryptionRotationDuringFirstMigration (e.g., call
exutil.IsMicroShiftCluster() and t.Skipf when true) so ClusterOperator-dependent
logic is not executed on MicroShift, and apply the same change to the other
related test block (the similar test around lines 64-83) that also uses
GetClusterOperatorConditions.
In `@test/library/encryption/helpers.go`:
- Around line 93-94: Wrap the single operatorClient.Update call in a
retry.RetryOnConflict loop using retry.DefaultBackoff from
k8s.io/client-go/util/retry to handle update conflicts; inside the closure call
operatorClient.Update(ctx, apiServerOperator, metav1.UpdateOptions{}), and on
conflict fetch the latest apiServerOperator (via operatorClient.Get) and reapply
your changes before retrying so the update eventually succeeds. Ensure you
import k8s.io/client-go/util/retry and keep using metav1.UpdateOptions{} and the
same context (context.TODO or passed ctx).
---
Duplicate comments:
In `@go.mod`:
- Line 140: The go.mod currently contains a replace directive pointing to a
personal fork (replace github.com/openshift/library-go =>
github.com/tjungblu/library-go ...); remove that forked replace before merge and
restore the official module path (github.com/openshift/library-go) by either
deleting the replace line or updating it to the official upstream commit/tag
once the upstream PR (e.g. openshift/library-go#2218) has merged; locate the
replace entry in go.mod and replace or remove it so the module resolution uses
the canonical github.com/openshift/library-go source and update the go.sum via
`go mod tidy` afterwards.
🪄 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: 38d9f4e2-b382-46f2-b53c-6db3d659100e
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/test/library/encryption/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/kms/k8s_mock_kms_plugin_deployer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modtest/e2e-encryption-rotation/encryption_rotation_test.gotest/library/encryption/helpers.go
| func TestEncryptionRotationDuringFirstMigration(t *testing.T) { | ||
| library.TestEncryptionRotationDuringFirstMigration(t, library.RotationScenario{ | ||
| 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.AssertSecretsAndConfigMaps, | ||
| }, | ||
| CreateResourceFunc: operatorencryption.CreateAndStoreSecretOfLife, | ||
| GetRawResourceFunc: operatorencryption.GetRawSecretOfLife, | ||
| GetOperatorConditionsFunc: operatorencryption.GetClusterOperatorConditions, | ||
| UnsupportedConfigFunc: func(raw []byte) error { | ||
| return operatorencryption.UpdateUnsupportedConfig(t, raw) | ||
| }, | ||
| EncryptionProvider: configv1.APIServerEncryption{Type: configv1.EncryptionType(*provider)}, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Guard new rotation tests for MicroShift incompatibility.
These new tests depend on ClusterOperator conditions (GetClusterOperatorConditions), which are not available on MicroShift. Add the usual MicroShift protection (label/tag or runtime skip) so they don’t run in unsupported environments.
As per coding guidelines, “Flag tests referencing unavailable APIs: … ClusterOperator … Use [apigroup:...] tags for API-specific tests, add [Skipped:MicroShift] label, or use exutil.IsMicroShiftCluster() runtime checks.”
Also applies to: 64-83
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 56-56: : # github.com/openshift/cluster-kube-apiserver-operator/test/e2e-encryption [github.com/openshift/cluster-kube-apiserver-operator/test/e2e-encryption.test]
test/e2e-encryption/encryption_test.go:56:35: cannot use configv1.APIServerEncryption{…} (value of struct type "github.com/openshift/api/config/v1".APIServerEncryption) as "github.com/openshift/library-go/test/library/encryption".EncryptionProvider value in struct literal
(typecheck)
🤖 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-rotation/encryption_rotation_test.go` around lines 41 -
60, This test (TestEncryptionRotationDuringFirstMigration) calls
GetClusterOperatorConditions via
library.TestEncryptionRotationDuringFirstMigration and must be guarded from
running on MicroShift; add the standard MicroShift protection by either tagging
the test with the API-specific tag/[Skipped:MicroShift] or performing a runtime
skip at the top of TestEncryptionRotationDuringFirstMigration (e.g., call
exutil.IsMicroShiftCluster() and t.Skipf when true) so ClusterOperator-dependent
logic is not executed on MicroShift, and apply the same change to the other
related test block (the similar test around lines 64-83) that also uses
GetClusterOperatorConditions.
| _, err = operatorClient.Update(context.TODO(), apiServerOperator, metav1.UpdateOptions{}) | ||
| return err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether update helpers in this file already use retry-on-conflict.
rg -n -C2 'func UpdateUnsupportedConfig|RetryOnConflict|operatorClient\.Update\(' test/library/encryption/helpers.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 481
🏁 Script executed:
# Check the full file content to understand context and imports
head -30 test/library/encryption/helpers.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 939
🏁 Script executed:
# Look for all Update calls in the file
rg -n '\.Update\(' test/library/encryption/helpers.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 179
🏁 Script executed:
# Check if retry pattern is used elsewhere in the file or related test utilities
rg -n 'RetryOnConflict|retry\.Retry' test/library/encryption/helpers.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 67
🏁 Script executed:
# Search for similar update patterns in other test helper files
fd -e go -path "*/test/library/*" | head -10Repository: openshift/cluster-kube-apiserver-operator
Length of output: 254
🏁 Script executed:
# Check if k8s.io/client-go/util/retry is used anywhere in the codebase
rg -n 'client-go/util/retry|RetryOnConflict' --type goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 3188
🏁 Script executed:
# Look for similar Update patterns in other test files
rg -n -B2 'operatorClient.*Update' --type go test/Repository: openshift/cluster-kube-apiserver-operator
Length of output: 813
🏁 Script executed:
# Check if there are any retry patterns in the broader codebase
rg -n 'retry\.Retry|retry\.DefaultRetry' --type go | head -20Repository: openshift/cluster-kube-apiserver-operator
Length of output: 2696
🏁 Script executed:
# Check what exists in the vendor code - DefaultRetry vs DefaultBackoff
rg -n 'var DefaultRetry|var DefaultBackoff|DefaultRetry =|DefaultBackoff =' vendor/k8s.io/client-go/util/retry/ --type goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 245
🏁 Script executed:
# Check the library-go encryption helpers that are imported
rg -n -A10 'func UpdateUnsupportedConfig' vendor/github.com/openshift/library-go/test/library/encryption/helpers.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 67
🏁 Script executed:
# Check the exact signature of RetryOnConflict in vendor
rg -n -A5 'func RetryOnConflict' vendor/k8s.io/client-go/util/retry/util.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 215
🏁 Script executed:
# Check the difference between DefaultRetry and DefaultBackoff
rg -n -A10 'var DefaultRetry = wait.Backoff|var DefaultBackoff = wait.Backoff' vendor/k8s.io/client-go/util/retry/util.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 877
🏁 Script executed:
# Check if the library-go helpers are actually being used by this file
rg -n 'library\.|library-go' test/library/encryption/helpers.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 429
🏁 Script executed:
# Look more broadly for any UpdateUnsupportedConfig patterns in library-go test helpers
rg -n 'UpdateUnsupportedConfig\|UnsupportedConfigOverrides' vendor/github.com/openshift/library-go/test/library/encryption/Repository: openshift/cluster-kube-apiserver-operator
Length of output: 67
🏁 Script executed:
# Check what the recommended pattern is in similar test helpers
rg -n 'RetryOnConflict.*DefaultRetry|RetryOnConflict.*DefaultBackoff' --type go | head -15Repository: openshift/cluster-kube-apiserver-operator
Length of output: 1410
Use retry-on-conflict when updating operator resources.
Line 93 performs a single Update on the operator resource without handling conflicts. Wrap this in a retry loop using the standard k8s.io/client-go/util/retry pattern to handle concurrent modifications.
Proposed fix
import (
"context"
"fmt"
"testing"
"time"
@@
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
+ "k8s.io/client-go/util/retry"
@@
func UpdateUnsupportedConfig(t *testing.T, raw []byte) error {
operatorClient := GetOperator(t)
- apiServerOperator, err := operatorClient.Get(context.TODO(), "cluster", metav1.GetOptions{})
- if err != nil {
- return err
- }
- apiServerOperator.Spec.UnsupportedConfigOverrides.Raw = raw
- _, err = operatorClient.Update(context.TODO(), apiServerOperator, metav1.UpdateOptions{})
- return err
+ return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+ apiServerOperator, err := operatorClient.Get(context.TODO(), "cluster", metav1.GetOptions{})
+ if err != nil {
+ return err
+ }
+ apiServerOperator.Spec.UnsupportedConfigOverrides.Raw = raw
+ _, err = operatorClient.Update(context.TODO(), apiServerOperator, metav1.UpdateOptions{})
+ return err
+ })
}Note: Use retry.DefaultBackoff (not DefaultRetry) to align with project convention and the documented recommendation for conflicts on actively-managed resources.
🤖 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/library/encryption/helpers.go` around lines 93 - 94, Wrap the single
operatorClient.Update call in a retry.RetryOnConflict loop using
retry.DefaultBackoff from k8s.io/client-go/util/retry to handle update
conflicts; inside the closure call operatorClient.Update(ctx, apiServerOperator,
metav1.UpdateOptions{}), and on conflict fetch the latest apiServerOperator (via
operatorClient.Get) and reapply your changes before retrying so the update
eventually succeeds. Ensure you import k8s.io/client-go/util/retry and keep
using metav1.UpdateOptions{} and the same context (context.TODO or passed ctx).
2 participants
from library-go openshift/library-go#2218
/hold
Summary by CodeRabbit
Tests
Chores