Database authorization resource consumption enhancements

[wethreetrees]

This change sets out to address reported performance issues related to GetDatabaseServers in specific scenarios, e.g. many database servers with concurrent database connection attempts, or database server set larger than available memory.

Contributes to #63728

In this change, we add a watcher which acts as an in-memory view of all database servers. The call to CachingAccessPoint.GetDatabaseServers in ProxyServer.Authorize has been replaced with a DatabaseServerWatcher lookup using CurrentResourceWithFilter. Previously, every inbound connection allocated and iterated a full copy of all database servers, then discarded all but the matching entries. Under high concurrency with large numbers of databases this caused significant GC pressure and OOM conditions. The new watcher lookup only allocates the servers matching the requested database names.

Note: This approach has been discussed at length and was chosen primarily because it satisfies the short term needs, greatly improves performance at scale, and minimally impacts public interfaces. The intention is to holistically refactor this approach at a later date.

Manual Test Plan

Test Environment

Cloud tenant created in staging

Name: tyler-dbwatcher-testing
Features:

• RollingAuthUpdate
• WildcardDNS
• AccessMonitoring
• BootstrapInit
• InjectAWSCredSh
• R53HealthCheck
• TLSCertReload
• KMS
• EnablePyroscope
• AutoUpdates
• BootstrapCNC
• SetEnvGOMEMLIMIT90
• ACKDedicatedS3Buckets
• EKSPodIdentity
• UnmountServiceAccountToken
• IAMJoinToken
• DisableStaticJoinToken
• DeferConfigChanges
• QUICProxyPeering
• DisableAWSCredentialsFile
• VPADeployment
• EnableDefaultBedrockSummarizer

Trusted cluster tests were run locally with two instances of teleport.

Test Cases

• Register a new database
• Connect to the newly registered database and confirm the database is found with tsh db connect (if you registered a fake database, you should get a “failed to connect” error)
• Restart the proxy and confirm a database connection works immediately after startup, verifying the watcher initializes correctly on startup
• Confirm that connecting to an unregistered database returns an appropriate error (e.g. database not found)
• (Optional - can be skipped if a functional database was registered in the first test case) Register a functional database and confirm successful connection
• Remove a database and confirm it is no longer connectable (e.g. database not found)
• Register multiple database agents proxying the same database and confirm successful connection
• (Optional - this is technically covered by the go test benchmark) Register >3000 databases and confirm connection to one database, verify that no significant memory increase occurs
• (Trusted cluster - requires self-hosted setup, not testable on cloud) Connect to a database registered on a leaf cluster via the root proxy and confirm the database is found (if you registered a fake database, you should get a “failed to connect” error)

Automated Tests and Benchmarks

• go test -bench=BenchmarkConnectGetDatabaseServers ./lib/srv/db/common/connect/...

  goos: darwin
  goarch: arm64
  pkg: github.com/gravitational/teleport/lib/srv/db/common/connect
  cpu: Apple M4 Pro
  BenchmarkConnectGetDatabaseServers/total=1000-12                   80637             13659 ns/op            5552 B/op         25 allocs/op
  PASS
  ok      github.com/gravitational/teleport/lib/srv/db/common/connect     2.989s
  

• go test ./lib/srv/db/common/connect/...
• go test ./lib/services/... -run TestDatabaseServerWatcher

Benchmark Results

The benchmark was run against master (base) and wethreetrees/getdatabaseservers-oom (watcher). The benchstat comparison is pretty dramatic, but I assure you it's real.

goos: darwin
goarch: arm64
pkg: github.com/gravitational/teleport/lib/srv/db/common/connect
cpu: Apple M4 Pro
                                         │      base      │               watcher               │
                                         │     sec/op     │   sec/op     vs base                │
ConnectGetDatabaseServers/total=1000-12    14339.15µ ± 1%   14.01µ ± 1%  -99.90% (p=0.000 n=10)
ConnectGetDatabaseServers/total=5000-12    71059.63µ ± 4%   61.75µ ± 1%  -99.91% (p=0.000 n=10)
ConnectGetDatabaseServers/total=10000-12   142094.2µ ± 1%   141.9µ ± 2%  -99.90% (p=0.000 n=10)
geomean                                       52.51m        49.69µ       -99.91%

                                         │       base       │               watcher                │
                                         │       B/op       │     B/op      vs base                │
