Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

differentiate discovered resource names #28845

Merged
merged 7 commits into from
Jul 31, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/cloud/clients.go
Original file line number Diff line number Diff line change
Expand Up @@ -578,9 +578,9 @@ func (c *cloudClients) getAWSSessionForRegion(region string) (*awssession.Sessio

// getAWSSessionForRole returns AWS session for the specified region and role.
func (c *cloudClients) getAWSSessionForRole(ctx context.Context, region string, options awsAssumeRoleOpts) (*awssession.Session, error) {
assumeRoler := sts.New(options.baseSession)
cacheKey := fmt.Sprintf("Region[%s]:RoleARN[%s]:ExternalID[%s]", region, options.assumeRoleARN, options.assumeRoleExternalID)
return utils.FnCacheGet(ctx, c.awsSessionsCache, cacheKey, func(ctx context.Context) (*awssession.Session, error) {
assumeRoler := sts.New(options.baseSession)
GavinFrazar marked this conversation as resolved.
Show resolved Hide resolved
return newSessionWithRole(ctx, assumeRoler, region, options.assumeRoleARN, options.assumeRoleExternalID)
})
}
Expand Down
49 changes: 28 additions & 21 deletions lib/integrations/awsoidc/listdatabases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/aws/aws-sdk-go-v2/service/rds"
rdsTypes "github.com/aws/aws-sdk-go-v2/service/rds/types"
"github.com/google/go-cmp/cmp"
"github.com/gravitational/trace"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -187,12 +188,14 @@ func TestListDatabases(t *testing.T) {
Name: "my-db",
Description: "RDS instance in ",
Labels: map[string]string{
"account-id": "123456789012",
"endpoint-type": "instance",
"engine": "postgres",
"engine-version": "",
"region": "",
"status": "available",
"account-id": "123456789012",
"endpoint-type": "instance",
"engine": "postgres",
"engine-version": "",
"region": "",
"status": "available",
"teleport.dev/cloud": "AWS",
"teleport.dev/origin": "cloud",
},
},
types.DatabaseSpecV3{
Expand All @@ -208,7 +211,7 @@ func TestListDatabases(t *testing.T) {
},
)
require.NoError(t, err)
require.Equal(t, expectedDB, ldr.Databases[0])
require.Empty(t, cmp.Diff(expectedDB, ldr.Databases[0]))
},
errCheck: noErrorFunc,
},
Expand Down Expand Up @@ -250,12 +253,14 @@ func TestListDatabases(t *testing.T) {
Name: "my-db",
Description: "RDS instance in ",
Labels: map[string]string{
"account-id": "123456789012",
"endpoint-type": "instance",
"engine": "postgres",
"engine-version": "",
"region": "",
"status": "available",
"account-id": "123456789012",
"endpoint-type": "instance",
"engine": "postgres",
"engine-version": "",
"region": "",
"status": "available",
"teleport.dev/cloud": "AWS",
"teleport.dev/origin": "cloud",
},
},
types.DatabaseSpecV3{
Expand All @@ -271,7 +276,7 @@ func TestListDatabases(t *testing.T) {
},
)
require.NoError(t, err)
require.Equal(t, expectedDB, ldr.Databases[0])
require.Empty(t, cmp.Diff(expectedDB, ldr.Databases[0]))
},
errCheck: noErrorFunc,
},
Expand Down Expand Up @@ -300,12 +305,14 @@ func TestListDatabases(t *testing.T) {
Name: "my-dbc",
Description: "Aurora cluster in ",
Labels: map[string]string{
"account-id": "123456789012",
"endpoint-type": "primary",
"engine": "aurora-postgresql",
"engine-version": "",
"region": "",
"status": "available",
"account-id": "123456789012",
"endpoint-type": "primary",
"engine": "aurora-postgresql",
"engine-version": "",
"region": "",
"status": "available",
"teleport.dev/cloud": "AWS",
"teleport.dev/origin": "cloud",
},
},
types.DatabaseSpecV3{
Expand All @@ -322,7 +329,7 @@ func TestListDatabases(t *testing.T) {
},
)
require.NoError(t, err)
require.Equal(t, expectedDB, ldr.Databases[0])
require.Empty(t, cmp.Diff(expectedDB, ldr.Databases[0]))
},
errCheck: noErrorFunc,
},
Expand Down
4 changes: 2 additions & 2 deletions lib/services/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -925,8 +925,8 @@ func newElastiCacheDatabase(cluster *elasticache.ReplicationGroup, endpoint *ela
})
}

