Fix CSI volume grouping for multi-collection bundles#5293
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR moves CSI secrets configuration and helper logic into ChangesCSI secrets migration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/steps/multi_stage/gen.go (1)
637-662: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign SPC name generation between
createSPCsandaddCredentials.createSPCshashes the merged GSM credentials across all steps, butaddCredentialshashes only the current step’s subset. If two steps share a mount path but not the same GSM tuple set, the pod will reference an SPC name that was never created. Make both paths use the same credential set or derive the SPC from the same per-step grouping.🤖 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 `@pkg/steps/multi_stage/gen.go` around lines 637 - 662, The SPC name used in addCredentials for CSI-mounted GSM secrets is derived from only the current step’s grouped credentials, while createSPCs builds names from the merged GSM credential set, so the pod can reference an SPC that was never created. Update addCredentials and createSPCs to use the same credential grouping logic for csi_secrets.GetSPCName and csi_secrets.GetCSIVolumeName, ideally by sharing the same per-mount-path grouping from csi_secrets.GroupCredentialsByMountPath or another common helper, so the generated SPC name stays consistent across both paths.
🧹 Nitpick comments (2)
pkg/steps/csi_secrets/gsm_bundle_resolver_test.go (1)
484-484: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name is stale.
TestValidateNoGroupCollisionsOnMountPathnow testsValidateNoFileCollisionsOnMountPath(line 556); the name still references the old group-collision semantics.As per coding guidelines, "A good name is long enough to fully communicate what the item is or does."✏️ Rename fix
-func TestValidateNoGroupCollisionsOnMountPath(t *testing.T) { +func TestValidateNoFileCollisionsOnMountPath(t *testing.T) {🤖 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 `@pkg/steps/csi_secrets/gsm_bundle_resolver_test.go` at line 484, The test name is stale and still refers to the old group-collision behavior. Rename TestValidateNoGroupCollisionsOnMountPath to match the current ValidateNoFileCollisionsOnMountPath behavior, keeping the new name aligned with the test’s actual purpose and the related validation function.Source: Coding guidelines
pkg/steps/csi_secrets/csi_utils.go (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd doc comments to newly exported functions.
IsK8sSecretReference,IsGSMReference,BuildGCPSecretsParameter,GetCensorMountPath, andBuildSecretProviderClassare now exported from this new package but lack doc comments, unlikeGroupCredentialsByMountPath/GetSPCName/GetCSIVolumeNamein the same file.As per coding guidelines, "Go documentation on Classes/Functions/Fields should be written properly." As per path instructions, "Comment important exported functions with their purpose, parameters, and return values."
📝 Example fix
+// IsK8sSecretReference reports whether the credential reference points to a Kubernetes Secret. func IsK8sSecretReference(c api.CredentialReference) bool { return c.Namespace != "" && c.Name != "" } +// IsGSMReference reports whether the credential reference points to a Google Secret Manager field. func IsGSMReference(c api.CredentialReference) bool { return c.Collection != "" && c.Group != "" && c.Field != "" }Also applies to: 46-46, 138-142
🤖 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 `@pkg/steps/csi_secrets/csi_utils.go` around lines 27 - 33, Add Go doc comments for the newly exported helpers in csi_utils.go so they follow the same style as GroupCredentialsByMountPath, GetSPCName, and GetCSIVolumeName. Update the declarations of IsK8sSecretReference, IsGSMReference, BuildGCPSecretsParameter, GetCensorMountPath, and BuildSecretProviderClass with concise comments that start with the function name and explain each function’s purpose, inputs, and return value where applicable.Sources: Coding guidelines, Path instructions
🤖 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 `@pkg/steps/multi_stage/init.go`:
- Line 80: The mount-path collision check in resolveCredentials() is only
step-local, but createSPCs() and addCredentials() derive SPC names from job-wide
mount-path groupings, so two steps can still diverge on the same mount_path. Add
a job-wide validation or shared grouping in init.go that covers the resolved
credentials across the entire job before createSPCs() and addCredentials() run,
using the existing resolveCredentials(), createSPCs(), and addCredentials() flow
to keep SPC naming consistent.
---
Outside diff comments:
In `@pkg/steps/multi_stage/gen.go`:
- Around line 637-662: The SPC name used in addCredentials for CSI-mounted GSM
secrets is derived from only the current step’s grouped credentials, while
createSPCs builds names from the merged GSM credential set, so the pod can
reference an SPC that was never created. Update addCredentials and createSPCs to
use the same credential grouping logic for csi_secrets.GetSPCName and
csi_secrets.GetCSIVolumeName, ideally by sharing the same per-mount-path
grouping from csi_secrets.GroupCredentialsByMountPath or another common helper,
so the generated SPC name stays consistent across both paths.
---
Nitpick comments:
In `@pkg/steps/csi_secrets/csi_utils.go`:
- Around line 27-33: Add Go doc comments for the newly exported helpers in
csi_utils.go so they follow the same style as GroupCredentialsByMountPath,
GetSPCName, and GetCSIVolumeName. Update the declarations of
IsK8sSecretReference, IsGSMReference, BuildGCPSecretsParameter,
GetCensorMountPath, and BuildSecretProviderClass with concise comments that
start with the function name and explain each function’s purpose, inputs, and
return value where applicable.
In `@pkg/steps/csi_secrets/gsm_bundle_resolver_test.go`:
- Line 484: The test name is stale and still refers to the old group-collision
behavior. Rename TestValidateNoGroupCollisionsOnMountPath to match the current
ValidateNoFileCollisionsOnMountPath behavior, keeping the new name aligned with
the test’s actual purpose and the related validation function.
🪄 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: 604cd25d-2c81-4ec8-a095-c154cbf8f8ae
📒 Files selected for processing (14)
cmd/ci-operator/main.gopkg/defaults/config.gopkg/prowgen/podspec.gopkg/steps/csi_secrets/config.gopkg/steps/csi_secrets/csi_utils.gopkg/steps/csi_secrets/csi_utils_test.gopkg/steps/csi_secrets/gsm_bundle_resolver.gopkg/steps/csi_secrets/gsm_bundle_resolver_test.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/init.gopkg/steps/multi_stage/init_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
psalajova
force-pushed
the
fix-csi-volume-grouping
branch
from
571f1dd to
4ca54bc
Compare
Group credentials by mount_path only instead of (collection, group, mount_path). When a bundle spans multiple collections, the old grouping produced multiple CSI volumes at the same mount path, which Kubernetes rejects. A single SPC can reference secrets from any number of collections, so collection/group boundaries don't require separate volumes. Also replace ValidateNoGroupCollisionsOnMountPath with ValidateNoFileCollisionsOnMountPath, which checks for actual file name collisions across all credentials at the same mount path regardless of collection or group. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
psalajova
force-pushed
the
fix-csi-volume-grouping
branch
from
4ca54bc to
6548ba4
Compare
|
/test e2e |
|
@psalajova: The following test 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. |
1 participant
When a GSM bundle references secrets from multiple collections (or groups),
ResolveCredentialReferencesexpands them into credentials that all share the bundle's singlemount_path. The previous grouping function keyed on(collection, group, mount_path), which split these into separate CSI volumes at the same path — Kubernetes rejects duplicate mount paths.This is the same class of bug fixed in #4619 (pre-group/field era). The grouping key was
(collection, mount_path)then, later widened to(collection, group, mount_path). That was safe when each credential had its own mount path, but breaks now that bundles can span collection boundaries.Fix: group by
mount_pathonly. A single SPC can reference secrets from any number of GSM collections — each object entry has its own full GSM path. Collection/group boundaries don't require separate volumes.Example: multi-collection bundle
A bundle with entries from two different collections:
A test mounts it at a single path:
After resolution, 3 credentials all have
mount_path: /var/secrets:Before (broken): grouped by
(collection:group:mount_path)→ 2 groups → 2 CSI volumes at/var/secrets→ K8s error:mount path must be uniqueAfter (fixed): grouped by
mount_pathonly → 1 group → 1 CSI volumeThe pod sees three files:
/var/secrets/access-key,/var/secrets/secret-key,/var/secrets/sa-key.jsonExample: file name collision detection
The new
ValidateNoFileCollisionsOnMountPathcatches actual file name collisions regardless of collection/group:The old check only looked within the same collection, so this cross-collection collision would have been missed.
Changes
GroupCredentialsByCollectionGroupAndMountPath→GroupCredentialsByMountPath— key on mount path onlyGetSPCName— hash(mountPath, sorted collection:group:field tuples)instead of(collection, group, mountPath, fields)GetCSIVolumeName— hash(mountPath)instead of(collection, group, mountPath)ValidateNoGroupCollisionsOnMountPath→ValidateNoFileCollisionsOnMountPath— check actual file name collisions across all credentials at the same mount path, regardless of collection/groupCo-Authored-By: Claude
This PR fixes how CI generates and validates CSI-mounted Google Secret Manager secrets for GSM multi-collection/multi-group bundles in multi-stage Prow jobs.
Practically, when a single bundle resolves to multiple GSM (collection, group, field) credentials that all use the same Kubernetes
mountPath, CI now groups CSI volumes purely bymountPath(not by(collection, group, mountPath)), preventing Kubernetes rejections caused by duplicate mounts at the same path.To keep generated Kubernetes resources deterministic and consistent under the new grouping:
GetSPCNamenow hashesmountPathplus sorted(collection:group:field)tuples.GetCSIVolumeNameuses a hash derived frommountPath(then combines it with namespace for the final DNS-safe name).Validation was renamed and strengthened:
ValidateNoGroupCollisionsOnMountPath→ValidateNoFileCollisionsOnMountPathmountPathafter computing the effective file name (Asif set, otherwiseField) and denormalizing forbidden symbols back to the mounted filename, across all collections/groups sharing that mount path.Implementation-wise, CSI GSM credential handling was centralized and rewired to
pkg/steps/csi_secrets, and all affected generators/callers/tests (ci-operator, prowgen, and multi-stage initialization/CSI volume/SPC generation) were updated to use the new grouping, naming, resolution (ResolveCredentialReferences), and collision validation logic.