fix(provx/gcp): make the connection provisionable, and safe to share - #3
Merged
Conversation
Five things stood between this package and a working connection. The IAM binding named roles/project.Owner, which is not a role id GCP has: the basic roles are roles/owner, roles/editor and roles/viewer, so the binding could never have been granted. It becomes roles/editor plus roles/resourcemanager.projectIamAdmin, matching what formae-bootstrap grants a self-hosted agent for the same job. That pair is near-owner and the comment says so rather than implying editor is a meaningful reduction. The pool and provider ids are fixed per project, but the attribute condition pinned one subject, and EnsureOIDCProvider patches on drift. Connecting a second installation to a project therefore rewrote the condition and silently revoked the first, against a case the AWS path supports deliberately. The condition now admits the issuer's subject namespace and the per-installation distinction lives in the IAM binding, so a second connect is additive. Sharing those objects means a name collision is no longer harmless, so Create refuses a provider that trusts a different issuer instead of adopting and rewriting it, and Delete removes only this installation's binding members, leaving the shared pool and provider standing. Create returns the provider resource name, which the caller needs as both the registration coordinate and the token audience, and pins allowedAudiences to it rather than relying on GCP's implicit default. Verified against the live API: a provider created without the field comes back with it absent, so the default is never materialized into the object, and pinning it is a real narrowing from the implicit default. Failures are classified on Google's structured error reason rather than the HTTP status, because 403 is equally a disabled API, a missing permission, a VPC Service Controls perimeter and an org-policy denial, and telling an operator to enable an API that is already on is the most confusing way available to waste their time. New takes a context and client options so the provisioner is testable without a network or a credential; the tests drive a fake that records the actual IAM policy requests, since a fake that merged members itself would hide a destructive whole-policy replace.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
provx/gcpcould not have provisioned a working connection, and once its objects are shared it could have broken one.The IAM binding named a role that does not exist.
roles/project.Owneris not a GCP role id — the basic roles areroles/owner,roles/editorandroles/viewer— so the binding could never be granted. It becomesroles/editorplusroles/resourcemanager.projectIamAdmin, which is whatformae-bootstrapgrants a self-hosted agent for the same job. The comment says plainly that the pair is near-owner rather than implying editor is a meaningful reduction in blast radius.A second installation silently revoked the first. The pool and provider ids are fixed per project while the attribute condition pinned a single subject, and
EnsureOIDCProviderpatches on drift. Connecting one project to two installations — which the AWS path supports deliberately, warning and continuing — rewrote the condition to the newcomer. The condition now admits the issuer's subject namespace, and the per-installation distinction moves to the IAM binding, so a second connect is additive: same provider spec, one more member.Sharing those objects makes a name collision consequential, so two guards come with it:
Createrefuses a provider trusting a different issuer instead of adopting and rewriting it, andDeleteremoves only this installation's binding members and leaves the pool and provider standing.Createnow returns the provider resource name, which the caller needs as both the registration coordinate and the token audience, and pinsallowedAudiencesto it.Failures are classified on Google's structured error reason, not the HTTP status. A 403 is equally a disabled API, a missing permission, a VPC Service Controls perimeter and an org-policy denial. Telling someone to enable an API that is already enabled is the most confusing way available to waste their time.
Verified against the live API
A provider created without
allowedAudiencescomes back with the field absent — the server does not materialize its default into the resource. A comment inprovider.goasserted the opposite, and is corrected. The consequence worth knowing: pinning the field on a provider that lacked it is a real narrowing, from the implicit default (which also accepts thehttps://spelling of the resource name) to exactly the listed audiences. For providers this package owns that is the intent.Tests
The fake records the actual
GetIamPolicy/SetIamPolicyrequests rather than answering from merged state, because a fake that merged members itself would hide a destructive whole-policy replace. Mutating the implementation back to the single-subject condition, toroles/project.Owner, to no ownership check, or to aDeletethat removes the shared provider each turns a test red.Newtakes a context and client options, so the provisioner is testable without a network or a credential.