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 |
WalkthroughAdds Google Secret Manager (GSM) integration: moves GSM types into the public api package, extends credential references to bundle/group/field, threads GSM configuration and client through defaults and multi-stage steps, and implements GSM bundle resolution, auto-discovery, and related test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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"`
| // Sort credential field names for deterministic hashing | ||
| var credFields []string | ||
| for _, cred := range credentials { | ||
| credNames = append(credNames, cred.Name) | ||
| credFields = append(credFields, cred.Field) | ||
| } | ||
| sort.Strings(credNames) | ||
| parts = append(parts, credNames...) | ||
| sort.Strings(credFields) | ||
| parts = append(parts, credFields...) |
There was a problem hiding this comment.
Include group in the SPC hash to avoid cross-group collisions.
getSPCName hashes only credential.Field; if two credentials share collection+mountPath+field but differ by group, the SPC name collides and later creation overwrites the earlier SPC. That can mount the wrong secret for a step.
🛠️ Proposed fix
- // Sort credential field names for deterministic hashing
- var credFields []string
- for _, cred := range credentials {
- credFields = append(credFields, cred.Field)
- }
- sort.Strings(credFields)
- parts = append(parts, credFields...)
+ // Sort full GSM secret names for deterministic hashing
+ var credSecretNames []string
+ for _, cred := range credentials {
+ credSecretNames = append(credSecretNames, gsm.GetGSMSecretName(cred.Collection, cred.Group, cred.Field))
+ }
+ sort.Strings(credSecretNames)
+ parts = append(parts, credSecretNames...)🤖 Prompt for AI Agents
In `@pkg/steps/multi_stage/csi_utils.go` around lines 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.
|
@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. |
3 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