Added labels support for ci-secret-bootstrap#5189
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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:
📝 WalkthroughWalkthroughAdds Vault-driven secret labels: a new exported Vault key, proxy validation skip for that key, parsing/validation and application of labels in ci-secret-bootstrap (including reserved-key handling), and update-on-label-drift for managed Secrets. go.mod dependency versions are also updated. ChangesSecret sync target labels support
🎯 3 (Moderate) | ⏱️ ~20 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
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
from
f93da75 to
ff9898d
Compare
There was a problem hiding this comment.
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 `@go.mod`:
- Line 5: The go.mod replace directive downgrades github.com/docker/distribution
to v2.7.1, reintroducing known vulnerabilities; remove or update that replace
entry so the project uses v2.8.3 or newer (i.e., delete or change the line
"replace github.com/docker/distribution => github.com/docker/distribution
v2.7.1+incompatible" to omit the replace or point to v2.8.3+incompatible), then
run go mod tidy to ensure module graph is refreshed and the secure version is
used.
🪄 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: 92bacdb6-52c2-4727-b315-c5086234b321
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (4)
cmd/ci-secret-bootstrap/main.gocmd/vault-subpath-proxy/kv_update_transport.gogo.modpkg/api/vault/vault.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/api/vault/vault.go
- cmd/vault-subpath-proxy/kv_update_transport.go
- cmd/ci-secret-bootstrap/main.go
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
from
ff9898d to
05c4a78
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@go.mod`:
- Line 157: Update the pinned indirect dependency k8s.io/kubernetes in go.mod to
a patched 1.29.x release (at least v1.29.7) to eliminate CVE-2024-10220 and
CVE-2024-5321; open go.mod, replace the existing k8s.io/kubernetes v1.29.2 entry
with v1.29.7 (or later) and run go get k8s.io/kubernetes@v1.29.7 followed by go
mod tidy to update the lock file, or if the package is unnecessary remove the
k8s.io/kubernetes line and run go mod tidy to prune it from the module graph.
- Line 59: The go.mod currently pins k8s.io/apimachinery to v0.35.5 while other
Kubernetes modules (k8s.io/api, k8s.io/apiserver, k8s.io/client-go) remain at
v0.32.8 causing API/type/protobuf skew; update the Kubernetes module versions so
their minor versions match (e.g., set k8s.io/apimachinery to v0.32.x to match
k8s.io/api, k8s.io/apiserver, k8s.io/client-go or bump all peers to the same
v0.35.x), then run go get / go mod tidy to resolve transitive deps and ensure
builds pass — target the same minor for k8s.io/apimachinery, k8s.io/api,
k8s.io/apiserver, and k8s.io/client-go.
🪄 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: c20093fc-2c29-482d-a2d6-827116a19025
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (4)
cmd/ci-secret-bootstrap/main.gocmd/vault-subpath-proxy/kv_update_transport.gogo.modpkg/api/vault/vault.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/vault-subpath-proxy/kv_update_transport.go
- pkg/api/vault/vault.go
- cmd/ci-secret-bootstrap/main.go
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
2 times, most recently
from
bea027f to
4bba3a9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
go.mod (2)
5-5:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the docker/distribution downgrade to v2.7.1 — it reintroduces known security vulnerabilities.
This issue was previously flagged and remains unresolved. The replace directive downgrades from v2.8.3 to v2.7.1, reintroducing HIGH severity vulnerabilities including OOM via malicious catalog API input and OCI manifest type confusion.
🤖 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 `@go.mod` at line 5, The go.mod contains a replace directive "replace github.com/docker/distribution => github.com/docker/distribution v2.7.1+incompatible" which downgrades to a vulnerable version; remove or update that replace so the module uses a secure version (e.g., v2.8.3 or later) instead of v2.7.1+incompatible: edit the go.mod to delete or change the replace line referencing github.com/docker/distribution and run go mod tidy to ensure dependencies are resolved to the non-vulnerable release.
157-157:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPin k8s.io/kubernetes to a patched version to resolve HIGH severity vulnerabilities.
This issue was previously flagged and remains unresolved. Version 1.29.2 contains multiple HIGH severity vulnerabilities including CVE-2024-10220 (kubelet arbitrary command execution) and CVE-2024-5321 (incorrect Windows container log permissions). Upgrade to v1.29.7 or later in the 1.29.x series.
🤖 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 `@go.mod` at line 157, Update the k8s.io/kubernetes module pin in go.mod (currently "k8s.io/kubernetes v1.29.2") to a patched 1.29.x release (e.g., v1.29.7 or later) to address the reported HIGH severity CVEs, then run the Go tooling to apply and verify the change (e.g., `go get k8s.io/kubernetes@v1.29.7`, followed by `go mod tidy` and `go test`/build) so go.sum is updated and the project still compiles/tests pass.
🧹 Nitpick comments (1)
go.mod (1)
191-191: ⚡ Quick winConsider investigating the reason for pinning k8s.io/metrics to v0.32.0.
k8s.io/metricsis atv0.32.0while core modules (k8s.io/api,k8s.io/apimachinery,k8s.io/apiserver,k8s.io/client-go) are atv0.35.5. The codebase uses it only inpkg/metrics/(3 files) and there are no observed build or type compatibility issues. This version is likely pinned to satisfy a transitive dependency requirement (e.g., fromsigs.k8s.io/prow). Verify whether aligning versions is safe or if the lower version is intentional for compatibility.🤖 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 `@go.mod` at line 191, The go.mod pins k8s.io/metrics to v0.32.0 while core k8s modules are v0.35.5; inspect whether this downgrade is required by a transitive dependency and if it's safe to align to v0.35.5. In the repo, check usage in pkg/metrics/* and run go list -m all / go mod graph to find which module requires v0.32.0 (e.g., sigs.k8s.io/prow), attempt to update by running go get k8s.io/metrics@v0.35.5 and go mod tidy, then build and run tests to confirm no type/API breakage; if a transitive consumer prevents the bump, document that dependency and keep the pin.
🤖 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.
Duplicate comments:
In `@go.mod`:
- Line 5: The go.mod contains a replace directive "replace
github.com/docker/distribution => github.com/docker/distribution
v2.7.1+incompatible" which downgrades to a vulnerable version; remove or update
that replace so the module uses a secure version (e.g., v2.8.3 or later) instead
of v2.7.1+incompatible: edit the go.mod to delete or change the replace line
referencing github.com/docker/distribution and run go mod tidy to ensure
dependencies are resolved to the non-vulnerable release.
- Line 157: Update the k8s.io/kubernetes module pin in go.mod (currently
"k8s.io/kubernetes v1.29.2") to a patched 1.29.x release (e.g., v1.29.7 or
later) to address the reported HIGH severity CVEs, then run the Go tooling to
apply and verify the change (e.g., `go get k8s.io/kubernetes@v1.29.7`, followed
by `go mod tidy` and `go test`/build) so go.sum is updated and the project still
compiles/tests pass.
---
Nitpick comments:
In `@go.mod`:
- Line 191: The go.mod pins k8s.io/metrics to v0.32.0 while core k8s modules are
v0.35.5; inspect whether this downgrade is required by a transitive dependency
and if it's safe to align to v0.35.5. In the repo, check usage in pkg/metrics/*
and run go list -m all / go mod graph to find which module requires v0.32.0
(e.g., sigs.k8s.io/prow), attempt to update by running go get
k8s.io/metrics@v0.35.5 and go mod tidy, then build and run tests to confirm no
type/API breakage; if a transitive consumer prevents the bump, document that
dependency and keep the pin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 76b6c04c-57c4-48f8-a89c-827a3173069b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (5)
cmd/ci-secret-bootstrap/main.gocmd/vault-subpath-proxy/kv_update_transport.gogo.modpkg/api/vault/vault.gopkg/lease/client_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/api/vault/vault.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/vault-subpath-proxy/kv_update_transport.go
- cmd/ci-secret-bootstrap/main.go
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
2 times, most recently
from
f18c83b to
76c88d8
Compare
|
/test lint unit |
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
2 times, most recently
from
6449a1f to
f601659
Compare
|
/retest |
|
Scheduling tests matching the |
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
from
f601659 to
9100afd
Compare
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
from
9100afd to
33c0d2c
Compare
|
Scheduling tests matching the |
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
from
33c0d2c to
c7c607c
Compare
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
from
c7c607c to
fc4e9a1
Compare
|
Scheduling tests matching the |
|
/test e2e |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
2 similar comments
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
| continue | ||
| } | ||
| if key == vault.SecretSyncTargetLabelsKey { | ||
| if len(strings.TrimSpace(value)) == 0 { |
There was a problem hiding this comment.
You are mutating the real value here.
There was a problem hiding this comment.
This happens during the put/patch, I am mutating, indeed, but this is the only way to check if this value is not empty or some blank string.
Why is this method a problem?
| entry, alreadyExists := secretsMap[cluster][secretName] | ||
| labels := map[string]string{api.DPTPRequesterLabel: "ci-secret-bootstrap"} | ||
| if vaultValue, ok := secretKeys[vaultapi.SecretSyncTargetLabelsKey]; ok { | ||
| if vaultValue != "" { |
There was a problem hiding this comment.
This is already now validated, and also you don't want to check that.. If the value is empty, then we go with the empty value and fail in the next validation either way.
There was a problem hiding this comment.
Ok I will avoid testing if the value is empty during secrets synchronization, but when this "next validation" will happen? Are you talking about IsQualifiedName and IsValidLabelValue?
| labels := map[string]string{api.DPTPRequesterLabel: "ci-secret-bootstrap"} | ||
| if vaultValue, ok := secretKeys[vaultapi.SecretSyncTargetLabelsKey]; ok { | ||
| if vaultValue != "" { | ||
| for _, label := range strings.Split(vaultValue, ",") { |
There was a problem hiding this comment.
Let's get this logic out of here. Put it on its own function, or even better a method of the secretKeys if its easy to refactor. Either way, you need to have a separate unittests for that logic as well.
| Data: map[string][]byte{}, | ||
| Type: coreapi.SecretTypeOpaque, | ||
| } | ||
| } else { |
There was a problem hiding this comment.
What you do inside this else is not related to what you are checking on if. Also, the existance check has to do with the objectMeta namespace and name, since we will get the existing secret. Either way, you will override the labels in the end.
There was a problem hiding this comment.
I can see a relation, if the secret exists I just overwrite the labels because someone could changed it for another value.
Should I just overwrite it, declaring an ObjectMeta without labels and after the if define it?
if !alreadyExists {
entry = coreapi.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: secretName.Namespace,
Name: secretName.Name,
},
Data: map[string][]byte{},
Type: coreapi.SecretTypeOpaque,
}
}
entry.ObjectMeta.Labels = labels
| } | ||
| for vaultKey, vaultValue := range secretKeys { | ||
| if vaultKey == vaultapi.SecretSyncTargetClusterKey { | ||
| if vaultKey == vaultapi.SecretSyncTargetClusterKey || vaultKey == vaultapi.SecretSyncTargetLabelsKey { |
There was a problem hiding this comment.
I don't understand why you changed that. Can you elaborate?
There was a problem hiding this comment.
Since this key is not used to create an entry inside the data secret stanza, we need to skip it the same way we skip the target cluster vault data.
hector-vido
force-pushed
the
labels-support-secret-bootstrap
branch
from
fc4e9a1 to
fd40304
Compare
|
@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. |
2 participants
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.
Labels support for ci-secret-bootstrap
This PR enables ci-secret-bootstrap to apply Kubernetes labels to Secrets it syncs from Vault so operators can manage label-dependent workflows (notably ArgoCD cluster-secret discovery) automatically.
What changed (practical impact)
ci-secret-bootstrap (cmd/ci-secret-bootstrap)
vault-subpath-proxy (cmd/vault-subpath-proxy)
Why this matters for CI operators
Technical notes