Skip to content

Add per-resource SkipEntitlementsAndGrants for Sparse ACLs#119

Open
c1-dev-bot[bot] wants to merge 2 commits into
mainfrom
epd-2125/sparse-acls
Open

Add per-resource SkipEntitlementsAndGrants for Sparse ACLs#119
c1-dev-bot[bot] wants to merge 2 commits into
mainfrom
epd-2125/sparse-acls

Conversation

@c1-dev-bot
Copy link
Copy Markdown
Contributor

@c1-dev-bot c1-dev-bot Bot commented Apr 21, 2026

Summary

  • Adds a skip_entitlements_and_grants CEL expression field to ResourceMapping config, enabling per-resource Sparse ACL support
  • When the expression evaluates to true for a given resource row, the SkipEntitlementsAndGrants annotation is applied to that individual resource, causing the SDK syncer to skip entitlement and grant processing
  • Previously this was only supported at the resource-type level (all-or-nothing); now individual resources can be selectively excluded based on row data

Example Usage

resource_types:
  database:
    name: "Database"
    list:
      query: "SELECT id, name, is_public FROM databases LIMIT ?<Limit> OFFSET ?<Offset>"
      map:
        id: ".id"
        display_name: ".name"
        skip_entitlements_and_grants: ".is_public == true"
        traits:
          app:
            profile:
              name: ".name"
      pagination:
        strategy: "offset"
        primary_key: "id"

In the example above, databases where is_public is true will skip entitlement and grant processing, while private databases will have their ACLs synced normally.

Test plan

  • Verify config parsing test (TestParse_SparseACLs) passes - validates the new field is correctly parsed from YAML
  • Verify resource type annotation test (TestConfig_GetResourceTypes_SkipEntitlementsAndGrants) passes - validates resource-type-level skip still works
  • Manual testing with a YAML config that uses the new skip_entitlements_and_grants CEL expression at the resource mapping level

Fixes: CXH-1391


Automated PR Notice

This PR was automatically created by c1-dev-bot as a potential implementation.

This code requires:

  • Human review of the implementation approach
  • Manual testing to verify correctness
  • Approval from the appropriate team before merging

Add a `skip_entitlements_and_grants` CEL expression field to
ResourceMapping config. When the expression evaluates to true for a
given resource row, the resource is annotated with
SkipEntitlementsAndGrants, causing the SDK syncer to skip entitlement
and grant processing for that individual resource.

This enables Sparse ACLs: previously, skip was only supported at the
resource-type level (all-or-nothing). Now it can be controlled
per-resource based on row data, so only resources that actually have
ACLs get entitlement/grant processing.

Fixes: CXH-1391
@c1-dev-bot c1-dev-bot Bot requested a review from a team April 21, 2026 20:55
@linear
Copy link
Copy Markdown

linear Bot commented Apr 21, 2026

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

Claude finished @c1-dev-bot[bot]'s task in 1m 20s —— View job


PR Review: Add per-resource SkipEntitlementsAndGrants for Sparse ACLs

  • Gather context and read changed files
  • Review implementation for correctness, security, and best practices
  • Post detailed review feedback

Overview

This PR adds a skip_entitlements_and_grants CEL expression field to ResourceMapping, enabling per-resource Sparse ACL support. The implementation is clean and minimal — 4 files, 127 additions, no deletions.

Verdict: Looks good ✓

The implementation is straightforward, correct, and follows existing patterns well. A few observations below, but nothing blocking.


What works well

  1. Follows existing patterns. The per-resource CEL expression in resources.go:464-475 mirrors how other CEL-evaluated fields (like Description, SkipIf) work in this codebase — evaluate via s.env.EvaluateBool(), then conditionally apply an annotation. Consistent and easy to understand.

  2. Two levels play well together. The resource-type-level SkipEntitlementsAndGrants (boolean on ResourceType) and the new per-resource-level one (CEL string on ResourceMapping) don't conflict. The SDK syncer checks both ResourceType annotations and individual Resource annotations independently (syncer.go:950-973), so either mechanism works and they compose correctly.

  3. Tests cover both dimensions. TestParse_SparseACLs validates config parsing (both resource-type-level bool and resource-mapping-level CEL string), and TestConfig_GetResourceTypes_SkipEntitlementsAndGrants validates that the resource-type-level annotation is applied correctly.


Observations

1. No integration test for CEL evaluation at the resource level

The tests validate that the config field parses correctly and that resource-type annotations are applied, but there's no test that exercises the actual CEL evaluation path in getMappedResource() — i.e., creating a SQLSyncer, calling getMappedResource() with a row map where is_public is true vs false, and verifying the annotation is present/absent on the resulting resource.

The PR description acknowledges this ("Manual testing with a YAML config..."). This is a reasonable approach given that the CEL evaluation machinery is already well-tested elsewhere, but it's worth noting as a gap.

