[v17] Define Backend.GetRange to be inclusive (#51025)

* Test for Backend.GetRange being inclusive

* Get rid of the unused prefixItem

* Fix memorybk GetRange not being inclusive

* Fix etcdbk GetRange not being inclusive

* Document the desired behavior of GetRange

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

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)))
 	}

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)
-}

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

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)

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) {
@@ -490,7 +489,6 @@ func (c *UnifiedResourceCache) getResourcesAndUpdateCurrent(ctx context.Context)
 	c.stale = false
 	c.defineCollectorAsInitialized()
 	return nil
-
 }
 
 // getNodes will get all nodes
@@ -582,7 +580,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")
 		}
@@ -784,25 +781,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
@@ -912,7 +895,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{
@@ -921,7 +905,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)
 		}