NO-JIRA: kms to kms key identifier check#941
Conversation
|
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:
WalkthroughThis PR updates Go module dependencies, redirects ChangesAuthentication operator updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-encryption-kms/encryption_kms_2.go (1)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated namespace/label-selector literals.
"openshift-config-managed"and the component label selector string at Lines 43-44 duplicate the values already set in theBasicScenarioliteral at Lines 30-31. Consider referencing local variables to avoid drift if either changes.♻️ Suggested consolidation
+ namespace := "openshift-config-managed" + labelSelector := "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver" + library.TestEncryptionProvidersMigration(ctx, t, library.ProvidersMigrationScenario{ BasicScenario: library.BasicScenario{ - Namespace: "openshift-config-managed", - LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", + Namespace: namespace, + LabelSelector: labelSelector, ... }, AssertResourceEncryptedFunc: func(t testing.TB, cs library.ClientSet, resource runtime.Object) { library.AssertTokenOfLifeEncrypted(t, cs, resource) - library.AssertKMSEncryptedWithWriteKey(t, cs.Kube, "openshift-config-managed", - "encryption.apiserver.operator.openshift.io/component=openshift-oauth-apiserver", + library.AssertKMSEncryptedWithWriteKey(t, cs.Kube, namespace, labelSelector, []byte(library.GetRawTokenOfLife(t, cs)), library.AuthTargetGRs[0], ) },🤖 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 43 - 44, The namespace and label-selector literals in the KMS encryption assertion are duplicated from the BasicScenario setup, so update the call in the encryption_kms_2 test to reuse the existing local values instead of hardcoding them. Use the BasicScenario fields/variables already initialized in this test to keep the namespace and component selector in sync with the scenario configuration.
🤖 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.
Nitpick comments:
In `@test/e2e-encryption-kms/encryption_kms_2.go`:
- Around line 43-44: The namespace and label-selector literals in the KMS
encryption assertion are duplicated from the BasicScenario setup, so update the
call in the encryption_kms_2 test to reuse the existing local values instead of
hardcoding them. Use the BasicScenario fields/variables already initialized in
this test to keep the namespace and component selector in sync with the scenario
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e6f33694-055f-4a92-935d-9b53742d6eb9
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/kms_preflight_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/preflight/assets/kms-preflight-pod.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/preflight/checker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/preflight/cmd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/preflight/deployer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/preflight/pod_status.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/preflight/pod_template.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/assertion.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)
Untitledgo.modtest/e2e-encryption-kms/encryption_kms_2.gotest/library/waits.go
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
04c8bab to
5af977b
Compare
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 112: Update the go.opentelemetry.io/otel/sdk dependency from v1.40.0 to
v1.43.0 or later, then regenerate the module dependency metadata and verify the
dependency graph contains no remaining vulnerable v1.40.0 reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.40.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.40.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.41.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.40.0 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
curl -sSf https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' \
--data '{"package":{"name":"go.opentelemetry.io/otel/sdk","ecosystem":"Go"},"version":"v1.40.0"}' |
jq .Repository: openshift/cluster-authentication-operator
Length of output: 3980
Upgrade go.opentelemetry.io/otel/sdk to v1.43.0+
v1.40.0 is affected by GHSA-hfvc-g4fc-pqhx (PATH hijacking). Update the pin and regenerate dependency metadata so no vulnerable version remains in the graph.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 112-112: go.opentelemetry.io/otel/sdk 1.40.0: opentelemetry-go: BSD kenv command not using absolute path enables PATH hijacking
🤖 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 112, Update the go.opentelemetry.io/otel/sdk dependency from
v1.40.0 to v1.43.0 or later, then regenerate the module dependency metadata and
verify the dependency graph contains no remaining vulnerable v1.40.0 reference.
Source: Linters/SAST tools
69ed7f0 to
cac41fa
Compare
Wrap KMS-to-KMS assert for the new helper signature, add Bookmark methods for Indexer mocks, and fix Eventf float formatting.
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
we need to unblock ourselves here with the kube rebase /label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tjungblu 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
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
gangwgr
changed the title
|
@gangwgr: 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-robot
added
the
jira/valid-reference
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: 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. |
|
/retest required |
|
/test e2e-agnostic |
|
/test e2e-agnostic-upgrade |
|
/test e2e-agnostic |
2 similar comments
|
/test e2e-agnostic |
|
/test e2e-agnostic |
|
@gangwgr: 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. |
3 participants
kms to kms key identifier check
Summary by CodeRabbit