CCO-824: feat(gcp): add key-storage-method flag for workload identity providers#1004
CCO-824: feat(gcp): add key-storage-method flag for workload identity providers#1004jstuever wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
@jstuever: This pull request references CCO-824 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a CLI flag to choose GCP JWK key storage method and implements two provisioning flows: publish keys to a public GCS bucket or attach JWK JSON directly to the workload identity provider. Updates command wiring, options, client interface and mock, provisioning logic, tests, and rotation docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstuever 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 |
|
@jstuever: This pull request references CCO-824 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
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/cmd/provisioning/gcp/create_workload_identity_provider.go`:
- Around line 100-106: The pool-jwk-file branch currently synthesizes issuerURL
as "https://storage.googleapis.com/<name>-oidc" but never ensures the bucket
exists or is owned by the admin; update createIdentityProviderWithJwkFile usage
to require a validated/explicit issuer URL or perform bucket creation/ownership
verification: either (A) add an explicit issuerURL parameter/flag that the
caller must provide and validate (reject if empty) and use that instead of
synthesizing, or (B) implement creation/verification logic before calling
createIdentityProviderWithJwkFile (create the bucket, set ACLs, upload JWKS and
confirm HTTP 200) and only write that verified issuerURL into
ClusterAuthentication; ensure the code path that currently builds issuerURL (the
variable issuerURL and the call site) is updated to enforce one of these
behaviors and reject the pool-jwk-file mode when verification fails.
- Around line 147-159: The generated create identity provider script currently
passes only the JWKS basename (gcpOidcKeysFilename) to the gcloud command,
making the script fail when executed from a different cwd; modify the script
generation where script := fmt.Sprintf(createIdentityProviderScript, ...) to
pass a script-relative path for the jwks file (e.g. prepend '$(dirname "$0")/'
to gcpOidcKeysFilename or use the absolute jwksFilePath) so the
createIdentityProviderWithJwkFileCmd in createIdentityProviderScript always
references the correct file location regardless of caller cwd.
🪄 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: 35d9c41d-7bbb-4a92-8cde-006d01d38711
📒 Files selected for processing (6)
pkg/cmd/provisioning/gcp/create_all.gopkg/cmd/provisioning/gcp/create_workload_identity_provider.gopkg/cmd/provisioning/gcp/create_workload_identity_provider_test.gopkg/cmd/provisioning/gcp/gcp.gopkg/gcp/client.gopkg/gcp/mock/client_generated.go
|
/test e2e-gcp-manual-oidc |
|
/test e2e-gcp-manual-oidc |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go (1)
255-259: Pin the patch contract in this mock.The new behavior depends on a JWKS-only patch, but
gomock.Any()on the update mask won't catch a future regression that broadens the patch.Suggested tightening
func mockUpdateWorkloadIdentityProviderSuccess(mockGCPClient *mockgcp.MockClient) { - mockGCPClient.EXPECT().UpdateWorkloadIdentityProvider(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&iam.Operation{ - Done: true, - }, nil).Times(1) + mockGCPClient.EXPECT(). + UpdateWorkloadIdentityProvider( + gomock.Any(), + gomock.Any(), + gomock.AssignableToTypeOf(&iam.WorkloadIdentityPoolProvider{}), + gomock.Eq(oidcJwksUpdateMask), + ). + Return(&iam.Operation{ + Done: true, + }, nil). + Times(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go` around lines 255 - 259, The mock in mockUpdateWorkloadIdentityProviderSuccess currently uses gomock.Any() for the update mask which allows regressions; change the third (update mask) argument matcher in the UpdateWorkloadIdentityProvider EXPECT call to assert a JWKS-only patch, e.g. use gomock.Eq(&fieldmaskpb.FieldMask{Paths: []string{"jwks"}}) (or an equivalent matcher that checks the FieldMask.Paths contains only "jwks") instead of gomock.Any(), keeping the other argument matchers the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/rotate-oidc-key.md`:
- Around line 41-42: The doc currently reuses CLUSTER_NAME derived via
CLUSTER_NAME=${GCP_BUCKET%-*} for both the OIDC provider name and the workload
identity pool, which is incorrect when the pool ID differs; update the docs to
define separate variables (e.g., OIDC_PROVIDER_ID and WORKLOAD_IDENTITY_POOL_ID)
instead of reusing CLUSTER_NAME/`${GCP_BUCKET%-*}`, and add a short note or
command showing how to discover the actual workload identity pool name (for
example, via gcloud iam workload-identity-pools list or by inspecting the Google
Cloud console) before running the rotation commands; ensure references in the
sections around the current CLUSTER_NAME usage (including the ranges noted:
~110-145 and ~188-190) are changed to use the new distinct variables.
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider.go`:
- Around line 183-202: The create/update calls discard the returned
*iam.Operation so success is logged before the long-running IAM operation
finishes; modify the code paths that call client.CreateWorkloadIdentityProvider
and client.UpdateWorkloadIdentityProvider (and the variables receiving their
results) to capture the returned operation and poll it until completion (similar
to the Azure runtime.Poller usage), waiting for Done==true or PollUntilDone
before logging success; ensure you use the operation object from
CreateWorkloadIdentityProvider and UpdateWorkloadIdentityProvider (for the
providerResource/oidcJwksUpdateMask update case too) and handle/preserve any
final error or response from the completed operation.
---
Nitpick comments:
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go`:
- Around line 255-259: The mock in mockUpdateWorkloadIdentityProviderSuccess
currently uses gomock.Any() for the update mask which allows regressions; change
the third (update mask) argument matcher in the UpdateWorkloadIdentityProvider
EXPECT call to assert a JWKS-only patch, e.g. use
gomock.Eq(&fieldmaskpb.FieldMask{Paths: []string{"jwks"}}) (or an equivalent
matcher that checks the FieldMask.Paths contains only "jwks") instead of
gomock.Any(), keeping the other argument matchers the same.
🪄 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: 5a3648b5-99aa-4f48-a6b5-c3ad7759a355
📒 Files selected for processing (7)
docs/rotate-oidc-key.mdpkg/cmd/provisioning/gcp/create_all.gopkg/cmd/provisioning/gcp/create_workload_identity_provider.gopkg/cmd/provisioning/gcp/create_workload_identity_provider_test.gopkg/cmd/provisioning/gcp/gcp.gopkg/gcp/client.gopkg/gcp/mock/client_generated.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cmd/provisioning/gcp/gcp.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cmd/provisioning/gcp/create_all.go
- pkg/gcp/mock/client_generated.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1004 +/- ##
==========================================
+ Coverage 46.27% 46.46% +0.19%
==========================================
Files 98 98
Lines 12262 12346 +84
==========================================
+ Hits 5674 5737 +63
- Misses 5938 5954 +16
- Partials 650 655 +5
🚀 New features to boost your workflow:
|
|
@jstuever: This pull request references CCO-824 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-gcp-manual-oidc |
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/cmd/provisioning/gcp/create_workload_identity_provider_test.go (1)
204-215:⚠️ Potential issue | 🟡 MinorAssert success in the non-error cases.
The
elsebranch never checkserr == nil, so the no-opverifycases can still pass on unexpected failures. That weakens the table test for the new storage-method coverage.Suggested fix
if test.expectError { require.Error(t, err, "expected error returned") } else { + require.NoError(t, err, "unexpected error returned") test.verify(t, tempDirName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go` around lines 204 - 215, The test's non-error branch doesn't assert the call succeeded, so add an explicit assertion that err is nil before running the verification; specifically, in the table test after calling createWorkloadIdentityProvider(...) when test.expectError is false, call require.NoError(t, err) (or equivalent) prior to invoking test.verify(t, tempDirName) to ensure failures don't silently pass the verification step. Ensure this change is applied in the same test block that uses tempDirName, testPublicKeyPath and the createWorkloadIdentityProvider invocation.
🧹 Nitpick comments (2)
pkg/cmd/provisioning/gcp/create_workload_identity_provider.go (1)
172-186: Extract a shared provider builder.This
iam.WorkloadIdentityPoolProviderbody is now assembled here and again increateIdentityProvider, so future changes to issuer/audience/attribute-mapping defaults have to stay in sync in two paths. A small helper that builds the common provider shape and optionally injectsOidc.JwksJsonwould reduce drift.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider.go` around lines 172 - 186, Extract the duplicated iam.WorkloadIdentityPoolProvider construction into a single helper (e.g., buildWorkloadIdentityPoolProvider) that returns *iam.WorkloadIdentityPoolProvider given the common inputs (name, issuerURL, openShiftAudience, createdByCcoctl) and an optional jwks string parameter; have the helper set Name, DisplayName, Description, Disabled, State, Oidc with AllowedAudiences and IssuerUri, and the AttributeMapping, and only populate Oidc.JwksJson when the jwks argument is non-empty. Replace the two inline constructions (the block creating provider in create_workload_identity_provider.go and the one in createIdentityProvider) with calls to this helper and pass jwks where available. Ensure the helper is used in both places so issuer/audience/attribute-mapping defaults remain consistent.pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go (1)
353-370: Check the embedded-JWKS payload, not just that a call happened.These subtests only prove that create/update was invoked. They do not verify that the provider carries a non-empty embedded JWKS or that the update uses
oidc.jwks_json, so an embedded-JWKS regression can still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go` around lines 353 - 370, The tests "pool-jwk-file: identity provider created with embedded JWK" and "...updated with new JWK" only assert that create/update calls occurred; update the mock expectations (in mockCreateWorkloadIdentityProviderSuccess and mockUpdateWorkloadIdentityProviderSuccess or inline gomock.Expect calls used in those subtests) to capture the WorkloadIdentityProvider request argument and assert its oidc.jwks_json field is present and non-empty (and/or that the OIDC config contains the embedded JWKS JSON) instead of only verifying the call happened; use gomock Do/DoAndReturn or a gomock.Matcher to inspect the request object and fail the test if jwks_json is empty or missing so the tests validate the actual embedded-JWKS payload produced by the code paths using setupTempDirWithKeyAndManifests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/rotate-oidc-key.md`:
- Around line 105-108: Update the documentation heading and examples that
currently say "private-bucket" to use the supported mode name "public-bucket" so
they match the supported modes in
pkg/cmd/provisioning/gcp/create_workload_identity_provider.go; specifically
replace occurrences of the term "private-bucket" (e.g., the heading "GCP
private-bucket" and the block at lines 137–140) with "GCP public-bucket" and
ensure the examples reference the supported "public-bucket" and "pool-jwk-file"
options consistently (matching the behavior referenced around Line 183).
---
Outside diff comments:
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go`:
- Around line 204-215: The test's non-error branch doesn't assert the call
succeeded, so add an explicit assertion that err is nil before running the
verification; specifically, in the table test after calling
createWorkloadIdentityProvider(...) when test.expectError is false, call
require.NoError(t, err) (or equivalent) prior to invoking test.verify(t,
tempDirName) to ensure failures don't silently pass the verification step.
Ensure this change is applied in the same test block that uses tempDirName,
testPublicKeyPath and the createWorkloadIdentityProvider invocation.
---
Nitpick comments:
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go`:
- Around line 353-370: The tests "pool-jwk-file: identity provider created with
embedded JWK" and "...updated with new JWK" only assert that create/update calls
occurred; update the mock expectations (in
mockCreateWorkloadIdentityProviderSuccess and
mockUpdateWorkloadIdentityProviderSuccess or inline gomock.Expect calls used in
those subtests) to capture the WorkloadIdentityProvider request argument and
assert its oidc.jwks_json field is present and non-empty (and/or that the OIDC
config contains the embedded JWKS JSON) instead of only verifying the call
happened; use gomock Do/DoAndReturn or a gomock.Matcher to inspect the request
object and fail the test if jwks_json is empty or missing so the tests validate
the actual embedded-JWKS payload produced by the code paths using
setupTempDirWithKeyAndManifests.
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider.go`:
- Around line 172-186: Extract the duplicated iam.WorkloadIdentityPoolProvider
construction into a single helper (e.g., buildWorkloadIdentityPoolProvider) that
returns *iam.WorkloadIdentityPoolProvider given the common inputs (name,
issuerURL, openShiftAudience, createdByCcoctl) and an optional jwks string
parameter; have the helper set Name, DisplayName, Description, Disabled, State,
Oidc with AllowedAudiences and IssuerUri, and the AttributeMapping, and only
populate Oidc.JwksJson when the jwks argument is non-empty. Replace the two
inline constructions (the block creating provider in
create_workload_identity_provider.go and the one in createIdentityProvider) with
calls to this helper and pass jwks where available. Ensure the helper is used in
both places so issuer/audience/attribute-mapping defaults remain consistent.
🪄 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: b6bb978c-6656-4b86-9ce0-7f54305998db
📒 Files selected for processing (7)
docs/rotate-oidc-key.mdpkg/cmd/provisioning/gcp/create_all.gopkg/cmd/provisioning/gcp/create_workload_identity_provider.gopkg/cmd/provisioning/gcp/create_workload_identity_provider_test.gopkg/cmd/provisioning/gcp/gcp.gopkg/gcp/client.gopkg/gcp/mock/client_generated.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cmd/provisioning/gcp/gcp.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/gcp/client.go
- pkg/cmd/provisioning/gcp/create_all.go
Introduce a new --key-storage-method flag that allows users to choose how JWK files are stored when provisioning GCP workload identity providers. Two methods are now supported: - "public-bucket" (default): Creates a public GCS bucket to host OIDC configuration and JWK files - "pool-jwk-file": Attaches the JWK directly to the workload identity pool provider without creating a bucket The pool-jwk-file method is useful for environments with strict bucket policies or when a bucketless configuration is preferred. The implementation includes create and update operations for identity providers with embedded JWKs. Assisted-by: Claude Sonnet 4.6
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go (1)
313-317: Make the update-path mock verify the field mask and JWKS payload.
gomock.Any()here means the new update test still passes if the implementation patches the wrong field or sends an emptyOidc.JwksJson, which is the core behavior this PR adds. This helper should at least pinoidcJWKSUpdateMaskand inspect the provider body.As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go` around lines 313 - 317, The mock helper mockUpdateWorkloadIdentityProviderSuccess should assert that UpdateWorkloadIdentityProvider is called with the correct field mask and non-empty JWKS payload: replace the third/fourth gomock.Any() matchers with expectations that the update mask equals the oidcJWKSUpdateMask and that the provider body contains Oidc.JwksJson matching the expected JWKS string (or non-empty) — i.e., in mockUpdateWorkloadIdentityProviderSuccess set the gomock matcher for the mask to compare to oidcJWKSUpdateMask and add a custom matcher or predicate that inspects the request body to ensure Oidc.JwksJson is present and correct before returning the iam.Operation{Done:true}, nil.pkg/cmd/provisioning/gcp/create_workload_identity_provider.go (1)
172-186: Extract the common provider spec before the two modes drift.Lines 172-186 now build the same
WorkloadIdentityPoolProvidershape thatcreateIdentityProvider()already assembles on Lines 334-349, withOidc.JwksJsonas the main difference. Pulling the shared fields into a helper will keep issuer, audience, description, and attribute mapping changes consistent acrosspublic-bucketandpool-jwk-file.As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider.go` around lines 172 - 186, The provider construction is duplicated between the block that builds the iam.WorkloadIdentityPoolProvider (with Oidc.JwksJson set from jwks) and createIdentityProvider(); extract the shared fields into a helper like buildBaseProvider(name, issuerURL, openShiftAudience, createdByCcoctl) that returns an *iam.WorkloadIdentityPoolProvider with Name, DisplayName, Description, Disabled, State, Oidc with AllowedAudiences and IssuerUri, and AttributeMapping populated, then in the two call sites (the code in create_workload_identity_provider.go and createIdentityProvider()) call that helper and only set or override the differing JwksJson (e.g., provider.Oidc.JwksJson = string(jwks) or provider.Oidc.JwksJson = jwksFromFile) before using the provider.
🤖 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/cmd/provisioning/gcp/create_workload_identity_provider_test.go`:
- Around line 313-317: The mock helper mockUpdateWorkloadIdentityProviderSuccess
should assert that UpdateWorkloadIdentityProvider is called with the correct
field mask and non-empty JWKS payload: replace the third/fourth gomock.Any()
matchers with expectations that the update mask equals the oidcJWKSUpdateMask
and that the provider body contains Oidc.JwksJson matching the expected JWKS
string (or non-empty) — i.e., in mockUpdateWorkloadIdentityProviderSuccess set
the gomock matcher for the mask to compare to oidcJWKSUpdateMask and add a
custom matcher or predicate that inspects the request body to ensure
Oidc.JwksJson is present and correct before returning the
iam.Operation{Done:true}, nil.
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider.go`:
- Around line 172-186: The provider construction is duplicated between the block
that builds the iam.WorkloadIdentityPoolProvider (with Oidc.JwksJson set from
jwks) and createIdentityProvider(); extract the shared fields into a helper like
buildBaseProvider(name, issuerURL, openShiftAudience, createdByCcoctl) that
returns an *iam.WorkloadIdentityPoolProvider with Name, DisplayName,
Description, Disabled, State, Oidc with AllowedAudiences and IssuerUri, and
AttributeMapping populated, then in the two call sites (the code in
create_workload_identity_provider.go and createIdentityProvider()) call that
helper and only set or override the differing JwksJson (e.g.,
provider.Oidc.JwksJson = string(jwks) or provider.Oidc.JwksJson = jwksFromFile)
before using the provider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc353cd9-50c3-4a10-8f06-7ea3d51d70e4
📒 Files selected for processing (7)
docs/rotate-oidc-key.mdpkg/cmd/provisioning/gcp/create_all.gopkg/cmd/provisioning/gcp/create_workload_identity_provider.gopkg/cmd/provisioning/gcp/create_workload_identity_provider_test.gopkg/cmd/provisioning/gcp/gcp.gopkg/gcp/client.gopkg/gcp/mock/client_generated.go
✅ Files skipped from review due to trivial changes (2)
- pkg/cmd/provisioning/gcp/gcp.go
- pkg/gcp/mock/client_generated.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/gcp/client.go
- docs/rotate-oidc-key.md
|
/test e2e-gcp-manual-oidc |
|
/override ci/prow/security |
|
@jstuever: Overrode contexts on behalf of jstuever: ci/prow/security DetailsIn response to this:
Instructions 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. |
|
@jstuever: all tests passed! 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. |
|
/assign @dlom |
Introduce a new --key-storage-method flag that allows users to choose how JWK files are stored when provisioning GCP workload identity providers. Two methods are now supported:
The pool-jwk-file method is useful for environments with strict bucket policies or when a bucketless configuration is preferred. The implementation includes create and update operations for identity providers with embedded JWKs.
Assisted-by: Claude Sonnet 4.6