Skip to content

Sanitize aws service custom endpoints#48992

Merged
rosstimothy merged 1 commit intomasterfrom
tross/aws_endpoint_scheme
Nov 14, 2024
Merged

Sanitize aws service custom endpoints#48992
rosstimothy merged 1 commit intomasterfrom
tross/aws_endpoint_scheme

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

There is a slight behavior change in aws-sdk-go-v2 regarding custom endpoints. The new sdk requires the endpoints to be a valid URI, and the legacy sdk would allow a hostname only enpoint and automatically apply a scheme. To prevent breaking existing configurations when upgrading to v17, we now have to apply the same logic to endpoints manually that the legacy sdk used to apply for us.

Closes #48760.

@rosstimothy rosstimothy added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Nov 14, 2024
@aws-amplify-us-west-2
Copy link
Copy Markdown

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48992.d3pp5qlev8mo18.amplifyapp.com

Copy link
Copy Markdown
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

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

LGTM, a few files are missing licenses

"regexp"
)

var schemeRegex = regexp.MustCompile("^([^:]+)://")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Do we really have to write our own regular expression? There is no function in the standard packages doing that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that we could do a url.Parse and inspect the scheme, I believe that would fail if someone was using an ip:port instead of a dns name.

There is a slight behavior change in aws-sdk-go-v2 regarding custom
endpoints. The new sdk requires the endpoints to be a valid URI, and
the legacy sdk would allow a hostname only enpoint and automatically
apply a scheme. To prevent breaking existing configurations when
upgrading to v17, we now have to apply the same logic to endpoints
manually that the legacy sdk used to apply for us.

Closes #48760.
@rosstimothy rosstimothy force-pushed the tross/aws_endpoint_scheme branch from 1f8dd5d to 80c71ab Compare November 14, 2024 18:06
@rosstimothy rosstimothy marked this pull request as ready for review November 14, 2024 18:35
@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log size/md labels Nov 14, 2024
@github-actions github-actions Bot requested review from bl-nero and kimlisa November 14, 2024 18:35
@rosstimothy rosstimothy requested a review from r0mant November 14, 2024 18:47
@rosstimothy rosstimothy added this pull request to the merge queue Nov 14, 2024
Merged via the queue into master with commit b439b03 Nov 14, 2024
@rosstimothy rosstimothy deleted the tross/aws_endpoint_scheme branch November 14, 2024 19:44
@public-teleport-github-review-bot
Copy link
Copy Markdown

@rosstimothy See the table below for backport results.

Branch Result
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-log Issues related to Teleports Audit Log backport/branch/v17 merge-for-v17 no-changelog Indicates that a PR does not require a changelog entry size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Teleport does not seem to honour the s3 endpoint parameter

3 participants