Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions pkg/connectorstore/connectorstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,21 +192,29 @@ type EntitlementGrantDigestReader interface {
//
// For 0 <= level <= the native Level (GrantDigest.Level) this folds
// the stored leaves — one contiguous scan of the digest keyspace, no
// grant-index scan. For a finer level it falls back to scanning the
// grant index directly (O(grants)) — slower, but it never errors on a
// "too deep" level. The principal-hash carries a bounded number of
// bits, so a level beyond that resolution is served at the maximum
// (you may get fewer than 2^level distinct buckets). found is false
// when no digest exists.
// grant-index scan. For a finer level, up to the principal-hash's
// resolution, it falls back to scanning the grant index directly
// (O(grants)) — slower, but exact. A level outside that resolution
// (a negative level, or one past the implementation's bucket-hash
// width — any level <= the digest's native Level is always in range;
// the Pebble engine exports its full width as DigestBucketHashBits)
// errors rather than silently serving the maximum resolution:
// a caller that placed its own records by hash (e.g. the Pebble
// engine's PrincipalDigestBucket) must get the same bucket set the
// engine reports, not a quietly coarser one. found is false when no
// digest exists.
Comment thread
mj-palanker marked this conversation as resolved.
GetEntitlementGrantDigestNodes(ctx context.Context, entitlement *v2.Entitlement, level int) (nodes []GrantDigestNode, found bool, err error)
Comment thread
mj-palanker marked this conversation as resolved.

// ScanEntitlementGrantBucket yields every grant in one digest bucket
// of the entitlement (see GrantDigestBucket) as a v2.Grant, stopping
// early if yield returns false. Bucket Level 0 scans the whole
// entitlement; a Level finer than the bucket-hash resolution is
// clamped (matching GetEntitlementGrantDigestNodes). It reads the
// grant hash index, which exists only on files whose digest was
// built (they are derived together at seal): callers must check
// entitlement; a Level outside the bucket-hash resolution errors
// (matching GetEntitlementGrantDigestNodes) rather than clamping,
// and an Index outside [0, 2^Level) errors rather than wrapping —
// silently folding either coordinate would scan a bucket other than
// the one addressed. It reads the grant hash index,
// which exists only on files whose digest was built (they are
// derived together at seal): callers must check
// GetEntitlementGrantDigest first and treat found=false as "scan
// unavailable — read the grants directly", not as "no grants". It
// yields nothing when there is no active sync or no matching grants.
Expand Down
43 changes: 30 additions & 13 deletions pkg/dotc1z/engine/pebble/adapter_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -719,13 +719,18 @@ func (e *Engine) GetEntitlementGrantDigest(ctx context.Context, ent *v2.Entitlem
// grant-digest rollup nodes at the requested level (2^level buckets;
// level 0 = the root). For 0 <= level <= the digest's native level it
// folds the stored leaves — one scan of the digest keyspace. For a finer
// level it scans the grant index directly (O(grants)) instead of
// erroring; the level is clamped to the bucket-hash resolution
// (digestMaxWidthBits).
// level, up to digestMaxWidthBits, it scans the grant index directly
// (O(grants)) instead. A level outside [0, digestMaxWidthBits] errors:
// the bucket hash carries no more resolution than digestMaxWidthBits, so
// silently clamping would report buckets a caller's own precomputed
// index (see PrincipalDigestBucket) does not agree with.
func (e *Engine) GetEntitlementGrantDigestNodes(ctx context.Context, ent *v2.Entitlement, level int) ([]connectorstore.GrantDigestNode, bool, error) {
if level < 0 {
return nil, false, fmt.Errorf("pebble: negative grant-digest level %d", level)
}
if level > digestMaxWidthBits {
return nil, false, fmt.Errorf("pebble: grant-digest level %d exceeds bucket-hash resolution %d", level, digestMaxWidthBits)
}
Comment on lines +731 to +733

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: this is a deliberate default-behavior break on an exported interface method — a caller that previously passed level > 16 got a clamped result set, and now gets an error (same for ScanEntitlementGrantBucket). The rationale is well argued in the doc comments, but the PR body only describes the PrincipalBucketHash export, and pkg/sdk/version.go stays at v0.24.1. Per this repo's SDK criteria, a default-behavior change should carry a 0.x minor bump and a line in the PR description so downstreams have a signal beyond reading the diff. (confidence: medium — the surface looks new enough that no external caller is likely relying on the clamp) (confidence: medium)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR description now calls out the default-behavior change (error instead of clamp past the bucket-hash resolution). Left pkg/sdk/version.go alone: version bumps in this repo land as standalone "Update SDK version to vX" release commits, not inside feature PRs — flagging that the next release should be a minor (0.25.0) rather than a patch. Leaving this thread open for a maintainer call on the bump.

syncID, err := e.resolveActiveSyncForReader(ctx, nil)
if err != nil {
return nil, false, err
Expand All @@ -746,11 +751,10 @@ func (e *Engine) GetEntitlementGrantDigestNodes(ctx context.Context, ent *v2.Ent
if level == 0 {
return []connectorstore.GrantDigestNode{{Index: 0, Hash: root.Hash, Count: root.Count}}, true, nil
}
// The bucket hash carries at most digestMaxWidthBits of resolution;
// a finer level can't address more buckets, so clamp.
bits := min(level, digestMaxWidthBits)
// At or below the stored width, fold the digest leaves (cheap). Finer
// than what we stored, scan the grant index to compute the rollup.
// level is already bounded to [0, digestMaxWidthBits] above. At or
// below the stored width, fold the digest leaves (cheap); finer than
// what we stored, scan the grant index to compute the rollup.
bits := level
partition := digestPartitionForEntitlement(id)
var folded []foldedBucket
if bits <= root.Bits {
Expand All @@ -775,13 +779,27 @@ func (e *Engine) GetEntitlementGrantDigestNodes(ctx context.Context, ent *v2.Ent
// ScanEntitlementGrantBucket implements
// connectorstore.EntitlementGrantDigestReader. It yields every grant in
// the given digest bucket of the entitlement, translated to v2.Grant.
// Bucket Level 0 scans the whole entitlement; a finer Level is clamped
// to the bucket-hash resolution. Yields nothing when there is no active
// sync or a bare entitlement id resolves to nothing.
// Bucket Level 0 scans the whole entitlement. A Level outside
// [0, digestMaxWidthBits] errors rather than clamping to the bucket-hash
// resolution, and an Index outside [0, 2^Level) errors rather than
// wrapping to its low Level bits: either kind of silent folding would
// scan a bucket other than the one the caller addressed (see
// PrincipalDigestBucket, which only builds in-range buckets). Yields
// nothing when there is no active sync or a bare entitlement id
// resolves to nothing.
func (e *Engine) ScanEntitlementGrantBucket(ctx context.Context, ent *v2.Entitlement, bucket connectorstore.GrantDigestBucket, yield func(*v2.Grant) bool) error {
if bucket.Level < 0 {
return fmt.Errorf("pebble: negative grant-digest level %d", bucket.Level)
}
if bucket.Level > digestMaxWidthBits {
return fmt.Errorf("pebble: grant-digest level %d exceeds bucket-hash resolution %d", bucket.Level, digestMaxWidthBits)
}
// Level 0 ignores Index (whole-entitlement scan) per the
// GrantDigestBucket contract; past that, bucketBounds would shift an
// oversized index's high bits away and scan Index mod 2^Level.
if bucket.Level > 0 && uint64(bucket.Index) >= 1<<uint(bucket.Level) {
return fmt.Errorf("pebble: grant-digest bucket index %d out of range [0, 2^%d)", bucket.Index, bucket.Level)
}
syncID, err := e.resolveActiveSyncForReader(ctx, nil)
if err != nil {
return err
Expand All @@ -793,8 +811,7 @@ func (e *Engine) ScanEntitlementGrantBucket(ctx context.Context, ent *v2.Entitle
if err != nil || !ok {
return err
}
bits := min(bucket.Level, digestMaxWidthBits)
return e.IterateGrantsByEntitlementBucket(ctx, id, DigestBucket{Index: bucket.Index, Bits: bits}, func(r *v3.GrantRecord) bool {
return e.IterateGrantsByEntitlementBucket(ctx, id, DigestBucket{Index: bucket.Index, Bits: bucket.Level}, func(r *v3.GrantRecord) bool {
Comment thread
mj-palanker marked this conversation as resolved.
return yield(V3GrantToV2(r))
})
}
13 changes: 13 additions & 0 deletions pkg/dotc1z/engine/pebble/digest.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,19 @@ const (
digestLeafPrefixLen = 2
)

// The stored bucket-hash width (digestBucketHashLen), the leaf-key
// prefix width (digestLeafPrefixLen), and the read-side resolution
// bound (digestMaxWidthBits) are independent constants that must
// agree — these fail the build the moment any pair diverges. The
// leaf-prefix coupling is what makes the ">> (16 - bits)" shifts in
// bucketOfHash / foldedLeafBuckets / computeBucketsAtWidth safe:
// growing digestMaxWidthBits without digestLeafPrefixLen would leave
// bits able to exceed 16 and panic on a negative shift at read time.
const _ uint = digestMaxWidthBits - digestBucketHashLen*8
const _ uint = digestBucketHashLen*8 - digestMaxWidthBits
const _ uint = digestMaxWidthBits - digestLeafPrefixLen*8
const _ uint = digestLeafPrefixLen*8 - digestMaxWidthBits

// Node-key levels: the root is level 0 (empty prefix); the single leaf
// level is 1 (digestLeafPrefixLen-byte prefix). See encodeDigestNodeKey.
const (
Expand Down
169 changes: 166 additions & 3 deletions pkg/dotc1z/engine/pebble/digest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,10 @@ func TestAdapterGrantDigestNodes(t *testing.T) {
t.Fatalf("finer-level scan: sum=%d xor-matches-root=%v, want sum %d and matching root", sum, bytes.Equal(xor, d.Hash), n)
}

// Absurdly fine level → clamped to the hash resolution, still no error.
if _, found, err := a.GetEntitlementGrantDigestNodes(ctx, ent, 999); err != nil || !found {
t.Fatalf("nodes(999): found=%v err=%v, want clamped scan with no error", found, err)
// Absurdly fine level → errors: past the bucket-hash resolution there
// is no addressable bucket to clamp into.
if _, _, err := a.GetEntitlementGrantDigestNodes(ctx, ent, 999); err == nil {
t.Fatal("nodes(999): want error past the bucket-hash resolution, got nil")
}
}

Expand Down Expand Up @@ -517,6 +518,168 @@ func TestAdapterScanGrantBucket(t *testing.T) {
}
}

// TestPrincipalBucketHashMatchesServedBuckets pins the EXPORTED
// PrincipalBucketHash against what the read APIs actually serve, over a
// sealed file with a non-zero digest width.
//
// TestGrantDigestSpliceMatchesEncode already pins the internal hash
// against the spliced key region, but that is an internal consistency
// check: it cannot catch the exported symbol drifting away from the
// buckets GetEntitlementGrantDigestNodes reports and
// ScanEntitlementGrantBucket serves. That drift is exactly the failure
// this export exists to prevent, and it is silent — a downstream caller
// placing its own rows with this hash would scan buckets that quietly
// miss records, with no error and no panic. So the assertions here run
// through the public read APIs only.
//
// Both directions are checked at each level: every principal's predicted
// index is a reported node with a matching count (no misses), and a
// sampled bucket's scan yields exactly the principals whose hash prefix
// is that index (no extras).
func TestPrincipalBucketHashMatchesServedBuckets(t *testing.T) {
// The stored truncation width is what makes levels past it unusable
// to callers. If digestBucketHashLen ever changes, fail here — in the
// SDK that owns the ABI — rather than downstream.
if DigestBucketHashBits != 16 {
t.Fatalf("DigestBucketHashBits = %d, want 16; digestBucketHashLen changed — this is an ABI break for downstream callers", DigestBucketHashBits)
}

ctx := context.Background()
e, _ := newTestEngine(t)
a := NewAdapter(e)
if _, err := a.StartNewSync(ctx, connectorstore.SyncTypeFull, ""); err != nil {
t.Fatalf("StartNewSync: %v", err)
}
putEnt(t, e, ctx, "ent-A")

// 4000 grants → chooseDigestWidth = 3 (512 → 1024 → 2048 → 4096), so
// the file is sealed with a real leaf level, not a root-only digest.
const n = 4000
principals := make([]string, n)
grants := make([]*v3.GrantRecord, 0, n)
for i := range principals {
principals[i] = fmt.Sprintf("user-%04d", i)
grants = append(grants, makeGrant("", fmt.Sprintf("g-%04d", i), "ent-A", principals[i]))
}
if err := e.PutGrantRecords(ctx, grants...); err != nil {
t.Fatalf("PutGrantRecords: %v", err)
}
if err := a.EndSync(ctx); err != nil {
t.Fatalf("EndSync: %v", err)
}

ent := testV2Ent("ent-A")
d, found, err := a.GetEntitlementGrantDigest(ctx, ent)
if err != nil || !found {
t.Fatalf("digest: found=%v err=%v", found, err)
}
if d.Level == 0 {
t.Fatalf("native level = 0 for %d grants; test needs a non-zero digest width", n)
}

// Levels 1-3 are at or below the native width (3) and so are served
// by folding the stored leaves (foldedLeafBuckets); 4, 8, 12 and 16
// are finer than native and fall back to the index scan
// (computeBucketsAtWidth). Both paths must bucket identically to
// PrincipalDigestBucket. 16 is DigestBucketHashBits itself — the
// finest permitted level, where the shifts on both sides consume the
// full stored width; past it the engine errors instead of clamping
// (see the level-17 subtest below).
for _, level := range []int{1, 2, 3, 4, 8, 12, 16} {
t.Run(fmt.Sprintf("level-%d", level), func(t *testing.T) {
// The placement a downstream caller computes for itself, from
// the principal identity alone (type "user", per makeGrant).
Comment thread
mj-palanker marked this conversation as resolved.
want := make(map[uint32]map[string]bool)
for _, p := range principals {
bucket, err := PrincipalDigestBucket("user", p, level)
if err != nil {
t.Fatalf("PrincipalDigestBucket(%q, %d): %v", p, level, err)
}
idx := bucket.Index
if want[idx] == nil {
want[idx] = make(map[string]bool)
}
want[idx][p] = true
}

nodes, found, err := a.GetEntitlementGrantDigestNodes(ctx, ent, level)
if err != nil || !found {
t.Fatalf("nodes(%d): found=%v err=%v", level, found, err)
}
// Nodes are sparse and non-empty, so the reported index set is
// exactly the set of predicted indexes: same size, and every
// reported node predicted with the same count. A principal
// hashing into an index the engine never reports (a "miss")
// fails the size check or the lookup below.
if len(nodes) != len(want) {
t.Fatalf("level %d: engine reports %d non-empty buckets, the hash predicts %d", level, len(nodes), len(want))
}
for _, nd := range nodes {
w, ok := want[nd.Index]
if !ok {
t.Fatalf("level %d: engine reports bucket %d, which no principal hashes into", level, nd.Index)
}
if nd.Count != int64(len(w)) {
t.Fatalf("level %d bucket %d: node count %d, the hash predicts %d principals there", level, nd.Index, nd.Count, len(w))
}
}

// Sampled buckets: the scan yields exactly the predicted
// principals — no extras and no misses.
step := max(1, len(nodes)/8)
for i := 0; i < len(nodes); i += step {
idx := nodes[i].Index
got := make(map[string]bool, len(want[idx]))
if err := a.ScanEntitlementGrantBucket(ctx, ent, connectorstore.GrantDigestBucket{Level: level, Index: idx}, func(g *v2.Grant) bool {
got[g.GetPrincipal().GetId().GetResource()] = true
return true
}); err != nil {
t.Fatalf("scan level %d bucket %d: %v", level, idx, err)
}
for p := range want[idx] {
if !got[p] {
t.Fatalf("level %d bucket %d: principal %q hashes here, but the bucket scan did not yield it", level, idx, p)
}
}
for p := range got {
if !want[idx][p] {
gotBucket, _ := PrincipalDigestBucket("user", p, level)
t.Fatalf("level %d bucket %d: scan yielded principal %q, which the hash places in bucket %d",
level, idx, p, gotBucket.Index)
}
}
}
})
}

t.Run("level-17-errors", func(t *testing.T) {
// One bit past DigestBucketHashBits: no addressable bucket, so
// every level-taking entry point on this contract must error
// rather than silently clamp.
if _, err := PrincipalDigestBucket("user", principals[0], 17); err == nil {
t.Fatal("PrincipalDigestBucket(level 17): want error, got nil")
}
if _, _, err := a.GetEntitlementGrantDigestNodes(ctx, ent, 17); err == nil {
t.Fatal("GetEntitlementGrantDigestNodes(level 17): want error, got nil")
}
if err := a.ScanEntitlementGrantBucket(ctx, ent, connectorstore.GrantDigestBucket{Level: 17, Index: 0}, func(*v2.Grant) bool { return true }); err == nil {
t.Fatal("ScanEntitlementGrantBucket(level 17): want error, got nil")
}
})

t.Run("index-out-of-range-errors", func(t *testing.T) {
// Same silent-folding class on the Index axis: bucketBounds shifts
// an oversized index's high bits away, so {Level: 4, Index: 20}
// would otherwise scan bucket 4 (20 mod 16) without complaint.
// 16 is the first out-of-range index at level 4.
for _, idx := range []uint32{16, 20} {
if err := a.ScanEntitlementGrantBucket(ctx, ent, connectorstore.GrantDigestBucket{Level: 4, Index: idx}, func(*v2.Grant) bool { return true }); err == nil {
t.Fatalf("ScanEntitlementGrantBucket(level 4, index %d): want out-of-range error, got nil", idx)
}
}
})
}

// seedEntitlement writes the entitlement record + grants and runs the
// seal-time build (hash index + digests), returning the syncID.
func seedEntitlement(t testing.TB, e *Engine, entID string, grants []*v3.GrantRecord) string {
Expand Down
Loading
Loading