ci-operator: Implement 3-level naming hierarchy changes to CSI mechanism#4900
ci-operator: Implement 3-level naming hierarchy changes to CSI mechanism#4900psalajova wants to merge 2 commits intoopenshift:mainfrom
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:
WalkthroughThreads Google Secret Manager (GSM) support into the codebase: promotes GSM types to the public api, extends credential references to bundle/group/field, loads/wires GSMConfiguration and client from CLI flags, implements GSM bundle resolution/auto-discovery, and updates CSI/SPC naming and related tests and docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[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
pkg/steps/multi_stage/multi_stage.go
Outdated
| gsmClient *secretmanager.Client | ||
| gsmCredentialsFile string | ||
| gsmConfig *api.GSMConfig | ||
| gsmProjectConfig gsm.Config |
There was a problem hiding this comment.
Is it necessary to send all these params to the multistage step? Couldn't it just use the gsmClient that would be created in defaults maybe?
There was a problem hiding this comment.
Yeah, you have a point, I don't like this amount of new parameters as well... I'll look into it, maybe I can simplify it
There was a problem hiding this comment.
I've moved them all into a common struct
psalajova
force-pushed
the
ci-operator-include-groups-and-mapping-file
branch
2 times, most recently
from
d3b02d3 to
d796b98
Compare
psalajova
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/defaults/defaults.go`:
- Around line 171-197: When enableSecretsStoreCSIDriver is true, do not swallow
errors from gsm.GetConfigFromEnv or secretmanager.NewClient; instead return the
initialization error immediately so GSM is not left nil. In the block that
constructs gsmConfiguration (symbols: gsmConfiguration,
enableSecretsStoreCSIDriver, gsm.GetConfigFromEnv, secretmanager.NewClient),
replace the logrus.WithError(...).Error calls with returning a wrapped error
(e.g. fmt.Errorf("gsm init: %w", err)) from the surrounding function; ensure the
function signature and callers are updated to propagate this error instead of
assuming success. Also keep assigning gsmConfiguration.ProjectConfig and
gsmConfiguration.Client only on success.
In `@pkg/steps/multi_stage/multi_stage.go`:
- Around line 225-231: Remove the per-step deferred close of the shared GSM
client: inside the run method where you check s.enableSecretsStoreCSIDriver and
s.gsm (and currently call defer s.gsm.Client.Close()), delete that defer so the
shared s.gsm.Client is not closed by each step (this prevents breaking later
steps and race conditions); ensure the GSM client lifecycle is managed centrally
(closed once at job shutdown) and leave the rest of the logic (the nil checks
and call to s.createSPCs) unchanged.
🧹 Nitpick comments (3)
cmd/ci-secret-generator/main_test.go (1)
581-586: Consider adding a dedicated field for disabled clusters instead of matching test name.Using
tc.namestring matching to determine test behavior is fragile. If the test name changes, this conditional silently stops working correctly.♻️ Suggested improvement
Add a
disabledClustersfield to the test case struct:testCases := []struct { name string config secretgenerator.Config GSMsyncEnabled bool expectedIndexCalls int verifyIndexPayload func(t *testing.T, itemName string, payload []byte) + disabledClusters sets.Set[string] }{Then use
tc.disabledClustersdirectly instead of the name-based conditional.pkg/steps/multi_stage/gsm_bundle_resolver.go (1)
46-54: Non-deterministic group ordering in error message.The
groupListslice is populated by iterating over a map, which has non-deterministic order. This can cause the error message to vary between runs, making debugging and test assertions harder.♻️ Suggested fix
+import "sort" + for key, groups := range mountPathGroups { if len(groups) > 1 { var groupList []string for group := range groups { groupList = append(groupList, group) } + sort.Strings(groupList) return fmt.Errorf("multiple groups (%v) found for collection=%s, mount_path=%s - different groups in the same collection must use different mount paths to avoid file name collisions", groupList, key.collection, key.mountPath) } }pkg/gsm-secrets/secrets.go (1)
87-89: Consider creating a defensive copy of the input slice.The function appends to and sorts the input
secretsListin place, which mutates the caller's slice. While current usage passes an empty slice, this could cause subtle bugs if callers expect their input to remain unchanged.♻️ Suggested defensive copy
func ConstructIndexSecretContent(secretsList []string) []byte { - secretsList = append(secretsList, UpdaterSASecretName) - sort.Strings(secretsList) + result := make([]string, len(secretsList), len(secretsList)+1) + copy(result, secretsList) + result = append(result, UpdaterSASecretName) + sort.Strings(result) var formattedSecrets []string - for _, secret := range secretsList { + for _, secret := range result { formattedSecrets = append(formattedSecrets, fmt.Sprintf("- %s", secret)) } return []byte(strings.Join(formattedSecrets, "\n")) }
psalajova
force-pushed
the
ci-operator-include-groups-and-mapping-file
branch
from
05cd174 to
11ea941
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/steps/multi_stage/csi_utils.go (1)
77-95:⚠️ Potential issue | 🟠 MajorInclude
Groupfield in the hash to prevent potential SPC name collisions.The function hashes only
credential.Field, ignoring theGroupfield which is part of the credential's identity in the collection__group__field hierarchy. Two credentials with identical collection, mountPath, and Field but different Group values would produce the same SPC name, even though they reference different GSM secrets. Although such credentials would be grouped together for the same mount path, the current test coverage (TestGetSPCNameCollisionPrevention) does not verify Group differentiation. AddingGroupto the hash input would eliminate this collision risk.pkg/steps/multi_stage/multi_stage.go (1)
503-541:⚠️ Potential issue | 🔴 CriticalUse the same deduplication key as init.go to match full credential identity.
The code deduplicates credentials using only
credential.Name(line 508), butinit.gocreates SPCs using the full credential identifier fromgsm.GetGSMSecretName(credential.Collection, credential.Group, credential.Field). This inconsistency can cause mismatches: if two credentials share the sameNamebut differ inCollection,Group, orField, the second one would be incorrectly skipped here even thoughinit.gowould create separate SPCs for both.Align the deduplication key with
init.goby using the full credential identity (collection + group + field) instead of justName.
🤖 Fix all issues with AI agents
In `@cmd/ci-operator/main.go`:
- Around line 761-766: The startup attempts to load GSM config unconditionally
when o.enableSecretsStoreCSIDriver is true, but o.gsmConfigPath defaults to
empty which causes a file read error; in Complete() validate that if
enableSecretsStoreCSIDriver (o.enableSecretsStoreCSIDriver) is true then
gsmConfigPath (o.gsmConfigPath) must be non-empty and return a clear error if
missing, or alternatively make GSM loading conditional only when gsmConfigPath
is set (i.e., check o.gsmConfigPath != "" before calling
api.LoadGSMConfigFromFile), updating the code paths that call
LoadGSMConfigFromFile to use this validation and preserving existing behavior in
the main startup block that loads the config.
In `@pkg/defaults/defaults.go`:
- Around line 128-149: The GSM secretmanager.Client created by
secretmanager.NewClient in FromConfig (variable gsmClient) is never closed;
immediately after successfully creating gsmClient (right after the
secretmanager.NewClient call) add a defer gsmClient.Close() so the gRPC
connections and background resources are released when FromConfig returns;
ensure this cleanup is placed before assigning gsmClient into gsmConfig to
guarantee the client is closed if FromConfig exits early due to an error.
In `@pkg/steps/multi_stage/init.go`:
- Around line 91-115: The resolved credentials from ResolveCredentialReferences
are only written into the local allSteps copy, so s.pre/s.test/s.post remain
unchanged and later addCredentialsToCensoring reads stale credentials causing
SPC name mismatches; update the real slices after resolving by writing
resolvedCredentials back into the corresponding original step entries (s.pre,
s.test, s.post) or refactor to only use the single resolved allSteps for
subsequent operations (createSPCs, addCredentialsToCensoring,
groupCredentialsByCollectionAndMountPath) so SPC creation and censoring
reference the same credential objects.
🧹 Nitpick comments (1)
pkg/steps/multi_stage/gsm_bundle_resolver.go (1)
88-159: Consider simplifying error handling - aggregate is unnecessary with fail-fast behavior.The code uses
breakafter each error (lines 92, 97, 103, 118, 130, 153), meaning at most one error is ever collected. Usingutilerrors.NewAggregateis unnecessary overhead when the slice will never contain multiple errors.Either:
- Remove the aggregate and return the single error directly, or
- If multiple errors should be collected for better diagnostics, use
continueinstead ofbreakThe current behavior (fail-fast) is reasonable for credential resolution, so option 1 aligns better with the intent.
♻️ Simplify to direct error return
for _, cred := range credentials { if cred.IsBundleReference() { if gsmConfig == nil { - errs = append(errs, fmt.Errorf("bundle reference %q requires gsm-config file, but config is not loaded", cred.Bundle)) - break + return nil, fmt.Errorf("bundle reference %q requires gsm-config file, but config is not loaded", cred.Bundle) } // ... similar pattern for other errors
|
Scheduling required tests: |
psalajova
force-pushed
the
ci-operator-include-groups-and-mapping-file
branch
from
11ea941 to
91b098b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/steps/multi_stage/csi_utils.go`:
- Around line 82-88: getSPCName currently builds the SPC hash using collection,
mountPath and credential.Field which can collide when two credentials share
those values but have different credential.Group; update getSPCName to include
credential.Group in the hashed inputs (e.g., add credential.Group to the
per-credential components you collect before sorting/concatenating) so the parts
used for hashing include group and avoid SPC name collisions that cause wrong
secret mounts.
🧹 Nitpick comments (1)
pkg/api/types.go (1)
1134-1151: Clarify legacyNameusage to avoid config ambiguity.
Now that Collection/Group/Field (and Bundle) are first-class, keepingNamerequired and documented as the primary field is confusing. Consider marking it optional and calling it legacy in the comment.Suggested refactor
- // Name is the name of the secret, without the collection prefix. - Name string `json:"name"` + // Name is the legacy secret name (pre collection/group/field). Prefer Bundle/Collection/Group/Field. + Name string `json:"name,omitempty"`
psalajova
force-pushed
the
ci-operator-include-groups-and-mapping-file
branch
from
91b098b to
f0ad7a6
Compare
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
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/multi_stage.go (1)
503-538:⚠️ Potential issue | 🔴 CriticalAdd validation or fallback for empty credential Name in GSM credentials.
addCredentialsToCensoringdedupes and builds mount paths usingcredential.Name(lines 508, 517, 537). GSM credentials created bygsm_bundle_resolver.gonever populate theNamefield—they only setCollection,Group, andField. This causes all GSM credentials to collapse under an empty string key in the deduplication map and mount at the same paths (/censor/and/secrets/), defeating censoring for all but the first credential.The initialization code in
init.goavoids this by usinggsm.GetGSMSecretName(credential.Collection, credential.Group, credential.Field)for deduping (line 141). Either populateNameconsistently using the same approach, or apply the same deduplication logic here to ensure each credential receives a unique mount path.
psalajova
force-pushed
the
ci-operator-include-groups-and-mapping-file
branch
from
f0ad7a6 to
0e4d80d
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
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/multi_stage.go (1)
503-538:⚠️ Potential issue | 🔴 CriticalUse derived credential identifiers for censoring deduplication instead of empty
Namefield.
addCredentialsToCensoring()usescredential.Namefor deduplication (lines 507-538), but GSM credentials resolved byResolveCredentialReferences()haveNameunset. This causes all such credentials to collapse to a single deduplication key, mounting only the first credential at/secrets/while silently skipping the rest.Adopt the same approach as
createSPCs()in init.go: derive a stable identifier usinggsm.GetGSMSecretName(credential.Collection, credential.Group, credential.Field)for both deduplication and mount path construction.
🤖 Fix all issues with AI agents
In `@pkg/steps/multi_stage/gsm_bundle_resolver.go`:
- Around line 178-215: The code that expands GSMSecrets into
api.CredentialReference currently always sets Field to FieldEntry.Name (in the
loop that appends to resolvedCredentials), which ignores the FieldEntry.As alias
and causes CSI file names to use the original GSM field name instead of the
alias; update the append logic in the loop that handles explicit fields (the for
_, fieldEntry := range secretEntry.Fields block) to populate the
CredentialReference.Field (or an appropriate new property if needed) with
fieldEntry.As when present (falling back to fieldEntry.Name), and ensure
whatever key csi_utils.FileName expects (see FileName handling in
pkg/steps/multi_stage/csi_utils.go) receives the alias so downstream CSI
filename computation uses the alias.
🧹 Nitpick comments (1)
cmd/ci-operator/main.go (1)
795-800: Minor: Inconsistent indentation style.Lines 795-800 have different alignment compared to lines 781-794 above. While this doesn't affect functionality, it creates visual inconsistency in the struct literal. Consider aligning all fields consistently.
| for _, secretEntry := range bundle.GSMSecrets { | ||
| if len(secretEntry.Fields) == 0 { // if fields are not specified, discover them using GSM listing | ||
| if gsmClient == nil { | ||
| return nil, fmt.Errorf("bundle auto-discovery for %s__%s requires GSM client, but client is not initialized", secretEntry.Collection, secretEntry.Group) | ||
| } | ||
| key := collectionGroupKey{ | ||
| secretEntry.Collection, | ||
| secretEntry.Group, | ||
| } | ||
|
|
||
| // check if we've already discovered fields for this collection+group | ||
| if _, alreadyDiscovered := discoveredFields[key]; !alreadyDiscovered { | ||
| fieldNames, err := gsm.ListSecretFieldsByCollectionAndGroup( | ||
| ctx, gsmClient, gsmProjectConfig, secretEntry.Collection, secretEntry.Group) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list fields for collection=%s, group=%s: %w", | ||
| secretEntry.Collection, secretEntry.Group, err) | ||
| } | ||
| discoveredFields[key] = fieldNames | ||
| logrus.Debugf("discovered %d fields for collection=%s, group=%s", len(fieldNames), secretEntry.Collection, secretEntry.Group) | ||
| } | ||
|
|
||
| for _, fieldName := range discoveredFields[key] { | ||
| resolvedCredentials = append(resolvedCredentials, api.CredentialReference{ | ||
| Collection: secretEntry.Collection, | ||
| Group: secretEntry.Group, | ||
| Field: fieldName, | ||
| }) | ||
| } | ||
| } else { | ||
| // use explicit fields | ||
| for _, fieldEntry := range secretEntry.Fields { | ||
| resolvedCredentials = append(resolvedCredentials, api.CredentialReference{ | ||
| Collection: secretEntry.Collection, | ||
| Group: secretEntry.Group, | ||
| Field: fieldEntry.Name, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Honor bundle field aliases (as) when generating CSI file names.
Line 209-214 expands bundles using only FieldEntry.Name, so bundles that specify as will mount files using the GSM field name instead of the alias. That diverges from the key names produced by ci-secret-bootstrap and can break consumers expecting the alias. Consider propagating As into the credential and preferring it when computing the CSI FileName (see Line 39-46 in pkg/steps/multi_stage/csi_utils.go).
🛠️ Suggested fix
@@
- for _, fieldEntry := range secretEntry.Fields {
- resolvedCredentials = append(resolvedCredentials, api.CredentialReference{
- Collection: secretEntry.Collection,
- Group: secretEntry.Group,
- Field: fieldEntry.Name,
- })
- }
+ for _, fieldEntry := range secretEntry.Fields {
+ resolvedCredentials = append(resolvedCredentials, api.CredentialReference{
+ Collection: secretEntry.Collection,
+ Group: secretEntry.Group,
+ Field: fieldEntry.Name,
+ Name: fieldEntry.As,
+ })
+ }# pkg/steps/multi_stage/csi_utils.go
@@
- fileName, err := restoreForbiddenSymbolsInSecretName(credential.Field)
+ fileNameSource := credential.Field
+ if credential.Name != "" {
+ fileNameSource = credential.Name
+ }
+ fileName, err := restoreForbiddenSymbolsInSecretName(fileNameSource)🤖 Prompt for AI Agents
In `@pkg/steps/multi_stage/gsm_bundle_resolver.go` around lines 178 - 215, The
code that expands GSMSecrets into api.CredentialReference currently always sets
Field to FieldEntry.Name (in the loop that appends to resolvedCredentials),
which ignores the FieldEntry.As alias and causes CSI file names to use the
original GSM field name instead of the alias; update the append logic in the
loop that handles explicit fields (the for _, fieldEntry := range
secretEntry.Fields block) to populate the CredentialReference.Field (or an
appropriate new property if needed) with fieldEntry.As when present (falling
back to fieldEntry.Name), and ensure whatever key csi_utils.FileName expects
(see FileName handling in pkg/steps/multi_stage/csi_utils.go) receives the alias
so downstream CSI filename computation uses the alias.
|
@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. |
4 participants
This PR implements support for the 3-level GSM secret naming structure (
collection__group__field) in ci-operator, as per the design document.Changes
3-level hierarchy
Secrets now use
collection__group__fieldnaming instead ofcollection__name:Before:
After:
Supported credential resolution modes
There are three ways one can reference credentials:
gsm-config.yaml):Implementation details
gsm-config.yamlavailable at all times, as this is critical for bundle resolution.Jira: https://issues.redhat.com/browse/DPTP-4656