feat: use Cloud Connector ID in GCP Workload Identity Federation session name#3955
feat: use Cloud Connector ID in GCP Workload Identity Federation session name#3955amirbenun wants to merge 3 commits intoelastic:mainfrom
Conversation
|
This pull request does not have a backport label. Could you fix it @amirbenun? 🙏
|
There was a problem hiding this comment.
Pull request overview
This PR updates the GCP Cloud Connectors authentication flow to include a deployment-specific Cloud Connector ID in the AWS AssumeRoleWithWebIdentity session name, matching the GCP Workload Identity Federation-required format (ResourceID-CloudConnectorID).
Changes:
- Add
CloudConnectorID(cloud_connector_id) toconfig.GcpClientOpt. - Introduce
GCPCloudConnectorsParamsand refactorFindCloudConnectorsCredentialsto accept it and setRoleSessionNametoResourceID-CloudConnectorID. - Update call sites, mocks, and tests to use the new API shape.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/resources/providers/gcplib/auth/credentials_test.go | Updates test configs to include CloudConnectorID and adjusts mock expectations for the refactored auth provider API. |
| internal/resources/providers/gcplib/auth/credentials_mock.go | Regenerates/updates mock signatures to accept GCPCloudConnectorsParams. |
| internal/resources/providers/gcplib/auth/credentials.go | Passes audience, service account email, and connector ID via GCPCloudConnectorsParams to the auth provider. |
| internal/resources/providers/gcplib/auth/auth_provider.go | Adds GCPCloudConnectorsParams, validates connector ID, and sets AWS RoleSessionName to the concatenated format. |
| internal/config/config.go | Adds the cloud_connector_id config field to GcpClientOpt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Session name must match GCP Workload Identity Federation: elastic_resource_id-cloud_connector_id | ||
| sessionName := ccConfig.ResourceID + "-" + params.CloudConnectorID |
There was a problem hiding this comment.
sessionName is used as stscreds.WebIdentityRoleOptions.RoleSessionName, which must be <= 64 characters for AWS STS. Since it concatenates ResourceID + "-" + CloudConnectorID, this can exceed the limit and cause runtime STS failures. Consider validating the combined length (and returning a clear error) before calling STS.
| sessionName := ccConfig.ResourceID + "-" + params.CloudConnectorID | |
| sessionName := ccConfig.ResourceID + "-" + params.CloudConnectorID | |
| const maxSessionNameLen = 64 | |
| if len(sessionName) > maxSessionNameLen { | |
| return nil, fmt.Errorf("cloud connectors session name %q exceeds AWS STS limit of %d characters (got %d); check ResourceID and CloudConnectorID", sessionName, maxSessionNameLen, len(sessionName)) | |
| } |
| } | ||
|
|
||
| if params.CloudConnectorID == "" { | ||
| return nil, errors.New("cloud connectors config CloudConnectorID is required") |
There was a problem hiding this comment.
The error message for a missing params.CloudConnectorID says "cloud connectors config CloudConnectorID is required", but CloudConnectorID is not part of ccConfig (it comes from the GCP credentials params). Updating the message to point to the correct config source will make troubleshooting easier.
| return nil, errors.New("cloud connectors config CloudConnectorID is required") | |
| return nil, errors.New("GCP cloud connectors params CloudConnectorID is required") |
| cfg := externalaccount.Config{ | ||
| Audience: audience, | ||
| Audience: params.Audience, | ||
| SubjectTokenType: awsTokenType, | ||
| TokenURL: gcpSTSTokenURL, | ||
| Scopes: []string{gcpCloudPlatformScope}, | ||
| AwsSecurityCredentialsSupplier: credSupplier, | ||
| ServiceAccountImpersonationURL: gcpIAMCredentialsURL + serviceAccountEmail + ":generateAccessToken", | ||
| ServiceAccountImpersonationURL: gcpIAMCredentialsURL + params.ServiceAccountEmail + ":generateAccessToken", | ||
| } |
There was a problem hiding this comment.
params.Audience and params.ServiceAccountEmail are now passed via GCPCloudConnectorsParams but aren't validated. If either is empty, externalaccount.NewTokenSource will fail later with a less actionable error. Consider adding explicit required-field validation for these params alongside the existing ccConfig checks.
| func mockGoogleAuthProviderWithCloudConnectors(err error) *MockGoogleAuthProviderAPI { | ||
| googleProviderAPI := &MockGoogleAuthProviderAPI{} | ||
| on := googleProviderAPI.EXPECT().FindCloudConnectorsCredentials(mock.Anything, mock.Anything, testAudience, testServiceAccountEmail) | ||
| on := googleProviderAPI.EXPECT().FindCloudConnectorsCredentials(mock.Anything, mock.Anything, mock.Anything) | ||
| if err == nil { |
There was a problem hiding this comment.
mockGoogleAuthProviderWithCloudConnectors now matches FindCloudConnectorsCredentials with mock.Anything for the params argument, which stops the test from asserting that Audience, ServiceAccountEmail, and CloudConnectorID are correctly plumbed into the auth provider call. Consider using a matcher (e.g., mock.MatchedBy) to verify the expected fields on GCPCloudConnectorsParams.
1 participant
Adds a deployment-specific Cloud Connector ID to the GCP Cloud Connectors auth flow so that the AWS AssumeRoleWithWebIdentity session name matches the format required by GCP Workload Identity Federation (ResourceID-CloudConnectorID). Introduces a
cloud_connector_idconfig option onGcpClientOpt, aGCPCloudConnectorsParamsstruct to pass audience, service account email, and connector ID into the auth provider, and refactorsFindCloudConnectorsCredentialsto accept this struct and setRoleSessionNameto the concatenated value. Config and credentials call sites, mocks, and tests are updated accordingly.