[api] Add LockFilter proto type

[okraport]

This commit moves the lock filter parameters to a
filter message in preparation for adding paginated variants of GetLocks. The proposed signature is:

ListLocks(context.Context, int, string, *types.LockFilter) ([]types.Lock, string, error)
RangeLocks(context.Context, string, string, *types.LockFilter) iter.Seq2[types.Lock, error]

The filter type is required in e to satisfy LockGetter hence the new type is added first.

[juliaogris]

LGTM with a couple of questions.

[juliaogris]

It would have helped me to see the use of this convenience function; specifically, why not use targets ...*LockTarget and save the conversion? Aren't proto messages typically passed as pointers, as with older generators, you used to get error messages like assignment copies lock value: *MessageType contains sync.RWMutex?

[okraport]

Aren't proto messages typically passed as pointer

This is true, the helper is to help with migrations for the legacy call sites which use this format:

https://github.com/gravitational/teleport/blob/master/api/client/client.go#L3279

It is especially convenient as my plan is to attempt the paginated getter within the client side implementation for the migration (until we can deleted non-paginated endpoints).

[espadolini]

FWIW it's basically never correct to copy a protobuf message by value even if go vet doesn't complain about it because we can't update our codegen (see golang/protobuf#1155 (comment)). +1 for changing NewLockFilter to take a slice of *LockTarget (or getting rid of this helper function entirely, seeing InForceOnly: true in a struct literal is certainly better than seeing a true in a function call).

[juliaogris]

IIUC in_force_only means not yet expired, according to

  // IsInForce returns whether the lock is in force at a particular time.
  func (c *LockV2) IsInForce(t time.Time) bool {
  	if c.Spec.Expires == nil {
  		return true
  	}
  	return t.Before(*c.Spec.Expires)
  }

in api/types/lock.go.

I think an extra sentence to that extent in the comment would have helped me.

I also wonder if this proto message is intended to be used with pagination, should it also take a timestamp in_force_at
as defaulting to now could lead to temporal inconsistency for multiple queries?
Or would the semantics be: Evaluate all "in force" at the time of the first request, in which case, does this deserve another comment?

[espadolini]

There's no guarantee of atomicity when doing any range read of any kind; even (*lib/services/local.AccessService)GetLocks (which sits right on top of storage and returns every lock) manages to check against a new value of Now() for each item in the slice of locks. 🥲

[okraport]

I think an extra sentence to that extent in the comment would have helped me.

Yes the proto comment is misleading, for transparency I aim to keep the same params and docs as our legacy endpoints:
https://github.com/gravitational/teleport/blob/master/api/proto/teleport/legacy/client/proto/authservice.proto#L1608-L1614

I will reword this to be clearer, thank you for spotting it.

I also wonder if this proto message is intended to be used with pagination, should it also take a timestamp in_force_at

My current understanding is as follows:

• For most sections where we check for active locks in the current codebase uses targets, and as such I don't actually expect us to use more than one page (1k by default chunk size) which would match non paginated endpoint of a single RPC round trip. In these call patterns I would actually argue that it may be beneficial to use a small pagesize to short circuit the checks as we only check for any active locks. [1][2]
• The getters that return all locks regardless of their expiry such as the cache fetcher and tctl won't be affected by this and our main concern is avoiding GRPC message limits and lowering memory overhead when accessing large collections.
• The two exceptions to the above are the lockCollector and UserMonitor. Both of which refetch the state periodically so in the case we hit the edge case of a lock expiring during the fetch we fail safe and the lock is cleared on the next fetch.

[1] https://github.com/gravitational/teleport.e/blob/master/lib/okta/users.go#L171-L183
[2]

teleport/lib/accesslists/hierarchy.go

Lines 275 to 285 in d90e4ea

	if lockGetter != nil {
	locks, err := lockGetter.GetLocks(ctx, true, types.LockTarget{
	User: user.GetName(),
	})
	if err != nil {
	return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(err)
	}
	if len(locks) > 0 {
	return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, newUserLockedError(user.GetName())
	}
	}