Skip to content

Add IsScopeToResource#130

Merged
MarcusGoldschmidt merged 3 commits into
mainfrom
goldschmidt/exclusion-group-2
May 28, 2026
Merged

Add IsScopeToResource#130
MarcusGoldschmidt merged 3 commits into
mainfrom
goldschmidt/exclusion-group-2

Conversation

@MarcusGoldschmidt

Copy link
Copy Markdown
Contributor

Description

  • Bug fix
  • New feature

Useful links:

@MarcusGoldschmidt MarcusGoldschmidt requested a review from a team May 27, 2026 17:14
@MarcusGoldschmidt MarcusGoldschmidt changed the title Goldschmidt/exclusion group 2 Add IsScopeToResource May 27, 2026
if cfg.IsScopeToResource != "" {
isScopeToResource, err = s.env.EvaluateBool(ctx, cfg.IsScopeToResource, inputs)
if err != nil {
return nil, fmt.Errorf("exclusion_group.is_scoped_to_resource evaluation failed: %w", err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: The error message says is_scoped_to_resource (with a "d") but the config YAML field is is_scope_to_resource. This mismatch could confuse users debugging config issues.

Suggested change
return nil, fmt.Errorf("exclusion_group.is_scoped_to_resource evaluation failed: %w", err)
return nil, fmt.Errorf("exclusion_group.is_scope_to_resource evaluation failed: %w", err)

if err := s.recordEntitlementExclusionGroup(exclusionGroup, entID, resource.GetId().GetResourceType()); err != nil {
return err
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: The annos slice is declared outside the inner for _, resource loop and shared across all entitlements created from the same static entitlement template. Each iteration calls annos.Update(exclusionGroup) which mutates the shared underlying array. Since all entitlements reference the same slice, they would all end up with the last resource's scoped exclusion group ID after PutEntitlements marshals them. This is an upstream SDK issue — if scope_to_resource is used with multiple resources, the per-resource scoping may not work correctly.

@github-actions

Copy link
Copy Markdown
Contributor

Connector PR Review: Add IsScopeToResource

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Review mode: full
View review run

Review Summary

This PR adds a new is_scope_to_resource config field to ExclusionGroupMapping and bumps the vendored baton-sdk from v0.9.15 to v0.9.20. The connector-side implementation in exclusion_group.go is clean and follows existing patterns (matching IsDefault), with a test covering the new field. The SDK upgrade includes exclusion group validation in sync state, migration return-value tracking, span reduction, panic recovery, and HTTP/2 error handling improvements. No blocking issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • pkg/bsql/exclusion_group.go:63 — Error message says is_scoped_to_resource (with "d") but the config field is is_scope_to_resource.
  • vendor/.../pkg/sync/syncer.go:~1265-1278 — (Upstream SDK) The annos slice is shared across all entitlements in the inner resource loop; annos.Update(exclusionGroup) mutates it in place, so all entitlements in a batch may end up with the last resource's scoped exclusion group ID when scope_to_resource is true.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/bsql/exclusion_group.go`:
- Around line 63: The error message string reads "exclusion_group.is_scoped_to_resource evaluation failed" but the YAML config field name is `is_scope_to_resource` (no "d"). Change "is_scoped_to_resource" to "is_scope_to_resource" in the fmt.Errorf call.

In `vendor/github.com/conductorone/baton-sdk/pkg/sync/syncer.go` (upstream SDK fix needed):
- Around lines 1265-1278: In `syncStaticEntitlementsForResourceType`, the `annos` variable is a shared slice declared before the inner `for _, resource` loop. When `scope_to_resource` is true, `annos.Update(exclusionGroup)` mutates the shared underlying array each iteration. All entitlements created in that batch reference the same slice, so they all see the last resource's scoped exclusion group ID. The fix should copy the annotations slice per resource iteration when `scope_to_resource` is true (e.g., `entAnnos := append(annotations.Annotations{}, annos...)` before updating).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@MarcusGoldschmidt MarcusGoldschmidt merged commit 33d592c into main May 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants