Skip to content

wire real plugin#2248

Draft
gangwgr wants to merge 1 commit into
openshift:masterfrom
gangwgr:wire-real-plugin
Draft

wire real plugin#2248
gangwgr wants to merge 1 commit into
openshift:masterfrom
gangwgr:wire-real-plugin

Conversation

@gangwgr
Copy link
Copy Markdown
Contributor

@gangwgr gangwgr commented May 29, 2026

Summary by CodeRabbit

  • Chores
    • Updated internal test encryption configuration: switched the default test KMS plugin image to a CI-built registry image for test runs, and pinned the fake KMS plugin image to a specific upstream artifact to ensure consistent and reproducible test behavior.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 29, 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 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 29, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 42df1dbf-153f-46ff-90e7-b56c74323ddf

📥 Commits

Reviewing files that changed from the base of the PR and between 90a0b07 and a742402.

📒 Files selected for processing (1)
  • test/library/encryption/kms/vault.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/library/encryption/kms/vault.go

Walkthrough

Replace the Vault KMS test plugin image with a CI registry digest reference (registry.ci.openshift.org/.../vault-kube-kms@sha256:...) and add a new pinned defaultfakevaultkmspluginimage used by DefaultFakeKMSPluginConfig.

Changes

Vault KMS Test Configuration

Layer / File(s) Summary
Vault KMS plugin test image updates
test/library/encryption/kms/vault.go
defaultVaultKMSPluginImage is updated to a CI registry digest (registry.ci.openshift.org/.../vault-kube-kms@sha256:...). A new defaultfakevaultkmspluginimage constant (pinned quay.io/.../mock-kms-plugin@sha256:...) is introduced and DefaultFakeKMSPluginConfig.Vault.KMSPluginImage now uses it instead of WellKnownUpstreamMockKMSPluginImage.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 inconclusive)

Check name Status Explanation Resolution
Container-Privileges ❌ Error Manifest test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml contains securityContext: privileged: true in both init and main containers without documented justification. Document why privileged mode is required or use specific Linux capabilities (CAP_*) instead of privileged: true to follow least-privilege principle.
Title check ❓ Inconclusive The title 'wire real plugin' is vague and does not clearly convey what specific change was made; it lacks descriptive detail about updating Vault KMS plugin image references. Use a more descriptive title such as 'Update Vault KMS plugin image to use CI registry' to clearly communicate the main change in the changeset.
✅ Passed checks (13 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 The modified file contains only constants and helper functions with no Ginkgo test declarations, so this check is not applicable to this PR.
Test Structure And Quality ✅ Passed The modified file is a configuration and helper file, not a Ginkgo test file. It contains no Ginkgo test code (no It/Describe/BeforeEach blocks), only config constants and helper functions.
Microshift Test Compatibility ✅ Passed PR does not add new Ginkgo e2e tests. It adds only a helper library file (vault.go) containing configuration structs and utility functions for encryption tests, not test definitions.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PR adds test/library/encryption/kms/vault.go, a test helper library with no Ginkgo tests (no g.It/Describe/Context/When). SNO compatibility check only applies to new e2e tests.
Topology-Aware Scheduling Compatibility ✅ Passed This PR modifies only test configuration constants in test/library/encryption/kms/vault.go. No deployment manifests, operator code, controllers, or scheduling constraints are introduced.
Ote Binary Stdout Contract ✅ Passed File is a test library with no process-level code (main, init, TestMain, BeforeSuite, etc.) and contains no stdout writes; OTE Binary Stdout Contract is not applicable to this test helper module.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed This PR adds a test library file with Vault KMS configuration and helper functions, not new Ginkgo e2e tests (no It(), Describe(), Context(), When() present).
No-Weak-Crypto ✅ Passed The modified file test/library/encryption/kms/vault.go is a test configuration utility with no weak crypto algorithms, custom crypto implementations, or non-constant-time comparisons.
No-Sensitive-Data-In-Logs ✅ Passed The PR updates KMS plugin image constants in vault.go. Logging statements in the file do not expose passwords, tokens, API keys, PII, session IDs, or credentials. Image URLs are never logged.
✨ 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 29, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gangwgr
Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial 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

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/library/encryption/kms/vault.go`:
- Line 29: Replace the mutable image tag used in the defaultVaultKMSPluginImage
constant with a digest-pinned reference (append `@sha256`:<digest> or replace the
:0.0.1 tag entirely) so the Vault KMS plugin image is immutable in CI; locate
defaultVaultKMSPluginImage in test/library/encryption/kms/vault.go and update
its value to the specific image@sha256:... digest to ensure deterministic,
reproducible test runs.
🪄 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: 94679588-fe43-4664-aaa1-f57e880b991b

📥 Commits

Reviewing files that changed from the base of the PR and between c7d4322 and 2ba893b.

📒 Files selected for processing (1)
  • test/library/encryption/kms/vault.go

Comment thread test/library/encryption/kms/vault.go Outdated
defaultVaultPodName = "vault-0"
defaultVaultCredentialsSecret = "vault-credentials"
defaultVaultAppRoleSecretName = "vault-approle-secret"
defaultVaultKMSPluginImage = "quay.io/openshifttest/mock-kms-plugin@sha256:03bb07a2c08b509653c4c70217a06a4b389c10b4d87922f50ee5eac82db5e140"
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.

We can keep this in new variable with the name defaultfakevaultkmspluginimage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still on my first coffee, but I think we should make this more visually distinct.

Like, FAKE_defaultVaultKMSPluginImage, hiding fake in between is a bit difficult to spot where we're using it. Just to make sure we don't mix it up.

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.

Good idea, that works for me

@gangwgr gangwgr force-pushed the wire-real-plugin branch from 2ba893b to 90a0b07 Compare May 29, 2026 04:39
Comment thread test/library/encryption/kms/vault.go Outdated
defaultVaultCredentialsSecret = "vault-credentials"
defaultVaultAppRoleSecretName = "vault-approle-secret"
defaultfakevaultkmspluginimage = "quay.io/openshifttest/mock-kms-plugin@sha256:03bb07a2c08b509653c4c70217a06a4b389c10b4d87922f50ee5eac82db5e140"
defaultVaultKMSPluginImage = "registry.ci.openshift.org/control-plane-custom-builds/vault-kube-kms:0.0.1"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do you mind to open a quick KAS-O PR with this? so we can see what CI says

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes

@gangwgr gangwgr force-pushed the wire-real-plugin branch from 90a0b07 to a742402 Compare May 29, 2026 07:28
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants