Added labels support for ci-secret-bootstrap#5189
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughThis PR adds support for Kubernetes secret labels controlled by Vault. A new sync target labels key is defined, validated in the Vault proxy, parsed from Vault in the bootstrap tool, and used to update secrets when labels differ from the stored state. ChangesSecret sync target labels support
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hector-vido 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 |
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.
Actionable comments posted: 3
🤖 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 `@cmd/ci-secret-bootstrap/main.go`:
- Around line 674-680: User-provided labels parsed from
secretKeys[vaultapi.SecretSyncTargetLabelsKey] can overwrite the seeded
controller label api.DPTPRequesterLabel in the labels map; update the parsing
logic (the block that builds labels from vaultValue in
cmd/ci-secret-bootstrap/main.go) to reject or ignore any entry whose key equals
api.DPTPRequesterLabel so the initially-seeded labels map entry is never
replaced (ensure the same behavior is enforced before updateSecrets is called).
- Around line 675-680: The current Vault label parsing loop can panic on
malformed entries; update the block that handles
secretKeys[vaultapi.SecretSyncTargetLabelsKey] (variables vaultValue and labels)
to robustly parse each label: use strings.SplitN(entry, ":", 2),
strings.TrimSpace on both key and value, validate that you got exactly 2
non-empty parts, and either skip invalid entries or return a contextual error
(with which entry failed) instead of letting label[1] panic; ensure the final
parsed key/value are stored into labels only after trimming and validation.
In `@cmd/vault-subpath-proxy/kv_update_transport.go`:
- Around line 125-126: The branch only updated the regex but you must treat
SecretSyncTargetLabelsKey as a reserved key throughout this transport: update
validateKeysDontConflict to ignore vault.SecretSyncTargetLabelsKey when checking
for claimed keys, change syncSecret so it does not include
vault.SecretSyncTargetLabelsKey in Secret.Data or write it as normal secret
data, and update any cache builder routines that enumerate keys (e.g., functions
building the local cache or mapping keys to clusters) to skip
vault.SecretSyncTargetLabelsKey so it is never persisted or compared in conflict
checks; keep the same logic/guard used for vault.SecretSyncTargetClusterKey for
consistency.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1b2b4634-8cfc-4261-99af-ce14c9e57856
📒 Files selected for processing (3)
cmd/ci-secret-bootstrap/main.gocmd/vault-subpath-proxy/kv_update_transport.gopkg/api/vault/vault.go
| labels := map[string]string{api.DPTPRequesterLabel: "ci-secret-bootstrap"} | ||
| if vaultValue, ok := secretKeys[vaultapi.SecretSyncTargetLabelsKey]; ok { | ||
| if vaultValue != "" { | ||
| for _, label := range strings.Split(vaultValue, ",") { | ||
| label := strings.Split(label, ":") | ||
| labels[strings.Trim(label[0], " ")] = strings.Trim(label[1], " ") | ||
| } |
There was a problem hiding this comment.
Keep api.DPTPRequesterLabel reserved.
User-supplied labels are applied after the controller label is seeded, so a Vault entry can overwrite api.DPTPRequesterLabel. That breaks the ownership marker used later in updateSecrets and lets external input change controller-managed metadata. Reject or ignore that key when applying custom labels. As per coding guidelines, "Review whether APIs could be abused and ensure proper security measures".
🤖 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/ci-secret-bootstrap/main.go` around lines 674 - 680, User-provided labels
parsed from secretKeys[vaultapi.SecretSyncTargetLabelsKey] can overwrite the
seeded controller label api.DPTPRequesterLabel in the labels map; update the
parsing logic (the block that builds labels from vaultValue in
cmd/ci-secret-bootstrap/main.go) to reject or ignore any entry whose key equals
api.DPTPRequesterLabel so the initially-seeded labels map entry is never
replaced (ensure the same behavior is enforced before updateSecrets is called).
| if vaultValue, ok := secretKeys[vaultapi.SecretSyncTargetLabelsKey]; ok { | ||
| if vaultValue != "" { | ||
| for _, label := range strings.Split(vaultValue, ",") { | ||
| label := strings.Split(label, ":") | ||
| labels[strings.Trim(label[0], " ")] = strings.Trim(label[1], " ") | ||
| } |
There was a problem hiding this comment.
Harden the Vault label parser.
A malformed value like foo, foo:bar,, or a whitespace-only entry will panic on label[1]. strings.Split here also discards everything after the first :. Parse with a bounded split, trim both sides, and return a contextual error instead of crashing the bootstrap.
Suggested fix
labels := map[string]string{api.DPTPRequesterLabel: "ci-secret-bootstrap"}
if vaultValue, ok := secretKeys[vaultapi.SecretSyncTargetLabelsKey]; ok {
if vaultValue != "" {
- for _, label := range strings.Split(vaultValue, ",") {
- label := strings.Split(label, ":")
- labels[strings.Trim(label[0], " ")] = strings.Trim(label[1], " ")
+ for _, raw := range strings.Split(vaultValue, ",") {
+ raw = strings.TrimSpace(raw)
+ if raw == "" {
+ continue
+ }
+ parts := strings.SplitN(raw, ":", 2)
+ if len(parts) != 2 {
+ errs = append(errs, fmt.Errorf("secret %s has invalid %s entry %q: expected key: value", secretName.String(), vaultapi.SecretSyncTargetLabelsKey, raw))
+ continue
+ }
+ key := strings.TrimSpace(parts[0])
+ value := strings.TrimSpace(parts[1])
+ if key == "" {
+ errs = append(errs, fmt.Errorf("secret %s has invalid %s entry %q: empty label key", secretName.String(), vaultapi.SecretSyncTargetLabelsKey, raw))
+ continue
+ }
+ labels[key] = value
}
}
}🤖 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/ci-secret-bootstrap/main.go` around lines 675 - 680, The current Vault
label parsing loop can panic on malformed entries; update the block that handles
secretKeys[vaultapi.SecretSyncTargetLabelsKey] (variables vaultValue and labels)
to robustly parse each label: use strings.SplitN(entry, ":", 2),
strings.TrimSpace on both key and value, validate that you got exactly 2
non-empty parts, and either skip invalid entries or return a contextual error
(with which entry failed) instead of letting label[1] panic; ensure the final
parsed key/value are stored into labels only after trimming and validation.
| if key == vault.SecretSyncTargetClusterKey || key == vault.SecretSyncTargetLabelsKey { | ||
| continue |
There was a problem hiding this comment.
Treat SecretSyncTargetLabelsKey as reserved everywhere in this transport.
This branch only fixes the regex check. validateKeysDontConflict and syncSecret still treat the new key as normal secret data, so a Vault write can both raise false "key ... is already claimed" conflicts and persist secretsync/target-labels into Secret.Data. Please extend the same reserved-key handling there as well, and keep the cache builders aligned too.
🤖 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/kv_update_transport.go` around lines 125 - 126, The
branch only updated the regex but you must treat SecretSyncTargetLabelsKey as a
reserved key throughout this transport: update validateKeysDontConflict to
ignore vault.SecretSyncTargetLabelsKey when checking for claimed keys, change
syncSecret so it does not include vault.SecretSyncTargetLabelsKey in Secret.Data
or write it as normal secret data, and update any cache builder routines that
enumerate keys (e.g., functions building the local cache or mapping keys to
clusters) to skip vault.SecretSyncTargetLabelsKey so it is never persisted or
compared in conflict checks; keep the same logic/guard used for
vault.SecretSyncTargetClusterKey for consistency.
|
@hector-vido: 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. |
1 participant
Since we are using ci-secret-bootstrap to manage ArgoCD cluster secrets we need a way specify labels inside the secret stored on vault to avoid manual work.
ArgoCD identify cluster secrets based on the label
argocd.argoproj.io/secret-type: clusteradded to the secrets that maps aServiceAccountand a token from other clusters insideopenshift-gitops.This PR add this possibility.