CORE-2157: cloud_storage_clients: add support for path-style addressing #17806

WillemKauf · 2024-04-11T15:56:33Z

This PR adds support for path-style requests to S3 cloud storage.

cloud_storage_url_style is a configurable option in redpanda. It defaults to the existing virtual_host style, but can be set to path. The s3_client will now have its headers for requests generated based on the configured URL style.

Support for ducktape has been included, and path-style requests have been verified as working using the s3_test_client_main test. Ideally, the cloud storage self-test in PR #17586 would be integrated with these changes so as to verify both virtual-hosted style requests and path style requests.

Backports Required

Release Notes

Features

Adds path-style addressing to S3 client requests.
This option is configurable through rpk cluster config edit:

# The addressing style to use for S3 requests. (one of virtual_host, path, restart required)
cloud_storage_url_style: path

Lazin · 2024-04-11T16:05:05Z

src/v/cloud_storage_clients/s3_url_style.h

@@ -0,0 +1,28 @@
+/*


I think stuff like this could go into the cloud_storage_clients/types.h.

Agreed. Fixed!

Lazin · 2024-04-11T16:06:05Z

src/v/cloud_storage_clients/s3_client.cc

+        return fmt::format("{}.{}", name(), _ap());
+    case s3_url_style::path:
+        // Host: s3.region-code.amazonaws.com
+        return fmt::format("{}", _ap());


why the bucket name is not included?

Following the S3 User Guide's examples of path-style requests, this seems to be the proper form for the host in the header.

The bucket name is instead used in the target of the header.

In commit 8edf7d5, I added some comments in the various request_creator::make_*() functions to reflect this style.

Following the S3 User Guide's examples of path-style requests, this seems to be the proper form for the host in the header.

The bucket name is instead used in the target of the header.

In commit 8edf7d5, I added some comments in the various request_creator::make_*() functions to reflect this style.

I was also confused here. But the answer is that this is make_host, not make_target, right?

Yes, correct. This is make_host.

Lazin

LGTM

Lazin · 2024-04-17T07:46:25Z

src/v/cloud_storage_clients/test_client/s3_test_client_main.cc

@@ -76,6 +77,11 @@ void cli_opts(boost::program_options::options_description_easy_init opt) {
      po::value<std::string>()->default_value("us-east-1"),
      "aws region");

+    opt(
+      "url_style",
+      po::value<std::string>()->default_value("virtual_host"),


This is good for development but we also need a proper unit-test. In the src/v/cloud_storage/remote.h we have a remote class which serves as a proxy for the code that has to interact with the cloud storage. All code paths which are downloading or uploading or deleting are using this class instead of the client. The unit-test has to show that when the path-style is enabled no REST API request uses virtual host style URL. The cloud_storage::remote has a unit-test suite. It invokes every method of the remote and checks the results using the s3_imposter. The imposter allows you to examine parameters of the request and also validate the URL. So basically, we need to have all these tests to be invoked with virtual host style URLs and also with path-style URLs.

The motivation here is that some cloud storage providers may not support one style or the other. In this situation we don't want to accidentally use the wrong style.

Looks like all existing REST API's that we're using are supported. But we can add new request in the future so some check is nice to have.

Could be done in a followup.

Could be done in a followup.

@Lazin Yes, and we are also going to wire up ducktape wiht minio running in both modes (pretty sure it supports it). You can see all teh follow up work in CORE-725.

@WillemKauf can you capture Evgeny's suggestions about additional unit testing in a new or existing jira ticket as a subtask for the overall project?

WillemKauf · 2024-04-17T14:35:13Z

Removed a commit that attempted to add virtual-host style configuration to the boto3.client.

Contrary to what the understanding was, minIO by default works with path style requests, unless MINIO_DOMAIN is set.

This environmental variable appears to be set in docker/docker_compose.yml, but ducktape tests utilizing the boto3.client are still failing to make requests with the virtual-host style to minIO.

I have removed the broken code for now, and will follow up in a future PR to add virtual-host support to minIO and boto3.client in our ducktape tests once I figure out why it is failing.

Redpanda currently supports only virtual-hosted style URLs for use with S3. Add `s3_url_style` to `cloud_storage_clients/types.h" to allow for future work towards supporting path-style URLs.

To support path-style requests in addition to the currently supported virtual-host style, 's3_url_style' has been added to 's3_configuration', as well as 'request_creator'. New methods 'make_host()' and 'make_target()' in 'request_creator' will generate host and target strings according to the url style set for 's3_client' requests (currently defaulted to virtual-host).

Allow user to set s3_url_style in their redpanda configuration file. New setting 'cloud_storage_url_style' can be set to 'virtual_host' or 'path'.

dotnwat

This looks great. There are a few small nits that looks like they could be a commit tacked onto the end. Then I think we should approve this, merge it right after we branch 24.1, and move on to the other tasks in CORE-725 like expanded testing.

BTW, please update this PR so that it isn't setup to close CORE-725 when it merges, which is the high-level task. Instead, it looks like CORE-2157 is the right one for this PR. Wdyt?

dotnwat · 2024-04-17T21:07:07Z

src/v/cloud_storage_clients/types.h

+    switch (us) {
+    case s3_url_style::virtual_host:
+        return os << "virtual_host";
+    case s3_url_style::path:
+        return os << "path";
+    }


can you stick the implementation in a .cc file?

It's a bit tricky, because the config library is going to depend on this implementation. Having it inline and accessible from the header saves us from having to adding a dependency on the cloud_storage_clients library from config (and adding this would create a cyclic dependency).

ahh, yeh, then don't worry about it. we'll fix those dependencies as we are switching modules over to be new-style. it's a lot of work no need to do that here.

dotnwat · 2024-04-17T21:25:03Z

src/v/cloud_storage_clients/s3_client.cc

+        return fmt::format("{}.{}", name(), _ap());
+    case s3_url_style::path:
+        // Host: s3.region-code.amazonaws.com
+        return fmt::format("{}", _ap());


Following the S3 User Guide's examples of path-style requests, this seems to be the proper form for the host in the header.

The bucket name is instead used in the target of the header.

In commit 8edf7d5, I added some comments in the various request_creator::make_*() functions to reflect this style.

I was also confused here. But the answer is that this is make_host, not make_target, right?

dotnwat · 2024-04-17T21:26:29Z

src/v/cloud_storage_clients/s3_client.cc

@@ -343,6 +346,30 @@ request_creator::make_delete_objects_request(
        std::make_unique<delete_objects_body>(std::move(body))}}};
 }

+std::string
+request_creator::make_host([[maybe_unused]] const bucket_name& name) const {


you do don't need maybe_unused -- it's only needed if the code generated doesn't use it (e.g. in conjunction with constexpr or templates etc...). in this case it is used in the virtual host case.

dotnwat · 2024-04-17T21:26:41Z

src/v/cloud_storage_clients/s3_client.cc

+}
+
+std::string request_creator::make_target(
+  [[maybe_unused]] const bucket_name& name, const object_key& key) const {


you can drop maybe_unused.

dotnwat · 2024-04-17T21:27:11Z

src/v/cloud_storage_clients/s3_client.h

+    std::string make_host([[maybe_unused]] const bucket_name& name) const;
+
+    std::string make_target(
+      [[maybe_unused]] const bucket_name& name, const object_key& key) const;


you can drop maybe_unused.

src/v/cloud_storage_clients/s3_client.h

dotnwat · 2024-04-17T21:30:22Z

src/v/config/configuration.cc

+  , cloud_storage_url_style(
+      *this,
+      "cloud_storage_url_style",
+      "The addressing style to use for S3 requests.",
+      {.needs_restart = needs_restart::yes,
+       .example = "virtual_host",
+       .visibility = visibility::user},
+      cloud_storage_clients::s3_url_style::virtual_host,
+      {
+        cloud_storage_clients::s3_url_style::virtual_host,
+        cloud_storage_clients::s3_url_style::path,
+      })


Should we add an auto option? From what I can tell the strategy is basically to try virtual-host and then fallback to path-style, and cache the decision.

See https://github.com/boto/botocore/blob/develop/botocore/utils.py#L1375-L1381

@Lazin what do you think?

@WillemKauf if we all think this is a good idea, let's create a new jira ticket to capture it--no need to expand the scope of this PR.

I think this is a great idea for future work! +1

dotnwat · 2024-04-17T21:34:45Z

src/v/cloud_storage_clients/s3_client.cc

+    // Virtual Style:
    // POST /?delete HTTP/1.1
-    // Host: <Bucket>.s3.amazonaws.com
+    // Host: {bucket-name}.s3.{region}.amazonaws.com
+    // Path Style:
+    // POST /{bucket-name}/?delete HTTP/1.1
+    // Host: s3.{region}.amazonaws.com
+    //


Thanks for the comments!

dotnwat · 2024-04-17T21:37:35Z

src/v/cloud_storage_clients/test_client/s3_test_client_main.cc

@@ -76,6 +77,11 @@ void cli_opts(boost::program_options::options_description_easy_init opt) {
      po::value<std::string>()->default_value("us-east-1"),
      "aws region");

+    opt(
+      "url_style",
+      po::value<std::string>()->default_value("virtual_host"),


Could be done in a followup.

@Lazin Yes, and we are also going to wire up ducktape wiht minio running in both modes (pretty sure it supports it). You can see all teh follow up work in CORE-725.

@WillemKauf can you capture Evgeny's suggestions about additional unit testing in a new or existing jira ticket as a subtask for the overall project?

Added the option for `url_style` to `s3_test_client_main.cc`, allowing for testing of both virtual-style and path-style requests. Also make sure the `url_style` is initialized correctly everywhere the `s3_configuration` object is created.

dotnwat

Approved -- let's just wait a couple days for 24.1 branching before merging.

dotnwat · 2024-04-23T18:48:15Z

/ci-repeat 1

asdf

vbotbuildovich · 2024-04-23T20:44:53Z

new failures in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c84-6c7f-4a56-ae78-5ca002edee99:

"rptest.tests.upgrade_test.UpgradeFromPriorFeatureVersionCloudStorageTest.test_rolling_upgrade.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c84-6c7c-438e-9ff7-2734d2d8c424:

"rptest.tests.read_replica_e2e_test.ReadReplicasUpgradeTest.test_upgrades.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c84-6c83-4834-9456-7d5fc0d51a8e:

"rptest.tests.license_upgrade_test.UpgradeMigratingLicenseVersion.test_license_upgrade.cloud_storage_type=CloudStorageType.S3"
"rptest.tests.rbac_upgrade_test.UpgradeMigrationCreatingDefaultRole.test_rbac_migration"

new failures in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c8a-f91c-456b-81a5-9e99f5925d52:

"rptest.tests.read_replica_e2e_test.ReadReplicasUpgradeTest.test_upgrades.cloud_storage_type=CloudStorageType.S3"
"rptest.tests.license_upgrade_test.UpgradeMigratingLicenseVersion.test_license_upgrade.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c8a-f919-4df8-a841-e6f389e3a455:

"rptest.tests.rbac_upgrade_test.UpgradeMigrationCreatingDefaultRole.test_rbac_migration"

new failures in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c8a-f914-4373-ae01-97032c260087:

"rptest.tests.partition_movement_test.SIPartitionMovementTest.test_cross_shard.num_to_upgrade=2.cloud_storage_type=CloudStorageType.S3"
"rptest.tests.partition_movement_test.SIPartitionMovementTest.test_shadow_indexing.num_to_upgrade=2.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c8a-f917-47dc-9d33-9c5d0e34f7b9:

"rptest.tests.upgrade_test.UpgradeFromPriorFeatureVersionCloudStorageTest.test_rolling_upgrade.cloud_storage_type=CloudStorageType.S3"
"rptest.tests.workload_upgrade_runner_test.RedpandaUpgradeTest.test_workloads_through_releases.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48189#018f0dd7-4e8b-4c86-8133-375b14f250c7:

"rptest.tests.rbac_upgrade_test.UpgradeMigrationCreatingDefaultRole.test_rbac_migration"

new failures in https://buildkite.com/redpanda/redpanda/builds/48189#018f0ddf-027e-41c8-b10c-d4b050167d86:

"rptest.tests.rbac_upgrade_test.UpgradeMigrationCreatingDefaultRole.test_rbac_migration"

vbotbuildovich · 2024-04-23T21:23:18Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48181#018f0c8a-f917-47dc-9d33-9c5d0e34f7b9

micheleRP

lgtm from docs side!

dotnwat

changes look great, we should merge this as soon as we take care of a what looks like some related failures.

looks like a few ducktape tests are complaining about a new configuration option.

INFO  2024-04-23 20:34:59,789 [shard 0] redpanda::main - application.cc:269 - Failure during startup: std::invalid_argument (Unknown property cloud_storage_url_style)

The addressing style used by `redpanda` for ducktape tests can now be set with the variable `cloud_storage_url_style` in `SISettings`, or injected into a test similar to `cloud_storage_type` with a decorator. e.g: `@matrix(cloud_storage_url_style=['path','virtual_host'])`

dotnwat

Failures are from the 24.1 branching issue in upgrade tests, and #17920.

WillemKauf requested review from Lazin and dotnwat April 11, 2024 15:56

github-actions bot added the area/redpanda label Apr 11, 2024

WillemKauf force-pushed the path_style_url branch from fbb50b4 to f8240dd Compare April 11, 2024 15:59

Lazin reviewed Apr 11, 2024

View reviewed changes

WillemKauf force-pushed the path_style_url branch from f8240dd to 8d815c7 Compare April 11, 2024 17:14

WillemKauf requested a review from Lazin April 16, 2024 15:35

WillemKauf marked this pull request as ready for review April 16, 2024 15:46

WillemKauf force-pushed the path_style_url branch 5 times, most recently from 7727b0c to f27b536 Compare April 16, 2024 21:45

Lazin previously approved these changes Apr 17, 2024

View reviewed changes

WillemKauf dismissed Lazin’s stale review via df9d817 April 17, 2024 14:27

WillemKauf force-pushed the path_style_url branch from f27b536 to df9d817 Compare April 17, 2024 14:27

WillemKauf added 4 commits April 17, 2024 14:15

cloud_storage_clients: add s3_url_style

9f5c3e8

Redpanda currently supports only virtual-hosted style URLs for use with S3. Add `s3_url_style` to `cloud_storage_clients/types.h" to allow for future work towards supporting path-style URLs.

config: add s3_url_style to configuration options

df5b47f

Allow user to set s3_url_style in their redpanda configuration file. New setting 'cloud_storage_url_style' can be set to 'virtual_host' or 'path'.

cloud_storage_clients: add comments for path-style

7ad5b02

WillemKauf force-pushed the path_style_url branch from df9d817 to e02e84c Compare April 17, 2024 19:36

WillemKauf requested a review from a team as a code owner April 17, 2024 19:36

dotnwat reviewed Apr 17, 2024

View reviewed changes

WillemKauf changed the title ~~CORE-725: cloud_storage_clients: add support for path-style addressing~~ CORE-2157: cloud_storage_clients: add support for path-style addressing Apr 17, 2024

WillemKauf force-pushed the path_style_url branch from e02e84c to 080ae70 Compare April 18, 2024 01:01

cloud_storage_clients: nits, remove [[maybe_unused]]

2c8c094

dotnwat previously requested changes Apr 18, 2024

View reviewed changes

redpanda-data deleted a comment from vbotbuildovich Apr 23, 2024

dotnwat requested review from Lazin and dotnwat April 23, 2024 18:57

dotnwat approved these changes Apr 23, 2024

View reviewed changes

micheleRP previously approved these changes Apr 23, 2024

View reviewed changes

dotnwat requested changes Apr 23, 2024

View reviewed changes

WillemKauf dismissed micheleRP’s stale review via 5e9ac31 April 24, 2024 00:58

WillemKauf force-pushed the path_style_url branch from 1e09d37 to 5e9ac31 Compare April 24, 2024 00:58

dotnwat approved these changes Apr 24, 2024

View reviewed changes

dotnwat merged commit b9adac6 into redpanda-data:dev Apr 24, 2024
13 of 18 checks passed

andrwng mentioned this pull request May 14, 2024

CORE-2417: cloud_storage_clients: self configure s3_url_style #18107

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-2157: cloud_storage_clients: add support for path-style addressing #17806

CORE-2157: cloud_storage_clients: add support for path-style addressing #17806

WillemKauf commented Apr 11, 2024 •

edited

Loading

Lazin Apr 11, 2024

WillemKauf Apr 11, 2024

Lazin Apr 11, 2024

WillemKauf Apr 11, 2024 •

edited

Loading

dotnwat Apr 17, 2024

WillemKauf Apr 17, 2024

Lazin left a comment

Lazin Apr 17, 2024

Lazin Apr 17, 2024

Lazin Apr 17, 2024

dotnwat Apr 17, 2024

WillemKauf commented Apr 17, 2024 •

edited

Loading

dotnwat left a comment

dotnwat Apr 17, 2024

WillemKauf Apr 17, 2024 •

edited

Loading

dotnwat Apr 18, 2024

dotnwat Apr 17, 2024

dotnwat Apr 17, 2024

dotnwat Apr 17, 2024

dotnwat Apr 17, 2024

dotnwat Apr 17, 2024

WillemKauf Apr 17, 2024

dotnwat Apr 17, 2024

dotnwat Apr 17, 2024

dotnwat left a comment

dotnwat commented Apr 23, 2024

vbotbuildovich commented Apr 23, 2024 •

edited

Loading

vbotbuildovich commented Apr 23, 2024

micheleRP left a comment

dotnwat left a comment

dotnwat left a comment

CORE-2157: cloud_storage_clients: add support for path-style addressing #17806

CORE-2157: cloud_storage_clients: add support for path-style addressing #17806

Conversation

WillemKauf commented Apr 11, 2024 • edited Loading

Backports Required

Release Notes

Features

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lazin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf commented Apr 17, 2024 • edited Loading

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

dotnwat commented Apr 23, 2024

vbotbuildovich commented Apr 23, 2024 • edited Loading

vbotbuildovich commented Apr 23, 2024

micheleRP left a comment

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

WillemKauf commented Apr 11, 2024 •

edited

Loading

WillemKauf Apr 11, 2024 •

edited

Loading

WillemKauf commented Apr 17, 2024 •

edited

Loading

WillemKauf Apr 17, 2024 •

edited

Loading

vbotbuildovich commented Apr 23, 2024 •

edited

Loading