ccl/sqlproxyccl: add private endpoints ACL support #103070

jaylim-crl · 2023-05-10T20:53:30Z

ccl/sqlproxyccl: remove the Load field from the Pod proto message

The Load field has been deprecated for months, and is no longer being used
in production. The tenant directory implementation is no longer emitting pod
loads. This commit addresses an old TODO.

ccl/sqlproxyccl: add WatchTenants API to the directory server

This commit adds a new WatchTenants API to the directory server interface. The
goal is for this API to stream tenant metadata change notifications. At the
same time, this commit adds a new Tenant proto message to represent tenant
fields instead. This would allow us to add more fields to the tenant object
(e.g. IP allowlist and private endpoint entries).

ccl/sqlproxyccl: add tenant metadata watcher to the directory cache

This commits adds a new tenant metadata watcher mechanism to the directory
cache. This will be used to receive notifications about tenants being created,
modified, or deleted. Previously, we were only calling GetTenant once
throughout the lifetime of the tenant entry. In this new model, GetTenant is
only called if the tenant entry isn't valid.

ccl/sqlproxyccl: include PrivateEndpoints in the tenant proto message

This commit includes two new fields in the tenant proto: ConnectivityType and
PrivateEndpoints. The first will be used to indicate how the cluster can be
accessed (i.e. public only, private only, or both), whereas the second will
be used to indicate which endpoints can access the cluster, assuming that the
cluster allows private connectivity.

A subsequent commit will implement the necessary ACLs.

ccl/sqlproxyccl: plumb context.Context into the CheckConnection API

Previously, the CheckConnection API in the AccessController interface took in
a TimeSource object, and that seemed a little awkward. Given that the
timeSource field is only used by the denylist ACL, we will plumb this into
the denylis ACL. At the same time, we plumb context.Context into the
CheckConnection API. Once we add a new PrivateEndpoints ACL, that will have
to use a context.Context object for methods on the directory cache interface.

ccl/sqlproxyccl: add private endpoints ACL support

This commit adds a new PrivateEndpoints ACL support to the access controller.
This ACL will only apply if the cluster allows private connectivity. If the
connection does not have an EndpointID, it is assumed to be a public
connection, and the ACL is a no-op, which is the current behavior. A
subsequent commit will extract the private endpoints from the PROXY header,
and populate the EndpointID for the connection.

ccl/sqlproxyccl: parse private endpoint IDs in the PROXY header

This commit parses the private endpoint IDs within the PROXY header. Doing
this would also mean that the ACL for private endpoints will be enforced under
the right conditions. The parsing logic assumes that there can only be one
type of endpoint ID within the PROXY header (i.e. AWS, GCP, or Azure).

ccl/sqlproxyccl: add an interval poller for private endpoint updates

There is a requirement of automatically disconnecting existing connections if
the ACL rules no longer match the connection's state. For the file-based ACLs,
we poll every 1 minute, read the file, and check on every single connection.

Here, we implement a similar mechanism for private endpoint updates. This is
reasonable for now as:

Calls to LookupTenant are cached most of the time.
This is only applied to existing connections, and a polling interval of
1 minute isn't too bad.
We are already iterating all the connections for the other types ACLs

That said, this approach isn't efficient and we're limited by the existing
design of AccessController. Checking all the connections each time the watch
ticks isn't ideal. In fact, we're checking three times here, one for each ACL.
The directory cache already knows which tenants were updated through
WatchTenants, and we can definitely do better here. A follow up TODO has been
added to refactor AccessController in a way that allows updates to be batched.
We should also only check connections only for tenants that have been updated.

Release note: None

Epic: none

cockroach-teamcity · 2023-05-10T20:53:47Z

This change is

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl, @jeffswenson, and @pjtatlow)

pkg/ccl/sqlproxyccl/tenant/directory_cache.go line 33 at r3 (raw file):

	// ID. If the tenant cannot be found, this will return a GRPC NotFound
	// error.
	LookupTenant(ctx context.Context, tenantID roachpb.TenantID) (*Tenant, error)

I thought the plan was to keep Tenant and Pod separate in tenant.Directory, but to combine them in the cache? What if we had a Pods field in Tenant (or maybe a separate []*Pod return value), so that callers don't need to call twice? Or do we expect callers to always care only about one or the other?

pkg/ccl/sqlproxyccl/tenant/directory_cache.go line 70 at r3 (raw file):

// SkipTenantWatcher indicates that the directory cache should not run the
// tenant watcher.
func SkipTenantWatcher() func(opts *dirOptions) {

What caller will use this option?

pkg/ccl/sqlproxyccl/tenant/directory_cache.go line 499 at r3 (raw file):

// tenant's entry to reflect that change.
func (d *directoryCache) updateTenantPodEntry(ctx context.Context, pod *Pod) {
	if pod == nil || pod.Addr == "" || pod.TenantID == 0 {

When can this be nil? Are you sure it doesn't indicate a bug in the caller if it's ever nil?

pkg/ccl/sqlproxyccl/tenant/directory_cache.go line 574 at r3 (raw file):

				// them as stale allows LookupTenant to fetch a new right away
				// if needed.
				d.markAllEntriesInvalid()

Shouldn't we also do this in watchPods? Or is the idea that we'll just rely on reportFailure to invalidate pod metadata?

pkg/ccl/sqlproxyccl/tenant/entry.go line 43 at r3 (raw file):

		syncutil.Mutex

		// pods represents the tenant's SQL pods.

NIT: can you add a similar comment here that the pods slice is copy-on-write?

pkg/ccl/sqlproxyccl/tenant/entry.go line 251 at r3 (raw file):

	// as nil). When that happens, the tenant watcher may race to update a
	// tenant, so we'd have to perform a nil check here.
	if (e.mu.valid && e.mu.tenant != nil && tenant.Version >= e.mu.tenant.Version) ||

NIT: Just a suggestion, feel free to disregard: the "if" logic here is a bit tricky, and also forces you to indent the "meat" of this method. I'd typically choose this instead:

// I try not to combine && and || in the same logic statement, b/c it's trickier to understand.
// It also makes comments harder to write, b/c it's often ambiguous which part(s) of the "if" they apply to.
// So I break apart the && into multiple lines like this:
if e.mu.valid {
  if e.mu.tenant == nil || tenant.Version < e.mu.tenant.Version {
    return
  }
}
// And now for the main logic of method, which does not need to be nested b/c we've checked the short-circuit conditions above.

pkg/ccl/sqlproxyccl/tenantdirsvr/test_static_directory_svr.go line 74 at r2 (raw file):

		// eventListeners stores a list of listeners that are watching changes
		// to pods through WatchPods.
		eventListeners *list.List

NIT: I'd rename to podEventListeners now that we two different lists.

jaylim-crl

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jeffswenson, and @pjtatlow)

pkg/ccl/sqlproxyccl/tenant/directory_cache.go line 33 at r3 (raw file):