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.

Sign up for GitHub

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

Jump to bottom

CORE-2417: cloud_storage_clients: self configure s3_url_style #18107

Merged
merged 3 commits into from
May 16, 2024
Merged

CORE-2417: cloud_storage_clients: self configure s3_url_style #18107

merged 3 commits into from
May 16, 2024

Conversation

WillemKauf
Copy link
Contributor

@WillemKauf WillemKauf commented Apr 26, 2024 •
edited
Loading

Allows the s3_client to self configure its addressing style for S3 requests.

If cloud_storage_url_style is not specified (left nil in cluster config), upon start-up, the s3_client will attempt to self configure a working addressing style.

First, virtual_host addressing is tested.

  • A list_objects() request using is attempted, if cloud_storage_enable_remote_read is true.
  • If cloud_storage_enable_remote_write is enabled, put_object() and delete_object() requests are used to verify instead.

If the request is unsuccessful, the client reattempts the request with path style. If both requests fail, start-up is terminated.

JIRA Issue: https://redpandadata.atlassian.net/browse/CORE-2417

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

Features

  • redpanda will now self configure its cloud_storage_url_style for use with the internal S3 Client on start-up if it is left unspecified.

@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch from c14ecff to d26725c Compare April 26, 2024 17:33
@dotnwat dotnwat changed the title CORE-2417: cloud_storage_clients: self configure s3_url_style [CORE-2417] cloud_storage_clients: self configure s3_url_style Apr 29, 2024
@WillemKauf WillemKauf changed the title [CORE-2417] cloud_storage_clients: self configure s3_url_style CORE-2417: cloud_storage_clients: self configure s3_url_style Apr 30, 2024
@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch 3 times, most recently from ea20efe to c45484f Compare May 6, 2024 15:16
@WillemKauf WillemKauf requested review from dotnwat and andrwng May 13, 2024 17:16
@WillemKauf WillemKauf marked this pull request as ready for review May 13, 2024 17:17
@WillemKauf WillemKauf requested a review from a team as a code owner May 13, 2024 17:17
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented May 13, 2024 •
edited
Loading

new failures in https://buildkite.com/redpanda/redpanda/builds/49035#018f7330-4f02-446d-a60b-2d4d57dd3610:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_cloud_validation"

new failures in https://buildkite.com/redpanda/redpanda/builds/49035#018f734f-6e36-46a7-b856-71369b9d321f:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_cloud_validation"

new failures in https://buildkite.com/redpanda/redpanda/builds/49097#018f7762-a177-4bab-b902-b32eeceadd60:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_edit_string"

new failures in https://buildkite.com/redpanda/redpanda/builds/49097#018f7762-a179-42c2-b9d0-2c7175a9d4b4:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_export_import"

new failures in https://buildkite.com/redpanda/redpanda/builds/49097#018f7762-a17b-4f6f-b8b4-656dfa386481:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_import_validation"

new failures in https://buildkite.com/redpanda/redpanda/builds/49097#018f776b-c783-4efc-94d2-e482e0ae790e:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_import_validation"

new failures in https://buildkite.com/redpanda/redpanda/builds/49097#018f776b-c77e-4d2d-b9e7-1fcd99a737b8:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_edit_string"

new failures in https://buildkite.com/redpanda/redpanda/builds/49097#018f776b-c780-4bac-ba24-8d683c993393:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_export_import"

@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch 2 times, most recently from e9bfcfe to beea76f Compare May 14, 2024 02:14
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented May 14, 2024 •
edited
Loading

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49059#018f751c-ddb5-44c5-90ae-656056540ff1

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49112#018f780d-a6a0-43aa-8174-b62c690728e6

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49131#018f79a1-f572-47c4-bbab-69402576de3e

@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch from beea76f to be20a74 Compare May 14, 2024 12:50
Feediver1
Feediver1 previously approved these changes May 14, 2024
View reviewed changes
@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch from be20a74 to 03c7b8c Compare May 14, 2024 15:49
@andrwng andrwng requested a review from abhijat May 14, 2024 19:20
andrwng
andrwng reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

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

Pretty much LGTM

src/v/config/configuration.cc Show resolved Hide resolved
src/v/cloud_storage_clients/s3_client.h Show resolved Hide resolved
src/v/cloud_storage_clients/s3_client.cc Show resolved Hide resolved
src/v/cloud_storage_clients/configuration.cc Show resolved Hide resolved
tests/rptest/tests/cluster_self_config_test.py Show resolved Hide resolved
src/v/cloud_storage_clients/s3_client.cc Outdated Show resolved Hide resolved
src/v/cloud_storage_clients/s3_client.cc Outdated Show resolved Hide resolved
@WillemKauf
config: switch cloud_storage_url_style to std::optional
8222446 
To allow for self-configuration, the `cloud_storage_url_style` is
now an `std::optional`, and can be left unspecified in the `redpanda`
cluster config. If it is not set, it will default to `virtual_host`.
@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch from 03c7b8c to 3922017 Compare May 14, 2024 23:09
andrwng
andrwng previously approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

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

LGTM, though would be good to get a look from @dotnwat given he suggested it #17806 (comment)

@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch from 3922017 to 6885cdb Compare May 15, 2024 13:12
WillemKauf added 2 commits May 15, 2024 09:33
@WillemKauf
cloud_storage: use self configuration for url style
05b1327 
If the user does not specify `cloud_storage_url_style` in their
cluster config file (i.e leaves as `nit`), the `s3_client` will
set its url addressing style through self configuration.

The default behavior is to attempt `virtual_host` addressing first,
falling back to `path` style requests if `virtual_host` does not work.
If both are attempted and neither work, the `redpanda` start-up is terminated.
@WillemKauf
rptest: add cluster_self_config_test
f55d01e
@WillemKauf WillemKauf force-pushed the path_style_url_auto_config branch from 6885cdb to f55d01e Compare May 15, 2024 13:33
abhijat
abhijat approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@abhijat abhijat left a comment

Choose a reason for hiding this comment

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

LGTM

@piyushredpanda piyushredpanda merged commit 40c7cc3 into redpanda-data:dev May 16, 2024
18 checks passed
dotnwat
dotnwat reviewed Jul 13, 2024
View reviewed changes
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

lgtm

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

@JakeSCahill JakeSCahill JakeSCahill left review comments

@dotnwat dotnwat dotnwat left review comments

@abhijat abhijat abhijat approved these changes

@Feediver1 Feediver1 Feediver1 left review comments

@andrwng andrwng andrwng left review comments

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@WillemKauf @vbotbuildovich @dotnwat @abhijat @andrwng @JakeSCahill @Feediver1 @piyushredpanda