multi-stage: support projected SA token volumes with custom audiences#5093
multi-stage: support projected SA token volumes with custom audiences#5093patjlm wants to merge 7 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:
WalkthroughAdds projected ServiceAccount token support: new API types for token volumes and allowed-audiences mapping, pod generation to add projected token volumes/mounts, validation (including audience ownership checks using metadata), loader/CLI for allowed-audiences config, and tests/fixtures updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: patjlm The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/validation/test.go (1)
946-965: Consider validating thatmount_pathis an absolute path.The validation for credentials (line 815-816) enforces that
mountPathmust be an absolute path usingfilepath.IsAbs(). For consistency and to prevent potential issues with relative paths, the same check should be applied here.♻️ Proposed fix to add absolute path validation
if token.MountPath == "" { ret = append(ret, fmt.Errorf("%s.mount_path: must not be empty", fieldPath)) } else if mountPaths.Has(token.MountPath) { ret = append(ret, fmt.Errorf("%s.mount_path: duplicate mount path %q", fieldPath, token.MountPath)) + } else if !filepath.IsAbs(token.MountPath) { + ret = append(ret, fmt.Errorf("%s.mount_path: must be an absolute path", fieldPath)) } else { mountPaths.Insert(token.MountPath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validation/test.go` around lines 946 - 965, In validateServiceAccountTokens, add an absolute-path check for token.MountPath (similar to the other credentials validation using filepath.IsAbs): after confirming token.MountPath is non-empty and before/when inserting into mountPaths, call filepath.IsAbs(token.MountPath) and if false append an error like "%s.mount_path: must be an absolute path" to the returned errors; keep existing duplicate/mountPaths logic intact and reference token.MountPath, mountPaths, and filepath.IsAbs when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/validation/test.go`:
- Around line 946-965: In validateServiceAccountTokens, add an absolute-path
check for token.MountPath (similar to the other credentials validation using
filepath.IsAbs): after confirming token.MountPath is non-empty and before/when
inserting into mountPaths, call filepath.IsAbs(token.MountPath) and if false
append an error like "%s.mount_path: must be an absolute path" to the returned
errors; keep existing duplicate/mountPaths logic intact and reference
token.MountPath, mountPaths, and filepath.IsAbs when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef412c8e-8b7f-4f31-9512-2fcb501d4432
📒 Files selected for processing (5)
pkg/api/types.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/testdata/zz_fixture_TestGeneratePods_service_account_token_projection.yamlpkg/validation/test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/load/load.go`:
- Around line 324-331: The YAML loader currently uses yaml.Unmarshal into
[]api.AllowedAudienceDetails and allows unknown fields and silent duplicate
audience entries; change it to use a yaml.Decoder with KnownFields(true) (or
equivalent strict unmarshalling) when decoding configContents into a slice of
api.AllowedAudienceDetails to reject unknown fields, and when building
allowedAudiencesMap check for existing keys and return an error if a duplicate
a.Audience is found instead of overwriting; reference symbols: yaml.Unmarshal,
api.AllowedAudienceDetails, audiencesList, allowedAudiencesMap, and the loop
that assigns allowedAudiencesMap[a.Audience] = a.
In `@pkg/validation/test.go`:
- Around line 950-968: The validateServiceAccountTokens function currently only
rejects empty or exact-duplicate mount paths; update it to apply the same
absolute-path and overlap validation used in validateCredentials: after
verifying token.MountPath is non-empty, ensure it is an absolute, cleaned path
(starts with "/" and normalized via path.Clean) and then check for parent/child
overlaps against existing mountPaths (not just exact duplicates) using the same
prefix/ancestor logic from validateCredentials; use mountPaths (and the same
representation used by validateCredentials) to detect overlaps and append
appropriate fmt.Errorf messages when a mount path is not absolute or overlaps an
existing mount.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4a95ce9-2562-4760-9911-8d1625818e98
📒 Files selected for processing (6)
cmd/ci-operator-checkconfig/main.gopkg/api/types.gopkg/load/load.gopkg/validation/config.gopkg/validation/test.gopkg/validation/test_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/validation/test.go (1)
950-988: SA token mount paths are validated only against each other, not against credentials.The overlap detection correctly prevents conflicts between
service_account_tokensentries, but does not cross-validate againststep.Credentialsmount paths. Since both are mounted into the same container (pergen.go:661-665), overlapping paths could still cause runtime mount conflicts.If this is intentional scope for this PR, consider adding cross-validation in a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validation/test.go` around lines 950 - 988, Service account token mount paths in validateServiceAccountTokens are only checked against other service_account_tokens and not against step.Credentials, allowing cross-conflicts; extend validation to also compare each token.MountPath against all credentials mount paths (the same way other tokens are checked) and report errors using the same fieldPath formatting (e.g., "%s.mount_path: ...") when a token path is equal to, under, or contains any credentials mount path from step.Credentials; locate validateServiceAccountTokens and the credentials source (step.Credentials in gen.go) and reuse the same filepath.Rel logic and duplicate/under-path error messages to perform the cross-validation before inserting into mountPaths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/validation/test.go`:
- Around line 992-995: The verifyAudienceOwnership function currently returns
nil when metadata (m) is nil or m.Org is empty which is inconsistent with
verifyClusterProfileOwnership; change verifyAudienceOwnership to reject
validation in that case by returning an error (matching the behavior/pattern
used in verifyClusterProfileOwnership) and update the error message to clearly
state metadata is required for ownership checks; if the permissive return was
intentional for registry reference flows, instead add a comment in
verifyAudienceOwnership explaining why nil/empty metadata should be allowed.
---
Nitpick comments:
In `@pkg/validation/test.go`:
- Around line 950-988: Service account token mount paths in
validateServiceAccountTokens are only checked against other
service_account_tokens and not against step.Credentials, allowing
cross-conflicts; extend validation to also compare each token.MountPath against
all credentials mount paths (the same way other tokens are checked) and report
errors using the same fieldPath formatting (e.g., "%s.mount_path: ...") when a
token path is equal to, under, or contains any credentials mount path from
step.Credentials; locate validateServiceAccountTokens and the credentials source
(step.Credentials in gen.go) and reuse the same filepath.Rel logic and
duplicate/under-path error messages to perform the cross-validation before
inserting into mountPaths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16777245-7313-413b-b82a-1d580b155598
📒 Files selected for processing (1)
pkg/validation/test.go
Add a `service_account_tokens` field to LiteralTestStep that allows
step authors to mount projected service account tokens with custom
audiences into their test containers. The kubelet handles the token
request transparently — no additional RBAC is required.
This enables workloads that need to exchange tokens with external
identity providers (e.g., GCP Workload Identity Federation, Vault)
without requiring `create` permission on `serviceaccounts/token`.
Each entry specifies:
- audience: the intended recipient of the token
- mount_path: where the token file is mounted
- expiration_seconds: token TTL (default 3600, min 600)
Example usage in step config:
service_account_tokens:
- audience: gcp-hcp-ci-wif
mount_path: /var/run/secrets/wif
expiration_seconds: 3600
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add an allowed-audiences-config mechanism that restricts which org/repo combinations can use specific service account token audiences. This prevents unauthorized CI configs from using audiences belonging to other teams' WIF pools. Follows the existing cluster profile ownership pattern: - Config file maps audiences to allowed org/repo owners - ci-operator-checkconfig validates at PR time - Unlisted audiences are unrestricted (permissive by default) - Missing config file is treated as empty (no restrictions) New flag: --allowed-audiences-config (optional) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Regenerate deepcopy functions for new types - Use strict YAML unmarshal to catch typos - Reject empty or duplicate audience entries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…aths Match the validation behavior of validateCredentials: reject relative paths and detect parent/child mount path overlaps that would shadow each other at runtime. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6329cba to
a7c96e9
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/steps/multi_stage/gen.go`:
- Line 252: When adding projected token volumes via
addServiceAccountTokenVolumes(step.ServiceAccountTokens, pod), ensure we do not
clear the job's service account when ServiceAccountTokens is non-empty: if
step.ServiceAccountTokens has entries, set pod.Spec.ServiceAccountName = s.name
(so tokens are minted for the intended SA) and also set
pod.Spec.AutomountServiceAccountToken = pointer to false to avoid legacy
automount; only clear ServiceAccountName in the no_kubeconfig branch when
ServiceAccountTokens is empty. Update the logic around
addServiceAccountTokenVolumes and the previous no_kubeconfig branch to follow
this conditional.
In `@pkg/validation/test.go`:
- Around line 950-988: The current validateServiceAccountTokens function only
checks mount-path collisions among service_account_tokens; extend
validateServiceAccountTokens to also accept the step credentials slice (e.g.,
add a credentials parameter) and for each token compare token.MountPath against
every credential.MountPath to reject exact matches and parent/child overlaps
using the same filepath.Rel + ".." logic used for tokens, and ensure you still
track mountPaths as before; finally update the call site that invokes
validateServiceAccountTokens to pass step.Credentials so the new credential
checks run.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dbcf363-f75d-4247-b6da-d00d39085770
📒 Files selected for processing (10)
cmd/ci-operator-checkconfig/main.gopkg/api/types.gopkg/api/zz_generated.deepcopy.gopkg/load/load.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/testdata/zz_fixture_TestGeneratePods_service_account_token_projection.yamlpkg/validation/config.gopkg/validation/test.gopkg/validation/test_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/validation/test_test.go
- pkg/steps/multi_stage/gen_test.go
- pkg/steps/multi_stage/testdata/zz_fixture_TestGeneratePods_service_account_token_projection.yaml
- pkg/api/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/load/load.go
|
Scheduling tests matching the |
- Keep ServiceAccountName when ServiceAccountTokens is non-empty, even for no_kubeconfig steps, so tokens are minted for the correct SA - Cross-validate SA token mount paths against credential mount paths to detect collisions and parent/child overlaps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/retest |
|
Scheduling tests matching the |
|
@patjlm: 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. |
Summary
Two features for multi-stage test steps:
Projected SA token volumes with custom audiences — new
service_account_tokensfield onLiteralTestStepthat mounts projected tokens with custom audiences into step containers. Enables WIF/Vault token exchange withoutserviceaccounts/tokenRBAC.Audience ownership validation — new
--allowed-audiences-configflag forci-operator-checkconfigthat restricts which org/repo combinations can use specific audiences. Follows the existing cluster profile ownership pattern. Prevents unauthorized CI configs from using other teams' WIF audiences.Motivation
We're migrating GCP HCP CI jobs from static service account JSON keys to Workload Identity Federation (WIF). WIF requires a token with a custom audience, but CI step pods don't have RBAC to call
kubectl create token --audience=...(verified on build01). The standard Kubernetes solution is a projected volume withserviceAccountToken.audience— the kubelet handles the token request transparently.The audience ownership validation ensures that only authorized repos can use a given WIF audience, preventing other teams' CI configs from authenticating to our GCP resources.
See openshift-online/gcp-hcp#38 and openshift-online/gcp-hcp#39 for context.
Changes
Commit 1: Projected SA token volumes
pkg/api/types.go:ServiceAccountTokenVolumetype andServiceAccountTokensfield onLiteralTestSteppkg/steps/multi_stage/gen.go:addServiceAccountTokenVolumes()injects projected volumes into step podspkg/validation/test.go: Validates audience/mount_path are non-empty, no duplicate mount paths, expiration >= 600sCommit 2: Audience ownership validation
pkg/api/types.go:AllowedAudienceDetails,AllowedAudienceOwners,AllowedAudiencesMaptypespkg/load/load.go:AllowedAudiencesConfig()loads config from YAML (missing file = empty, no restrictions)pkg/validation/config.go:allowedAudiencesfield onValidator,WithAllowedAudiencesoptionpkg/validation/test.go:verifyAudienceOwnership(), metadata threading through contextcmd/ci-operator-checkconfig/main.go:--allowed-audiences-configflag (optional)Example usage
Step config (openshift/release)
Audience ownership config (openshift/release)
Test plan
TestGeneratePods/service_account_token_projection— verifies projected volumesTestGeneratePodssubtests passpkg/validationtests pass (including updatednewContextcallers)go build ./pkg/...andgo build ./cmd/ci-operator-checkconfig/succeedkubectl create token(no RBAC)🤖 Generated with Claude Code