Skip to content

Add uri flag for s3 path style addressing configuration#49835

Merged
EdwardDowling merged 2 commits intomasterfrom
edwarddowling/path-style
Feb 10, 2025
Merged

Add uri flag for s3 path style addressing configuration#49835
EdwardDowling merged 2 commits intomasterfrom
edwarddowling/path-style

Conversation

@EdwardDowling
Copy link
Copy Markdown
Contributor

@EdwardDowling EdwardDowling commented Dec 5, 2024

Add ability to disable path-style S3 access for third-party endpoints.

part of: #48451
Fixes https://github.com/gravitational/customer-sensitive-requests/issues/346

changelog: Add ability to disable path-style S3 access for third-party endpoints.

@EdwardDowling EdwardDowling marked this pull request as ready for review December 11, 2024 17:34
@github-actions github-actions Bot requested review from hugoShaka and tcsc December 11, 2024 17:34
@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log size/sm labels Dec 11, 2024
@programmerq
Copy link
Copy Markdown
Contributor

This change looks good to me, but should probably have the storage backend doc reflect the change: https://github.com/gravitational/teleport/blob/master/docs/pages/reference/backends.mdx#s3-session-recordings

I think we'd only need to add this bullet point underneath the endpoint description.

  • use_s3_path_style - Whether to use path-style instead of virtual-host-style URLs for the bucket. Only applies when a custom endpoint is set. Defaults to true when unset. If used without a custom endpoint set, this option has no effect.

I'm happy to add this doc commit to the branch for this PR if it's helpful.

@EdwardDowling
Copy link
Copy Markdown
Contributor Author

This change looks good to me, but should probably have the storage backend doc reflect the change: https://github.com/gravitational/teleport/blob/master/docs/pages/reference/backends.mdx#s3-session-recordings

I think we'd only need to add this bullet point underneath the endpoint description.

* `use_s3_path_style` - Whether to use path-style instead of virtual-host-style URLs for the bucket. Only applies when a custom `endpoint` is set. Defaults to `true` when unset. If used without a custom `endpoint` set, this option has no effect.

I'm happy to add this doc commit to the branch for this PR if it's helpful.

Added it in there

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 2, 2025

Amplify deployment status

Branch Commit Job ID Status Preview Updated (UTC)
edwarddowling/path-style 5c5cd76 6 ✅SUCCEED edwarddowling-path-style 2025-02-10 16:09:47

@EdwardDowling
Copy link
Copy Markdown
Contributor Author

@hugoShaka @tcsc Can you take a look at this when you get a chance

@hugoShaka
Copy link
Copy Markdown
Contributor

Can we do a real-world test against an s3 backend not supporting path-style adressing and see if we can make teleport work? Or maybe send a dev build to the affected customer?

@milos-teleport milos-teleport added the c-gj Internal Customer Reference label Jan 13, 2025
Comment thread docs/pages/reference/backends.mdx Outdated
Comment thread lib/events/s3sessions/s3handler.go Outdated
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm after you fix the docs

Comment thread docs/pages/reference/backends.mdx Outdated
Comment thread lib/events/s3sessions/s3handler.go
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Please make sure to retest everything again since we've inverted the flag here and if everything is ok let's ship it.

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from tcsc January 17, 2025 18:23
@EdwardDowling
Copy link
Copy Markdown
Contributor Author

Confirmation that the dev build worked for the customer here

@EdwardDowling EdwardDowling added this pull request to the merge queue Feb 10, 2025
Merged via the queue into master with commit e8435a5 Feb 10, 2025
@EdwardDowling EdwardDowling deleted the edwarddowling/path-style branch February 10, 2025 17:07
@public-teleport-github-review-bot
Copy link
Copy Markdown

@EdwardDowling See the table below for backport results.

Branch Result
branch/v15 Failed
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 c-gj Internal Customer Reference size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants