differentiate discovered resource names #28845

GavinFrazar · 2023-07-08T07:36:37Z

This PR implements discovery resource (database/kube cluster) renaming for RFD 129:

RFD 129: Avoid Discovery Resource Name Collisions #27258

TODO followup PRs:

tsh kube/app UX updates
tctl UX update

Related issue:

Add ability to differentiate auto-discovered databases #22438

Changelog: Teleport Discovery Service will automatically rename database and kube cluster resources to add a suffix consisting of additional distinguishing metadata. This change is intended to avoid resource name collisions where multiple resources have the same name across regions, accounts, clouds, etc.

lib/cloud/clients.go

lib/srv/discovery/common/renaming.go

lib/srv/discovery/fetchers/db/aws.go

[skip ci]

lib/srv/discovery/common/renaming.go

greedy52

Left two comments but overall LGTM. And thanks for the refactor!

greedy52 · 2023-07-21T16:49:12Z

lib/srv/discovery/common/renaming.go

+	suffix := makeAWSDiscoverySuffix(databaseNameValidator,
+		db.GetName(),
+		matcherType,
+		meta.Region,
+		meta.AccountID,
+	)


does this guarantee the name is unique? For e.g. an RDS instance and an RDS aroura cluster with same name in same region, same account? Similarly, Azure Redis vs Azure Redis enterprise.

ah, thanks for pointing this out!

For Azure: Redis vs Redis Enterprise names must be unique because of the way Azure sets up DNS. I double checked that Azure enforces this just now.
Redis: <name>.redis.cache.windows.net
SQL Server: <name>.database.windows.net
Postgres: <name.postgres.database.azure.com
MySQL: <name>.mysql.database.azure.com

After experimenting some more in aws console, within a region database instance name must be unique amongst other instances (regardless of whether mysql or postgres), and Aurora cluster name must be unique amongst clusters (regardless of whether mysql or postgres). Since we group these under the single matcher type "rds", it won't be guaranteed to be unique :( I'll have to figure out some way to handle this

I believe Redis enterprise uses a different suffix:

teleport/api/utils/azure/endpoints.go

Lines 123 to 130 in 2b15263

// Redis (non-Enterprise) endpoint looks like this:

// name.redis.cache.windows.net

case strings.HasSuffix(host, RedisEndpointSuffix):

return strings.TrimSuffix(host, RedisEndpointSuffix), nil

// Redis Enterprise endpoint looks like this:

// name.region.redisenterprise.cache.azure.net

case strings.HasSuffix(host, RedisEnterpriseEndpointSuffix):

But regardless, we may have a problem when multiple fetchers share a matcher.

@greedy52 coming back to this:

We are already renaming Aurora cluster dbs with a suffix: -custom, or -reader, but we use the aurora cluster identifier as the first part of the name, not the individual instances' names.

Therefore, I think we just need to distinguish between cluster dbs and instance dbs to get (most likely) unique naming for RDS/Aurora dbs.

changed the suffix to use either rds or rds-aurora.

For Redis, I tested the naming incorrectly in Azure when I tried, you're right that the names can overlap:

changed the suffix to use eitherredis or redis-enterprise

The others are not an issue due to naming uniqueness rules in Azure. Even SQL server vs managed SQL instance.

commit: 09d6033

and 3163807

lib/srv/db/watcher_test.go

lib/srv/discovery/fetchers/db/aws.go

lib/srv/discovery/fetchers/db/db.go

[skip ci]

…ases

After merging #28845, the cluster name is different and the test failed. Since the AWS E2E tests are not required, the merge happened and broke all tests. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>

GavinFrazar · 2023-08-14T20:20:40Z

for future reference - a lot of the changes from this PR will be backported to make backporting other changes possible.

The renaming functionality itself is not being backported though, and only v14+ discovery service will rename discovered resources.

* backport #28845 to branch/v13 * remove renaming of discovered resources for backport

* backport #28845 to branch/v13 * remove renaming of discovered resources for backport Co-authored-by: Gavin Frazar <gavin.frazar@goteleport.com>

GavinFrazar changed the base branch from master to gavinfrazar/tsh-resource-selection-ux July 8, 2023 07:37

GavinFrazar force-pushed the gavinfrazar/differentiate-discovered-databases branch 2 times, most recently from 36d881d to 7a0a16d Compare July 8, 2023 08:13

GavinFrazar force-pushed the gavinfrazar/tsh-resource-selection-ux branch from 88c5196 to 0abf7a0 Compare July 14, 2023 22:30

Base automatically changed from gavinfrazar/tsh-resource-selection-ux to master July 14, 2023 23:59