ConnectGetDatabaseServers/total=1000-12     8843.241Ki ± 0%   5.422Ki ± 0%  -99.94% (p=0.000 n=10)
ConnectGetDatabaseServers/total=5000-12    44219.700Ki ± 0%   5.422Ki ± 0%  -99.99% (p=0.000 n=10)
ConnectGetDatabaseServers/total=10000-12   87767.027Ki ± 0%   5.422Ki ± 0%  -99.99% (p=0.000 n=10)
geomean                                        31.74Mi        5.422Ki       -99.98%

                                         │      base       │               watcher               │
                                         │    allocs/op    │ allocs/op   vs base                 │
ConnectGetDatabaseServers/total=1000-12     134049.00 ± 0%   25.00 ± 0%   -99.98% (p=0.000 n=10)
ConnectGetDatabaseServers/total=5000-12     670076.00 ± 0%   25.00 ± 0%  -100.00% (p=0.000 n=10)
ConnectGetDatabaseServers/total=10000-12   1340097.00 ± 0%   25.00 ± 0%  -100.00% (p=0.000 n=10)
geomean                                        493.8k        25.00        -99.99%

changelog: Improved performance and reduced resource usage of the database proxy for clusters with large numbers of registered databases.

[wethreetrees]

There is still discussion around this, opening as draft for now

[rosstimothy]

Could you update the benchmark results in the PR description with a before and after comparison using benchstat?

Could you add test cases to the test plan that exercise connecting to real databases in a Teleport cluster. We should ideally be adding a similar number of databases as when the OOM was triggered to have confidence that it is no longer an issue.

[greedy52]

As Tim mentioned, do you mind adding some manual testing on real databases? Since it touches trusted cluster, may be good to test that out too.

[wethreetrees]

As Tim mentioned, do you mind adding some manual testing on real databases? Since it touches trusted cluster, may be good to test that out too.

For sure. Was hoping to get that updated this morning, got caught up in something else. I will update shortly!

[espadolini]

We could get rid of this and just have the watcher use a string containing the database name for the "read only" version of the resource. This is not something that just applies to DatabaseServer, but plenty of things returned by this supposedly "read only" variant are straight up mutable: metadata and database, for example.

[wethreetrees]

Love this. I removed the readonly interface.

[rosstimothy]

I would rather follow the same pattern as every other watcher. This is also limiting if any future use cases need more than the name.

[espadolini]

If the pattern is bad we shouldn't keep using it while waiting for some unspecified time in the future when we get to fix it, because that means that more and more code will accumulate using the bad pattern.

If we need more than the name in the future we can change the watcher to return a struct rather than just the name, or maybe a wrapper that actively prohibits access to mutable things like we do with sealedClusterNetworkingConfig and sealedSessionRecordingConfig, but since there's no requirement for the R parameter to be a subset of the interface of the T anymore (that was an unenforced requirement of the GenericWatcher before we introduced ReadOnlyFunc) we can add readonly.DatabaseServer as some type that has a dedicated GetDatabaseName() string method instead. Would you be ok with that?

[rosstimothy]

Yes I would rather make readonly.Foo actually readonly and be consistent with other watchers.

[wethreetrees]

Thanks for the discussion, looking at this now.

[wethreetrees]

I've reverted the removal of the readonly DatabaseServer and implemented a pared down version with just a GetDatabaseName() method. Hopefully this is a good middle ground.

[wethreetrees]

@rosstimothy @greedy52 I've updated the test plan, please let me know if I missed anything!

[okraport]

Code looks good.

Looking over the test plan let's add one more manual test to sanity test a connection to one real DB then happy to land.

[greedy52]

I've updated the test plan, please let me know if I missed anything!

Have we tested multiple database agents that proxies the same database in manual testing? LGTM otherwise. thank you 🙏

[wethreetrees]

I've updated the test plan, please let me know if I missed anything!

Have we tested multiple database agents that proxies the same database in manual testing? LGTM otherwise. thank you 🙏

This afternoon, I spun up multiple database agents proxying the same database and confirmed connection. Added test case above as well.

[espadolini]

I like the new approach for readonly.DatabaseServer.

[backport-bot-workflows]

@wethreetrees See the table below for backport results.

Branch	Result
branch/v18	Failed