Skip to content

CCO-824: feat(gcp): add key-storage-method flag for workload identity providers#1004

Open
jstuever wants to merge 1 commit intoopenshift:masterfrom
jstuever:CCO-824
Open

CCO-824: feat(gcp): add key-storage-method flag for workload identity providers#1004
jstuever wants to merge 1 commit intoopenshift:masterfrom
jstuever:CCO-824

Conversation

@jstuever
Copy link
Copy Markdown
Contributor

@jstuever jstuever commented Mar 30, 2026

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

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 30, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Mar 30, 2026

@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.

Details

In response to this:

Introduce a new --oidc-storage-method flag that allows users to choose how OIDC 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, gemini-3.1-pro-preview

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

Walkthrough

Adds 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

Cohort / File(s) Summary
CLI — create-all wiring
pkg/cmd/provisioning/gcp/create_all.go
Added persistent --key-storage-method flag, validate via validateKeyStorageMethod, and pass CreateAllOpts.KeyStorageMethod into workload identity provider creation call.
CLI — workload identity provider logic
pkg/cmd/provisioning/gcp/create_workload_identity_provider.go
Extended createWorkloadIdentityProvider signature to accept keyStorageMethod; added branching for public-bucket vs pool-jwk-file; implemented createIdentityProviderWithJWKFile; handle generate-only vs live create/update flows and explicit patch via update mask; added GCP resource format constants.
Options struct
pkg/cmd/provisioning/gcp/gcp.go
Added KeyStorageMethod string field to the options struct.
GCP client API & mock
pkg/gcp/client.go, pkg/gcp/mock/client_generated.go
Added UpdateWorkloadIdentityProvider(context.Context, string, *iam.WorkloadIdentityPoolProvider, string) to Client interface and gcpClient implementation; updated/generated mock with UpdateWorkloadIdentityProvider expectation.
Tests
pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go
Expanded tests to include keyStorageMethod parameter and defaulting; added TestValidateKeyStorageMethod; new TestCreateWorkloadIdentityProviderWithPoolJwkFile suite; added helpers mockUpdateWorkloadIdentityProviderSuccess and setupTempDirWithKeyAndManifests; added failure and generate-only cases.
Docs
docs/rotate-oidc-key.md
Split rotation instructions into public-bucket and pool-jwk-file variants, adding commands to read/write JWKS from the workload identity provider and use --jwk-json-path for updates.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot requested a review from 2uasimojo March 30, 2026 22:18
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Mar 30, 2026

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot requested a review from suhanime March 30, 2026 22:18
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Mar 30, 2026

@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.

Details

In response to this:

Introduce a new --oidc-storage-method flag that allows users to choose how OIDC 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

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b5affa7 and 7e80129.

📒 Files selected for processing (6)
  • pkg/cmd/provisioning/gcp/create_all.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go
  • pkg/cmd/provisioning/gcp/gcp.go
  • pkg/gcp/client.go
  • pkg/gcp/mock/client_generated.go

@jstuever
Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-manual-oidc
Validate the default works.

@jstuever
Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-manual-oidc

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7e80129 and 5ba7e56.

📒 Files selected for processing (7)
  • docs/rotate-oidc-key.md
  • pkg/cmd/provisioning/gcp/create_all.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go
  • pkg/cmd/provisioning/gcp/gcp.go
  • pkg/gcp/client.go
  • pkg/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
Copy link
Copy Markdown

codecov bot commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 70.83333% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.46%. Comparing base (2ed316a) to head (870a14e).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...visioning/gcp/create_workload_identity_provider.go 74.68% 13 Missing and 7 partials ⚠️
pkg/cmd/provisioning/gcp/create_all.go 0.00% 4 Missing ⚠️
pkg/gcp/client.go 0.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
pkg/cmd/provisioning/gcp/gcp.go 0.00% <ø> (ø)
pkg/gcp/mock/client_generated.go 84.29% <100.00%> (+0.43%) ⬆️
pkg/cmd/provisioning/gcp/create_all.go 0.00% <0.00%> (ø)
pkg/gcp/client.go 0.00% <0.00%> (ø)
...visioning/gcp/create_workload_identity_provider.go 58.33% <74.68%> (+8.90%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Mar 31, 2026

@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.

Details

In response to this:

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

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.

@jstuever jstuever changed the title CCO-824: feat(gcp): add oidc-storage-method flag for workload identity providers CCO-824: feat(gcp): add key-storage-method flag for workload identity providers Mar 31, 2026
@jstuever
Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-manual-oidc

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Assert success in the non-error cases.

The else branch never checks err == nil, so the no-op verify cases 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.WorkloadIdentityPoolProvider body is now assembled here and again in createIdentityProvider, 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 injects Oidc.JwksJson would 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5ba7e56 and 921a037.

📒 Files selected for processing (7)
  • docs/rotate-oidc-key.md
  • pkg/cmd/provisioning/gcp/create_all.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go
  • pkg/cmd/provisioning/gcp/gcp.go
  • pkg/gcp/client.go
  • pkg/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
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 empty Oidc.JwksJson, which is the core behavior this PR adds. This helper should at least pin oidcJWKSUpdateMask and 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 WorkloadIdentityPoolProvider shape that createIdentityProvider() already assembles on Lines 334-349, with Oidc.JwksJson as the main difference. Pulling the shared fields into a helper will keep issuer, audience, description, and attribute mapping changes consistent across public-bucket and pool-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

📥 Commits

Reviewing files that changed from the base of the PR and between 921a037 and 870a14e.

📒 Files selected for processing (7)
  • docs/rotate-oidc-key.md
  • pkg/cmd/provisioning/gcp/create_all.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider.go
  • pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go
  • pkg/cmd/provisioning/gcp/gcp.go
  • pkg/gcp/client.go
  • pkg/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

@jstuever
Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-manual-oidc

@jstuever
Copy link
Copy Markdown
Contributor Author

/override ci/prow/security
I have created a card to track this.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Mar 31, 2026

@jstuever: Overrode contexts on behalf of jstuever: ci/prow/security

Details

In response to this:

/override ci/prow/security
I have created a card to track 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.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 1, 2026

@jstuever: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@jstuever
Copy link
Copy Markdown
Contributor Author

jstuever commented Apr 1, 2026

/assign @dlom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants