feat: initial Bonbon (Account Access) connector scaffold#120
Conversation
Adds the private-preview Bonbon connector under
pkg/connector/bonbon/ alongside the existing baton-aws resource
builders, gated by --global-bonbon-enabled.
The Bonbon service (account-access-preview.amazonaws.com) is not
yet in aws-sdk-go-v2, so this introduces a hand-rolled REST/SigV4
client under pkg/connector/bonbon/client/. The 11 operations from
the AWS service-2.json are exposed as typed Go methods. Signing
uses github.com/aws/aws-sdk-go-v2/aws/signer/v4 directly, sourced
from the existing baton-aws AWS credential chain so AssumeRole +
external-id flows are inherited as-is.
Resource model:
- bonbon_application — one per Bonbon Application (IdC binding)
- bonbon_role — one per target IAM Role ARN seen across
entitlements
- Grants on bonbon_role: assigned-to IdC user (sso_user) or IdC
group (sso_group). Grant = CreateEntitlement,
Revoke = DeleteEntitlement.
All three Bonbon resource types are annotated with OptInRequired{}
since Bonbon is private preview. CapabilityPermissions list the
account-access:* actions the connector calls.
Test scaffold:
- test/bonbon-testserver/ — in-memory mock for the 11 routes
- pkg/connector/bonbon/connector_test.go covers ValidateRegion;
TestBonbonFullSync is t.Skip()'d for the follow-up wiring PR
docs/bonbon.md covers the customer-side onboarding (Org-wide vs
standalone enablement, the required trust policy with sts:AssumeRole
+ sts:SetContext, IAM permissions, and the private-preview
limitations carried over from the AWS onboarding guide).
This is a scaffold PR — the sync wiring is in place and compiles,
but TestBonbonFullSync needs the testserver auth path finalized
before assertions can land. Refs CXH-1558.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for _, appArn := range b.applicationArns { | ||
| token := "" | ||
| for { | ||
| in := &client.ListEntitlementsInput{ | ||
| ApplicationArn: appArn, | ||
| Filter: client.EntitlementFilter{}, | ||
| NextToken: token, | ||
| } | ||
| out, err := b.client.ListEntitlements(ctx, in) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("baton-aws/bonbon: ListEntitlements(%s): %w", appArn, err) | ||
| } | ||
| for _, member := range out.Entitlements { | ||
| if member.Entitlement.PrincipalRole == nil { | ||
| continue | ||
| } | ||
| roleArn := member.Entitlement.PrincipalRole.RoleArn | ||
| account := member.Entitlement.PrincipalRole.Account | ||
| seenRoles[roleArn] = account | ||
| } | ||
| if out.NextToken == "" { | ||
| break | ||
| } | ||
| token = out.NextToken | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Bug: F2 violation — List() fetches all entitlement pages internally. This nested loop iterates through every application and every entitlement page to build the seenRoles map, bypassing SDK-driven pagination entirely. The SyncOpAttrs page token parameter is ignored. This means no checkpointing (crash = restart from scratch), no SDK rate-limit feedback, and unbounded memory for accounts with many entitlements.
The dedup requirement (discovering unique role ARNs) makes this harder to paginate than a simple list, but the current approach can OOM on large deployments. Consider using the SDK pagination bag to track progress across applications and pages, yielding discovered roles incrementally (the SDK deduplicates resources by ID).
| for _, appArn := range b.applicationArns { | ||
| token := "" | ||
| for { | ||
| in := &client.ListEntitlementsInput{ | ||
| ApplicationArn: appArn, | ||
| Filter: client.EntitlementFilter{ | ||
| PrincipalRole: &client.PrincipalRoleEntitlementFilter{RoleArn: roleArn}, | ||
| }, | ||
| NextToken: token, | ||
| } | ||
| page, err := b.client.ListEntitlements(ctx, in) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("baton-aws/bonbon: ListEntitlements(%s, %s): %w", appArn, roleArn, err) | ||
| } | ||
| for _, member := range page.Entitlements { | ||
| if member.Entitlement.PrincipalRole == nil { | ||
| continue | ||
| } | ||
| if member.Entitlement.PrincipalRole.RoleArn != roleArn { | ||
| continue | ||
| } | ||
| grant, err := principalGrant(resource, &member) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if grant != nil { | ||
| out = append(out, grant) | ||
| } | ||
| } | ||
| if page.NextToken == "" { | ||
| break | ||
| } | ||
| token = page.NextToken | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Bug: F2 violation — Grants() fetches all entitlement pages internally. Same issue as List(): the inner for loop fetches every page of entitlements across all applications in a single Grants() call. The SyncOpAttrs page token is ignored and SyncOpResults is returned as nil, so the SDK treats this as one giant page.
Unlike List(), this is straightforward to fix with SDK pagination bags — use the bag to track which (appArn, pageToken) pair you're on, yield one page of grants per call, and return NextPageToken to let the SDK drive the loop.
| if _, err := b.client.CreateEntitlement(ctx, in); err != nil { | ||
| if client.IsCode(err, client.ErrAlreadyCreated) { | ||
| return nil, nil |
There was a problem hiding this comment.
🟡 Suggestion: The AlreadyCreated case is handled correctly (returns nil error), but the standard SDK convention is to return a GrantAlreadyExists annotation so ConductorOne can distinguish "grant was already in place" from "grant was just created." Same applies to the Revoke method below — ResourceNotFound should return a GrantAlreadyRevoked annotation.
| if err != nil { | ||
| l.Error("baton-aws/bonbon: failed to resolve calling config", zap.Error(err)) | ||
| return rs | ||
| } | ||
| builders, err := bonbon.NewBuilders(ctx, bonbonCfg, bonbon.Options{ | ||
| Region: c.bonbonRegion, | ||
| ApplicationArn: c.bonbonApplicationArn, | ||
| BaseURL: c.bonbonBaseURL, | ||
| HTTPClient: c.baseClient, | ||
| }) | ||
| if err != nil { | ||
| l.Error("baton-aws/bonbon: NewBuilders failed", zap.Error(err)) | ||
| return rs | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Both error paths log and return rs without the Bonbon builders. If Bonbon was previously syncing successfully, the absence of its builders this time could cause the SDK to interpret existing Bonbon resources as deleted (F1/F3 concern). Since DefaultCapabilityBuilders() always declares Bonbon resource types, the safest approach may be to return the nil-client capability builders here (matching what DefaultCapabilityBuilders does) so the resource types stay registered even when the client can't be created, and let the builders themselves error at sync time.
Connector PR Review: feat: initial Bonbon (Account Access) connector scaffoldBlocking Issues: 2 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThis PR scaffolds a new Bonbon (AWS Account Access) connector with a hand-rolled SigV4 client, three resource types, provisioning (Grant/Revoke), a test server, and config wiring. The client, types, error handling, application builder pagination, and resource type definitions are well-structured. However, the role builder's Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agents |
1 participant
Summary
Scaffolds the AWS Account Access (codename Bonbon) connector as an extension to
baton-aws. Gated behind--global-bonbon-enabled; existing baton-aws syncs are unchanged when the flag is off.pkg/connector/bonbon/client/(Bonbon is not in aws-sdk-go-v2 yet). All 11 operations from the AWS service-2.json are exposed as typed Go methods. Signing usesgithub.com/aws/aws-sdk-go-v2/aws/signer/v4directly, sourced from the existingbaton-awsAWS credential chain.bonbon_application,bonbon_role, and grants onbonbon_rolefor IdC user / IdC group principals. All annotated withv2.OptInRequired{}since Bonbon is private preview, plusv2.CapabilityPermissionsdeclaring theaccount-access:*actions per resource type.test/bonbon-testserver/— in-memory mock of the 11 routes for unit tests without a real Bonbon-enabled account.docs/bonbon.md— customer onboarding runbook covering Org-wide vs standalone enablement, the required trust policy withsts:AssumeRole+sts:SetContext, IAM permissions, and the private-preview limitations from the AWS onboarding guide.Background and decisions (path-selection rationale, AWS SDK availability, region scoping, opt-in semantics) live in the design plan at arena-fs
/engineer/baton-bonbon-plan.md.Refs: CXH-1558
Status
DRAFT. Scaffold compiles and
go test ./pkg/connector/bonbon/...passes.TestBonbonFullSyncist.Skip()'d — the testserver loopback auth path needs finalization before the full sync round-trip can be asserted in unit tests.Test plan
go build ./...cleango test ./pkg/connector/bonbon/...passes (TestValidateRegion)TestBonbonFullSyncagainst the testserver — follow-up PRus-east-1:Validate()returns success with the trust policy attachedCreateEntitlement+ assume against an IdC userDeleteEntitlementremoves the binding🤖 Generated with Claude Code