Skip to content

Kms vault lifecycle#2165

Draft
gangwgr wants to merge 4 commits into
openshift:mainfrom
gangwgr:kms-vault-lifecycle
Draft

Kms vault lifecycle#2165
gangwgr wants to merge 4 commits into
openshift:mainfrom
gangwgr:kms-vault-lifecycle

Conversation

@gangwgr
Copy link
Copy Markdown
Contributor

@gangwgr gangwgr commented May 26, 2026

Summary by CodeRabbit

  • Bug Fixes

    • KMS integration for the kube-apiserver switched to a sidecar-based approach to improve encryption management and reliability.
  • Tests

    • End-to-end encryption tests are now context-aware; rotation, performance, migration, on/off, and invalid-image scenarios use context-driven helpers and timeouts.
  • Chores

    • Module dependency resolutions updated and a test framework version pinned.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Replaces KMS volume/mount injection with a KMS plugin sidecar in the operator, extends managePods to accept a Secrets client, threads contexts through e2e encryption tests and library test helpers, and updates go.mod replace directives.

Changes

KMS Sidecar Integration and Context-Aware E2E Testing

Layer / File(s) Summary
KMS sidecar integration in operator
pkg/operator/targetconfigcontroller/targetconfigcontroller.go, go.mod
Imports switch from encryptionkms to kmspluginlifecycle. managePods now accepts a SecretsGetter; caller updated to pass c.kubeClient.CoreV1(). KMS injection call replaced with kmspluginlifecycle.AddKMSPluginSidecarToPodSpec. go.mod adds replace directives pointing to forked library-go and onsi-ginkgo modules.
KMS e2e helper refactor
test/e2e-encryption-kms/encryption_kms.go
Ginkgo It callbacks and KMS e2e helpers now accept context.Context. Mock KMS plugin deployments and invalid-image recovery wait callbacks use the provided ctx. Scenarios use librarykms.DefaultFakeKMSPluginConfig.
General test context propagation
test/e2e/encryption.go, test/e2e-encryption/*.go, test/e2e-encryption-perf/encryption_perf_test.go
Multiple test entrypoints and helpers updated to pass t.Context()/tt.Context() or Ginkgo ctx into library-go test helpers and local helpers; added context import where needed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • benluddy
  • dgrisonnet
  • ricardomaraschini
🚥 Pre-merge checks | ✅ 8 | ❌ 4

❌ Failed checks (3 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning Test code has missing assertion messages in require.NoError calls (lines 55,59,66,70 of encryption_perf_test.go lack 3rd argument descriptions). Add descriptive messages as 3rd argument to require.NoError calls, e.g. require.NoError(t, err, "failed to validate secret count") to help diagnose test failures.
Microshift Test Compatibility ⚠️ Warning Test "[Operator][Serial][Timeout:40m] TestEncryptionTypeAESCBC" in test/e2e/encryption.go references flagged namespace openshift-kube-apiserver without MicroShift protection tags or checks. Add [OCPFeatureGate:KMSEncryption] tag to test name or add [Skipped:MicroShift] label to skip it on MicroShift.
Title check ❓ Inconclusive The title "Kms vault lifecycle" is vague and generic, lacking specific details about the actual changes made in the PR. Clarify the title to reflect the main changes, such as "Switch KMS integration to use plugin sidecar instead of volumes" or "Thread context through KMS encryption e2e tests and update KMS plugin integration."
✅ Passed checks (8 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All Ginkgo test names in the PR are stable and deterministic with no dynamic values (timestamps, UUIDs, pod names). Context is properly threaded through test helpers, not embedded in test titles.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests are added in this PR. All changes modify existing encryption tests to add context parameter passing. The SNO check applies only to newly added tests.
Topology-Aware Scheduling Compatibility ✅ Passed PR introduces no new scheduling constraints. Changes are KMS sidecar integration and test context threading. Static pods bypass scheduler; pod template has no affinity rules.
Ote Binary Stdout Contract ✅ Passed Test code threads context through Ginkgo It() blocks. All stdout writes are within test code (framework-intercepted), not process-level code. No violations of OTE stdout contract detected.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests are added in this PR. The changes only modify existing tests to thread context through helpers; the custom check applies specifically to NEW tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 26, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 26, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gangwgr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 26, 2026

/test e2e-aws-operator-encryption-kms-ote

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 26, 2026

/test e2e-aws-operator-encryption-kms

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 26, 2026

/test e2e-gcp-operator-encryption-kms

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 26, 2026

/test e2e-gcp-operator-encryption-kms-ote

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 rewiring
github.com/openshift/library-go to a personal fork (replace
github.com/openshift/library-go => github.com/gangwgr/library-go
v0.0.0-20260526073859-78d95d2abac4); remove that replace (or replace it with an
explicit upstream version of github.com/openshift/library-go) so CI/release
provenance is preserved, and if this fork is strictly temporary add a short
in-repo doc and a tracking issue/PR reference in the repository (e.g., note
scope, timeframe, and rollback plan) so the change is auditable and reversible.

In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 127-131: The polling callbacks passed to
wait.PollUntilContextTimeout are currently swallowing errors from
operatorClient.Get by returning (false, nil); instead, propagate non-nil errors
so the poll aborts with the real error. Update the anonymous funcs used in
wait.PollUntilContextTimeout (the ones calling operatorClient.Get) to return
(false, err) when operatorClient.Get returns a non-nil error (or only suppress
specific transient errors if intentionally desired), and make the same change in
the second occurrence that currently swallows Get errors.
🪄 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: a2ae947c-7340-4f07-b683-fc6e7335705b

📥 Commits

Reviewing files that changed from the base of the PR and between 3f0fc12 and 0e4a0e1.

⛔ Files ignored due to path filters (16)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/sidecar.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/vault.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (8)
  • go.mod
  • pkg/operator/targetconfigcontroller/targetconfigcontroller.go
  • test/e2e-encryption-kms/encryption_kms.go
  • test/e2e-encryption-perf/encryption_perf_test.go
  • test/e2e-encryption-rotation/encryption_rotation_test.go
  • test/e2e-encryption/encryption_test.go
  • test/e2e/encryption.go
  • test/e2e/encryption_test.go

Comment thread go.mod Outdated

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/gangwgr/library-go v0.0.0-20260526073859-78d95d2abac4
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 | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify all replace directives and whether org->personal fork redirects exist.
set -euo pipefail

echo "== replace directives in go.mod =="
rg -n '^\s*replace\s+' go.mod

echo
echo "== specifically check library-go replacement =="
rg -n '^\s*replace\s+github\.com/openshift/library-go\s*=>' go.mod

Repository: openshift/cluster-kube-apiserver-operator

Length of output: 507


Avoid committing a go.mod replace from github.com/openshift/library-go to github.com/gangwgr/library-go
go.mod line 140 rewires github.com/openshift/library-go to a personal fork (github.com/gangwgr/library-go v0.0.0-20260526073859-78d95d2abac4), creating supply-chain provenance/reproducibility risk in CI/release builds unless this is strictly temporary and explicitly documented (scope, timeframe, rollback plan).

replace github.com/openshift/library-go => github.com/gangwgr/library-go v0.0.0-20260526073859-78d95d2abac4
🤖 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
rewiring github.com/openshift/library-go to a personal fork (replace
github.com/openshift/library-go => github.com/gangwgr/library-go
v0.0.0-20260526073859-78d95d2abac4); remove that replace (or replace it with an
explicit upstream version of github.com/openshift/library-go) so CI/release
provenance is preserved, and if this fork is strictly temporary add a short
in-repo doc and a tracking issue/PR reference in the repository (e.g., note
scope, timeframe, and rollback plan) so the change is auditable and reversible.

Comment thread test/e2e-encryption-kms/encryption_kms.go Outdated
@gangwgr gangwgr force-pushed the kms-vault-lifecycle branch from 0e4a0e1 to d20f728 Compare May 26, 2026 12:46
@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 26, 2026

/test e2e-gcp-operator-encryption-kms

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (2)
go.mod (1)

140-140: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid merging a long-lived replace to a personal fork.

Keeping github.com/openshift/library-go redirected to github.com/gangwgr/library-go in go.mod risks non-reproducible upstream builds and weakens dependency provenance. Prefer upstream/org-hosted source before merge, or explicitly document this as temporary with rollback tracking.

#!/bin/bash
set -euo pipefail

echo "== library-go replacement in go.mod =="
rg -n '^replace github\.com/openshift/library-go =>' go.mod

echo
echo "== any in-repo documentation for temporary fork usage =="
rg -n 'library-go.*replace|gangwgr/library-go|temporary|rollback|tracking issue|follow-up' README* docs/ || true
🤖 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 replaces
github.com/openshift/library-go with a personal fork (replace
github.com/openshift/library-go => github.com/gangwgr/library-go ...), which
should not be merged long-term; change this by removing or reverting that
replace directive and instead point to the upstream module (or a vetted
org-hosted fork) or, if this is strictly temporary, add a clear comment in the
repository (README or docs) documenting why the replace exists, its
expiration/rollback plan and a tracking issue/PR number so maintainers can
follow up; update or remove the replace line referencing
github.com/gangwgr/library-go accordingly and ensure any CI/build docs mention
the temporary override.
test/e2e-encryption-kms/encryption_kms.go (1)

127-131: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Propagate Get errors in poll conditions instead of swallowing them.

Returning (false, nil) on non-transient operatorClient.Get(...) errors hides real failures and can turn immediate errors into opaque timeouts.

Suggested fix
err := wait.PollUntilContextTimeout(ctx, 10*time.Second, 10*time.Minute, true, func(ctx context.Context) (bool, error) {
    operator, err := operatorClient.Get(ctx, "cluster", metav1.GetOptions{})
    if err != nil {
-       return false, nil
+       return false, fmt.Errorf("failed to get kube-apiserver operator: %w", err)
    }
    ...
})
err := wait.PollUntilContextTimeout(ctx, 10*time.Second, 20*time.Minute, true, func(ctx context.Context) (bool, error) {
    operator, err := operatorClient.Get(ctx, "cluster", metav1.GetOptions{})
    if err != nil {
-       return false, nil
+       return false, fmt.Errorf("failed to get kube-apiserver operator: %w", err)
    }
    ...
})

Also applies to: 145-149

🤖 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 127 - 131, The poll
callback currently swallows non-transient errors from operatorClient.Get by
returning (false, nil); change the callback to propagate real Get errors by
returning (false, err) when operatorClient.Get(ctx, "cluster",
metav1.GetOptions{}) returns an error so the poll exits immediately with the
original error; update both the block around operatorClient.Get in the
wait.PollUntilContextTimeout call at lines ~127-131 and the similar occurrence
at ~145-149 to return the error instead of nil.
🤖 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 140: The go.mod currently replaces github.com/openshift/library-go with a
personal fork (replace github.com/openshift/library-go =>
github.com/gangwgr/library-go ...), which should not be merged long-term; change
this by removing or reverting that replace directive and instead point to the
upstream module (or a vetted org-hosted fork) or, if this is strictly temporary,
add a clear comment in the repository (README or docs) documenting why the
replace exists, its expiration/rollback plan and a tracking issue/PR number so
maintainers can follow up; update or remove the replace line referencing
github.com/gangwgr/library-go accordingly and ensure any CI/build docs mention
the temporary override.

In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 127-131: The poll callback currently swallows non-transient errors
from operatorClient.Get by returning (false, nil); change the callback to
propagate real Get errors by returning (false, err) when operatorClient.Get(ctx,
"cluster", metav1.GetOptions{}) returns an error so the poll exits immediately
with the original error; update both the block around operatorClient.Get in the
wait.PollUntilContextTimeout call at lines ~127-131 and the similar occurrence
at ~145-149 to return the error instead of nil.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 18f76826-cdb4-439b-87b6-c7e4a6047f5b

📥 Commits

Reviewing files that changed from the base of the PR and between 0e4a0e1 and d20f728.

⛔ Files ignored due to path filters (14)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (7)
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go
  • test/e2e-encryption-perf/encryption_perf_test.go
  • test/e2e-encryption-rotation/encryption_rotation_test.go
  • test/e2e-encryption/encryption_test.go
  • test/e2e/encryption.go
  • test/e2e/encryption_test.go

@gangwgr gangwgr force-pushed the kms-vault-lifecycle branch from d20f728 to 600987a Compare May 26, 2026 16:54
@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 26, 2026

/test e2e-gcp-operator-encryption-kms

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (2)
go.mod (1)

140-140: ⚠️ Potential issue | 🟠 Major

Personal fork replace directive creates supply-chain risk.

The replace directive redirects github.com/openshift/library-go to a personal fork, which was already flagged in previous reviews. This remains a supply-chain and provenance concern for CI/release builds.

🤖 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 contains a risky replace directive pointing
github.com/openshift/library-go to a personal fork; remove that replace
directive in go.mod and restore a dependency on the official upstream module
(github.com/openshift/library-go) with a vetted tagged version, or if a patched
fork is required, host the fork in an org-controlled repository and reference
that repo instead; after updating go.mod run `go mod tidy`/`go mod vendor` to
regenerate go.sum and ensure CI verifies the provenance of the chosen module.
test/e2e-encryption-kms/encryption_kms.go (1)

125-138: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Error swallowing in polling callbacks prevents early failure detection.

Both WaitForDegraded and WaitForRecovery suppress errors from operatorClient.Get(ctx, ...) by returning (false, nil). This issue was flagged in previous reviews and remains unresolved. Propagating these errors would provide faster, more actionable feedback when the operator client fails.

Also applies to: 143-164

🤖 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 125 - 138, The
polling callbacks used in WaitForDegraded/WaitForRecovery swallow errors from
operatorClient.Get by returning (false, nil), hiding client failures; update the
callback inside wait.PollUntilContextTimeout to propagate errors from
operatorClient.Get (return false, err) so polling fails fast on Get errors and
test helpers (WaitForDegraded/WaitForRecovery) return that error upward instead
of masking it.
🤖 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 140: The go.mod contains a risky replace directive pointing
github.com/openshift/library-go to a personal fork; remove that replace
directive in go.mod and restore a dependency on the official upstream module
(github.com/openshift/library-go) with a vetted tagged version, or if a patched
fork is required, host the fork in an org-controlled repository and reference
that repo instead; after updating go.mod run `go mod tidy`/`go mod vendor` to
regenerate go.sum and ensure CI verifies the provenance of the chosen module.

In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 125-138: The polling callbacks used in
WaitForDegraded/WaitForRecovery swallow errors from operatorClient.Get by
returning (false, nil), hiding client failures; update the callback inside
wait.PollUntilContextTimeout to propagate errors from operatorClient.Get (return
false, err) so polling fails fast on Get errors and test helpers
(WaitForDegraded/WaitForRecovery) return that error upward instead of masking
it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f2e2cd8e-efa1-41d4-9108-6b8f49b186e8

📥 Commits

Reviewing files that changed from the base of the PR and between d20f728 and 600987a.

⛔ Files ignored due to path filters (14)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (7)
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go
  • test/e2e-encryption-perf/encryption_perf_test.go
  • test/e2e-encryption-rotation/encryption_rotation_test.go
  • test/e2e-encryption/encryption_test.go
  • test/e2e/encryption.go
  • test/e2e/encryption_test.go

// testKMSEncryptionInvalidImageRecovery tests that an invalid KMS plugin image
// causes degradation and that fixing the image restores the cluster.
func testKMSEncryptionInvalidImageRecovery(ctx context.Context, t testing.TB) {
librarykms.DeployUpstreamMockKMSPlugin(ctx, t, library.GetClients(t).Kube, librarykms.WellKnownUpstreamMockKMSPluginNamespace, librarykms.WellKnownUpstreamMockKMSPluginImage, librarykms.DefaultKMSPluginCount)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm, I see the lifecycle changes in this PR (commit kms: support deploying Vault mock plugin), so I assume you want to rely on that to deploy the fake Vault plugin?

return false, nil
}
for _, cond := range operator.Status.Conditions {
if cond.Type == "Degraded" && cond.Status == operatorv1.ConditionTrue {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IIUC, this is a condition for kubeapiservers/cluster, is that right? If so, I think it has many conditions postfixed with Degraded, but none of them are just Degraded

@gangwgr gangwgr force-pushed the kms-vault-lifecycle branch 2 times, most recently from ef98104 to f3530ad Compare May 27, 2026 04:53
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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-kms/encryption_kms.go`:
- Around line 118-123: getKASOperatorConditions currently drops the test context
and uses context.Background() in its Get calls; change getKASOperatorConditions
to accept a context.Context parameter and replace context.Background() with that
ctx inside its Kubernetes Get calls, then update all call sites (e.g. the
closures passed to WaitForOperatorDegraded and WaitForOperatorAvailable such as
WaitForRecovery and the other wait closures) to pass the received ctx into
getKASOperatorConditions(t) -> getKASOperatorConditions(ctx, t) so
timeouts/cancellations propagate end-to-end.
🪄 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: 01aff279-b63a-433d-b68d-34d84cb09c73

📥 Commits

Reviewing files that changed from the base of the PR and between 600987a and ef98104.

⛔ Files ignored due to path filters (14)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (7)
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go
  • test/e2e-encryption-perf/encryption_perf_test.go
  • test/e2e-encryption-rotation/encryption_rotation_test.go
  • test/e2e-encryption/encryption_test.go
  • test/e2e/encryption.go
  • test/e2e/encryption_test.go
✅ Files skipped from review due to trivial changes (1)
  • test/e2e-encryption-rotation/encryption_rotation_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/e2e-encryption-perf/encryption_perf_test.go
  • test/e2e/encryption_test.go
  • test/e2e/encryption.go

Comment thread test/e2e-encryption-kms/encryption_kms.go Outdated
@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 27, 2026

/test e2e-aws-operator-encryption-kms

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 27, 2026

/test e2e-gcp-operator-encryption-kms

@gangwgr gangwgr force-pushed the kms-vault-lifecycle branch from f3530ad to 92aa8ba Compare May 27, 2026 07:39
@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 27, 2026

/test e2e-gcp-operator-encryption-kms

@gangwgr gangwgr force-pushed the kms-vault-lifecycle branch 2 times, most recently from 11a0db6 to 396667c Compare May 27, 2026 08:39
@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 27, 2026

/test e2e-aws-operator-encryption-kms

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 27, 2026

/test e2e-gcp-operator-encryption-kms

@gangwgr gangwgr force-pushed the kms-vault-lifecycle branch from 396667c to 98a3204 Compare May 27, 2026 12:25
@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented May 27, 2026

/test e2e-gcp-operator-encryption-kms

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 27, 2026

@gangwgr: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 29, 2026

PR needs rebase.

Details

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 kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants