From b3f8eafabe9df53d9e4202a960fd9d2737d7a76f Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 13 Jan 2025 20:22:11 +0100 Subject: [PATCH 1/5] Test for Backend.GetRange being inclusive --- lib/backend/test/suite.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/backend/test/suite.go b/lib/backend/test/suite.go index 6dce1f678ea31..ec00969240a6b 100644 --- a/lib/backend/test/suite.go +++ b/lib/backend/test/suite.go @@ -331,6 +331,11 @@ func testQueryRange(t *testing.T, newBackend Constructor) { require.NoError(t, err) RequireItems(t, []backend.Item{c1, c2}, result.Items) + // item at the end of the range + result, err = uut.GetRange(ctx, prefix("prefix", "c", "c1"), prefix("prefix", "c", "c2"), backend.NoLimit) + require.NoError(t, err) + RequireItems(t, []backend.Item{c1, c2}, result.Items) + // pagination result, err = uut.GetRange(ctx, prefix("prefix"), backend.RangeEnd(prefix("prefix")), 2) require.NoError(t, err) From 1405e80a99358795074f8823f03bc39ef573135c Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 13 Jan 2025 20:26:43 +0100 Subject: [PATCH 2/5] Get rid of the unused prefixItem --- lib/backend/memory/item.go | 14 -------------- lib/services/unified_resource.go | 23 ++++------------------- 2 files changed, 4 insertions(+), 33 deletions(-) diff --git a/lib/backend/memory/item.go b/lib/backend/memory/item.go index 3713508855442..cf0d04fc78b29 100644 --- a/lib/backend/memory/item.go +++ b/lib/backend/memory/item.go @@ -38,21 +38,7 @@ func (i *btreeItem) Less(iother btree.Item) bool { switch other := iother.(type) { case *btreeItem: return i.Key.Compare(other.Key) < 0 - case *prefixItem: - return !iother.Less(i) default: return false } } - -// prefixItem is used for prefix matches on a B-Tree -type prefixItem struct { - // prefix is a prefix to match - prefix backend.Key -} - -// Less is used for Btree operations -func (p *prefixItem) Less(iother btree.Item) bool { - other := iother.(*btreeItem) - return !other.Key.HasPrefix(p.prefix) -} diff --git a/lib/services/unified_resource.go b/lib/services/unified_resource.go index 1bf8bd95747c0..51c7a34742b31 100644 --- a/lib/services/unified_resource.go +++ b/lib/services/unified_resource.go @@ -202,7 +202,6 @@ func (c *UnifiedResourceCache) getSortTree(sortField string) (*btree.BTreeG[*ite default: return nil, trace.NotImplemented("sorting by %v is not supported in unified resources", sortField) } - } func (c *UnifiedResourceCache) getRange(ctx context.Context, startKey backend.Key, matchFn func(types.ResourceWithLabels) (bool, error), req *proto.ListUnifiedResourcesRequest) ([]resource, string, error) { @@ -503,7 +502,6 @@ func (c *UnifiedResourceCache) getResourcesAndUpdateCurrent(ctx context.Context) c.stale = false c.defineCollectorAsInitialized() return nil - } // getNodes will get all nodes @@ -595,7 +593,6 @@ func (c *UnifiedResourceCache) getSAMLApps(ctx context.Context) ([]types.SAMLIdP for { resp, nextKey, err := c.ListSAMLIdPServiceProviders(ctx, apidefaults.DefaultChunkSize, startKey) - if err != nil { return nil, trace.Wrap(err, "getting SAML apps for unified resource watcher") } @@ -818,25 +815,11 @@ func (i *item) Less(iother btree.Item) bool { switch other := iother.(type) { case *item: return i.Key.Compare(other.Key) < 0 - case *prefixItem: - return !iother.Less(i) default: return false } } -// prefixItem is used for prefix matches on a B-Tree -type prefixItem struct { - // prefix is a prefix to match - prefix backend.Key -} - -// Less is used for Btree operations -func (p *prefixItem) Less(iother btree.Item) bool { - other := iother.(*item) - return !other.Key.HasPrefix(p.prefix) -} - type resource interface { types.ResourceWithLabels CloneResource() types.ResourceWithLabels @@ -946,7 +929,8 @@ func MakePaginatedResource(ctx context.Context, requestType string, r types.Reso AppServer: appOrSP, }, }, - }, RequiresRequest: requiresRequest} + }, RequiresRequest: requiresRequest, + } case *types.SAMLIdPServiceProviderV1: protoResource = &proto.PaginatedResource{ Resource: &proto.PaginatedResource_AppServerOrSAMLIdPServiceProvider{ @@ -955,7 +939,8 @@ func MakePaginatedResource(ctx context.Context, requestType string, r types.Reso SAMLIdPServiceProvider: appOrSP, }, }, - }, RequiresRequest: requiresRequest} + }, RequiresRequest: requiresRequest, + } default: return nil, trace.BadParameter("%s has invalid type %T", resourceKind, resource) } From c2f17240beb843f2ba803d396979ac7c545b4c9b Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 13 Jan 2025 20:31:20 +0100 Subject: [PATCH 3/5] Fix memorybk GetRange not being inclusive --- lib/backend/memory/memory.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/backend/memory/memory.go b/lib/backend/memory/memory.go index cd00a6bb6efaa..5b139eadbe9e9 100644 --- a/lib/backend/memory/memory.go +++ b/lib/backend/memory/memory.go @@ -441,7 +441,12 @@ func (m *Memory) NewWatcher(ctx context.Context, watch backend.Watch) (backend.W func (m *Memory) getRange(ctx context.Context, startKey, endKey backend.Key, limit int) backend.GetResult { var res backend.GetResult - m.tree.AscendRange(&btreeItem{Item: backend.Item{Key: startKey}}, &btreeItem{Item: backend.Item{Key: endKey}}, func(item *btreeItem) bool { + startItem := &btreeItem{Item: backend.Item{Key: startKey}} + endItem := &btreeItem{Item: backend.Item{Key: endKey}} + m.tree.AscendGreaterOrEqual(startItem, func(item *btreeItem) bool { + if endItem.Less(item) { + return false + } res.Items = append(res.Items, item.Item) if limit > 0 && len(res.Items) >= limit { return false From d20327c5e6597af246f7da0661bb79b7ea5fb0ab Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 13 Jan 2025 20:42:37 +0100 Subject: [PATCH 4/5] Fix etcdbk GetRange not being inclusive --- lib/backend/etcdbk/etcd.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/backend/etcdbk/etcd.go b/lib/backend/etcdbk/etcd.go index 8c1143c4bc8b6..d5481bb74ed5a 100644 --- a/lib/backend/etcdbk/etcd.go +++ b/lib/backend/etcdbk/etcd.go @@ -537,7 +537,6 @@ type eventResult struct { // effective, this strategy still suffers from a "head of line blocking"-esque issue since event order // must be preserved. func (b *EtcdBackend) watchEvents(ctx context.Context) error { - // etcd watch client relies on context cancellation for cleanup, // so create a new subscope for this function. ctx, cancel := context.WithCancel(ctx) @@ -658,7 +657,11 @@ func (b *EtcdBackend) GetRange(ctx context.Context, startKey, endKey backend.Key if endKey.IsZero() { return nil, trace.BadParameter("missing parameter endKey") } - opts := []clientv3.OpOption{clientv3.WithRange(b.prependPrefix(endKey))} + // etcd's range query includes the start point and excludes the end point, + // but Backend.GetRange is supposed to be inclusive at both ends, so we + // query until the very next key in lexicographic order (i.e., the same key + // followed by a 0 byte) + opts := []clientv3.OpOption{clientv3.WithRange(b.prependPrefix(endKey) + "\x00")} if limit > 0 { opts = append(opts, clientv3.WithLimit(int64(limit))) } From e4222e55007a6fc677e0327a932da58e70b6569d Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 13 Jan 2025 20:43:33 +0100 Subject: [PATCH 5/5] Document the desired behavior of GetRange --- lib/backend/backend.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/backend/backend.go b/lib/backend/backend.go index c51695b7f9396..96f6d3a2bde26 100644 --- a/lib/backend/backend.go +++ b/lib/backend/backend.go @@ -67,7 +67,8 @@ type Backend interface { // Get returns a single item or not found error Get(ctx context.Context, key Key) (*Item, error) - // GetRange returns query range + // GetRange returns the items between the start and end keys, including both + // (if present). GetRange(ctx context.Context, startKey, endKey Key, limit int) (*GetResult, error) // Delete deletes item by key, returns NotFound error