// NewDatabaseFromOpenSearchDomain creates a database resource from an OpenSearch domain.
func NewDatabaseFromOpenSearchDomain(domain *opensearchservice.DomainStatus, tags []*opensearchservice.Tag) (types.Databases, error) {
// NewDatabasesFromOpenSearchDomain creates database resources from an OpenSearch domain.
func NewDatabasesFromOpenSearchDomain(domain *opensearchservice.DomainStatus, tags []*opensearchservice.Tag) (types.Databases, error) {
var databases types.Databases

if aws.StringValue(domain.Endpoint) != "" {
Expand Down
3 changes: 3 additions & 0 deletions lib/srv/db/watcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/gravitational/teleport/lib/cloud/mocks"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/services"
discovery "github.com/gravitational/teleport/lib/srv/discovery/common"
)

// TestWatcher verifies that database server properly detects and applies
Expand Down Expand Up @@ -263,6 +264,7 @@ func TestWatcherCloudFetchers(t *testing.T) {
redshiftServerlessDatabase.SetStatusAWS(redshiftServerlessDatabase.GetAWS())
setDiscoveryGroupLabel(redshiftServerlessDatabase, "")
redshiftServerlessDatabase.SetOrigin(types.OriginCloud)
discovery.ApplyAWSDatabaseNameSuffix(redshiftServerlessDatabase, services.AWSMatcherRedshiftServerless)
// Test an Azure fetcher.
azSQLServer, azSQLServerDatabase := makeAzureSQLServer(t, "discovery-azure", "group")
setDiscoveryGroupLabel(azSQLServerDatabase, "")
Expand Down Expand Up @@ -375,5 +377,6 @@ func makeAzureSQLServer(t *testing.T, name, group string) (*armsql.Server, types
}
database, err := services.NewDatabaseFromAzureSQLServer(server)
require.NoError(t, err)
discovery.ApplyAzureDatabaseNameSuffix(database, services.AzureMatcherSQLServer)
GavinFrazar marked this conversation as resolved.
Show resolved Hide resolved
return server, database
}
238 changes: 238 additions & 0 deletions lib/srv/discovery/common/renaming.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
/*
Copyright 2023 Gravitational, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package common

import (
"fmt"
"regexp"
"strings"

"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/services"
)

// ApplyAWSDatabaseNameSuffix applies the AWS Database Discovery name suffix to
// the given database.
// Format: <name>-<matcher type>-<region>-<account ID>.
func ApplyAWSDatabaseNameSuffix(db types.Database, matcherType string) {
if hasOverrideLabel(db, types.AWSDatabaseNameOverrideLabels...) {
// don't rewrite manual name override.
return
}
meta := db.GetAWS()
suffix := makeAWSDiscoverySuffix(databaseNameValidator,
db.GetName(),
matcherType,
meta.Region,
meta.AccountID,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Redis enterprise uses a different suffix:

Screenshot 2023-07-22 at 3 13 32 PM

// 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

applyDiscoveryNameSuffix(db, suffix)
}

// ApplyAzureDatabaseNameSuffix applies the Azure Database Discovery name suffix
// to the given database.
// Format: <name>-<matcher type>-<region>-<resource group>-<subscription ID>.
func ApplyAzureDatabaseNameSuffix(db types.Database, matcherType string) {
if hasOverrideLabel(db, types.AzureDatabaseNameOverrideLabel) {
// don't rewrite manual name override.
return
}
region, _ := db.GetLabel(types.DiscoveryLabelRegion)
group, _ := db.GetLabel(types.DiscoveryLabelAzureResourceGroup)
subID, _ := db.GetLabel(types.DiscoveryLabelAzureSubscriptionID)
suffix := makeAzureDiscoverySuffix(databaseNameValidator,
db.GetName(),
matcherType,
region,
group,
subID,
)
applyDiscoveryNameSuffix(db, suffix)
}

// ApplyEKSNameSuffix applies the AWS EKS Discovery name suffix to the given
// kube cluster.
// Format: <name>-eks-<region>-<account ID>.
func ApplyEKSNameSuffix(cluster types.KubeCluster) {
if hasOverrideLabel(cluster, types.AWSKubeClusterNameOverrideLabels...) {
// don't rewrite manual name override.
return
}
meta := cluster.GetAWSConfig()
suffix := makeAWSDiscoverySuffix(kubeClusterNameValidator,
cluster.GetName(),
services.AWSMatcherEKS,
meta.Region,
meta.AccountID,
)
applyDiscoveryNameSuffix(cluster, suffix)
}

// ApplyAKSNameSuffix applies the Azure AKS Discovery name suffix to the given
// kube cluster.
// Format: <name>-aks-<region>-<resource group>-<subscription ID>.
func ApplyAKSNameSuffix(cluster types.KubeCluster) {
if hasOverrideLabel(cluster, types.AzureKubeClusterNameOverrideLabel) {
// don't rewrite manual name override.
return
}
meta := cluster.GetAzureConfig()
region, _ := cluster.GetLabel(types.DiscoveryLabelRegion)
suffix := makeAzureDiscoverySuffix(kubeClusterNameValidator,
cluster.GetName(),
services.AzureMatcherKubernetes,
region,
meta.ResourceGroup,
meta.SubscriptionID,
)
applyDiscoveryNameSuffix(cluster, suffix)
}

// ApplyGKENameSuffix applies the GCP GKE Discovery name suffix to the given
// kube cluster.
// Format: <name>-gke-<location>-<project ID>.
func ApplyGKENameSuffix(cluster types.KubeCluster) {
if hasOverrideLabel(cluster, types.GCPKubeClusterNameOverrideLabel) {
// don't rewrite manual name override.
return
}
meta := cluster.GetGCPConfig()
suffix := makeGCPDiscoverySuffix(kubeClusterNameValidator,
cluster.GetName(),
services.GCPMatcherKubernetes,
meta.Location,
meta.ProjectID,
)
applyDiscoveryNameSuffix(cluster, suffix)
}

// hasOverrideLabel is a helper func to check for presence of a name override
// label.
func hasOverrideLabel(r types.ResourceWithLabels, overrideLabels ...string) bool {
for _, label := range overrideLabels {
if val, ok := r.GetLabel(label); ok && val != "" {
return true
}
}
return false
}

// makeAWSDiscoverySuffix makes a discovery suffix for AWS resources, of the
// form <matcher type>-<region>-<account ID>.
func makeAWSDiscoverySuffix(fn suffixValidatorFn, name, matcherType, region, accountID string) string {
return makeDiscoverySuffix(fn, name, matcherType, region, accountID)
}

// makeAzureDiscoverySuffix makes a discovery suffix for Azure resources, of the
// form <matcher type>-<region>-<resource group>-<subscription ID>.
func makeAzureDiscoverySuffix(fn suffixValidatorFn, name, matcherType, region, resourceGroup, subscriptionID string) string {
return makeDiscoverySuffix(fn, name, matcherType, region, resourceGroup, subscriptionID)
}

// makeGCPDiscoverySuffix makes a discovery suffix for GCP resources, of the
// form <matcher type>-<location>-<project ID>.
func makeGCPDiscoverySuffix(fn suffixValidatorFn, name, matcherType, location, projectID string) string {
return makeDiscoverySuffix(fn, name, matcherType, location, projectID)
}

// applyDiscoveryNameSuffix takes a resource with labels and a suffix to add
// to the name, then modifies the resource to add a label containing the
// original name and sets a new name with the suffix appended.
// This function does nothing if the suffix is empty.
func applyDiscoveryNameSuffix(resource types.ResourceWithLabels, suffix string) {
if suffix == "" {
// nop if suffix parts aren't given.
return
}
discoveredName := resource.GetName()
labels := resource.GetStaticLabels()
if labels == nil {
labels = make(map[string]string)
}
// set the originally discovered name as a label.
labels[types.DiscoveredNameLabel] = discoveredName
resource.SetStaticLabels(labels)
// update the resource name with a suffix.
resource.SetName(fmt.Sprintf("%s-%s", discoveredName, suffix))
}

// suffixValidatorFn is a func that validates a suffix.
type suffixValidatorFn func(string) error

// databaseNameValidator is a suffixValidatorFn for databases.
func databaseNameValidator(part string) error {
// validate the suffix part adding a simple stub prefix "a" and
// validating it as a full database name.
return types.ValidateDatabaseName("a" + part)
}

// kubeClusterNameValidator is a suffixValidatorFn for kube clusters.
func kubeClusterNameValidator(part string) error {
// validate the suffix part adding a simple stub prefix "a" and
// validating it as a full kube cluster name.
return types.ValidateKubeClusterName("a" + part)
}

// makeDiscoverySuffix takes a list of suffix parts and a suffix validator func,
// sanitizes each part and checks it for validity, then joins the result with
// hyphens "-".
func makeDiscoverySuffix(validatorFn suffixValidatorFn, name string, parts ...string) string {
// convert name to lower case for substring checking.
name = strings.ToLower(name)
var out []string
for _, part := range parts {
part = sanitizeSuffixPart(part)
// skip blank parts.
if part == "" {
continue
}
// skip redundant parts.
if strings.Contains(name, strings.ToLower(part)) {
continue
}
// skip invalid parts.
if err := validatorFn(part); err != nil {
GavinFrazar marked this conversation as resolved.
Show resolved Hide resolved
continue
}
out = append(out, part)
}
if len(out) == 0 {
return ""
}
suffix := strings.Join(out, "-")
if err := validatorFn(suffix); err != nil {
// sanity check for the full suffix - if it's somehow invalid, then
// discard it.
return ""
}
return suffix
}

// sanitizeSuffixPart cleans a suffix part to remove all whitespace, repeating
// and leading/trailing hyphens, and converts the string to all lowercase.
func sanitizeSuffixPart(part string) string {
// convert all whitespace to "-".
part = strings.ReplaceAll(part, " ", "-")
// compact repeating "-" into a single "-", e.g. "a--b----" => "a-b-".
part = repeatingHyphensRegexp.ReplaceAllLiteralString(part, "-")
// trim leading/trailing hyphens out.
part = strings.Trim(part, "-")
return part
}

// repeatingHyphensRegexp represents a repeating hyphen chars pattern.
var repeatingHyphensRegexp = regexp.MustCompile(`--+`)