CNTRLPLANE-3237: test/library/encryption/kms: add mock KMS plugin wrapper binary#2173
Merged
openshift-merge-bot[bot] merged 1 commit intoApr 27, 2026
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughAdds a Go wrapper CLI and multi-stage Docker build producing a wrapper image layered on the upstream mock KMS plugin, updates the build script to build upstream then wrapper, adds an embedded-asset reader helper, and documents the wrapper in the README. Changes
Sequence Diagram(s)sequenceDiagram
participant Wrapper as Wrapper\n(mock-kms-plugin-wrapper)
participant Assets as Embedded\nAssets (ConfigMap + b64 tar)
participant FS as Filesystem\n(/etc, /var/lib/softhsm)
participant Shell as Shell\n(base64 | tar)
participant Upstream as Upstream\n/usr/local/bin/mock-kms-plugin
Wrapper->>Assets: Read and template asset (k8s_mock_kms_plugin_configmap.yaml)
Wrapper->>FS: Write /etc/softhsm-config.json\nEnsure /var/lib/softhsm/tokens exists
Wrapper->>Shell: Pipe base64 token string to "base64 -d | tar xzf -" in tokens dir
Shell-->>FS: Extract token files into /var/lib/softhsm/tokens
Wrapper->>Upstream: Construct args (-listen-addr, -config-file-path) and syscall.Exec
Wrapper-->>Upstream: Replace process (exec)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
p0lyn0mial
force-pushed
the
kms-upstream-plugin-wrapper
branch
2 times, most recently
from
3e0f592 to
eb8901b
Compare
p0lyn0mial
commented
Apr 23, 2026
| } | ||
| } | ||
|
|
||
| func (o *options) addFlags(fs *pflag.FlagSet) { |
Contributor
Author
There was a problem hiding this comment.
this is where we would add more flags in the future.
p0lyn0mial
commented
Apr 23, 2026
| return syscall.Exec(upstreamBinary, argv, os.Environ()) | ||
| } | ||
|
|
||
| func initSoftHSM() error { |
Contributor
Author
There was a problem hiding this comment.
we embed the required assets to configure the plugin.
p0lyn0mial
commented
Apr 23, 2026
| -f "${SCRIPT_DIR}/Dockerfile" \ | ||
| "${SCRIPT_DIR}/../../../../.." | ||
|
|
||
| echo "Done. Image built: ${IMAGE_TAG}" |
Contributor
Author
There was a problem hiding this comment.
we will have one image that will have two binaries.
existing tests will work without any changes - they use the existing binary.
once we have the plugin lifecycle code we will stop deploying the upstream plugin from e2e tests.
the plugin lifecycle will call the wrapper (could be that we will have to rename the new binary) which doesn't require any extra configuration (the assets are embedded)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/library/encryption/kms/k8s_mock_kms_plugin_deployer.go (1)
58-66: Return an error fromReadAssetinstead of panicking.This helper is now part of the wrapper startup path, so an asset lookup/render failure will crash the process with a stack trace and bypass
run()'s normal error handling.♻️ Proposed fix
-func ReadAsset(assetName string) []byte { +func ReadAsset(assetName string) ([]byte, error) { assetFunc := wrapAssetWithTemplateDataFunc(yamlTemplateData{Namespace: "default"}) - raw, err := assetFunc(assetName) - if err != nil { - panic(err) - } - return raw + return assetFunc(assetName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/k8s_mock_kms_plugin_deployer.go` around lines 58 - 66, The ReadAsset function currently panics on template/render failures; change its signature to return ([]byte, error) instead of []byte, call wrapAssetWithTemplateDataFunc(yamlTemplateData{Namespace: "default"}) as before, and propagate the error from assetFunc(assetName) by returning nil, err on failure (or raw, nil on success) so callers can handle the error instead of the process crashing; update all callers of ReadAsset to handle the returned error accordingly (e.g., check error and pass it up to run()'s error handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/library/encryption/kms/k8s-mock-plugin/Dockerfile`:
- Around line 11-12: The DaemonSet is invoking the upstream binary
"mock-kms-plugin" while the image copies a wrapper "mock-kms-plugin-wrapper";
either change the DaemonSet exec to call /usr/local/bin/mock-kms-plugin-wrapper
with the same flags (-listen-addr=unix:///var/run/kmsplugin/kms-{{ .Index
}}.sock -config-file-path=/etc/softhsm-config.json) so the wrapper is used, or
remove the COPY of /usr/local/bin/mock-kms-plugin-wrapper from the Dockerfile if
the wrapper is not needed; also ensure the wrapper binary is present and
executable in the image if you choose to call it.
In `@test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go`:
- Around line 40-42: The current logging in cmd.Flags().VisitAll prints raw flag
values (via pflag.Flag) which may expose secrets like --vault-address; update
the VisitAll handler to redact values by default and only print actual values
for a tiny allowlist of safe flags (e.g., define a safeFlags set and check
f.Name against it), otherwise log the flag name with a constant placeholder such
as "<redacted>" (e.g., use cmd.Flags().VisitAll with pflag.Flag f and
log.Printf("FLAG: --%s=%q", f.Name, "<redacted>") unless f.Name is in
safeFlags).
- Around line 95-99: The current token unpacking uses exec.Command to run "sh -c
'base64 -d | tar xzf -'" which shells out; replace that with in-process decoding
and extraction: in the function where tokensB64 and defaultTokensDir are used,
base64-decode tokensB64 using encoding/base64, then create a gzip.Reader and an
archive/tar.Reader to iterate entries and write files into defaultTokensDir
(creating directories as needed and preserving file modes), returning any errors
instead of relying on external stdout/stderr. Ensure to trim whitespace from
tokensB64 (strings.Fields or strings.TrimSpace) before decoding, check for
decode/gzip/tar errors, and set appropriate file permissions when writing files.
---
Nitpick comments:
In `@test/library/encryption/kms/k8s_mock_kms_plugin_deployer.go`:
- Around line 58-66: The ReadAsset function currently panics on template/render
failures; change its signature to return ([]byte, error) instead of []byte, call
wrapAssetWithTemplateDataFunc(yamlTemplateData{Namespace: "default"}) as before,
and propagate the error from assetFunc(assetName) by returning nil, err on
failure (or raw, nil on success) so callers can handle the error instead of the
process crashing; update all callers of ReadAsset to handle the returned error
accordingly (e.g., check error and pass it up to run()'s error handling).
🪄 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: Pro Plus
Run ID: 9b33970f-242f-41da-8d1e-3d2d5d587390
📒 Files selected for processing (5)
test/library/encryption/kms/k8s-mock-plugin/Dockerfiletest/library/encryption/kms/k8s-mock-plugin/README.mdtest/library/encryption/kms/k8s-mock-plugin/build-from-k8s.shtest/library/encryption/kms/k8s-mock-plugin/wrapper/main.gotest/library/encryption/kms/k8s_mock_kms_plugin_deployer.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
p0lyn0mial
force-pushed
the
kms-upstream-plugin-wrapper
branch
2 times, most recently
from
74a6c42 to
7f5ef4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go`:
- Around line 25-28: The options.vaultAddress field is validated as required but
never used in run(); either pass it into the upstream binary invocation or stop
requiring it. Fix by updating run() (or the code that builds the upstream exec
args in main/run) to include "--vault-address", options.vaultAddress among the
command-line args passed to the upstream process (e.g., when constructing args
for exec.Command), or if the upstream does not need it yet, remove the required
validation and the related flag requirement that checks options.vaultAddress so
the flag is optional; refer to the options struct, the vaultAddress field, and
the run() function to locate where to add the arg or remove the validation.
🪄 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: Pro Plus
Run ID: 57b77e3a-3212-4993-a144-c1df92a9f384
📒 Files selected for processing (5)
test/library/encryption/kms/k8s-mock-plugin/Dockerfiletest/library/encryption/kms/k8s-mock-plugin/README.mdtest/library/encryption/kms/k8s-mock-plugin/build-from-k8s.shtest/library/encryption/kms/k8s-mock-plugin/wrapper/main.gotest/library/encryption/kms/k8s_mock_kms_plugin_deployer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/encryption/kms/k8s-mock-plugin/README.md
Comment on lines
+25
to
+28
| type options struct { | ||
| vaultAddress string | ||
| listenAddr string | ||
| } |
There was a problem hiding this comment.
--vault-address is required but never used.
The vaultAddress field is validated as required (line 60-62) but never referenced in run(). The upstream binary is only invoked with --listen-addr and --config-file-path. Either:
- Pass
--vault-addressto the upstream binary if it needs it, or - Remove the flag requirement if it's not needed yet.
Option A: If vault-address should be passed to upstream
upstreamArgs := []string{
"-listen-addr=" + o.listenAddr,
"-config-file-path=" + defaultConfigPath,
+ "-vault-address=" + o.vaultAddress,
}Option B: If vault-address is for future use, don't require it yet
func (o *options) validate() error {
- if o.vaultAddress == "" {
- return fmt.Errorf("--vault-address must be set")
- }
if o.listenAddr == "" {
return fmt.Errorf("--listen-addr must be set")
}
return nil
}Also applies to: 54-57, 59-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go` around lines 25
- 28, The options.vaultAddress field is validated as required but never used in
run(); either pass it into the upstream binary invocation or stop requiring it.
Fix by updating run() (or the code that builds the upstream exec args in
main/run) to include "--vault-address", options.vaultAddress among the
command-line args passed to the upstream process (e.g., when constructing args
for exec.Command), or if the upstream does not need it yet, remove the required
validation and the related flag requirement that checks options.vaultAddress so
the flag is optional; refer to the options struct, the vaultAddress field, and
the run() function to locate where to add the arg or remove the validation.
gangwgr
reviewed
Apr 23, 2026
gangwgr
reviewed
Apr 23, 2026
p0lyn0mial
force-pushed
the
kms-upstream-plugin-wrapper
branch
from
7f5ef4f to
b262d27
Compare
p0lyn0mial
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@p0lyn0mial: This pull request references CNTRLPLANE-3237 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. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/library/encryption/kms/k8s-mock-plugin/Dockerfile (2)
11-12: Harden final image runtime user (currently implicit root).Final stage does not declare
USER, so runtime is root by default. The wrapper writes to/etc/softhsm-config.jsonand/var/lib/softhsm/tokens, which require non-root ownership. Consider running as non-root after preparing these writable paths.Example hardening direction
FROM ${UPSTREAM_IMAGE} COPY --from=builder /workspace/mock-kms-plugin-wrapper /usr/local/bin/ +RUN mkdir -p /var/lib/softhsm/tokens /etc \ + && chown -R 65532:65532 /var/lib/softhsm /etc /usr/local/bin/mock-kms-plugin-wrapper +USER 65532:65532🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/k8s-mock-plugin/Dockerfile` around lines 11 - 12, Final Docker stage runs as root by default and the wrapper binary mock-kms-plugin-wrapper writes to /etc/softhsm-config.json and /var/lib/softhsm/tokens; update the final stage to create a non-root user/group (e.g., kmsuser), mkdir and chown the required writable paths (/etc/softhsm-config.json parent and /var/lib/softhsm/tokens) to that user, and set USER to that non-root account before entry so the wrapper runs unprivileged; ensure any files copied from the builder are also owned by the non-root account.
6-7: Set a safe default forTARGETARCHto avoid brittle builds.The documented build method (
./build-from-k8s.sh) uses plaindocker buildwithout--platform, which does not set theTARGETARCHvariable. This causesGOARCH=${TARGETARCH}to expand to an empty string, breaking thego buildcommand. Add a default value (amd64) to ensure builds succeed.Proposed patch
-ARG TARGETARCH -RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGETARCH} go build -mod=vendor \ +ARG TARGETARCH=amd64 +RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGETARCH:-amd64} go build -mod=vendor \ -o mock-kms-plugin-wrapper \ ./test/library/encryption/kms/k8s-mock-plugin/wrapper/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/k8s-mock-plugin/Dockerfile` around lines 6 - 7, The build breaks when TARGETARCH is empty; update the Dockerfile to provide a safe default so GOARCH isn't empty: change the ARG TARGETARCH declaration to include a default (e.g., ARG TARGETARCH=amd64) and ensure the RUN that invokes go build (the line using CGO_ENABLED=0 GOOS=linux GOARCH=${TARGETARCH}) will therefore always have a valid GOARCH; locate the ARG TARGETARCH and the RUN line in the Dockerfile and set the default as described.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/library/encryption/kms/k8s-mock-plugin/Dockerfile`:
- Around line 11-12: Final Docker stage runs as root by default and the wrapper
binary mock-kms-plugin-wrapper writes to /etc/softhsm-config.json and
/var/lib/softhsm/tokens; update the final stage to create a non-root user/group
(e.g., kmsuser), mkdir and chown the required writable paths
(/etc/softhsm-config.json parent and /var/lib/softhsm/tokens) to that user, and
set USER to that non-root account before entry so the wrapper runs unprivileged;
ensure any files copied from the builder are also owned by the non-root account.
- Around line 6-7: The build breaks when TARGETARCH is empty; update the
Dockerfile to provide a safe default so GOARCH isn't empty: change the ARG
TARGETARCH declaration to include a default (e.g., ARG TARGETARCH=amd64) and
ensure the RUN that invokes go build (the line using CGO_ENABLED=0 GOOS=linux
GOARCH=${TARGETARCH}) will therefore always have a valid GOARCH; locate the ARG
TARGETARCH and the RUN line in the Dockerfile and set the default as described.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a78bf77c-cfa3-4d5f-abf7-a136076e7bc6
📒 Files selected for processing (5)
test/library/encryption/kms/k8s-mock-plugin/Dockerfiletest/library/encryption/kms/k8s-mock-plugin/README.mdtest/library/encryption/kms/k8s-mock-plugin/build-from-k8s.shtest/library/encryption/kms/k8s-mock-plugin/wrapper/main.gotest/library/encryption/kms/k8s_mock_kms_plugin_deployer.go
✅ Files skipped from review due to trivial changes (1)
- test/library/encryption/kms/k8s-mock-plugin/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- test/library/encryption/kms/k8s-mock-plugin/build-from-k8s.sh
- test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go
- test/library/encryption/kms/k8s_mock_kms_plugin_deployer.go
gangwgr
reviewed
Apr 23, 2026
Contributor
Author
|
I think the CI is happy about this PR |
Contributor
CI is green, we can merge |
Contributor
|
/lgtm |
bertinatto
reviewed
Apr 23, 2026
bertinatto
reviewed
Apr 23, 2026
p0lyn0mial
force-pushed
the
kms-upstream-plugin-wrapper
branch
from
b262d27 to
fdcd93c
Compare
p0lyn0mial
force-pushed
the
kms-upstream-plugin-wrapper
branch
from
fdcd93c to
51ae953
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/library/encryption/kms/k8s_mock_kms_plugin_deployer.go (1)
58-62: Avoid hardcoded namespace literal inReadAsset.At Line 60,
"default"can drift fromWellKnownUpstreamMockKMSPluginNamespaceand makes this exported helper less predictable.💡 Suggested tweak
func ReadAsset(assetName string) ([]byte, error) { - assetFunc := wrapAssetWithTemplateDataFunc(yamlTemplateData{Namespace: "default"}) + assetFunc := wrapAssetWithTemplateDataFunc(yamlTemplateData{ + Namespace: WellKnownUpstreamMockKMSPluginNamespace, + }) return assetFunc(assetName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/k8s_mock_kms_plugin_deployer.go` around lines 58 - 62, The ReadAsset helper currently hardcodes the namespace string "default"; replace that literal with the canonical constant WellKnownUpstreamMockKMSPluginNamespace so the helper stays in sync with the project-wide namespace. Update the call that constructs assetFunc (wrapAssetWithTemplateDataFunc) to pass yamlTemplateData{Namespace: WellKnownUpstreamMockKMSPluginNamespace} and ensure any imports or references resolve to the defined constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/library/encryption/kms/k8s_mock_kms_plugin_deployer.go`:
- Around line 58-62: The ReadAsset helper currently hardcodes the namespace
string "default"; replace that literal with the canonical constant
WellKnownUpstreamMockKMSPluginNamespace so the helper stays in sync with the
project-wide namespace. Update the call that constructs assetFunc
(wrapAssetWithTemplateDataFunc) to pass yamlTemplateData{Namespace:
WellKnownUpstreamMockKMSPluginNamespace} and ensure any imports or references
resolve to the defined constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20aa2e3d-01c6-4932-abd1-dbe47d3bb8d2
📒 Files selected for processing (5)
test/library/encryption/kms/k8s-mock-plugin/Dockerfiletest/library/encryption/kms/k8s-mock-plugin/README.mdtest/library/encryption/kms/k8s-mock-plugin/build-from-k8s.shtest/library/encryption/kms/k8s-mock-plugin/wrapper/main.gotest/library/encryption/kms/k8s_mock_kms_plugin_deployer.go
✅ Files skipped from review due to trivial changes (1)
- test/library/encryption/kms/k8s-mock-plugin/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go
Member
|
Changes look good to me. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go (1)
60-86:⚠️ Potential issue | 🟡 MinorMultiple required flags are validated but never used.
The following flags are validated as required but are never passed to the upstream binary in
run():
vaultAddressvaultNamespacetransitMounttransitKeyapproleRoleIDapproleSecretIDPathThe upstream mock KMS plugin only receives
--listen-addrand--config-file-path. Either pass these flags to the upstream binary, remove the required validation if they're for future use, or document why they're validated but unused (e.g., to match the real Vault KMS plugin interface for compatibility testing).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go` around lines 60 - 86, The options.validate() enforces required flags (vaultAddress, vaultNamespace, transitMount, transitKey, approleRoleID, approleSecretIDPath) but run() only forwards --listen-addr and --config-file-path to the upstream mock KMS; fix by either removing the required checks or by forwarding these flags: update run() to append arguments like "--vault-address", o.vaultAddress, "--vault-namespace", o.vaultNamespace, "--transit-mount", o.transitMount, "--transit-key", o.transitKey, "--approle-role-id", o.approleRoleID and "--approle-secret-id-path", o.approleSecretIDPath (if the upstream expects a path, ensure the file exists or read/validate it beforehand); locate the options struct, validate() and run() functions in main.go to implement the change consistently and adjust tests/documentation accordingly.
🧹 Nitpick comments (1)
test/library/encryption/kms/k8s-mock-plugin/Dockerfile (1)
1-13: Consider: Running as non-root user (test infrastructure context).Static analysis flags that the image runs as root (DS-0002). While this is test infrastructure and not production code, adding a non-root user could align with defense-in-depth practices. This is optional given the test-only scope.
♻️ Optional: Add non-root user
FROM ${UPSTREAM_IMAGE} COPY --from=builder /workspace/mock-kms-plugin-wrapper /usr/local/bin/vault-kube-kms +USER 65532:65532 ENTRYPOINT ["/usr/local/bin/vault-kube-kms"]Note: This depends on the upstream image having appropriate permissions and user setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/k8s-mock-plugin/Dockerfile` around lines 1 - 13, The image runs as root; add a non-root user and switch to it in the final stage to follow defense-in-depth: in the Dockerfile ensure the built binary mock-kms-plugin-wrapper (copied to /usr/local/bin/vault-kube-kms) is owned by a non-root user (create a user/group, chown the binary) and then set USER to that non-root account before ENTRYPOINT; you can create the user either in the final stage (referencing the FROM ${UPSTREAM_IMAGE} stage) or ensure the upstream image supports the UID/GID used, and keep ARG/TARGETARCH and builder steps unchanged (symbols to edit: mock-kms-plugin-wrapper, vault-kube-kms, ENTRYPOINT, FROM ${UPSTREAM_IMAGE}).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/library/encryption/kms/k8s-mock-plugin/wrapper/main.go`:
- Around line 60-86: The options.validate() enforces required flags
(vaultAddress, vaultNamespace, transitMount, transitKey, approleRoleID,
approleSecretIDPath) but run() only forwards --listen-addr and
--config-file-path to the upstream mock KMS; fix by either removing the required
checks or by forwarding these flags: update run() to append arguments like
"--vault-address", o.vaultAddress, "--vault-namespace", o.vaultNamespace,
"--transit-mount", o.transitMount, "--transit-key", o.transitKey,
"--approle-role-id", o.approleRoleID and "--approle-secret-id-path",
o.approleSecretIDPath (if the upstream expects a path, ensure the file exists or
read/validate it beforehand); locate the options struct, validate() and run()
functions in main.go to implement the change consistently and adjust
tests/documentation accordingly.
---
Nitpick comments:
In `@test/library/encryption/kms/k8s-mock-plugin/Dockerfile`:
- Around line 1-13: The image runs as root; add a non-root user and switch to it
in the final stage to follow defense-in-depth: in the Dockerfile ensure the
built binary mock-kms-plugin-wrapper (copied to /usr/local/bin/vault-kube-kms)
is owned by a non-root user (create a user/group, chown the binary) and then set
USER to that non-root account before ENTRYPOINT; you can create the user either
in the final stage (referencing the FROM ${UPSTREAM_IMAGE} stage) or ensure the
upstream image supports the UID/GID used, and keep ARG/TARGETARCH and builder
steps unchanged (symbols to edit: mock-kms-plugin-wrapper, vault-kube-kms,
ENTRYPOINT, FROM ${UPSTREAM_IMAGE}).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4fbc97d4-2df2-4db2-aa64-b07016ef7ddc
📒 Files selected for processing (5)
test/library/encryption/kms/k8s-mock-plugin/Dockerfiletest/library/encryption/kms/k8s-mock-plugin/README.mdtest/library/encryption/kms/k8s-mock-plugin/build-from-k8s.shtest/library/encryption/kms/k8s-mock-plugin/wrapper/main.gotest/library/encryption/kms/k8s_mock_kms_plugin_deployer.go
✅ Files skipped from review due to trivial changes (1)
- test/library/encryption/kms/k8s-mock-plugin/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/encryption/kms/k8s-mock-plugin/build-from-k8s.sh
p0lyn0mial
force-pushed
the
kms-upstream-plugin-wrapper
branch
from
51ae953 to
bff854c
Compare
p0lyn0mial
commented
Apr 27, 2026
|
|
||
| // WellKnownUpstreamMockKMSPluginImage is the pre-built mock KMS plugin image. | ||
| WellKnownUpstreamMockKMSPluginImage = "quay.io/openshifttest/mock-kms-plugin@sha256:998e1d48eba257f589ab86c30abd5043f662213e9aeff253e1c308301879d48a" | ||
| WellKnownUpstreamMockKMSPluginImage = "quay.io/openshifttest/mock-kms-plugin@sha256:ab7a28ca60966753256db2d5d7df54e4fb7f4d9cf2cb6612056baf62eb997e37" |
Contributor
Author
There was a problem hiding this comment.
i'd like build an image and then update this line so that the test could use it.
Co-Authored-By: Lukasz Szaszkiewicz <lszaszki@redhat.com> Co-Authored-By: gangwgr <rgangwar@redhat.com>
p0lyn0mial
force-pushed
the
kms-upstream-plugin-wrapper
branch
from
bff854c to
115beeb
Compare
Member
|
/lgtm |
bertinatto
reviewed
Apr 27, 2026
Member
bertinatto
left a comment
bertinatto
left a comment
There was a problem hiding this comment.
/hold cancel
/lgtm
All the parameters I need for the lifecycle code are working.
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, bertinatto, gangwgr, p0lyn0mial 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 |
Contributor
|
@p0lyn0mial: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores