refactor: add GCP parent resolver abstraction for config provider#3974
refactor: add GCP parent resolver abstraction for config provider#3974amirbenun wants to merge 4 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 refactors the GCP authentication configuration provider by introducing a pluggable parent resolution abstraction. The refactoring extracts the logic for determining GCP parent resources (projects or organizations) from configuration, Workload Identity Federation audience URLs, or Application Default Credentials into dedicated resolver components.
Changes:
- Introduces
ParentResolver,ProjectParentResolver, andOrganizationParentResolverinterfaces with default implementations - Adds
projectNumberFromAudiencehelper function to extract project numbers from WIF audience URLs - Updates
ConfigProviderto useParentResolverand providesNewConfigProvider()factory for production use - Updates benchmark and asset inventory strategies to use the new factory function
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/resources/providers/gcplib/auth/parent_resolver.go | Defines ParentResolver interface and defaultParentResolver facade that delegates to project/org resolvers |
| internal/resources/providers/gcplib/auth/project_parent_resolver.go | Implements ProjectParentResolver with fallback logic: config → audience → ADC |
| internal/resources/providers/gcplib/auth/organization_parent_resolver.go | Implements OrganizationParentResolver using Resource Manager API to resolve org from ancestry |
| internal/resources/providers/gcplib/auth/audience.go | Helper function to extract project number from WIF audience URLs |
| internal/resources/providers/gcplib/auth/audience_test.go | Comprehensive tests for audience parsing logic |
| internal/resources/providers/gcplib/auth/parent_resolver_test.go | Unit tests for defaultParentResolver delegation logic |
| internal/resources/providers/gcplib/auth/parent_resolver_mock.go | Mock implementations for ProjectParentResolver and OrganizationParentResolver interfaces |
| internal/resources/providers/gcplib/auth/credentials_mock.go | Added mocks for DefaultCredentialsFinder and ParentResolver interfaces |
| internal/resources/providers/gcplib/auth/credentials.go | Refactored to use ParentResolver; adds NewConfigProvider() factory; removes old parent resolution functions |
| internal/resources/providers/gcplib/auth/credentials_test.go | Updated to initialize ParentResolver in test fixtures |
| internal/flavors/benchmark/strategy.go | Updated to use NewConfigProvider() factory |
| internal/flavors/assetinventory/strategy.go | Updated to use NewConfigProvider() factory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func resolveProjectIdFromAudience(ctx context.Context, clientOpts []option.ClientOption, audience string) (string, error) { | ||
| projectNumber, ok := projectNumberFromAudience(audience) | ||
| if !ok { | ||
| return "", fmt.Errorf("audience does not contain a valid project number (expected format: //iam.googleapis.com/projects/PROJECT_NUMBER/...): %w", ErrProjectNotFound) | ||
| } | ||
| opts := append([]option.ClientOption{option.WithScopes(cloudresourcemanager.CloudPlatformReadOnlyScope)}, clientOpts...) | ||
| svc, err := cloudresourcemanager.NewService(ctx, opts...) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create Resource Manager client: %w", err) | ||
| } | ||
| project, err := svc.Projects.Get("projects/" + projectNumber).Context(ctx).Do() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get project (number %s): %w", projectNumber, err) | ||
| } | ||
| if project.ProjectId == "" { | ||
| return "", fmt.Errorf("project response missing project_id: %w", ErrProjectNotFound) | ||
| } | ||
| return project.ProjectId, nil | ||
| } |
There was a problem hiding this comment.
The new functions resolveProjectIdFromAudience (line 65) and resolveOrganizationIdFromAudience in organization_parent_resolver.go (line 59) make external API calls to GCP Cloud Resource Manager but lack direct unit test coverage. While the audience.go helper function projectNumberFromAudience has comprehensive tests, the actual API integration logic for resolving project IDs and organization IDs from the audience is not tested.
Consider adding unit tests that mock the Cloud Resource Manager API responses to verify:
- Successful project ID resolution from project number
- Successful organization ID resolution via GetAncestry
- Error handling when the API calls fail
- Edge cases like missing project_id in API response or no organization in ancestry
The existing test coverage in parent_resolver_test.go only tests the delegation logic, not the actual implementation of these resolver functions.
| func resolveOrganizationIdFromAudience(ctx context.Context, clientOpts []option.ClientOption, audience string) (string, error) { | ||
| opts := append([]option.ClientOption{option.WithScopes(cloudresourcemanager.CloudPlatformReadOnlyScope)}, clientOpts...) | ||
| svcV3, err := cloudresourcemanager.NewService(ctx, opts...) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create Resource Manager client: %w", err) | ||
| } | ||
| projectNumber, ok := projectNumberFromAudience(audience) | ||
| if !ok { | ||
| return "", fmt.Errorf("audience does not contain a valid project number: %w", ErrMissingOrgId) | ||
| } | ||
| project, err := svcV3.Projects.Get("projects/" + projectNumber).Context(ctx).Do() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get project (number %s): %w", projectNumber, err) | ||
| } | ||
| optsV1 := append([]option.ClientOption{option.WithScopes(crmv1.CloudPlatformReadOnlyScope)}, clientOpts...) | ||
| svcV1, err := crmv1.NewService(ctx, optsV1...) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create Resource Manager v1 client: %w", err) | ||
| } | ||
| resp, err := svcV1.Projects.GetAncestry(project.ProjectId, &crmv1.GetAncestryRequest{}).Context(ctx).Do() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get project ancestry: %w", err) | ||
| } | ||
| for _, anc := range resp.Ancestor { | ||
| if anc.ResourceId != nil && anc.ResourceId.Type == resourceIdTypeOrganization && anc.ResourceId.Id != "" { | ||
| return anc.ResourceId.Id, nil | ||
| } | ||
| } | ||
| return "", fmt.Errorf("no organization found in resource hierarchy: %w", ErrMissingOrgId) | ||
| } |
There was a problem hiding this comment.
The function resolveOrganizationIdFromAudience makes external API calls to GCP Cloud Resource Manager v3 and v1 but lacks direct unit test coverage. While the facade pattern is tested in parent_resolver_test.go, the actual implementation logic including the two-step API call sequence (Projects.Get followed by Projects.GetAncestry) is not tested.
Consider adding unit tests that mock the Cloud Resource Manager API responses to verify:
- Successful organization ID resolution through ancestry lookup
- Error handling when Projects.Get fails
- Error handling when Projects.GetAncestry fails
- Edge case when no organization is found in the resource hierarchy
- Validation of the resource ID type filtering logic (line 83)
|
|
||
| // resolveOrganizationIdFromAudience uses the project number in the audience to get | ||
| // the project via v3, then calls v1 getAncestry to resolve the organization in one call. | ||
| // clientOpts must authenticate with resourcemanager.projects.get. |
There was a problem hiding this comment.
The comment states that clientOpts must authenticate with "resourcemanager.projects.get" permission, but the function also calls Projects.GetAncestry (line 78) which requires the "resourcemanager.projects.getAncestry" permission.
Update the comment to clarify that both permissions are required:
"clientOpts must authenticate as an identity with resourcemanager.projects.get and resourcemanager.projects.getAncestry permissions."
| // clientOpts must authenticate with resourcemanager.projects.get. | |
| // clientOpts must authenticate as an identity with resourcemanager.projects.get and resourcemanager.projects.getAncestry permissions. |
1 participant
Adds a pluggable parent resolution layer to the GCP auth config provider so that the GCP parent (e.g. projects/pid or organizations/oid) is determined by a dedicated ParentResolver interface. This enables clearer separation of concerns and testability when resolving project ID from config, application default credentials, or organization ID. The implementation introduces ProjectParentResolver and OrganizationParentResolver, a DefaultParentResolver that composes them, a helper to extract project number from Workload Identity Federation audience URLs, and updates the asset inventory and benchmark strategies to use the new NewConfigProvider() factory.