Skip to content

Validate unknown AWS regions from discovery matchers#31533

Merged
greedy52 merged 2 commits intomasterfrom
STeve/discovery_il-central-1
Sep 13, 2023
Merged

Validate unknown AWS regions from discovery matchers#31533
greedy52 merged 2 commits intomasterfrom
STeve/discovery_il-central-1

Conversation

@greedy52
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 commented Sep 6, 2023

Changelog: Support discovery for new AWS region il-central-1

As discussed in #31217, older branches may not get AWS SDK update to refresh the region list.

This change uses a regex to validate AWS regions from AWS discovery matchers. If a region is "valid" but "unknown", print a warning.

This avoids updating older branches for new regions.

Tested by cherry-picking the commit onto branch/v13 and enabling RDS discovery for il-central-1 in our dev account.

Comment thread lib/utils/aws/region.go Outdated
Comment thread lib/utils/aws/region.go
Comment thread lib/utils/aws/region.go Outdated
@marcoandredinis
Copy link
Copy Markdown
Contributor

@kimlisa We should also update this list


cc @rudream

@greedy52
Copy link
Copy Markdown
Contributor Author

@tigrato @smallinsky any thoughts on this approach? Or do we prefer a hard-coded list like the UI and IAM join? Then we can have a make script to upgrade them together.

Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

I like this approach. But I have one concern:
Are we sure that DB agent/kube agent/discovery service will properly handle unknown region on their side without updating GO AWS SDK ?
For instance if we will allow to use "unknown region by SDK in teleport v11 the request most likely will fail in Teleport Discovery Service due to updated v11 GO AWS SDK.

Copy link
Copy Markdown
Contributor

@tigrato tigrato left a comment

Choose a reason for hiding this comment

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

I like this approach. But I have one concern: Are we sure that DB agent/kube agent/discovery service will properly handle unknown region on their side without updating GO AWS SDK ? For instance if we will allow to use "unknown region by SDK in teleport v11 the request most likely will fail in Teleport Discovery Service due to updated v11 GO AWS SDK.

I am a bit reticent about this.
It solves the issue but it creates inconsistencies when we don't define the region - new regions won't be added - vs when we manually define it

@greedy52
Copy link
Copy Markdown
Contributor Author

greedy52 commented Sep 12, 2023

I like this approach. But I have one concern: Are we sure that DB agent/kube agent/discovery service will properly handle unknown region on their side without updating GO AWS SDK ? For instance if we will allow to use "unknown region by SDK in teleport v11 the request most likely will fail in Teleport Discovery Service due to updated v11 GO AWS SDK.

I've tested this fix on v13 where the SDK does not support il-central-1, and everything works fine for an RDS discovery in this new region (I've opened the new region in dev account).

Regular SDK calls can make endpoints with "unknown" regions, as long endpoints.StrictMatchingOption is not specified:

func TestEndpoint(t *testing.T) {                                
    resolver := endpoints.DefaultResolver()

    t.Run("default SDK behavior", func(t *testing.T) {
        endpoint, err := resolver.EndpointFor(rds.ServiceName, "xx-west-5")
        require.NoError(t, err)
        require.Equal(t, "https://rds.xx-west-5.amazonaws.com", endpoint.URL)
    })

    t.Run("strict", func(t *testing.T) {
        _, err := resolver.EndpointFor(rds.ServiceName, "xx-west-5", endpoints.StrictMatchingOption)
        // UnknownEndpointError: could not resolve endpoint
        // partition: "all partitions", service: "rds", region: "xx-west-5"
        require.Error(t, err)
    })
}

It solves the issue but it creates inconsistencies when we don't define the region - new regions won't be added - vs when we manually define it

It does create inconsistency. Consistency vs ease-of-use vs security. I don't mind doing the hard-coded list too. Or even an RFD?

@greedy52 greedy52 added this pull request to the merge queue Sep 13, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 13, 2023
@greedy52 greedy52 added this pull request to the merge queue Sep 13, 2023
Merged via the queue into master with commit 76e5747 Sep 13, 2023
@greedy52 greedy52 deleted the STeve/discovery_il-central-1 branch September 13, 2023 16:13
@public-teleport-github-review-bot
Copy link
Copy Markdown

@greedy52 See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Create PR
branch/v14 Create PR

greedy52 added a commit that referenced this pull request Sep 13, 2023
* Validate unknown AWS regions from discovery matchers

* move and simply regex check
github-merge-queue Bot pushed a commit that referenced this pull request Sep 14, 2023
* Validate unknown AWS regions from discovery matchers (#31533)

* Validate unknown AWS regions from discovery matchers

* move and simply regex check

* remove il-centrla-1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants