NO-JIRA: vault KMS fake plugin#2164
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
WalkthroughIntroduces a new mock vault KMS plugin implementation in Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Wrapper as vault-kube-kms<br/>(wrapper)
participant Mock as mock-kms-plugin<br/>(upstream)
participant SoftHSM as SoftHSM<br/>(token database)
User->>Wrapper: invoke with vault-kube-kms flags<br/>(--listen-address, --timeout, etc.)
Wrapper->>Wrapper: parse flags
Wrapper->>Wrapper: translate --listen-address to -listen-addr<br/>inject -config-file-path
Wrapper->>Mock: syscall.Exec() with translated args
Mock->>Mock: initialize with SoftHSM config path
Mock->>SoftHSM: load token & AES key config
SoftHSM-->>Mock: token/key ready
Mock-->>User: serve KMS requests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/library/encryption/kms/mock-vault-plugin/main.go (2)
128-136: Callingos.Exit(1)in the goroutine may race with graceful shutdown.When a shutdown signal arrives,
grpcService.Shutdown()callsGracefulStop(), which causesListenAndServe()to return. However, the error returned byListenAndServe()after a graceful stop is typicallynilor a benign "server closed" error—not a fatal condition. Exiting with code 1 here could preempt the clean shutdown path.♻️ Suggested fix: track server error via channel
+ errCh := make(chan error, 1) go func() { - if err := grpcService.ListenAndServe(); err != nil { - fmt.Fprintf(os.Stderr, "gRPC server error: %v\n", err) - os.Exit(1) - } + errCh <- grpcService.ListenAndServe() }() - <-ctx.Done() - grpcService.Shutdown() + select { + case err := <-errCh: + if err != nil { + fmt.Fprintf(os.Stderr, "gRPC server error: %v\n", err) + os.Exit(1) + } + case <-ctx.Done(): + grpcService.Shutdown() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/main.go` around lines 128 - 136, The goroutine that runs grpcService.ListenAndServe currently calls os.Exit(1) on any error which can race with graceful shutdown; change it to send the returned error to a dedicated errCh (e.g., make errCh := make(chan error,1)) instead of exiting, then in the main routine select on ctx.Done() and errCh to distinguish real Serve errors from normal shutdown, calling grpcService.Shutdown() on ctx.Done() and only exit/non-zero when errCh yields an actual fatal error; reference grpcService.ListenAndServe, grpcService.Shutdown, and the new errCh in your changes.
141-150:os.Interruptis redundant withsyscall.SIGINTon Unix.On Unix systems,
os.Interruptmaps toSIGINT. Registering both is harmless but unnecessary.♻️ Suggested fix
- signal.Notify(signalChan, syscall.SIGTERM, syscall.SIGINT, os.Interrupt) + signal.Notify(signalChan, syscall.SIGTERM, syscall.SIGINT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/main.go` around lines 141 - 150, In withShutdownSignal, remove the redundant os.Interrupt from the signal.Notify call (keep syscall.SIGTERM and syscall.SIGINT) so the function registers only SIGTERM and SIGINT; update the signal.Notify invocation in the withShutdownSignal function accordingly to avoid duplicate registration on Unix.test/library/encryption/kms/mock-vault-plugin/go.mod (1)
1-1: Module path doesn't match the repository.The module is declared as
github.com/gangwgr/mock-vault-kmsbut it's being added toopenshift/library-go. Consider using a path under the target repository to avoid import path confusion.♻️ Suggested fix
-module github.com/gangwgr/mock-vault-kms +module github.com/openshift/library-go/test/library/encryption/kms/mock-vault-plugin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/go.mod` at line 1, The module path in go.mod ("module github.com/gangwgr/mock-vault-kms") does not match the repository; update the module declaration in go.mod to a path under the target repository (for example change the module line to a path under openshift/library-go that reflects the package location) so imports resolve correctly; edit the module line in go.mod to the repository-correct module path and run `go mod tidy` to refresh module metadata (look for the module declaration in go.mod to change).test/library/encryption/kms/mock-vault-plugin/Dockerfile (1)
1-3: Consider running as non-root for defense in depth.The static analysis tool flagged that the container runs as root by default. While this is a test-only mock plugin, running as non-root is a security best practice. With a
scratchbase, you can specify a numeric UID.♻️ Suggested fix
FROM scratch +USER 65532:65532 COPY mock-vault-kms /vault-kube-kms ENTRYPOINT ["/vault-kube-kms"]Note: UID 65532 is commonly used as a "nonroot" user in distroless/minimal images. Ensure the socket path has appropriate permissions if this change is applied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/Dockerfile` around lines 1 - 3, The image runs the mock-vault-kms binary as root by default; update the Dockerfile to run as a non-root numeric UID (e.g. USER 65532) so /vault-kube-kms executes unprivileged, and ensure the plugin’s socket path (the directory where the plugin writes its socket) is owned and writable by that UID (adjust ownership/permissions during build or have the binary create the socket with the correct mode) so the plugin can still bind the socket when running as USER 65532.
🤖 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/mock-vault-plugin/build-from-source.sh`:
- Around line 1-9: The README refers to ./build-and-push.sh but the script in
the repo is named build-from-source.sh; fix this by either renaming
build-from-source.sh to build-and-push.sh (preserve executable bit and existing
variables like FULL_IMAGE and SCRIPT_DIR) or update the README to reference
build-from-source.sh (and any CI/job configs) so names match; ensure any calls
to the script (e.g., in CI or docs) are updated accordingly.
In `@test/library/encryption/kms/mock-vault-plugin/README.md`:
- Around line 54-56: The README references a non-existent script name; update
the invocation in README.md to use the actual script filename by replacing
occurrences of "./build-and-push.sh" with "./build-from-source.sh" so the
documented command matches the real script (look for the code block containing
./build-and-push.sh and change it to ./build-from-source.sh).
- Around line 60-62: Update the README example command to call the correct
script: replace the incorrect ./build-and-push.sh invocation with
./build-from-source.sh in the mock-vault-plugin README example (the block
showing REGISTRY=... IMAGE_NAME=... TAG=... ./build-and-push.sh) so the
documented example matches the actual script name.
---
Nitpick comments:
In `@test/library/encryption/kms/mock-vault-plugin/Dockerfile`:
- Around line 1-3: The image runs the mock-vault-kms binary as root by default;
update the Dockerfile to run as a non-root numeric UID (e.g. USER 65532) so
/vault-kube-kms executes unprivileged, and ensure the plugin’s socket path (the
directory where the plugin writes its socket) is owned and writable by that UID
(adjust ownership/permissions during build or have the binary create the socket
with the correct mode) so the plugin can still bind the socket when running as
USER 65532.
In `@test/library/encryption/kms/mock-vault-plugin/go.mod`:
- Line 1: The module path in go.mod ("module github.com/gangwgr/mock-vault-kms")
does not match the repository; update the module declaration in go.mod to a path
under the target repository (for example change the module line to a path under
openshift/library-go that reflects the package location) so imports resolve
correctly; edit the module line in go.mod to the repository-correct module path
and run `go mod tidy` to refresh module metadata (look for the module
declaration in go.mod to change).
In `@test/library/encryption/kms/mock-vault-plugin/main.go`:
- Around line 128-136: The goroutine that runs grpcService.ListenAndServe
currently calls os.Exit(1) on any error which can race with graceful shutdown;
change it to send the returned error to a dedicated errCh (e.g., make errCh :=
make(chan error,1)) instead of exiting, then in the main routine select on
ctx.Done() and errCh to distinguish real Serve errors from normal shutdown,
calling grpcService.Shutdown() on ctx.Done() and only exit/non-zero when errCh
yields an actual fatal error; reference grpcService.ListenAndServe,
grpcService.Shutdown, and the new errCh in your changes.
- Around line 141-150: In withShutdownSignal, remove the redundant os.Interrupt
from the signal.Notify call (keep syscall.SIGTERM and syscall.SIGINT) so the
function registers only SIGTERM and SIGINT; update the signal.Notify invocation
in the withShutdownSignal function accordingly to avoid duplicate registration
on Unix.
🪄 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: d25b8f93-1210-4aea-9c5c-5f7fb37cb4c3
⛔ Files ignored due to path filters (1)
test/library/encryption/kms/mock-vault-plugin/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
test/library/encryption/kms/mock-vault-plugin/Dockerfiletest/library/encryption/kms/mock-vault-plugin/README.mdtest/library/encryption/kms/mock-vault-plugin/build-from-source.shtest/library/encryption/kms/mock-vault-plugin/go.modtest/library/encryption/kms/mock-vault-plugin/main.go
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| REGISTRY="${REGISTRY:-quay.io}" | ||
| REPO="${REPO:-openshifttest}" | ||
| IMAGE_NAME="${IMAGE_NAME:-mock-kms-plugin-vault}" | ||
| TAG="${TAG:-latest}" | ||
| FULL_IMAGE="${REGISTRY}/${REPO}/${IMAGE_NAME}:${TAG}" | ||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" |
There was a problem hiding this comment.
Script name mismatch with README documentation.
The README references ./build-and-push.sh but this file is named build-from-source.sh. Either rename the script or update the README.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/library/encryption/kms/mock-vault-plugin/build-from-source.sh` around
lines 1 - 9, The README refers to ./build-and-push.sh but the script in the repo
is named build-from-source.sh; fix this by either renaming build-from-source.sh
to build-and-push.sh (preserve executable bit and existing variables like
FULL_IMAGE and SCRIPT_DIR) or update the README to reference
build-from-source.sh (and any CI/job configs) so names match; ensure any calls
to the script (e.g., in CI or docs) are updated accordingly.
| // service so the KMS plugin lifecycle controller can be tested without a | ||
| // real Vault Enterprise instance. | ||
| // | ||
| // Reference: https://github.com/kubernetes/kms/tree/main/internal/plugins/_mock |
There was a problem hiding this comment.
I would prefer to reuse https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/kms/internal/plugins/_mock instead of creating our own mock.
is there a way to wrap the original mock into a new binary?
gangwgr
force-pushed
the
mock-vault-plugin
branch
from
112881d to
c8729c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/library/encryption/kms/mock-vault-plugin/Dockerfile (1)
1-1: Pin base images instead of:latestfor reproducible builds.Lines 1 and 6 use mutable tags, which can change behavior without code changes and make failures hard to reproduce. Prefer explicit version tags (builder) and digest pinning (runtime image).
♻️ Suggested direction
-FROM golang:latest AS builder +FROM golang:1.24.2 AS builder ... -FROM quay.io/openshifttest/mock-kms-plugin:latest +FROM quay.io/openshifttest/mock-kms-plugin@sha256:<pinned_digest>Also applies to: 6-6
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/Dockerfile` at line 1, The Dockerfile uses mutable image tags (e.g., "FROM golang:latest AS builder") which makes builds unreproducible; update the build stage and the runtime stage (the second FROM at line 6) to use specific, immutable tags or digests (for example a concrete Go version tag like "golang:1.20.7" for the builder and a versioned image or digest for the runtime) so builds are reproducible and stability is ensured; locate the "AS builder" stage and the later runtime FROM and replace ":latest" with explicit version tags or digests.
🤖 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/mock-vault-plugin/main.go`:
- Line 43: After calling flag.Parse() in main(), add a fast-fail check that
inspects flag.Args(); if len(flag.Args()) > 0 write a clear error message to
stderr and terminate with os.Exit(2) to reject unexpected positional arguments
(use flag.Parse(), flag.Args(), and os.Exit(2) to implement).
In `@test/library/encryption/kms/mock-vault-plugin/README.md`:
- Around line 13-30: The fenced code block containing the architecture diagram
(the triple-backtick block that begins with "Lifecycle controller passes Vault
flags" and ends with "SoftHSM / PKCS#11 encryption on unix socket") is missing a
language tag; add the language identifier `text` after the opening ``` so the
block becomes ```text to satisfy MD040 linting while leaving the diagram
contents unchanged.
---
Nitpick comments:
In `@test/library/encryption/kms/mock-vault-plugin/Dockerfile`:
- Line 1: The Dockerfile uses mutable image tags (e.g., "FROM golang:latest AS
builder") which makes builds unreproducible; update the build stage and the
runtime stage (the second FROM at line 6) to use specific, immutable tags or
digests (for example a concrete Go version tag like "golang:1.20.7" for the
builder and a versioned image or digest for the runtime) so builds are
reproducible and stability is ensured; locate the "AS builder" stage and the
later runtime FROM and replace ":latest" with explicit version tags or digests.
🪄 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: 8bdbb20d-827e-49f6-a419-0518b23a2cd8
📒 Files selected for processing (5)
test/library/encryption/kms/mock-vault-plugin/Dockerfiletest/library/encryption/kms/mock-vault-plugin/README.mdtest/library/encryption/kms/mock-vault-plugin/build-from-source.shtest/library/encryption/kms/mock-vault-plugin/go.modtest/library/encryption/kms/mock-vault-plugin/main.go
✅ Files skipped from review due to trivial changes (2)
- test/library/encryption/kms/mock-vault-plugin/go.mod
- test/library/encryption/kms/mock-vault-plugin/build-from-source.sh
| _ = flag.String("log-level", "info", "(ignored) Log level") | ||
| _ = flag.String("metrics-port", "8080", "(ignored) Metrics/health port") | ||
| _ = flag.Bool("disable-runtime-metrics", false, "(ignored) Disable Go runtime metrics") | ||
| flag.Parse() |
There was a problem hiding this comment.
Fail fast on unexpected positional arguments.
Line 43 parses flags, but any extra positional args are silently ignored. This can hide misconfigured invocation parameters in test infra. Consider rejecting non-empty flag.Args() with exit code 2.
💡 Proposed fix
flag.Parse()
+if len(flag.Args()) > 0 {
+ fmt.Fprintf(os.Stderr, "vault-kube-kms wrapper: unexpected positional args: %v\n", flag.Args())
+ os.Exit(2)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| flag.Parse() | |
| flag.Parse() | |
| if len(flag.Args()) > 0 { | |
| fmt.Fprintf(os.Stderr, "vault-kube-kms wrapper: unexpected positional args: %v\n", flag.Args()) | |
| os.Exit(2) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/library/encryption/kms/mock-vault-plugin/main.go` at line 43, After
calling flag.Parse() in main(), add a fast-fail check that inspects flag.Args();
if len(flag.Args()) > 0 write a clear error message to stderr and terminate with
os.Exit(2) to reject unexpected positional arguments (use flag.Parse(),
flag.Args(), and os.Exit(2) to implement).
| ``` | ||
| Lifecycle controller passes Vault flags | ||
| │ | ||
| ▼ | ||
| /vault-kube-kms (this wrapper) | ||
| --listen-address=unix://... → translates to -listen-addr | ||
| --vault-address=... → dropped | ||
| --transit-key=... → dropped | ||
| ... → dropped | ||
| │ | ||
| ▼ syscall.Exec | ||
| /usr/local/bin/mock-kms-plugin (upstream Kubernetes mock) | ||
| -listen-addr=unix://... | ||
| -config-file-path=/etc/softhsm-config.json | ||
| │ | ||
| ▼ | ||
| SoftHSM / PKCS#11 encryption on unix socket | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced block to satisfy markdown linting.
Line 13 opens a fenced block without a language tag (MD040). Use text for this architecture diagram.
📝 Proposed fix
-```
+```text
Lifecycle controller passes Vault flags
│
▼
/vault-kube-kms (this wrapper)
@@
SoftHSM / PKCS#11 encryption on unix socket</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @test/library/encryption/kms/mock-vault-plugin/README.md around lines 13 -
30, The fenced code block containing the architecture diagram (the
triple-backtick block that begins with "Lifecycle controller passes Vault flags"
and ends with "SoftHSM / PKCS#11 encryption on unix socket") is missing a
language tag; add the language identifier text after the opening so the block becomestext to satisfy MD040 linting while leaving the diagram
contents unchanged.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:226787d8-6535-4b83-9d1f-5d931752c947 -->
<!-- This is an auto-generated comment by CodeRabbit -->
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
gangwgr
force-pushed
the
mock-vault-plugin
branch
from
c8729c3 to
cd6e791
Compare
|
|
||
| func main() { | ||
| listenAddr := flag.String("listen-address", "unix:///var/run/kmsplugin/kms.sock", "gRPC listen address") | ||
| configFile := flag.String("config-file-path", defaultConfigFile, "SoftHSM config file path") |
There was a problem hiding this comment.
Plugin lifecycle is supposed to be pass Vault compatible arguments and it won't pass config-file-path. Can't we internalize this directly in the code? or my statement is far from reality?
There was a problem hiding this comment.
@ardaguclu do we need to remove --config-file-path as a CLI flag and just hardcode the default path directly in the code?
There was a problem hiding this comment.
Just hardcoding to that path is not enough. Wrapper image needs to prepare this config as well. So that consumer just passes vault flags.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/library/encryption/kms/mock-vault-plugin/README.md (1)
13-13:⚠️ Potential issue | 🟡 MinorAdd language tag to fenced code block.
The fenced code block containing the architecture diagram is missing a language specifier, which triggers a markdown linting warning (MD040).
📝 Proposed fix
-``` +```text Lifecycle controller passes Vault flags │ ▼🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/README.md` at line 13, The fenced code block in README.md is missing a language tag causing MD040; update the opening fence for the architecture diagram (the triple backticks that precede the diagram) to include a language identifier such as text (e.g., change ``` to ```text) so the block is explicitly marked and the markdown lint warning is resolved.
🧹 Nitpick comments (1)
test/library/encryption/kms/mock-vault-plugin/Dockerfile (1)
1-1: Pin image versions for reproducible builds.Both the builder (
golang:latest) and base image (quay.io/openshifttest/mock-kms-plugin:latest) use the:latesttag, which can cause non-deterministic builds when upstream images change.📌 Proposed fix to pin specific versions
-FROM golang:latest AS builder +FROM golang:1.23 AS builder WORKDIR /workspace COPY main.go go.mod ./ RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags="-s -w" -o /vault-kube-kms . -FROM quay.io/openshifttest/mock-kms-plugin:latest +FROM quay.io/openshifttest/mock-kms-plugin:v1.0.0 COPY --from=builder /vault-kube-kms /vault-kube-kms ENTRYPOINT ["/vault-kube-kms"]Note: Replace
v1.0.0with the actual tagged version of the base image.Also applies to: 6-6
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/Dockerfile` at line 1, The Dockerfile uses unpinned images (FROM golang:latest and the base image quay.io/openshifttest/mock-kms-plugin:latest) which causes non-reproducible builds; update the Dockerfile to replace these :latest tags with specific, tested image tags (e.g., golang:1.20.7 or your approved Go version and the concrete quay.io/openshifttest/mock-kms-plugin:vX.Y.Z) so builds are deterministic—change the FROM lines accordingly and ensure any related CI/build docs are updated to reference the pinned tags used.
🤖 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/mock-vault-plugin/README.md`:
- Line 13: The fenced code block in README.md is missing a language tag causing
MD040; update the opening fence for the architecture diagram (the triple
backticks that precede the diagram) to include a language identifier such as
text (e.g., change ``` to ```text) so the block is explicitly marked and the
markdown lint warning is resolved.
---
Nitpick comments:
In `@test/library/encryption/kms/mock-vault-plugin/Dockerfile`:
- Line 1: The Dockerfile uses unpinned images (FROM golang:latest and the base
image quay.io/openshifttest/mock-kms-plugin:latest) which causes
non-reproducible builds; update the Dockerfile to replace these :latest tags
with specific, tested image tags (e.g., golang:1.20.7 or your approved Go
version and the concrete quay.io/openshifttest/mock-kms-plugin:vX.Y.Z) so builds
are deterministic—change the FROM lines accordingly and ensure any related
CI/build docs are updated to reference the pinned tags used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 509ee8cf-59ea-4658-9a69-e707ea0d4599
📒 Files selected for processing (5)
test/library/encryption/kms/mock-vault-plugin/Dockerfiletest/library/encryption/kms/mock-vault-plugin/README.mdtest/library/encryption/kms/mock-vault-plugin/build-from-source.shtest/library/encryption/kms/mock-vault-plugin/go.modtest/library/encryption/kms/mock-vault-plugin/main.go
✅ Files skipped from review due to trivial changes (3)
- test/library/encryption/kms/mock-vault-plugin/go.mod
- test/library/encryption/kms/mock-vault-plugin/build-from-source.sh
- test/library/encryption/kms/mock-vault-plugin/main.go
There was a problem hiding this comment.
The entrypoint should be able to run the following params for the lifecycle code, however, it's currently returning an error because it can't read a softhsm file:
$ mock-vault-plugin ❯ podman run --rm -it quay.io/bertinatto/mock-kms-plugin-vault:latest
-listen-address=unix:///tmp/kmsplugin/kms-1.sock \
-vault-address=vault-address \
-vault-namespace=vault-namespace \
-transit-mount=transit-mount \
-transit-key=transit-key \
-log-level=debug-extended \
-approle-role-id=1234-1234-1234 \
-approle-secret-id-path=/tmp/secret-id
vault-kube-kms wrapper: translating Vault flags → upstream mock
vault-kube-kms wrapper: exec /usr/local/bin/mock-kms-plugin [-listen-addr=unix:///var/run/kmsplugin/kms.sock -config-file-path=/etc/softhsm-config.json]
panic: failed to create remote service: could not open config file: /etc/softhsm-config.json: open /etc/softhsm-config.json: no such file or directory
goroutine 1 [running]:
main.main()
/workspace/kms/internal/plugins/_mock/plugin.go:48 +0x205
bash: -listen-address=unix:///tmp/kmsplugin/kms-1.sock: No such file or directory
| COPY main.go go.mod ./ | ||
| RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags="-s -w" -o /vault-kube-kms . | ||
|
|
||
| FROM quay.io/openshifttest/mock-kms-plugin:latest |
There was a problem hiding this comment.
Should we use our standard CI base images instead?
There was a problem hiding this comment.
we can use, but it provides the upstream mock binary and SoftHSM library that our wrapper exec's into our image so we don't want want to installation of softshm that's why I didn't use
|
|
||
| The wrapper binary (`/vault-kube-kms`) accepts all vault-kube-kms flags, translates | ||
| the relevant ones, and `exec`s the upstream `mock-kms-plugin` binary. This allows the | ||
| OpenShift plugin lifecycle controller to use this image as a drop-in `VaultImage` |
There was a problem hiding this comment.
nit: for now there's is no plugin lifecycle controller
| - Wrapper is a static Go binary (~2MB, no CGO) | ||
| - Base image is the upstream `quay.io/openshifttest/mock-kms-plugin` (Alpine + SoftHSM) | ||
| - Encryption is handled by upstream mock via SoftHSM/PKCS#11 | ||
| - Init container is still needed to unpack SoftHSM tokens |
There was a problem hiding this comment.
How does this work? Will be able to simply call the wrapper or there needs to be some preprocessing?
There was a problem hiding this comment.
no preprocessing needed — the wrapper can be called directly.
| ## Running as VaultImage (lifecycle controller) | ||
|
|
||
| The lifecycle controller reads `VaultImage` from the APIServer CRD and | ||
| automatically writes a static pod manifest to each control-plane node: |
There was a problem hiding this comment.
The plugin lifecycle will create sidecar containers that run alongside the apiservers, it won't write static pod manifests.
| } | ||
| } | ||
| } | ||
| }' |
There was a problem hiding this comment.
This doesn't currently work because we don't have the api yet.
gangwgr
force-pushed
the
mock-vault-plugin
branch
from
cd6e791 to
a223fc8
Compare
|
@bertinatto updated pr |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/library/encryption/kms/mock-vault-plugin/README.md (2)
13-30:⚠️ Potential issue | 🟡 MinorAdd a language tag to the diagram block.
This still triggers MD040.
textis enough here.📝 Proposed fix
-``` +```text Operator passes Vault flags via sidecar injection │ ▼ /vault-kube-kms (this wrapper) @@ SoftHSM / PKCS#11 encryption on unix socket</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@test/library/encryption/kms/mock-vault-plugin/README.mdaround lines 13 -
30, The fenced diagram block in the README (the code block that starts with
"Operator passes Vault flags via sidecar injection" and contains lines like
"/vault-kube-kms") is missing a language tag and triggers MD040; update that
fenced code block by adding the language tag "text" after the opening triple
backticks (i.e., changetotext) so the linter accepts the diagram.</details> --- `14-14`: _⚠️ Potential issue_ | _🟡 Minor_ **Mark the VaultImage flow as planned, not current.** The diagram and the `oc patch` example read like a working integration, but this path is not available yet. Please label it as future work or remove the copy-pasteable example for now. <details> <summary>📝 Proposed wording</summary> ```diff -Operator passes Vault flags via sidecar injection +Planned flow once VaultImage integration exists ``` ```diff -## Running as VaultImage +## Planned VaultImage integration -The operator reads `VaultImage` from the APIServer CRD and injects the -KMS plugin as a sidecar into API server pods: +Once the API/operator wiring exists, the intended flow is: ``` </details> Also applies to: 88-106 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/README.md` at line 14, The README currently presents the "VaultImage" flow (the line "Operator passes Vault flags via sidecar injection" plus the oc patch example and diagram) as an active integration; update the wording to mark this flow as planned/future work (e.g., prepend "Planned:" or "Future work:" to the heading/section and add a short note that the oc patch example is not yet functional) or remove the copy-pasteable oc patch example entirely; locate the section by searching for the "Operator passes Vault flags via sidecar injection" text, the "VaultImage" flow references, and the oc patch snippet in the README and change the language accordingly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@test/library/encryption/kms/mock-vault-plugin/Dockerfile:
- Around line 1-4: The Dockerfile currently forces an amd64 binary in the
builder stage by setting GOOS=linux GOARCH=amd64 in the RUN that produces
/vault-kube-kms, which will fail on arm64 nodes; update the RUN in the builder
stage (the line that runs go build and produces /vault-kube-kms) to build for
the image's target platform instead — either remove the GOARCH/GOOS overrides so
the binary is built for the build environment or wire in the build-time platform
via build args/BUILDPLATFORM and use that to set GOOS/GOARCH (or use docker
buildx multi-platform builds) so the produced /vault-kube-kms matches the node
architecture.- Around line 8-21: The SoftHSM token generation stage uses alpine:latest which
may differ from the runtime image and cause ABI incompatibilities; update the
Dockerfile to use the runtime image as the base for the keygen stage by
replacing "FROM alpine:latest AS softhsm-keygen" with "FROM
quay.io/openshifttest/mock-kms-plugin:latest AS softhsm-keygen", then adjust or
remove the apk/softhsm/opensc installs if the runtime image already provides
them, preserving the token creation steps that write /var/lib/softhsm/tokens and
/etc/softhsm2.conf so the COPY --from=softhsm-keygen targets remain valid.
Duplicate comments:
In@test/library/encryption/kms/mock-vault-plugin/README.md:
- Around line 13-30: The fenced diagram block in the README (the code block that
starts with "Operator passes Vault flags via sidecar injection" and contains
lines like "/vault-kube-kms") is missing a language tag and triggers MD040;
update that fenced code block by adding the language tag "text" after the
opening triple backticks (i.e., changetotext) so the linter accepts the
diagram.- Line 14: The README currently presents the "VaultImage" flow (the line
"Operator passes Vault flags via sidecar injection" plus the oc patch example
and diagram) as an active integration; update the wording to mark this flow as
planned/future work (e.g., prepend "Planned:" or "Future work:" to the
heading/section and add a short note that the oc patch example is not yet
functional) or remove the copy-pasteable oc patch example entirely; locate the
section by searching for the "Operator passes Vault flags via sidecar injection"
text, the "VaultImage" flow references, and the oc patch snippet in the README
and change the language accordingly.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository: openshift/coderabbit/.coderabbit.yaml **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `875df7d1-9dc4-43f2-90ed-057b9e880fe0` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cd6e7910c8b3fb7911b9edc461f297f06f092e11 and a223fc8d5435c88b1e1786220594a4951559da59. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `test/library/encryption/kms/mock-vault-plugin/Dockerfile` * `test/library/encryption/kms/mock-vault-plugin/README.md` * `test/library/encryption/kms/mock-vault-plugin/build-from-source.sh` * `test/library/encryption/kms/mock-vault-plugin/go.mod` * `test/library/encryption/kms/mock-vault-plugin/main.go` * `test/library/encryption/kms/mock-vault-plugin/mock-vault-plugin` * `test/library/encryption/kms/mock-vault-plugin/softhsm-config.json` </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * test/library/encryption/kms/mock-vault-plugin/go.mod * test/library/encryption/kms/mock-vault-plugin/softhsm-config.json * test/library/encryption/kms/mock-vault-plugin/build-from-source.sh </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * test/library/encryption/kms/mock-vault-plugin/main.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
gangwgr
force-pushed
the
mock-vault-plugin
branch
from
a223fc8 to
3ddf8aa
Compare
|
@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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/library/encryption/kms/mock-vault-plugin/Dockerfile (1)
20-27:⚠️ Potential issue | 🟠 MajorRun the final image as non-root.
Line 20-Line 27 defines no
USER, so the container defaults to root. Please drop privileges in the final stage and adjust file permissions accordingly.🔧 Proposed hardening change
FROM quay.io/openshifttest/mock-kms-plugin:latest COPY softhsm-config.json /etc/softhsm-config.json COPY --from=softhsm-keygen /var/lib/softhsm/tokens /var/lib/softhsm/tokens COPY --from=softhsm-keygen /etc/softhsm2.conf /etc/softhsm2.conf COPY --from=builder /vault-kube-kms /vault-kube-kms +RUN chgrp -R 0 /var/lib/softhsm /etc/softhsm2.conf /vault-kube-kms && \ + chmod -R g=u /var/lib/softhsm /etc/softhsm2.conf /vault-kube-kms +USER 1001 ENTRYPOINT ["/vault-kube-kms"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/mock-vault-plugin/Dockerfile` around lines 20 - 27, The final Dockerfile stage runs as root by default; create a non-root user (e.g., vaultkms) and switch to it before the ENTRYPOINT, ensuring all copied files/executables (/vault-kube-kms, /var/lib/softhsm/tokens, /etc/softhsm2.conf, /etc/softhsm-config.json) are chowned and have appropriate restrictive permissions so the non-root user can read/execute them; update ownership and permissions in the final stage (using chown/chmod on those unique artifacts) and add a USER vaultkms line prior to ENTRYPOINT ["/vault-kube-kms"] so the container drops privileges.
🤖 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/mock-vault-plugin/Dockerfile`:
- Around line 20-27: The final Dockerfile stage runs as root by default; create
a non-root user (e.g., vaultkms) and switch to it before the ENTRYPOINT,
ensuring all copied files/executables (/vault-kube-kms, /var/lib/softhsm/tokens,
/etc/softhsm2.conf, /etc/softhsm-config.json) are chowned and have appropriate
restrictive permissions so the non-root user can read/execute them; update
ownership and permissions in the final stage (using chown/chmod on those unique
artifacts) and add a USER vaultkms line prior to ENTRYPOINT ["/vault-kube-kms"]
so the container drops privileges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f845ad8-44c7-4fab-a6cc-96a1ad875669
📒 Files selected for processing (7)
test/library/encryption/kms/mock-vault-plugin/Dockerfiletest/library/encryption/kms/mock-vault-plugin/README.mdtest/library/encryption/kms/mock-vault-plugin/build-from-source.shtest/library/encryption/kms/mock-vault-plugin/go.modtest/library/encryption/kms/mock-vault-plugin/main.gotest/library/encryption/kms/mock-vault-plugin/mock-vault-plugintest/library/encryption/kms/mock-vault-plugin/softhsm-config.json
✅ Files skipped from review due to trivial changes (3)
- test/library/encryption/kms/mock-vault-plugin/go.mod
- test/library/encryption/kms/mock-vault-plugin/softhsm-config.json
- test/library/encryption/kms/mock-vault-plugin/build-from-source.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/encryption/kms/mock-vault-plugin/main.go
|
|
||
| FROM quay.io/openshifttest/mock-kms-plugin:latest | ||
|
|
||
| COPY softhsm-config.json /etc/softhsm-config.json |
There was a problem hiding this comment.
This is exactly what I suggested. Thanks. It is better to check that everything works properly.
There was a problem hiding this comment.
it is working, but we have to check with @bertinatto changes
|
an alternative to this PR #2173 |
|
/close |
|
@gangwgr: Closed this PR. 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 kubernetes-sigs/prow repository. |
5 participants
Add a mock Vault KMS v2 plugin for encryption e2e tests.
This introduces a lightweight mock KMS v2 plugin (mock-vault-plugin/) that mimics the vault-kube-kms command-line interface but performs local AES-256-GCM encryption with a static key. It allows the KMS encryption test infrastructure to validate the full API server encryption flow without requiring a real Vault Enterprise instance.
Summary by CodeRabbit