Skip to content

CNTRLPLANE-3236: support deploying Vault mock KMS plugin#899

Open
bertinatto wants to merge 3 commits into
openshift:masterfrom
bertinatto:kms-plugins-lifecycle-vault-mock
Open

CNTRLPLANE-3236: support deploying Vault mock KMS plugin#899
bertinatto wants to merge 3 commits into
openshift:masterfrom
bertinatto:kms-plugins-lifecycle-vault-mock

Conversation

@bertinatto
Copy link
Copy Markdown
Member

@bertinatto bertinatto commented May 20, 2026

Summary by CodeRabbit

  • Refactor
    • KMS encryption plugin provisioning moved to a sidecar-based lifecycle approach.
    • Encryption configuration now uses revisioned config names tied to available revisions.
    • Error reporting improved for failures when adding the KMS plugin to pod specs, aiding troubleshooting and reliability.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 20, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented May 20, 2026

@bertinatto: This pull request references CNTRLPLANE-3236 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set.

Details

In 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 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 20, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 20, 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

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 20, 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

This change migrates KMS integration in the OpenShift OAuth API server deployment from volume/mount injection to sidecar-based injection via the kmspluginlifecycle helper, and updates the relevant import and error messaging.

Changes

KMS Plugin Lifecycle Integration

Layer / File(s) Summary
Switch KMS configuration from volume mount to sidecar injection
pkg/operator/workload/sync_openshift_oauth_apiserver.go
Import updated from encryption/kms to encryption/kms/pluginlifecycle. syncStandardDeployment now calls kmspluginlifecycle.AddKMSPluginSidecarToPodSpec with the reconcile context, target PodSpec, component identifiers, target namespace, encryption-config-%d (from operatorStatus.LatestAvailableRevision), the CoreV1 client, and c.featureGateAccessor. Error message updated to “failed to add KMS plugin to pod spec”.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 8 | ❌ 4

