[WIP] GSM secrets support for container test secrets#5291
Open
psalajova wants to merge 3 commits into
Open
Conversation
Tests the full flow: Secret -> CredentialReference conversion, GSM resolution, SPC creation, and pod volume/mount generation. Uses cmp.Diff for resolved credentials and CompareWithFixture golden files for SPCs, volumes, and mounts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughGSM configuration and Secrets Store CSI utility logic are extracted from pkg/steps/multi_stage into a new pkg/steps/csi_secrets package. multi_stage code delegates to the new package, plain pod steps gain GSM secret resolution and SecretProviderClass creation, and call sites in cmd/ci-operator, pkg/defaults, and pkg/prowgen are updated accordingly. ChangesGSM/CSI Secrets Consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/steps/csi_secrets/csi_utils.go (1)
37-41: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse a collision-free grouping key.
fmt.Sprintf("%s:%s:%s", ...)can merge distinct(collection, group, mountPath)tuples when a component contains:, which can put unrelated GSM credentials into the same SPC/volume group.Suggested direction
+type CollectionGroupMountPathKey struct { + Collection string + Group string + MountPath string +} + -func GroupCredentialsByCollectionGroupAndMountPath(credentials []api.CredentialReference) map[string][]api.CredentialReference { - mountGroups := make(map[string][]api.CredentialReference) +func GroupCredentialsByCollectionGroupAndMountPath(credentials []api.CredentialReference) map[CollectionGroupMountPathKey][]api.CredentialReference { + mountGroups := make(map[CollectionGroupMountPathKey][]api.CredentialReference) for _, credential := range credentials { - key := fmt.Sprintf("%s:%s:%s", credential.Collection, credential.Group, credential.MountPath) + key := CollectionGroupMountPathKey{Collection: credential.Collection, Group: credential.Group, MountPath: credential.MountPath} mountGroups[key] = append(mountGroups[key], credential) } return mountGroups }🤖 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 37 - 41, The grouping key in GroupCredentialsByCollectionGroupAndMountPath can collide when Collection, Group, or MountPath contains “:”, causing unrelated credentials to be merged. Replace the fmt.Sprintf-based key with a collision-free composite key in GroupCredentialsByCollectionGroupAndMountPath, using the credential fields directly via a struct or another unambiguous encoding, and keep the rest of the grouping logic unchanged.
🧹 Nitpick comments (3)
pkg/steps/csi_secrets/csi_utils.go (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new exported API.
KubernetesDNSLabelLimit,IsK8sSecretReference,IsGSMReference,GetCensorMountPath, andBuildSecretProviderClassare exported without Go doc comments. 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.Also applies to: 149-153
🤖 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 25 - 33, Add Go doc comments for the exported API in csi_utils.go so it follows the documentation guideline: document KubernetesDNSLabelLimit, IsK8sSecretReference, IsGSMReference, GetCensorMountPath, and BuildSecretProviderClass with concise purpose statements and, for the functions, include what they accept and return; keep the comments attached directly above each symbol so they are discoverable even if the implementation moves.Sources: Coding guidelines, Path instructions
pkg/steps/csi_secrets/csi_utils_test.go (1)
544-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an error-path case for
BuildSPCsForCredentials.The
expectErrfield is declared but never exercised — all three cases expect success. Since this builds security-relevantSecretProviderClassobjects, a case that triggersexpectErr: truewould close a coverage gap on the error branch.🤖 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_test.go` around lines 544 - 613, The TestBuildSPCsForCredentials table currently never exercises the expectErr branch, leaving the error path in BuildSPCsForCredentials untested. Add at least one negative test case in the same test table that passes invalid input to BuildSPCsForCredentials and sets expectErr to true, then assert the function returns an error in that case while keeping the existing success cases for the normal credential and dedup behavior.pkg/steps/pod.go (1)
199-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestStepnow takes 7 parameters.Consider grouping
enableCSI/gsmConfig(and possiblynodeName) into an options struct to keep the signature manageable, especially since this pattern will likely grow further.♻️ Suggested direction
-func TestStep(config api.TestStepConfiguration, resources api.ResourceConfiguration, client kubernetes.PodClient, jobSpec *api.JobSpec, nodeName string, enableCSI bool, gsmConfig *csi_secrets.GSMConfiguration) api.Step { +type TestStepOptions struct { + NodeName string + EnableCSI bool + GSMConfig *csi_secrets.GSMConfiguration +} + +func TestStep(config api.TestStepConfiguration, resources api.ResourceConfiguration, client kubernetes.PodClient, jobSpec *api.JobSpec, opts TestStepOptions) api.Step {As per path instructions, "Keep function signatures small — if a function takes more than 3-4 parameters, consider grouping them into an options struct."
🤖 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/pod.go` around lines 199 - 213, The TestStep function signature is getting too large, so group related inputs into an options struct to keep it manageable. Move enableCSI and gsmConfig together (and include nodeName too if it fits the existing call pattern), then update PodStep/TestStepConfiguration usage so callers pass a single options value instead of multiple trailing parameters. Keep the existing TestStep and PodStep symbols as the main entry points when refactoring the call sites and struct fields.Source: 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/csi_secrets/csi_utils.go`:
- Around line 54-56: The CSI name handling in
RestoreForbiddenSymbolsInSecretName currently allows As values that can become
unsafe FileName paths. Update the logic in csi_utils.go so nested subpaths are
still allowed, but absolute paths and any .. segments are rejected before the
CSI FileName is written. Keep the validation close to
RestoreForbiddenSymbolsInSecretName and the code that consumes its returned
mountName so the bad values fail early with a clear error.
In `@pkg/steps/multi_stage/init_test.go`:
- Around line 69-70: The test setup in init_test.go is ignoring the error
returned by BuildGCPSecretsParameter, which can make the expected
SecretProviderClass use invalid empty CSI parameters. Update the test code
around BuildGCPSecretsParameter and BuildSecretProviderClass to handle the
returned error explicitly and fail the test immediately if SPC construction
cannot be built, using the existing test context in the affected helper/setup
block.
In `@pkg/steps/pod_test.go`:
- Around line 376-525: Add a test case in TestResolveAndCreateGSMSecrets that
sets EnableSecretsStoreCSIDriver to false while still providing GSM secrets in
step.config.Secrets, so the resolveAndCreateGSMSecrets path is covered for the
silent-drop scenario. Use the existing test table in
TestResolveAndCreateGSMSecrets and verify the behavior of
resolveAndCreateGSMSecrets, resolvedGSMCredentials, and the absence or handling
of SecretProviderClass resources matches the intended fix when CSI is disabled.
In `@pkg/steps/pod.go`:
- Around line 110-115: The GSM secret handling in pod setup is being silently
skipped when EnableSecretsStoreCSIDriver is false, which can leave unresolved
GSM/bundle secrets unmounted without any error. Update generatePodForStep and
the resolveAndCreateGSMSecrets path in pod.go so that if config.Secrets contains
GSMReference or BundleReference entries and the CSI driver is disabled, the code
fails fast with a clear error instead of relying on the K8s-secret loop to skip
empty secret.Name values. Keep the existing GSM resolution logic, but add an
explicit validation step before secret mounting using the existing Symbols like
resolveAndCreateGSMSecrets, generatePodForStep, and config.Secrets.
---
Outside diff comments:
In `@pkg/steps/csi_secrets/csi_utils.go`:
- Around line 37-41: The grouping key in
GroupCredentialsByCollectionGroupAndMountPath can collide when Collection,
Group, or MountPath contains “:”, causing unrelated credentials to be merged.
Replace the fmt.Sprintf-based key with a collision-free composite key in
GroupCredentialsByCollectionGroupAndMountPath, using the credential fields
directly via a struct or another unambiguous encoding, and keep the rest of the
grouping logic unchanged.
---
Nitpick comments:
In `@pkg/steps/csi_secrets/csi_utils_test.go`:
- Around line 544-613: The TestBuildSPCsForCredentials table currently never
exercises the expectErr branch, leaving the error path in
BuildSPCsForCredentials untested. Add at least one negative test case in the
same test table that passes invalid input to BuildSPCsForCredentials and sets
expectErr to true, then assert the function returns an error in that case while
keeping the existing success cases for the normal credential and dedup behavior.
In `@pkg/steps/csi_secrets/csi_utils.go`:
- Around line 25-33: Add Go doc comments for the exported API in csi_utils.go so
it follows the documentation guideline: document KubernetesDNSLabelLimit,
IsK8sSecretReference, IsGSMReference, GetCensorMountPath, and
BuildSecretProviderClass with concise purpose statements and, for the functions,
include what they accept and return; keep the comments attached directly above
each symbol so they are discoverable even if the implementation moves.
In `@pkg/steps/pod.go`:
- Around line 199-213: The TestStep function signature is getting too large, so
group related inputs into an options struct to keep it manageable. Move
enableCSI and gsmConfig together (and include nodeName too if it fits the
existing call pattern), then update PodStep/TestStepConfiguration usage so
callers pass a single options value instead of multiple trailing parameters.
Keep the existing TestStep and PodStep symbols as the main entry points when
refactoring the call sites and struct fields.
🪄 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: 56eb7dd7-4d18-4529-8431-0c940316ad5d
📒 Files selected for processing (39)
cmd/ci-operator/main.gopkg/defaults/config.gopkg/defaults/defaults.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.gopkg/steps/pod.gopkg/steps/pod_test.gopkg/steps/testdata/zz_fixture_mountsTestGetPodObjectMounts_with_GSM_secret_results_in_CSI_volume.yamlpkg/steps/testdata/zz_fixture_mountsTestGetPodObjectMounts_with_mixed_K8s_and_GSM_secrets.yamlpkg/steps/testdata/zz_fixture_mountsTestResolveAndCreateGSMSecrets_GSM_explicit_field_secrets__resolved_and_SPCs_created.yamlpkg/steps/testdata/zz_fixture_mountsTestResolveAndCreateGSMSecrets_default_mount_paths_when_not_specified.yamlpkg/steps/testdata/zz_fixture_mountsTestResolveAndCreateGSMSecrets_explicit_field_with_as__as_propagated.yamlpkg/steps/testdata/zz_fixture_mountsTestResolveAndCreateGSMSecrets_mixed_K8s_and_GSM_secrets.yamlpkg/steps/testdata/zz_fixture_mountsTestResolveAndCreateGSMSecrets_no_secrets__nothing_to_resolve.yamlpkg/steps/testdata/zz_fixture_mountsTestResolveAndCreateGSMSecrets_only_K8s_secrets__skipped_by_resolution.yamlpkg/steps/testdata/zz_fixture_spcsTestResolveAndCreateGSMSecrets_GSM_explicit_field_secrets__resolved_and_SPCs_created.yamlpkg/steps/testdata/zz_fixture_spcsTestResolveAndCreateGSMSecrets_default_mount_paths_when_not_specified.yamlpkg/steps/testdata/zz_fixture_spcsTestResolveAndCreateGSMSecrets_explicit_field_with_as__as_propagated.yamlpkg/steps/testdata/zz_fixture_spcsTestResolveAndCreateGSMSecrets_mixed_K8s_and_GSM_secrets.yamlpkg/steps/testdata/zz_fixture_spcsTestResolveAndCreateGSMSecrets_no_secrets__nothing_to_resolve.yamlpkg/steps/testdata/zz_fixture_spcsTestResolveAndCreateGSMSecrets_only_K8s_secrets__skipped_by_resolution.yamlpkg/steps/testdata/zz_fixture_volumesTestGetPodObjectMounts_with_GSM_secret_results_in_CSI_volume.yamlpkg/steps/testdata/zz_fixture_volumesTestGetPodObjectMounts_with_mixed_K8s_and_GSM_secrets.yamlpkg/steps/testdata/zz_fixture_volumesTestResolveAndCreateGSMSecrets_GSM_explicit_field_secrets__resolved_and_SPCs_created.yamlpkg/steps/testdata/zz_fixture_volumesTestResolveAndCreateGSMSecrets_default_mount_paths_when_not_specified.yamlpkg/steps/testdata/zz_fixture_volumesTestResolveAndCreateGSMSecrets_explicit_field_with_as__as_propagated.yamlpkg/steps/testdata/zz_fixture_volumesTestResolveAndCreateGSMSecrets_mixed_K8s_and_GSM_secrets.yamlpkg/steps/testdata/zz_fixture_volumesTestResolveAndCreateGSMSecrets_no_secrets__nothing_to_resolve.yamlpkg/steps/testdata/zz_fixture_volumesTestResolveAndCreateGSMSecrets_only_K8s_secrets__skipped_by_resolution.yaml
🔗 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)
Comment on lines
+54
to
56
| mountName, err := RestoreForbiddenSymbolsInSecretName(mountName) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid mount name '%s': %w", mountName, err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding code.
ast-grep outline pkg/steps/csi_secrets/csi_utils.go --view expanded || true
echo '---'
cat -n pkg/steps/csi_secrets/csi_utils.go | sed -n '1,220p'
echo '---'
rg -n "RestoreForbiddenSymbolsInSecretName|Field|As|FileName|mountName" pkg/steps/csi_secrets -nRepository: openshift/ci-tools
Length of output: 27937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- csi_utils tests around mount names ---'
sed -n '470,520p' pkg/steps/csi_secrets/csi_utils_test.go
echo '--- any tests for slash/dot handling ---'
rg -n "dot|slash|absolute|parent|traversal|FileName" pkg/steps/csi_secrets/csi_utils_test.go
echo '--- gsm validation denormalization ---'
ast-grep outline pkg/gsm-validation --view expanded || true
echo '--- files ---'
find pkg/gsm-validation -maxdepth 2 -type f | sort
echo '--- relevant source ---'
sed -n '1,220p' pkg/gsm-validation/*.goRepository: openshift/ci-tools
Length of output: 9765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- CredentialReference definition and validations ---'
rg -n "type CredentialReference|func .*Validate.*Credential|FieldEntry|As:" pkg/api pkg -g '*.go'
echo '--- csi_utils restore tests ---'
sed -n '340,520p' pkg/steps/csi_secrets/csi_utils_test.go
echo '--- any validation of field names in gsm validation tests ---'
rg -n "ValidateSecretName|ValidateGroupName|NormalizeName|DenormalizeName" pkg -g '*.go'Repository: openshift/ci-tools
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- api gsm types around FieldEntry and validation ---'
sed -n '1,220p' pkg/api/gsm.go
echo '--- credential reference definition ---'
sed -n '1190,1265p' pkg/api/types.go
echo '--- validation entry points for credentials ---'
rg -n "ValidateCredentials|ValidateSecretName|ValidateGroupName|ValidateCollectionName|FieldEntry" pkg/api pkg/validation -g '*.go'
echo '--- csi secret tests around slash / dot ---'
sed -n '340,520p' pkg/steps/csi_secrets/csi_utils_test.goRepository: openshift/ci-tools
Length of output: 20340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- validation around GSM secrets and fields ---'
sed -n '280,340p' pkg/api/gsm.go
echo '--- more validation around project config / fields ---'
sed -n '400,470p' pkg/api/gsm.go
echo '--- lines around credential validation tests ---'
sed -n '1460,1600p' pkg/validation/test_test.goRepository: openshift/ci-tools
Length of output: 9855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- validation function that checks credential references ---'
rg -n "field is required when `as` is specified|ValidateCredentials\\(|mountPath is not absolute|ValidateCredential" pkg/validation pkg/api -g '*.go'
echo '--- credential resolution path to see where As/Field originate ---'
sed -n '1,260p' pkg/steps/csi_secrets/gsm_bundle_resolver.goRepository: openshift/ci-tools
Length of output: 8572
🌐 Web query:
secrets-store-csi-driver provider-gcp fileName relative path absolute path fileName nested directories documentation
💡 Result:
For the GCP provider of the Secrets Store CSI Driver, you can use either the fileName or path parameter within your SecretProviderClass to define where a secret is mounted [1][2]. The path parameter serves as an alias for fileName [2]. Nested directories are supported for these mounts [2]. You can specify a relative path (e.g., "subdir/secret.txt") in either the fileName or path field, and the driver will create the necessary directory structure within the mounted volume [1][2]. Note that these paths are relative to the directory where the SecretProviderClass volume is mounted in your Pod [3][1]. Absolute paths are not used within the fileName or path fields; rather, the entire volume is mounted to a specific location defined in your Pod's volume mount configuration, and the fileName or path parameters define the location of the secret relative to that mount point [4][5]. In summary: - fileName/path: These parameters define the relative path of the file within the CSI volume mount [3][1]. - Nested Directories: You can include subdirectory names in these fields (e.g., "config/db/password") to organize your mounted secrets [2]. - Absolute Paths: You do not provide absolute paths in the SecretProviderClass; you control the absolute path of the secret by choosing where to mount the CSI volume in your Pod's spec [4][5].
Citations:
- 1: https://github.com/GoogleCloudPlatform/secrets-store-csi-driver-provider-gcp/blob/main/config/config.go
- 2: https://github.com/GoogleCloudPlatform/secrets-store-csi-driver-provider-gcp/blob/main/CHANGELOG.md
- 3: https://pkg.go.dev/sigs.k8s.io/secrets-store-csi-driver/provider/v1alpha1
- 4: https://secrets-store-csi-driver.sigs.k8s.io/concepts
- 5: https://deepwiki.com/kubernetes-sigs/secrets-store-csi-driver/4.2-mounting-secrets-in-pods
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
# Check the provider's path/fileName handling for validation of absolute paths or "..".
curl -L --silent https://raw.githubusercontent.com/GoogleCloudPlatform/secrets-store-csi-driver-provider-gcp/main/config/config.go | sed -n '1,240p'
echo '---'
curl -L --silent https://raw.githubusercontent.com/GoogleCloudPlatform/secrets-store-csi-driver-provider-gcp/main/README.md | rg -n "fileName|path|absolute|relative|..|nested" -n || true
Length of output: 15285
Reject path-traversal As values before writing CSI FileNames. As flows straight into the CSI FileName; allow nested subpaths, but block absolute paths and .. segments here.
🤖 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 54 - 56, The CSI name
handling in RestoreForbiddenSymbolsInSecretName currently allows As values that
can become unsafe FileName paths. Update the logic in csi_utils.go so nested
subpaths are still allowed, but absolute paths and any .. segments are rejected
before the CSI FileName is written. Keep the validation close to
RestoreForbiddenSymbolsInSecretName and the code that consumes its returned
mountName so the bad values fail early with a clear error.
Source: Path instructions
Comment on lines
+69
to
+70
| secret, _ := csi_secrets.BuildGCPSecretsParameter(credentials) | ||
| spc := csi_secrets.BuildSecretProviderClass(csi_secrets.GetSPCName(ns, credentials), ns, secret) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail the test when expected SPC construction fails.
BuildGCPSecretsParameter can return an error; ignoring it can make the expected object silently use empty CSI parameters. As per coding guidelines, Never ignore returned errors as _ without clear justification.
Suggested fix
newGroupedSPC := func(ns string, credentials []api.CredentialReference) csiapi.SecretProviderClass {
- secret, _ := csi_secrets.BuildGCPSecretsParameter(credentials)
+ secret, err := csi_secrets.BuildGCPSecretsParameter(credentials)
+ if err != nil {
+ t.Fatalf("build grouped SPC secrets: %v", err)
+ }
spc := csi_secrets.BuildSecretProviderClass(csi_secrets.GetSPCName(ns, credentials), ns, secret)
@@
- secret, _ := csi_secrets.BuildGCPSecretsParameter(credentials)
+ secret, err := csi_secrets.BuildGCPSecretsParameter(credentials)
+ if err != nil {
+ t.Fatalf("build censoring SPC secrets: %v", err)
+ }
spc := csi_secrets.BuildSecretProviderClass(csi_secrets.GetSPCName(ns, credentials), ns, secret)Also applies to: 81-83
🤖 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/init_test.go` around lines 69 - 70, The test setup in
init_test.go is ignoring the error returned by BuildGCPSecretsParameter, which
can make the expected SecretProviderClass use invalid empty CSI parameters.
Update the test code around BuildGCPSecretsParameter and
BuildSecretProviderClass to handle the returned error explicitly and fail the
test immediately if SPC construction cannot be built, using the existing test
context in the affected helper/setup block.
Source: Coding guidelines
Comment on lines
+376
to
+525
| func TestResolveAndCreateGSMSecrets(t *testing.T) { | ||
| scheme := runtime.NewScheme() | ||
| _ = csiapi.AddToScheme(scheme) | ||
|
|
||
| gsmConfig := &csi_secrets.GSMConfiguration{ | ||
| Config: &api.GSMConfig{}, | ||
| Client: &secretmanager.Client{}, | ||
| ProjectConfig: gsm.Config{ | ||
| ProjectIdString: "test-project", | ||
| ProjectIdNumber: "123456", | ||
| }, | ||
| } | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| secrets []*api.Secret | ||
| enableCSI bool | ||
| gsmConfig *csi_secrets.GSMConfiguration | ||
| expectedError string | ||
| expectedCredentials []api.CredentialReference | ||
| }{ | ||
| { | ||
| name: "no secrets - nothing to resolve", | ||
| secrets: []*api.Secret{}, | ||
| }, | ||
| { | ||
| name: "only K8s secrets - skipped by resolution", | ||
| secrets: []*api.Secret{ | ||
| {Name: "k8s-secret", MountPath: "/usr/k8s"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "GSM explicit field secrets - resolved and SPCs created", | ||
| secrets: []*api.Secret{ | ||
| {Collection: "test-col", Group: "test-group", Field: "field-a", MountPath: "/var/secrets"}, | ||
| {Collection: "test-col", Group: "test-group", Field: "field-b", MountPath: "/var/secrets"}, | ||
| }, | ||
| enableCSI: true, | ||
| gsmConfig: gsmConfig, | ||
| expectedCredentials: []api.CredentialReference{ | ||
| {Collection: "test-col", Group: "test-group", Field: "field-a", MountPath: "/var/secrets"}, | ||
| {Collection: "test-col", Group: "test-group", Field: "field-b", MountPath: "/var/secrets"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "mixed K8s and GSM secrets", | ||
| secrets: []*api.Secret{ | ||
| {Name: "k8s-secret-from-cluster", MountPath: "/usr/k8s"}, | ||
| {Collection: "test-col", Group: "test-group", Field: "cred1", MountPath: "/var/gsm"}, | ||
| }, | ||
| enableCSI: true, | ||
| gsmConfig: gsmConfig, | ||
| expectedCredentials: []api.CredentialReference{ | ||
| {Collection: "test-col", Group: "test-group", Field: "cred1", MountPath: "/var/gsm"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "GSM secret without GSM config", | ||
| secrets: []*api.Secret{ | ||
| {Collection: "test-col", Group: "test-group", Field: "field-a", MountPath: "/var/secrets"}, | ||
| }, | ||
| enableCSI: true, | ||
| gsmConfig: nil, | ||
| expectedError: "GSM client was not initialized - credentials file may be missing", | ||
| }, | ||
| { | ||
| name: "default mount paths when not specified", | ||
| secrets: []*api.Secret{ | ||
| {Collection: "test-col", Group: "group1", Field: "field-a"}, | ||
| {Collection: "test-col", Group: "group2", Field: "field-b"}, | ||
| }, | ||
| enableCSI: true, | ||
| gsmConfig: gsmConfig, | ||
| expectedCredentials: []api.CredentialReference{ | ||
| {Collection: "test-col", Group: "group1", Field: "field-a", MountPath: "/usr/test-secrets"}, | ||
| {Collection: "test-col", Group: "group2", Field: "field-b", MountPath: "/usr/test-secrets-2"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "explicit field with as - as propagated", | ||
| secrets: []*api.Secret{ | ||
| {Collection: "test-col", Group: "test-group", Field: "field-a", As: "renamed", MountPath: "/var/secrets"}, | ||
| }, | ||
| enableCSI: true, | ||
| gsmConfig: gsmConfig, | ||
| expectedCredentials: []api.CredentialReference{ | ||
| {Collection: "test-col", Group: "test-group", Field: "field-a", As: "renamed", MountPath: "/var/secrets"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| fakeClient := &testhelper_kube.FakePodClient{ | ||
| FakePodExecutor: &testhelper_kube.FakePodExecutor{ | ||
| LoggingClient: loggingclient.New( | ||
| fakectrlruntimeclient.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| Build(), nil), | ||
| }, | ||
| } | ||
|
|
||
| step := expectedPodStepTemplate() | ||
| step.client = fakeClient | ||
| step.config.Secrets = tc.secrets | ||
| step.config.EnableSecretsStoreCSIDriver = tc.enableCSI | ||
| step.config.GSMConfig = tc.gsmConfig | ||
|
|
||
| err := step.resolveAndCreateGSMSecrets(context.TODO()) | ||
| if tc.expectedError != "" { | ||
| if err == nil { | ||
| t.Fatal("expected error but got nil") | ||
| } | ||
| if diff := cmp.Diff(tc.expectedError, err.Error(), testhelper.EquateErrorMessage); diff != "" { | ||
| t.Errorf("unexpected error (-want +got):\n%s", diff) | ||
| } | ||
| return | ||
| } | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if diff := cmp.Diff(tc.expectedCredentials, step.resolvedGSMCredentials); diff != "" { | ||
| t.Errorf("resolved credentials differ:\n%s", diff) | ||
| } | ||
|
|
||
| spcs := &csiapi.SecretProviderClassList{} | ||
| if err := fakeClient.List(context.TODO(), spcs, ctrlruntimeclient.InNamespace(step.jobSpec.Namespace())); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| sort.Slice(spcs.Items, func(i, j int) bool { | ||
| return spcs.Items[i].Name < spcs.Items[j].Name | ||
| }) | ||
| testhelper.CompareWithFixture(t, spcs.Items, testhelper.WithPrefix("spcs")) | ||
|
|
||
| pod, err := step.generatePodForStep("test-image:latest", corev1.ResourceRequirements{}, false) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error generating pod: %v", err) | ||
| } | ||
| sort.Slice(pod.Spec.Volumes, func(i, j int) bool { | ||
| return pod.Spec.Volumes[i].Name < pod.Spec.Volumes[j].Name | ||
| }) | ||
| sort.Slice(pod.Spec.Containers[0].VolumeMounts, func(i, j int) bool { | ||
| return pod.Spec.Containers[0].VolumeMounts[i].Name < pod.Spec.Containers[0].VolumeMounts[j].Name | ||
| }) | ||
| testhelper.CompareWithFixture(t, pod.Spec.Volumes, testhelper.WithPrefix("volumes")) | ||
| testhelper.CompareWithFixture(t, pod.Spec.Containers[0].VolumeMounts, testhelper.WithPrefix("mounts")) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for GSM secrets configured without CSI driver enabled.
None of the new cases exercise enableCSI: false combined with GSM/bundle secrets present in Secrets, which is exactly the silent-drop scenario flagged in pod.go. Adding this case would lock in whatever fix is applied there.
Want me to draft the additional test case once the fix in pod.go lands?
🤖 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/pod_test.go` around lines 376 - 525, Add a test case in
TestResolveAndCreateGSMSecrets that sets EnableSecretsStoreCSIDriver to false
while still providing GSM secrets in step.config.Secrets, so the
resolveAndCreateGSMSecrets path is covered for the silent-drop scenario. Use the
existing test table in TestResolveAndCreateGSMSecrets and verify the behavior of
resolveAndCreateGSMSecrets, resolvedGSMCredentials, and the absence or handling
of SecretProviderClass resources matches the intended fix when CSI is disabled.
Comment on lines
+110
to
+115
| if s.config.EnableSecretsStoreCSIDriver { | ||
| if err := s.resolveAndCreateGSMSecrets(ctx); err != nil { | ||
| return fmt.Errorf("failed to set up GSM secrets: %w", err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
GSM secrets silently dropped when CSI driver is disabled.
resolveAndCreateGSMSecrets only runs when EnableSecretsStoreCSIDriver is true (Line 110). If it's false but config.Secrets still contains GSM/bundle references (which have no .Name, per Secrets() in podspec.go skipping IsGSMReference()/IsBundleReference()), generatePodForStep's K8s-secret loop skips them via secret.Name == "" (Line 309-311), and resolvedGSMCredentials stays nil. The secret is never mounted and no error is raised — a test could run to completion missing credentials it expects, failing confusingly downstream.
🐛 Proposed fix: fail fast instead of silently dropping
- if s.config.EnableSecretsStoreCSIDriver {
- if err := s.resolveAndCreateGSMSecrets(ctx); err != nil {
- return fmt.Errorf("failed to set up GSM secrets: %w", err)
- }
- }
+ if s.config.EnableSecretsStoreCSIDriver {
+ if err := s.resolveAndCreateGSMSecrets(ctx); err != nil {
+ return fmt.Errorf("failed to set up GSM secrets: %w", err)
+ }
+ } else {
+ for _, secret := range s.config.Secrets {
+ if secret.IsGSMReference() || secret.IsBundleReference() {
+ return errors.New("test declares GSM/bundle secrets but Secrets Store CSI driver is not enabled")
+ }
+ }
+ }Also applies to: 429-483
🤖 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/pod.go` around lines 110 - 115, The GSM secret handling in pod
setup is being silently skipped when EnableSecretsStoreCSIDriver is false, which
can leave unresolved GSM/bundle secrets unmounted without any error. Update
generatePodForStep and the resolveAndCreateGSMSecrets path in pod.go so that if
config.Secrets contains GSMReference or BundleReference entries and the CSI
driver is disabled, the code fails fast with a clear error instead of relying on
the K8s-secret loop to skip empty secret.Name values. Keep the existing GSM
resolution logic, but add an explicit validation step before secret mounting
using the existing Symbols like resolveAndCreateGSMSecrets, generatePodForStep,
and config.Secrets.
Contributor
|
@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. |
psalajova
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
secretsfield #5281 , for https://redhat.atlassian.net/browse/DPTP-5009pkg/steps/csi_secrets/package frommulti_stage/for shared GSM/CSI utilitiespod.goto resolve GSM secrets (collection/group/field) from thesecrets:field, create SecretProviderClass objects, and mount CSI volumes on the podresolveAndCreateGSMSecretscovering: no secrets, K8s-only, GSM explicit fields, mixed K8s+GSM, missing config error, default mount paths, andasfield propagationGenerated with Claude Code
This PR adds GSM-backed secret support to CI test pod generation and makes CSI secret handling reusable across ci-operator and multi-stage steps.
For CI users, secrets can now be resolved from the
secrets:field as either plain Kubernetes secrets or GSM references usingcollection,group, andfield. When GSM secrets are used, the pod spec now gets the neededSecretProviderClassresources and CSI volume mounts automatically, including default mount-path handling and support for theasrename field.The change also refactors shared CSI/GSM utilities into
pkg/steps/csi_secrets, updates ci-operator/defaults to use the new GSM config type, and wires the CSI driver flag through pod and multi-stage step creation so secret handling is consistent end to end.Integration coverage was added for GSM resolution, SPC creation, pod volume/mount generation, mixed K8s + GSM secrets, missing config errors, and deduplication/censoring behavior.