Skip to content

cloud_storage_clients: make s3 url style optional #29454

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

Open
nvartolomei wants to merge 1 commit into
base: dev
Choose a base branch
from
+29 −23
Open

cloud_storage_clients: make s3 url style optional #29454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/v/cloud_io/tests/s3_imposter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,10 @@ s3_imposter_fixture::get_object(const ss::sstring& url) const {
}

ss::sstring s3_imposter_fixture::url_base() const {
switch (conf.url_style) {
if (!conf.url_style.has_value()) {
return fmt::format("");
}
switch (*conf.url_style) {
case cloud_storage_clients::s3_url_style::virtual_host:
return fmt::format("");
case cloud_storage_clients::s3_url_style::path:
Expand Down
5 changes: 3 additions & 2 deletions src/v/cloud_storage_clients/configuration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,9 @@ struct s3_configuration : common_configuration {
std::optional<cloud_roles::public_key_str> access_key;
/// AWS secret key, optional if configuration uses temporary credentials
std::optional<cloud_roles::private_key_str> secret_key;
/// AWS URL style, either virtual-hosted-style or path-style.
s3_url_style url_style = s3_url_style::virtual_host;
/// AWS URL style, either virtual-hosted-style or path-style. Nullopt means
/// that the style needs to be determined with self-configuration.
std::optional<s3_url_style> url_style = std::nullopt;
/// Whether the s3-compatible backend is GCS. Used in the client pool to
/// select between s3_client and gcs_client at client creation time.
bool is_gcs{false};
Expand Down
35 changes: 17 additions & 18 deletions src/v/cloud_storage_clients/s3_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ request_creator::make_gcs_batch_delete_request(
}

std::string request_creator::make_host(const plain_bucket_name& name) const {
switch (_ap_style) {
switch (_ap_style.value()) {
Copy link
Contributor

@WillemKauf WillemKauf Feb 4, 2026

Choose a reason for hiding this comment

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

Ditto

case s3_url_style::virtual_host:
// Host: bucket-name.s3.region-code.amazonaws.com
return fmt::format("{}.{}", name(), _ap());
Expand All @@ -521,7 +521,7 @@ std::string request_creator::make_host(const plain_bucket_name& name) const {

std::string request_creator::make_target(
const plain_bucket_name& name, const object_key& key) const {
switch (_ap_style) {
switch (_ap_style.value()) {
Copy link
Contributor

@WillemKauf WillemKauf Feb 4, 2026

Choose a reason for hiding this comment

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

Can we add a dassert before using the raw .value(), here or elsewhere?

It seems reasonable to assume that this will always have a value in normal redpanda operations, but it would be nice to catch any issues in CI should anything change in such a way that that invariant is no longer true.

Copy link
Contributor Author

@nvartolomei nvartolomei Feb 4, 2026

Choose a reason for hiding this comment

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

This throws already but you want to crash the process instead on invariant violation?

Copy link
Contributor

@WillemKauf WillemKauf Feb 4, 2026

Choose a reason for hiding this comment

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

For CI, why not.

I don't feel super strongly so if you want to leave as is that's fine too.

case s3_url_style::virtual_host:
// Target: /homepage.html
return fmt::format("/{}", key().string());
Expand Down Expand Up @@ -907,8 +907,6 @@ s3_client::s3_client(

ss::future<result<client_self_configuration_output, error_outcome>>
s3_client::self_configure() {
auto result = s3_self_configuration_result{
.url_style = s3_url_style::virtual_host};
// Oracle cloud storage only supports path-style requests
// (https://www.oracle.com/ca-en/cloud/storage/object-storage/faq/#category-amazon),
// but self-configuration will misconfigure to virtual-host style due to a
Expand All @@ -926,17 +924,15 @@ s3_client::self_configure() {
if (
backend == model::cloud_storage_backend::oracle_s3_compat
|| backend == model::cloud_storage_backend::minio) {
result.url_style = s3_url_style::path;
co_return result;
co_return s3_self_configuration_result{.url_style = s3_url_style::path};
}

// Also handle possibly inferred backend.
auto inferred_backend = infer_backend_from_uri(_requestor._ap);
if (
inferred_backend == model::cloud_storage_backend::oracle_s3_compat
|| inferred_backend == model::cloud_storage_backend::minio) {
result.url_style = s3_url_style::path;
co_return result;
co_return s3_self_configuration_result{.url_style = s3_url_style::path};
}

// Test virtual host style addressing, fall back to path if necessary.
Expand All @@ -948,10 +944,11 @@ s3_client::self_configure() {
if (!bucket_config.value().has_value()) {
vlog(
s3_log.warn,
"Could not self-configure S3 Client, {} is not set. Defaulting to {}",
bucket_config.name(),
result.url_style);
co_return result;
"Could not self-configure S3 Client, {} is not set. Defaulting to "
"virtual_host",
bucket_config.name());
co_return s3_self_configuration_result{
.url_style = s3_url_style::virtual_host};
}

// TODO: Review this code. It is likely buggy when Remote Read Replicas are
Expand All @@ -963,12 +960,15 @@ s3_client::self_configure() {

// Test virtual_host style.
vassert(
_requestor._ap_style == s3_url_style::virtual_host,
"_ap_style should be virtual host by default before self configuration "
!_requestor._ap_style.has_value()
|| _requestor._ap_style == s3_url_style::virtual_host,
"_ap_style should be unset or virtual host before self configuration "
Copy link
Contributor

@WillemKauf WillemKauf Feb 4, 2026

Choose a reason for hiding this comment

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

This assert seems fairly useless, looking back on it. Could be removed.

Copy link
Contributor Author

@nvartolomei nvartolomei Feb 4, 2026

Choose a reason for hiding this comment

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

It makes sense to me to keep it for now. There are some footguns which this catches. Ie the fact that even though we attempt self configuring with virtual host we still connect to the server url which is also tried for path style. Ie s3.us-east2... and not bucket.s3.us-east2...

So if we were to have url style set to virtual host the path test attempt would have failed.

Some bad code here I'm trying to sidestep.

"begins");
_requestor._ap_style = s3_url_style::virtual_host;
if (co_await self_configure_test(bucket)) {
// Virtual-host style request succeeded.
co_return result;
// Virtual host style request succeeded.
co_return s3_self_configuration_result{
.url_style = s3_url_style::virtual_host};
}

// fips mode can only work in virtual_host mode, so if the above test failed
Expand All @@ -980,10 +980,9 @@ s3_client::self_configure() {

// Test path style.
_requestor._ap_style = s3_url_style::path;
result.url_style = _requestor._ap_style;
if (co_await self_configure_test(bucket)) {
// Path style request succeeded.
co_return result;
co_return s3_self_configuration_result{.url_style = s3_url_style::path};
}

// Both addressing styles failed.
Expand Down
2 changes: 1 addition & 1 deletion src/v/cloud_storage_clients/s3_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class request_creator {

access_point_uri _ap;

s3_url_style _ap_style;
std::optional<s3_url_style> _ap_style;
/// Applies credentials to http requests by adding headers and signing
/// request payload. Shared pointer so that the credentials can be rotated
/// through the client pool.
Expand Down
5 changes: 4 additions & 1 deletion src/v/redpanda/tests/fixture.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,10 @@ void redpanda_thread_fixture::configure(
.set_value(std::make_optional(s3_config->server_addr.host()));
config.get("cloud_storage_url_style")
.set_value(std::make_optional([&] {
switch (s3_config->url_style) {
if (!s3_config->url_style.has_value()) {
return config::s3_url_style::virtual_host;
}
switch (*s3_config->url_style) {
case cloud_storage_clients::s3_url_style::virtual_host:
return config::s3_url_style::virtual_host;
case cloud_storage_clients::s3_url_style::path:
Expand Down