❌ Failed checks (3 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Microshift Test Compatibility ⚠️ Warning Two Ginkgo e2e tests use configv1.APIServerEncryption from config.openshift.io/v1, unavailable on MicroShift, with no skip protection. Add [apigroup:config.openshift.io] tag to both test names so MicroShift CI automatically skips them due to unavailable API group.
Single Node Openshift (Sno) Test Compatibility ⚠️ Warning New Ginkgo e2e tests (TestKMSEncryptionOnOff, TestKMSEncryptionProvidersMigration) added in test/e2e-encryption-kms/encryption_kms.go lack SNO compatibility protection. Add [Skipped:SingleReplicaTopology] label to test names or add exutil.IsSingleNode() check with g.Skip() in test body, or verify tests work on SNO and add infrastructure topology check.
Title check ❓ Inconclusive The title references deploying a Vault mock KMS plugin, but the actual change is switching KMS integration from direct injection to sidecar-based provisioning via pluginlifecycle. The title mentions 'deploying Vault mock KMS plugin' but the code change shows a generic migration from direct KMS volume injection to sidecar-based KMS plugin provisioning without Vault-specific details visible in this file.
✅ 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 are static with no dynamic identifiers like pod names, timestamps, UUIDs, or node names. No fmt.Sprintf or string concatenation found in test declarations.
Test Structure And Quality ✅ Passed No Ginkgo tests exist in this PR. The repository uses standard Go testing (89 test functions) only. The Ginkgo test code check is not applicable.
Topology-Aware Scheduling Compatibility ✅ Passed PR only modifies KMS plugin injection (volume/mount to sidecar) with minimal changes. Does not introduce new scheduling constraints or topology awareness issues.
Ote Binary Stdout Contract ✅ Passed OTE binary properly sets klog.LogToStderr(true) in main(). Modified file is regular code with no process-level functions. No stdout violations detected.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed New Ginkgo e2e tests contain no IPv4 hardcoding, external connectivity requirements, or assumptions. They use cluster-internal services and are IPv6-compatible.

✏️ 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.

@bertinatto bertinatto changed the title CNTRLPLANE-3236: support deploying Vault mock KMS plugin [WIP] CNTRLPLANE-3236: support deploying Vault mock KMS plugin May 20, 2026
@bertinatto bertinatto force-pushed the kms-plugins-lifecycle-vault-mock branch from 85702d8 to d42bb8c Compare May 20, 2026 15:21
@bertinatto bertinatto marked this pull request as ready for review May 20, 2026 15:32
@bertinatto bertinatto force-pushed the kms-plugins-lifecycle-vault-mock branch from d42bb8c to 1667e9c Compare May 20, 2026 15:42
@bertinatto bertinatto changed the title [WIP] CNTRLPLANE-3236: support deploying Vault mock KMS plugin CNTRLPLANE-3236: support deploying Vault mock KMS plugin May 20, 2026
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 20, 2026
@bertinatto bertinatto force-pushed the kms-plugins-lifecycle-vault-mock branch from 1667e9c to e3bc61e Compare May 20, 2026 21:13
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 `@pkg/operator/workload/sync_openshift_oauth_apiserver.go`:
- Around line 318-324: The call to
kmspluginlifecycle.AddKMSPluginSidecarToPodSpec is passing
required.Spec.Template.Name (the pod template metadata name) but the function
expects the actual container name; update the call site to pass the real
container name "oauth-apiserver" (instead of required.Spec.Template.Name) so
AddKMSPluginSidecarToPodSpec can locate the container (keep the other args: ctx,
&required.Spec.Template.Spec, c.targetNamespace,
fmt.Sprintf("encryption-config-%d", operatorStatus.LatestAvailableRevision),
c.kubeClient.CoreV1(), c.featureGateAccessor).
🪄 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: 33936668-890b-4a82-a036-75d2d8eed111

📥 Commits

Reviewing files that changed from the base of the PR and between 1667e9c and e3bc61e.

⛔ Files ignored due to path filters (3)
  • 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/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (1)
  • pkg/operator/workload/sync_openshift_oauth_apiserver.go

Comment thread pkg/operator/workload/sync_openshift_oauth_apiserver.go
@gangwgr
Copy link
Copy Markdown
Contributor

gangwgr commented May 21, 2026

/retest

@gangwgr
Copy link
Copy Markdown
Contributor

gangwgr commented May 21, 2026

/test e2e-agnostic-upgrade

Copy link
Copy Markdown
Member

@ardaguclu ardaguclu left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold
for the confirmation

ctx, &required.Spec.Template.Spec,
"oauth-apiserver",
c.targetNamespace,
fmt.Sprintf("encryption-config-%d", operatorStatus.LatestAvailableRevision),
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.

I think it is better to get confirmation from @p0lyn0mial with this line.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2026
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 21, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu

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

The pull request process is described 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2026
@gangwgr
Copy link
Copy Markdown
Contributor

gangwgr commented May 21, 2026

/verified by ci runs

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label May 21, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@gangwgr: This PR has been marked as verified by ci runs.

Details

In response to this:

/verified by ci runs

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.

@bertinatto bertinatto force-pushed the kms-plugins-lifecycle-vault-mock branch from da32fb3 to 362db9b Compare May 21, 2026 23:51
@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label May 21, 2026
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label May 21, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 21, 2026

New changes are detected. LGTM label has been removed.

@bertinatto
Copy link
Copy Markdown
Member Author

/retest

1 similar comment
@bertinatto
Copy link
Copy Markdown
Member Author

/retest

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 22, 2026

@bertinatto: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-operator 362db9b link true /test e2e-operator
ci/prow/e2e-operator-encryption 362db9b link true /test e2e-operator-encryption
ci/prow/e2e-agnostic 362db9b link true /test e2e-agnostic
ci/prow/e2e-aws-operator-encryption-kms-ote 362db9b link false /test e2e-aws-operator-encryption-kms-ote
ci/prow/e2e-gcp-operator-encryption-kms 362db9b link false /test e2e-gcp-operator-encryption-kms

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.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants