feat: scaffold Bonbon (AWS Account Access Manager) connector path#121
feat: scaffold Bonbon (AWS Account Access Manager) connector path#121btipling wants to merge 1 commit into
Conversation
Adds an opt-in, plain-Go path for AWS Account Access Manager
(codename: Bonbon) alongside the existing IAM / IdC / Orgs syncs. The
service ships only as REST/JSON over SigV4 and is not in
aws-sdk-go-v2, so the path hand-rolls the request/response shapes
plus an aws/signer/v4 client and reuses the existing AssumeRole +
external-id credential chain — operators already onboarded for
baton-aws do not need new auth wiring.
The two resource types (bonbon_application, bonbon_role) are both
annotated OptInRequired{} because the service itself is still in
private preview; the connector is also gated by
--global-bonbon-enabled and the region is constrained to the
preview regions (us-east-1, us-west-2). Grants on bonbon_role map
to CreateEntitlement / DeleteEntitlement with idempotent semantics
on both ends.
In-memory testserver covers the 11 documented operations against a
seeded application + entitlement graph; TestBonbonFullSync and
TestBonbonGrantRevoke drive the production builders end-to-end. The
plan's reference to connectorbuilder.NewSyncTestRunner is not
available in the currently vendored baton-sdk — left as an open
question for the follow-up move to a standalone baton-bonbon repo.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // applicationsForProvisioning lists applications via a live API call — | ||
| // Grant/Revoke do not receive a SyncOpAttrs and therefore have no session to | ||
| // read from. CreateEntitlement/DeleteEntitlement run rarely enough that the | ||
| // extra pagination is acceptable. | ||
| func (o *roleResourceType) applicationsForProvisioning(ctx context.Context) ([]string, error) { | ||
| resp, err := o.client.ListApplications(ctx, &ListApplicationsRequest{}) | ||
| if err != nil { | ||
| return nil, WrapForRetry(fmt.Errorf("bonbon: ListApplications (provision): %w", err)) | ||
| } | ||
| out := make([]string, 0, len(resp.Applications)) | ||
| for _, a := range resp.Applications { | ||
| out = append(out, a.ApplicationArn) | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
🟠 Bug: applicationsForProvisioning only fetches the first page from ListApplications — if resp.NextToken is non-empty, subsequent applications are silently dropped. Grant/Revoke operations would fail with "no Bonbon application discovered" for any application beyond the first page. Paginate until NextToken is empty, matching the pattern in walkEntitlements.
| func (o *roleResourceType) walkEntitlements(ctx context.Context, applicationArn string) ([]EntitlementSummary, error) { | ||
| out := make([]EntitlementSummary, 0) | ||
| var nextToken string | ||
| for { | ||
| resp, err := o.client.ListEntitlements(ctx, &ListEntitlementsRequest{ | ||
| ApplicationArn: applicationArn, | ||
| Filter: EntitlementFilter{}, | ||
| NextToken: nextToken, | ||
| }) | ||
| if err != nil { | ||
| return nil, WrapForRetry(fmt.Errorf("bonbon: ListEntitlements: %w", err)) | ||
| } | ||
| out = append(out, resp.Entitlements...) | ||
| if resp.NextToken == "" { | ||
| break | ||
| } | ||
| nextToken = resp.NextToken | ||
| } | ||
| return out, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: walkEntitlements buffers all pages of entitlements into memory inside the sync path (called from List and Grants). For large entitlement sets this risks OOM and bypasses SDK-driven pagination checkpointing and context cancellation. The session caching mitigates repeat calls, but the initial walk still loads everything at once. Consider restructuring so the SDK drives pagination one page at a time, or at minimum add a ctx.Err() check inside the loop.
| resp, err := c.httpClient.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("bonbon: http: %w", err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: When httpClient.Do returns both a non-nil response and an error (e.g. redirect policy violations), the response body leaks. Add a nil check before returning:
| resp, err := c.httpClient.Do(req) | |
| if err != nil { | |
| return fmt.Errorf("bonbon: http: %w", err) | |
| } | |
| resp, err := c.httpClient.Do(req) | |
| if err != nil { | |
| if resp != nil && resp.Body != nil { | |
| resp.Body.Close() | |
| } | |
| return fmt.Errorf("bonbon: http: %w", err) | |
| } |
| awsCfg, err := c.getCallingConfig(ctx, c.globalRegion) | ||
| if err != nil { | ||
| l.Error("baton-aws: bonbon disabled — failed to load AWS calling config", zap.Error(err)) | ||
| return nil |
There was a problem hiding this comment.
🟡 Suggestion: When the user explicitly sets --global-bonbon-enabled=true, silently returning nil here makes credential/config errors invisible at sync time — the bonbon resource types simply vanish from the output. Consider returning an error so the connector fails visibly rather than producing a partial sync that omits the opted-in resource types.
Connector PR Review: feat: scaffold Bonbon (AWS Account Access Manager) connector pathBlocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0 Review SummaryNew opt-in Bonbon (AWS Account Access Manager) connector path adding two resource types ( Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agents |
1 participant
Summary
baton-aws. Two resource types —bonbon_application(TRAIT_APP) andbonbon_role(TRAIT_ROLE) — both annotatedOptInRequired{}because the AWS service itself is still in private preview.pkg/connector/bonbon/client.go) against the 11 documentedaccount-accessoperations because the service is not inaws-sdk-go-v2. Reuses the existing baton-aws AssumeRole + external-id credential chain — operators already onboarded forbaton-awsdo not need new auth wiring.--global-bonbon-enabled(defaultfalse). Region is restricted at parse time tous-east-1/us-west-2; outside-set values are rejected with an explicit "private preview region" error. Grants onbonbon_rolemap toCreateEntitlement/DeleteEntitlementwith idempotent semantics on both ends.test/bonbon-testserver/) serves the 11 routes against a seeded application + entitlement graph.TestBonbonFullSyncandTestBonbonGrantRevokedrive the production builders end-to-end through SigV4 → HTTP → JSON.Architecture note
The plan favored a standalone
ConductorOne/baton-bonbonrepo. The current scaffold lives underbaton-aws/pkg/connector/bonbon/because the gh App installation cannot create repos under theConductorOneorg — re-confirmed at scaffold time. The directory is self-contained and copy-movable to a new repo with a single import-path rename when a maintainer creates the repo.Test Plan
gofmt -l ./...clean across new and touched filesgo vet ./...cleango test ./...—TestBonbonFullSync,TestBonbonGrantRevoke,TestValidateRegion, plus added region-validation cases inTestConfigsgolangci-lint runcleandocs/bonbon.md. Connector remains DRAFT until this is done.Risks
account-access/ region) is correct.AlreadyCreatedExceptionHTTP status is not pinned by the service-2.json. The error parser reads__type/X-Amzn-ErrorTyperather than relying on status, but the actual status code wants real-account confirmation before GA.connectorbuilder.NewSyncTestRunnerreferenced in the plan is not in the currently vendored baton-sdk — direct builder-method tests are used instead, matching the existingsso_group_test.gopattern inbaton-aws. Open question for the eventual move to a standalone repo.Open questions
baton-bonbonrepo. Blocked at scaffold time by gh App permissions; the directory is self-contained for migration once a maintainer creates the repo.AlreadyCreatedExceptionHTTP code. Service-2.json declares the shape but does not pin a status. Parser reads__type/ header; real-account verification wanted before GA.🛰️ Built with stargate.