Add sort index trees to unified resource cache

[avatus]

This adds sort indexes to not only improve the performance of getting resources from our unified cache, but also to support #32077 being able to get a resource from the cache by the keys we use on the frontend (name/type) of the "contained" resource instead of the container resources we store them as (appserver, dbserver, etc).

I'm not married to any implementations here but the main parts I tried to solve were:

1. store our resources in a map instead with the same unique key we use in the current btree, name/type (derived from resourceKey).
2. Create a tree for each sort type we have ("name" and "type" currently). These items will be sorted by their key (generated by resourceSortKey depending on the type) with the value being the resourceKey from above so we can pull from the resources map.
3. be able to update our sorted trees that have the 'contained' keys when we receive an event that doesn't include the entire resource (like opDelete)

this removes the need for FakePaginate by passing in our matchers and rbac to each iteration of Ascend/Descend

// benchmarkListUnifiedResources
-// FakePaginate
-// ok      github.com/gravitational/teleport/lib/auth      25.135s

+// IterateUnifiedResources
+// ok      github.com/gravitational/teleport/lib/auth      2.878s

So far, all my tests pass with minimal changes (mostly to the input instead of the output) and the web doesn't even have to care about these changes.

i might be lost in the sauce on some parts

[rosstimothy]

Instead of using this switch statement can we create an access checker that works for all resources via newResourceAccessChecker like we do in ListResources?

[avatus]

Oh yeah thats way better. Will update

[rosstimothy]

It looks like resourceSortKey is always called twice in a row, once for each sort option, should we update it to just return both keys in a single invocation?

[rosstimothy]

Should we return early if one of the deletes fails? Should we attempt all deletes and return an aggregated error?

[avatus]

Yeah i suppose if the resource isn't found in one it probably won't be found in the other (my guess is it wouldn't be found due to name change and they both include the name).
I think I'm more a fan of returning early but I want to noodle on it for a bit

[rosstimothy]

I'm not sure if this makes it any more/less readable but you could store the correct function in a variable first and then call it inline with the iterate function.

Suggested change

	err := c.read(ctx, func(cache *UnifiedResourceCache) error {
	tree, err := cache.getSortTree(req.SortBy.Field)
	if err != nil {
	return trace.Wrap(err, "getting sort tree")
	}
	iterateFunc := func(item *item) bool {
	err := c.read(ctx, func(cache *UnifiedResourceCache) error {
	tree, err := cache.getSortTree(req.SortBy.Field)
	if err != nil {
	return trace.Wrap(err, "getting sort tree")
	}
	var iterateRange func(lessOrEqual, greaterThan *item, iterator btree.ItemIteratorG[*item])
	var endKey []byte
	if req.SortBy.IsDesc {
	iterateRange = tree.DescendRange
	endKey = backend.Key(prefix)
	} else {
	iterateRange = tree.AscendRange
	endKey = backend.RangeEnd(backend.Key(prefix))
	}
	iterateRange(&item{Key: startKey}, &item{Key: endKey}, func(item *item) bool {

[avatus]

I'd argue your suggestion is more readable if only for the fact that the code is read in the order it'd be executed (instead of my way which you'd see the iterate func and then it actually executes underneath it).

I was going for something like this anyway.

[capnspacehook]

If nil entries are not valid you could instead check if the resource exists in the map and if not return and keep ascending? And if the resource is nil then return an error

[capnspacehook]

For this and the types.DatabaseServer case, should app or db ever be nil or would that be invalid?

[avatus]

This was originally written to make "delete" not break if we received a resource header instead of the resource. It's vestigial and we can remove it, as our resource map will always have the contained resource available. (Or I suppose the check it fine to make sure we don't get a panic)

[rosstimothy]

Suggested change

	return match, err
	return match, trace.Wrap(err)

[rosstimothy]

Since this is only going to be backported to v14 we could use the new clear builtin

Suggested change

	c.resources = make(map[string]resource)
	clear(c.resources)

[avatus]

Oh thats sick

[avatus]

Thanks for this, it actually exposed a bug. When I was putting resources, using that make was building a map that I had forgotten to build in our fallbackCache so it paniced after this change because it couldn't clear something that didnt exist. Now I added the map during the fallbackCache creation and it works great!

[rosstimothy]

Suggestion: group the const blocks. Are SortByName and SortByKind used by anything external? If not can we unexport them. If they are can we add go docs for them?

Suggested change

	const (
	prefix = "unified_resource"
	)
	const (
	SortByName string = "name"
	SortByKind string = "kind"
	)
	const (
	prefix = "unified_resource"
	SortByName string = "name"
	SortByKind string = "kind"
	)

[rosstimothy]

Suggestion: You could unexport the fields since the type isn't exported

Suggested change

	type resourceSortKey struct {
	ByName []byte
	ByType []byte
	}
	type resourceSortKey struct {
	byName []byte
	byType []byte
	}

[public-teleport-github-review-bot]

@avatus See the table below for backport results.

Branch	Result
branch/v14	Failed