CXP-768 Gate account re-parenting on org/OU sync opt-in - #139
Conversation
Connector PR Review: CXP-768 Gate account re-parenting on org/OU sync opt-inBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. The change threads Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
|
Adjacent edge not addressed: the OU→Root parent edge (organizationalUnitResource → WithParentResourceID) has the same dangling shape if organizational_unit is synced while organization (Root) isn't. In practice the OU crawl is seeded from the Root, so "OU without org" is likely unreachable — but if that invariant changes, the same MISSING RESOURCE could reappear one tier up. Note on the ticket. Could you confirm with |
|
Hey @Bencheng21 ! About your question:
I can confirm. "OU without org" is unreachable. As you mentioned, this is covered by design. If the Root Org is not synced, no OU will be on the result set. |
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
Approving — the fix is correct and well-tested. Two follow-ups worth addressing before merge, neither changing the approach.
Resolving the [Major] on connector.go:257 — @sergiocorral-conductorone's reading of the SDK is accurate: in baton-sdk@v0.18.4, WillSyncResourceType (pkg/cli/cli.go:89) returns true whenever the filter is empty and never consults OptInRequired. Grepping the whole SDK, the only non-test references to OptInRequired are that doc comment and connectorbuilder.go:426, where it's just reported in capabilities metadata — the syncer never enforces it.
That said, the concern doesn't apply: C1 always sends an explicit SyncResourceTypeIDs list when it triggers a sync, so SyncFilterIsExplicit() is true and the gate does a real set lookup on a production path.
And the true default is positively correct, not merely harmless. In an unfiltered run (local CLI), the syncer syncs everything the connector advertises — org and OU included — so the resolved parent does get emitted and is not dangling. Defaulting to "will sync" is precisely right there. Gating on SyncFilterIsExplicit() && WillSyncResourceType(id) would make that case worse: it would suppress re-parenting on a run that actually does sync the hierarchy. Current code is right as-is.
Please address — [Minor], account.go:209 (raised by @sergiocorral-conductorone, seconded by @FeliLucero1). Both willSyncParentType branches already require o.willSyncOrganization, so the outer || guard is wider than the inner condition can ever use: in (org=false, OU=true) it fires a rate-limited ListParents per account that can never produce an attachment. Collapsing the outer guard to if o.willSyncOrganization removes the wasted call and makes the org→OU dependency structural rather than comment-only. @FeliLucero1's block above is the change verbatim.
While in there: the new tests cover (false,false) and (true,false), but (false,true) is the one combination not covered — and it's the case this guard change is about. Worth adding, asserting listParentsCalls == 0.
Non-blocking — [Nit], account.go:982 (@FeliLucero1). Agreed and confirmed: accountBuilder is 8 positional params with two adjacent untyped bools, and neither newOrgAccountWithSyncFilter nor the behavior-test helper exercises the real AWS{} → ResourceSyncers() wiring, so a transposition at that one live call site would be a silent behavior change rather than a compile error. Fine to defer, but if the tests are being touched for the (false,true) case anyway, a small named struct for the two flags is cheap insurance.
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Approving — the refactor holds up. Verified the guard simplification is behavior-equivalent for the 3 (Organization, OrganizationalUnit) combos where it matters (attach outcome unchanged), and traced through the OU-crawl seeding logic + the SDK's WillSyncResourceType to confirm the org→OU dependency claim in the new comment is accurate. Left 2 small non-blocking notes inline — one's a genuinely new observation building on the test-coverage gap already open on this thread, the other's just a stale comment. Neither should hold up merge.
accountResourceType.List() re-parented every account onto its Organizations Root/OU whenever organizations:ListParents succeeded, regardless of whether the organization/organizational_unit resource types (OptInRequired, off by default) were actually being synced. In the default orgs+sso configuration this left every account with a parent pointing at a resource that was never synced, and c1 silently drops the reference as a dangling MISSING RESOURCE. Thread cli.ConnectorOpts through New() (previously discarded) to learn whether this sync run will actually emit organization/ organizational_unit, and gate both the ListParents call and the resulting parent attachment on it -- including the resolved parent's specific type, so partial opt-in can't produce a dangling parent either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… parentIDs to accounts
7605814 to
6dba262
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
7 participants
accountResourceType.List() re-parented every account onto its Organizations Root/OU whenever organizations:ListParents succeeded, regardless of whether the organization/organizational_unit resource types (OptInRequired, off by default) were actually being synced. In the default orgs+sso configuration this left every account with a parent pointing at a resource that was never synced, and c1 silently drops the reference as a dangling MISSING RESOURCE.
Thread cli.ConnectorOpts through New() (previously discarded) to learn whether this sync run will actually emit organization/ organizational_unit, and gate both the ListParents call and the resulting parent attachment on it -- including the resolved parent's specific type, so partial opt-in can't produce a dangling parent either.