2. Error handling on CEL evaluation failure returns a bare error

At resources.go:465-467:

skip, err := s.env.EvaluateBool(ctx, mapping.SkipEntitlementsAndGrants, inputs)
if err != nil {
    return err
}

This is consistent with how other CEL evaluations in getMappedResource handle errors (e.g., EvaluateString for Id, DisplayName, Description), so it's fine as-is. A CEL expression error here is a config error and should fail the sync, which is the right behavior.

3. No config validation for the CEL expression

There's no staticValidate step that checks whether the CEL expression compiles before sync begins. Again, this matches how other CEL expressions in the config work (e.g., SkipIf on EntitlementMapping and GrantMapping aren't pre-validated either). If the expression has a syntax error, it'll fail at evaluation time with a clear error from the CEL library.

Worth considering for a follow-up: a staticValidate that pre-compiles all CEL expressions at startup would give earlier, clearer feedback to config authors.


Summary

The implementation is clean, minimal, follows established patterns, and the two levels of skip (resource-type vs per-resource) compose correctly via the SDK. The test coverage is adequate for parsing/config, though an integration test for the CEL evaluation path would strengthen confidence. No blocking issues.

@c1-dev-bot
Copy link
Copy Markdown
Contributor Author

c1-dev-bot Bot commented Apr 21, 2026

Checking lint failure details...

@c1-dev-bot
Copy link
Copy Markdown
Contributor Author

c1-dev-bot Bot commented Apr 21, 2026

Investigating verify / lint CI failure. Could not reproduce locally (Go version mismatch: need 1.25.2, have 1.24.4). Analyzing the diff against lint rules to identify the issue.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

Claude finished @c1-dev-bot[bot]'s task in 1m 44s —— View job


  • Gather context and read changed files
  • Review implementation for correctness, security, and best practices
  • Post detailed review feedback

PR Review: Add per-resource SkipEntitlementsAndGrants for Sparse ACLs

Verdict: Approve — Clean, minimal implementation that follows established patterns. One testing gap worth noting, no blocking issues.


What works well

  1. Follows existing patterns exactly. The CEL evaluation in resources.go:464-475 mirrors how SkipIf works in grants.go:124-131 and entitlements.go:115-122 — evaluate via EvaluateBool(), conditionally apply annotation. Consistent and easy to understand.

  2. Two levels compose correctly. The resource-type-level SkipEntitlementsAndGrants (boolean on ResourceType, applied in resource_types.go:71-75) and the new per-resource-level one (CEL string on ResourceMapping, applied in resources.go:464-475) don't conflict. The SDK syncer checks both ResourceType annotations and individual Resource annotations independently, so either mechanism works.

  3. Annotation ordering is safe. getMappedResource() is called before mapTraits() in mapResource() (resources.go:410-418). Both use annos.Update() which appends/updates without clearing, so the SkipEntitlementsAndGrants annotation survives through trait mapping.

  4. Tests cover both config dimensions. TestParse_SparseACLs validates that both field types parse correctly and don't cross-contaminate (resource-type boolean vs resource-mapping CEL string). TestConfig_GetResourceTypes_SkipEntitlementsAndGrants validates the resource-type-level annotation is applied to the right resources.


Observations

1. Missing integration test for the CEL evaluation path (non-blocking)

The tests validate config parsing and resource-type-level annotations, but there's no test that exercises the actual code path in getMappedResource() — i.e., creating a SQLSyncer, calling getMappedResource() with row data where the CEL expression evaluates to true vs false, and verifying the SkipEntitlementsAndGrants annotation is present/absent on the resulting resource.

This is the most important code path in the PR. The PR description acknowledges this as manual testing, which is reasonable since the CEL evaluation machinery is well-tested elsewhere and the pattern is identical to SkipIf. But a unit test here would strengthen confidence significantly and prevent regressions.

Add integration test →

2. Error handling is correct (confirming)

At resources.go:465-467, a CEL evaluation failure returns the error immediately, which is the right behavior — a malformed CEL expression is a config error and should fail the sync with a clear message from the CEL library. This matches how all other CEL evaluations in this function handle errors.

3. Struct tag consistency is fine

The new SkipEntitlementsAndGrants string field on ResourceMapping doesn't use omitempty, which is consistent with other string fields in that struct (Id, DisplayName, Description). The ResourceType.SkipEntitlementsAndGrants bool uses omitempty because it's a boolean — different convention for a different type. No issue here.


CI Status

Workflow Status
Check versions ✅ Passed
ci ✅ Passed
Verify ⌛ In progress

Summary

The implementation is clean, minimal, and correct. The two levels of skip (resource-type vs per-resource) compose correctly through the SDK's annotation system. The only meaningful gap is the lack of an integration test for the CEL evaluation path at the resource level — everything else follows established patterns precisely.

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.

0 participants