GavinFrazar force-pushed the gavinfrazar/differentiate-discovered-databases branch from 7a0a16d to e1d07b2 Compare July 15, 2023 04:30

GavinFrazar changed the base branch from master to gavinfrazar/move-discovery-added-labels-into-constants July 15, 2023 04:31

GavinFrazar force-pushed the gavinfrazar/differentiate-discovered-databases branch from e1d07b2 to 74271f7 Compare July 15, 2023 04:38

Base automatically changed from gavinfrazar/move-discovery-added-labels-into-constants to master July 18, 2023 18:35

implement RFD 129 cloud resource renaming

88d0c5c

GavinFrazar force-pushed the gavinfrazar/differentiate-discovered-databases branch from 74271f7 to 88d0c5c Compare July 19, 2023 00:01

GavinFrazar marked this pull request as ready for review July 19, 2023 00:01

github-actions bot added database-access Database access related issues and PRs discovery size/md labels Jul 19, 2023

github-actions bot requested review from capnspacehook and rosstimothy July 19, 2023 00:01

GavinFrazar requested a review from smallinsky July 19, 2023 15:38

smallinsky reviewed Jul 20, 2023

View reviewed changes

smallinsky requested a review from greedy52 July 20, 2023 09:53

GavinFrazar added 2 commits July 20, 2023 11:15

rename assumeRoler to stsClient

56698b0

[skip ci]

unit test makeDiscoverySuffix

c3b3822

[skip ci]

smallinsky approved these changes Jul 21, 2023

View reviewed changes

lib/srv/discovery/common/renaming.go Show resolved Hide resolved

greedy52 reviewed Jul 21, 2023

View reviewed changes

rename validator funcs

4d93110

[skip ci]

rosstimothy removed their request for review July 25, 2023 19:04

GavinFrazar added 3 commits July 28, 2023 17:51

subtype redis and RDS matchers for unique naming

09d6033

Merge branch 'master' into gavinfrazar/differentiate-discovered-datab…

6af4bb9

…ases

fix check for rds cluster vs instance

3163807

GavinFrazar requested a review from greedy52 July 29, 2023 04:45

greedy52 approved these changes Jul 31, 2023

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from capnspacehook July 31, 2023 14:37

GavinFrazar added this pull request to the merge queue Jul 31, 2023

Merged via the queue into master with commit 5c1d8d6 Jul 31, 2023
19 of 20 checks passed

GavinFrazar deleted the gavinfrazar/differentiate-discovered-databases branch July 31, 2023 18:08

tigrato mentioned this pull request Aug 1, 2023

Fix AWS EKS E2E tests #29869

Merged

GavinFrazar added the changelog label Aug 9, 2023

jentfoo mentioned this pull request Aug 14, 2023

[v13] - differentiate discovered resource names #30456

Merged

GavinFrazar mentioned this pull request Aug 15, 2023

update RFD 129 state to implemented #30479

Merged

GavinFrazar added a commit that referenced this pull request Aug 16, 2023

[v13] differentiate discovered resource names

2d3ec35

* backport #28845 to branch/v13 * remove renaming of discovered resources for backport

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

differentiate discovered resource names #28845

differentiate discovered resource names #28845

GavinFrazar commented Jul 8, 2023 •

edited

greedy52 left a comment

greedy52 Jul 21, 2023

GavinFrazar Jul 21, 2023 •

edited

greedy52 Jul 22, 2023

GavinFrazar Jul 29, 2023 •

edited

GavinFrazar commented Aug 14, 2023

	// Redis (non-Enterprise) endpoint looks like this:
	// name.redis.cache.windows.net
	case strings.HasSuffix(host, RedisEndpointSuffix):
	return strings.TrimSuffix(host, RedisEndpointSuffix), nil

	// Redis Enterprise endpoint looks like this:
	// name.region.redisenterprise.cache.azure.net
	case strings.HasSuffix(host, RedisEnterpriseEndpointSuffix):

differentiate discovered resource names #28845

differentiate discovered resource names #28845

Conversation

GavinFrazar commented Jul 8, 2023 • edited

greedy52 left a comment

Choose a reason for hiding this comment

greedy52 Jul 21, 2023

Choose a reason for hiding this comment

GavinFrazar Jul 21, 2023 • edited

Choose a reason for hiding this comment

greedy52 Jul 22, 2023

Choose a reason for hiding this comment

GavinFrazar Jul 29, 2023 • edited

Choose a reason for hiding this comment

GavinFrazar commented Aug 14, 2023

GavinFrazar commented Jul 8, 2023 •

edited

GavinFrazar Jul 21, 2023 •

edited

GavinFrazar Jul 29, 2023 •

edited