vault-subpath-proxy: Add read-only mode#5197
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughThis PR adds a ChangesRead-only mode implementation
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (12 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/vault-subpath-proxy/main.go (1)
119-119: ⚡ Quick winRefactor
createProxyServerto an options struct.Line 119 now has 6 parameters, which is getting brittle for future changes and call-site ordering safety.
As per coding guidelines, "Keep function signatures small — if a function takes more than 3-4 parameters, consider grouping them into an options struct".Proposed refactor
+type proxyServerOptions struct { + vaultAddr string + listenAddr string + kvMountPath string + clients func() map[string]ctrlruntimeclient.Client + privilegedVaultClient *vaultclient.VaultClient + readOnly bool +} - -func createProxyServer(vaultAddr string, listenAddr string, kvMountPath string, clients func() map[string]ctrlruntimeclient.Client, privilegedVaultClient *vaultclient.VaultClient, readOnly bool) (*http.Server, error) { +func createProxyServer(opts proxyServerOptions) (*http.Server, error) { - vaultClient, err := api.NewClient(&api.Config{Address: vaultAddr}) + vaultClient, err := api.NewClient(&api.Config{Address: opts.vaultAddr}) ... - vaultURL, err := url.Parse(vaultAddr) + vaultURL, err := url.Parse(opts.vaultAddr) ... - transport := &kvUpdateTransport{kvMountPath: kvMountPath, upstream: http.DefaultTransport, kubeClients: clients, privilegedVaultClient: privilegedVaultClient, readOnly: readOnly} + transport := &kvUpdateTransport{kvMountPath: opts.kvMountPath, upstream: http.DefaultTransport, kubeClients: opts.clients, privilegedVaultClient: opts.privilegedVaultClient, readOnly: opts.readOnly} ... - Addr: listenAddr, + Addr: opts.listenAddr,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/vault-subpath-proxy/main.go` at line 119, The createProxyServer function has too many positional parameters (vaultAddr, listenAddr, kvMountPath, clients, privilegedVaultClient, readOnly); refactor it to accept a single options struct (e.g., type ProxyServerOptions struct { VaultAddr string; ListenAddr string; KVMountPath string; Clients func() map[string]ctrlruntimeclient.Client; PrivilegedVaultClient *vaultclient.VaultClient; ReadOnly bool }) and update createProxyServer signature to createProxyServer(opts *ProxyServerOptions) (*http.Server, error), then update all call sites to construct and pass that struct (or a small constructor like NewProxyServerOptions) so ordering is explicit and future parameters can be added without changing call sites.
🤖 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.
Nitpick comments:
In `@cmd/vault-subpath-proxy/main.go`:
- Line 119: The createProxyServer function has too many positional parameters
(vaultAddr, listenAddr, kvMountPath, clients, privilegedVaultClient, readOnly);
refactor it to accept a single options struct (e.g., type ProxyServerOptions
struct { VaultAddr string; ListenAddr string; KVMountPath string; Clients func()
map[string]ctrlruntimeclient.Client; PrivilegedVaultClient
*vaultclient.VaultClient; ReadOnly bool }) and update createProxyServer
signature to createProxyServer(opts *ProxyServerOptions) (*http.Server, error),
then update all call sites to construct and pass that struct (or a small
constructor like NewProxyServerOptions) so ordering is explicit and future
parameters can be added without changing call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4732765b-2ec4-41f6-8423-7f1c11849b42
📒 Files selected for processing (3)
cmd/vault-subpath-proxy/kv_update_transport.gocmd/vault-subpath-proxy/main.gocmd/vault-subpath-proxy/main_test.go
|
/test e2e |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek, psalajova 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 |
|
@psalajova: The following tests failed, say
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. |
2 participants
Adds a
--read-onlyflag to vault-subpath-proxy that rejects all KV write operations (PUT/POST/PATCH/DELETE) with a 403 while allowing reads to pass through. This will be used to freeze Vault during the Vault-to-GSM migration, preventing secret modifications while the secrets are copied to GSM.To freeze Vault: add -read-only to the subpath-proxy args in the StatefulSet manifest here; to unfreeze: remove it. Release repo PR is prepared: openshift/release#79591
vault-subpath-proxy: Add read-only mode
This PR adds a
--read-onlyflag to the vault-subpath-proxy component, which is used to control access to Vault secrets in CI infrastructure.What changed
The vault-subpath-proxy now supports read-only mode, which can be enabled by adding the
--read-onlyflag to the proxy's command-line arguments (typically via StatefulSet manifest configuration). When enabled, this mode:Operational impact
This feature is designed to safely freeze Vault's write capabilities during a Vault-to-Google Secret Manager (GSM) migration. By enabling read-only mode on the vault-subpath-proxy, CI operators can:
Once the migration is complete, the flag can be removed from the configuration to restore normal read-write functionality.
Implementation details
The implementation adds:
readOnlyflag to the proxy options