fake bump kmstokms test#712
Conversation
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughLibrary-go dependency is redirected to a fork in ChangesKMS-to-KMS encryption testing with library-go fork
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-aws-operator-encryption-kms |
|
[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 |
adb5d68 to
b492d55
Compare
|
/test e2e-aws-operator-encryption-kms |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 138: The replace directive for library-go at line 138 lacks an
explanatory comment that describes why the fork github.com/gangwgr/library-go is
needed, unlike other replace directives in the file. Add a comment above the
library-go replace directive that clarifies the purpose—whether this is a
temporary measure for testing a fix before merging upstream, or a permanent
change needed for ongoing compatibility. This will provide necessary context for
future maintainers about the rationale for using this fork instead of the
upstream library.
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 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.
- 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.
- Around line 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.
🪄 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: e3049b16-95dd-4649-ab05-2d0f341b7e5b
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.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/scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
_output/golang-versions_output/named-golang-versionsgo.modtest/e2e-encryption-kms/encryption_kms.go
| // 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/gangwgr/library-go v0.0.0-20260616121325-7a109a33686c |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the go.mod file to understand the full context
head -150 go.mod | tail -20Repository: openshift/cluster-openshift-apiserver-operator
Length of output: 1060
🏁 Script executed:
# Check the full go.mod file for context around the replace directive
cat -n go.mod | grep -A 5 -B 5 "gangwgr/library-go"Repository: openshift/cluster-openshift-apiserver-operator
Length of output: 417
🏁 Script executed:
# Check if there's any git history or comments explaining this change
git log --oneline -p go.mod 2>/dev/null | head -200 || echo "Git history not available"Repository: openshift/cluster-openshift-apiserver-operator
Length of output: 7122
🌐 Web query:
gangwgr/library-go fork security openshift
💡 Result:
There is no evidence of a security-related fork of the OpenShift library-go repository maintained by user 'gangwgr' [1][2][3]. The user 'gangwgr' is an active contributor to the official openshift/library-go repository, as evidenced by numerous merged pull requests and commits to the main codebase [1][3][4][5]. The references to 'gangwgr' in various OpenShift operators (such as cluster-kube-apiserver-operator and cluster-openshift-apiserver-operator) are strictly for bumping dependencies to the latest versions of the official library-go repository, not to a personal or security fork [6][7]. In OpenShift development, it is common practice to use temporary 'replace' directives in go.mod files to point to personal forks when testing specific fixes or new features before they are merged into the official repository [8][9][10]. While other contributors have used such temporary forks for development purposes in the past [8][9], there is no indication that 'gangwgr' maintains a security-specific fork of library-go [1][3]. The library-go repository itself remains the canonical source for OpenShift library helpers [11][2].
Citations:
- 1: openshift/library-go@846538f
- 2: https://github.com/openshift/library-go
- 3: openshift/library-go@1de4598
- 4: openshift/library-go@bd6a3fc
- 5: openshift/library-go@21ab962
- 6: NO-JIRA: Bump latest libray-go changes cluster-kube-apiserver-operator#2173
- 7: NO-JIRA: Bump latest library-go changes #692
- 8: OCPBUGS-65626: update library-go to latest cluster-kube-apiserver-operator#2026
- 9: CNTRLPLANE-2241: Update library-go to get KMS encryption cluster-authentication-operator#837
- 10: Using fork of openshift/library-go kubevirt/containerized-data-importer#1097
- 11: https://pkg.go.dev/github.com/openshift/library-go
🌐 Web query:
openshift/library-go known CVEs osv.dev
💡 Result:
The openshift/library-go repository does not have a single, static list of known CVEs published directly on OSV.dev. Because library-go is a collection of helper libraries used heavily within the OpenShift ecosystem, security vulnerabilities affecting it are typically tracked through specific OpenShift Bugzilla issues (OCPBUGS), Red Hat Security Advisories (RHSA), or by updating its dependencies [1][2][3]. To check for vulnerabilities related to this package using OSV.dev, you can use the OSV API or web interface by querying the package using its Package URL (PURL): pkg:golang/github.com/openshift/library-go [4][5]. Key points regarding vulnerability management for this repository include: 1. Dependency Updates: Many CVEs associated with library-go are actually vulnerabilities in its upstream dependencies (such as logrus or various Kubernetes libraries) [1][2]. Updates to library-go are frequently issued to pull in patched versions of these dependencies [1][2][6]. 2. Tracking Mechanism: Vulnerabilities are often documented in Red Hat's internal issue tracker (referenced via OCPBUGS IDs in public GitHub pull requests) or via official Red Hat Security Advisories [1][2][3]. 3. Querying OSV.dev: You can query vulnerabilities programmatically by sending a POST request to the OSV API with the package details [4][5]: curl -X POST -d '{"package": {"name": "github.com/openshift/library-go", "ecosystem": "Go"}}' "https://api.osv.dev/v1/query" Historical examples of vulnerabilities linked to this repository or its sub-packages include CVE-2025-65637 (a DoS vulnerability fixed by updating a dependency) [1][2], CVE-2023-0229 [7], and CVE-2019-10213 [8]. Always verify the current status of your specific version by checking the Go module dependencies or consulting official OpenShift release notes [9][10].
Citations:
- 1: OCPBUGS-68150: Update library-go to fix CVE-2025-65637 cluster-authentication-operator#840
- 2: OCPBUGS-68170: CVE-2025-65637 - Update library-go to bump github.com/sirupsen/logrus from v1.9.0 to v1.9.3 [release-4.17] #653
- 3: https://vulmon.com/vendoradvisory?qid=RHSA-20194075+-+Security+Advisory&qidtp=red_hat_security_advisories
- 4: https://google.github.io/osv.dev/post-v1-query/
- 5: https://pkg.go.dev/osv.dev
- 6: openshift/router@f4de387
- 7: https://nvd.nist.gov/vuln/detail/cve-2023-0229
- 8: https://security.snyk.io/vuln/SNYK-GOLANG-GITHUBCOMOPENSHIFTLIBRARYGOPKGOPERATORRESOURCERESOURCEAPPLY-472066
- 9: https://pkg.go.dev/github.com/openshift/library-go
- 10: https://pkg.go.dev/github.com/openshift/apiserver-library-go
Add explanatory comment to the library-go replace directive.
Line 138 lacks context explaining why the fork is needed (unlike the Ginkgo replace on line 135-136, which includes a comment). Add a comment clarifying the purpose—whether this is temporary for testing before merging a fix, or permanent for ongoing compatibility.
🤖 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 138, The replace directive for library-go at line 138 lacks
an explanatory comment that describes why the fork github.com/gangwgr/library-go
is needed, unlike other replace directives in the file. Add a comment above the
library-go replace directive that clarifies the purpose—whether this is a
temporary measure for testing a fix before merging upstream, or a permanent
change needed for ongoing compatibility. This will provide necessary context for
future maintainers about the rationale for using this fork instead of the
upstream library.
Source: Coding guidelines
| 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{}) |
There was a problem hiding this comment.
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.
| 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{}) | ||
| require.NoError(t, err) | ||
| defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its content around the specified lines
wc -l test/e2e-encryption-kms/encryption_kms.goRepository: 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.goRepository: 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.goRepository: 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.goRepository: 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.goRepository: 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.
| 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
| return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f encryption_kms.goRepository: openshift/cluster-openshift-apiserver-operator
Length of output: 132
🏁 Script executed:
wc -l test/e2e-encryption-kms/encryption_kms.goRepository: openshift/cluster-openshift-apiserver-operator
Length of output: 136
🏁 Script executed:
sed -n '130,180p' test/e2e-encryption-kms/encryption_kms.goRepository: openshift/cluster-openshift-apiserver-operator
Length of output: 3466
🏁 Script executed:
sed -n '100,145p' test/e2e-encryption-kms/encryption_kms.goRepository: openshift/cluster-openshift-apiserver-operator
Length of output: 3141
🏁 Script executed:
sed -n '155,180p' test/e2e-encryption-kms/encryption_kms.goRepository: openshift/cluster-openshift-apiserver-operator
Length of output: 1880
🏁 Script executed:
sed -n '70,130p' test/e2e-encryption-kms/encryption_kms.go | head -70Repository: 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.
| 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
|
@gangwgr: The following test 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. |
|
/test e2e-aws-operator-encryption-kms |
b492d55 to
8d2ec38
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go.mod (1)
138-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing explanatory comment and supply chain justification for library-go fork.
Line 138 introduces a
replacedirective forgithub.com/openshift/library-gopointing to a personal fork (github.com/ardaguclu/library-go), but provides no explanatory comment. This mirrors a previous finding and additionally violates the coding guideline requirement to "justify need" for new dependencies.Per coding guidelines, you must:
- Add a comment clarifying whether this is temporary (e.g., testing a fix pre-merge) or permanent, consistent with the Ginkgo replace pattern at line 135–136.
- Justify the fork: explain why the personal fork is necessary instead of upstream
github.com/openshift/library-go.- Document the pre-release status: the version
v0.0.0-20260617092928-ed7dbb06ff70is a development snapshot (v0.0.0), not a release; confirm this is appropriate for your use case.💡 Suggested comment format
-replace github.com/openshift/library-go => github.com/ardaguclu/library-go v0.0.0-20260617092928-ed7dbb06ff70 +// TODO: Temporary fork to test KMS-to-KMS migration support; merge upstream before release. +replace github.com/openshift/library-go => github.com/ardaguclu/library-go v0.0.0-20260617092928-ed7dbb06ff70(Adjust the comment to match the actual justification.)
🤖 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 138, The replace directive for github.com/openshift/library-go pointing to the personal fork github.com/ardaguclu/library-go lacks required documentation. Add a comment above this replace directive explaining whether this is a temporary fork (e.g., testing a fix pre-merge) or permanent, justify why the personal fork is necessary instead of using the upstream version, and document that the version v0.0.0-20260617092928-ed7dbb06ff70 is a development snapshot to confirm this is appropriate for the use case. Follow the same documentation pattern used for the Ginkgo replace directive to maintain consistency.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@go.mod`:
- Line 138: The replace directive for github.com/openshift/library-go pointing
to the personal fork github.com/ardaguclu/library-go lacks required
documentation. Add a comment above this replace directive explaining whether
this is a temporary fork (e.g., testing a fix pre-merge) or permanent, justify
why the personal fork is necessary instead of using the upstream version, and
document that the version v0.0.0-20260617092928-ed7dbb06ff70 is a development
snapshot to confirm this is appropriate for the use case. Follow the same
documentation pattern used for the Ginkgo replace directive to maintain
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3ee819ee-8a06-4cf8-8210-387d0960b090
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.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/scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
_output/golang-versions_output/named-golang-versionsgo.modtest/e2e-encryption-kms/encryption_kms.go
✅ Files skipped from review due to trivial changes (2)
- _output/named-golang-versions
- _output/golang-versions
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e-encryption-kms/encryption_kms.go
|
/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms https://github.com/openshift/cluster-openshift-kube-apiserver-operator/pull/2203 openshift/cluster-authentication-operator#917 |
|
@gangwgr, |
|
/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms https://github.com/openshift/cluster-openshift-kube-apiserver-operator/pull/2203 openshift/cluster-authentication-operator#917 |
|
@gangwgr, |
|
/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms openshift/cluster-kube-apiserver-operator#2203 openshift/cluster-authentication-operator#917 |
8d2ec38 to
e7dd61f
Compare
|
/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms openshift/cluster-kube-apiserver-operator#2203 openshift/cluster-authentication-operator#917 |
|
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. |
Summary by CodeRabbit
Release Notes
Tests
Chores