iceberg: support for SigV4 and AWS Glue REST catalog #26363
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dd30b81 to
e7dc9fd
Compare
Retry command for Build#66933please wait until all jobs are finished before running the slash command |
CI test resultstest results on build#67011
test results on build#67070
test results on build#68191
|
rockwotj
reviewed
Jun 7, 2025
src/v/http/request_builder.h
Outdated
Comment on lines
80
to
81
| request_builder& with_apply_credentials( | ||
| ss::lw_shared_ptr<cloud_roles::apply_credentials> apply_creds); |
There was a problem hiding this comment.
Why does our http client have a bunch of aws specific logic in it? Is there a reason this can't go in a layer above this one or some wrapper around our client?
There was a problem hiding this comment.
The reason was so the builder could finalize the http request by doing the signing, but you're right that it's lousy to add this code here. I'll change it.
There was a problem hiding this comment.
@andrwng had taken steps to split out the http client during the iceberg project, but i suspect that a full separation of concerns would have been too much to make any progress on the separation. it would be great to fully separate things.
nvartolomei
reviewed
Jun 9, 2025
There was a problem hiding this comment.
This is too tangled because we assumed that the only cloud service we'll use is s3 and all our configuration is tied to s3...
It would be cool to break the tight dependency between auth_refresh_bg_op and s3. We can probably glance over this short term if there is urgency in shipping it. @andrwng ok to you?
Adding cloud_roles dependency to http client which Tyler raised must be still addressed imho.
src/v/cloud_roles/types.h
Outdated
| @@ -83,6 +83,7 @@ struct gcp_credentials { | |||
|
|
|||
| std::ostream& operator<<(std::ostream& os, const gcp_credentials& gc); | |||
|
|
|||
| using aws_service_name = named_type<ss::sstring, struct _aws_service_name>; | |||
There was a problem hiding this comment.
nit: Any reason this is prefixed with an underscore? That's unusual for Redpanda codebase.
Do you want to propose a new convention? It is probably better done outside of this PR.
There was a problem hiding this comment.
That was a goof that got changed later. I will move the fix so the leading underscore is not present in any commit.
| @@ -142,14 +145,15 @@ SEASTAR_THREAD_TEST_CASE(test_signature_computation_4) { | |||
| cloud_roles::private_key_str secret_key( | |||
| "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"); | |||
| cloud_roles::aws_region_name region("us-east-1"); | |||
| cloud_roles::aws_service_name service("s3"); | |||
There was a problem hiding this comment.
- Add a test that covers non-"s3" service name.
Some assurance that the implementation is not hard coded.
| {.needs_restart = needs_restart::yes, .visibility = visibility::user}, | ||
| "redpanda-iceberg-catalog") | ||
| , iceberg_rest_catalog_base_location( |
There was a problem hiding this comment.
How does this work? I'd expect this to be per-topic 🤔. Can you help me understand the following please:
If you set this to /foo/bar and you have topic named baz, what the s3 layout after data and metadata has been written?
There was a problem hiding this comment.
The base location is the root at which the catalog will manage the data and metadata for Iceberg tables. So, for a topic test6 and a base location of s3://wdberkeley-test, the layout looks like
s3://wdberkeley-test # base location
/redpanda # namespace
/test6 # table name (named after the topic)
/data
/metadata
For all other catalogs we have supported thus far, the catalog will pick a location for the table. The client doesn't have to choose. However, Glue rejects create table requests as malformed if they do not include a (non-empty) location.
|
|
||
| namespace datalake { | ||
|
|
||
| credential_manager::credential_manager() = default; |
There was a problem hiding this comment.
This is too tied to glue and aws to be called generically credential_manager. I can foresee a case where we might want to extend it so probably is fine. But then, can we extract the aws glue specific logic into a function at least (i.e. in anonymous namespace)?
There was a problem hiding this comment.
The idea is that this can be extended, but it's main purpose at the moment is as a home for the background refresh op that keeps AWS credentials up to date. For example, I think we could refactor the catalog client so OAuth credentials are refreshed by the manager.
It's definitely too Glue-specific at the moment. We also have the problem that aws_sigv4 authentication implies the catalog is AWS Glue. Untangling that would mean we need to infer or provide configuration for the AWS service being used with aws_sigv4, as the service name is part of sigv4's signing key. It's abstractly the right thing to do but I'm not sure if there's another option in practice.
I'm interested to hear what you and @andrwng think about configuring the AWS service. In the meantime I'll refactor the manager a bit so it's more obvious how it would generalize to other credentials.
There was a problem hiding this comment.
I don't mind having something like this to encapsulate these details, but I think the public interface could be refined a bit. As it feels like we're leaking some sigv4 specific details throughout usages of the catalog clients
|
This also needs some solid tests for newly introduce credential manager to
make sure it has least coverage under unit tests (debug mode+sanitizers).
…On Sat, 7 Jun 2025 at 03:07, Tyler Rockwood ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/v/http/request_builder.h
<#26363 (comment)>
:
> + request_builder& with_apply_credentials(
+ ss::lw_shared_ptr<cloud_roles::apply_credentials> apply_creds);
Why does our http client have a bunch of aws specific logic in it? Is
there a reason this can't go in a layer above this one or some wrapper
around our client?
—
Reply to this email directly, view it on GitHub
<#26363 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEETWOGYLIUIXE657I4NCT3CJCOJAVCNFSM6AAAAAB6UTDX7CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSMBWGU3TGNJVGY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Retry command for Build#67070please wait until all jobs are finished before running the slash command |
|
/ci-repeat 1 |
andrwng
reviewed
Jun 11, 2025
There was a problem hiding this comment.
Nice! I think this generally makes sense, but I've suggested one restructuring that I hope would make it feel a bit more extendible and less invasive
To Nicolae's point, it does read as a little odd to have the credential_manager use the auth_refresh_bg_op, which is used by all cloud types, when really we only care about sigv4 and AWS. I think it'd help make it read more cohesively to move the initialization logic from the catalog factory into the credential manager
src/v/datalake/credential_manager.cc
Outdated
| // TODO: Implement OAuth2 auth refresh via the bg op. | ||
| return std::nullopt; | ||
| case config::datalake_catalog_auth_mode::aws_sigv4: | ||
| // TODO: Perhaps this setting shouldn't entail using AWS glue? |
There was a problem hiding this comment.
I guess if this ever becomes a problem, it isn't too hard to add another cluster property, some iceberg_rest_catalog_sigv4_service_name (at least, there's precedence for these auth-type-specific configs, like the token for bearer or key+secret for auth)
One example is that s3tables offers a different REST endpoint that is separate from Glue, but also uses sigv4. If/when we support that, I imagine this code is all reusable except for the service name?
Comment on lines
+287
to
+279
| case config::datalake_catalog_auth_mode::aws_sigv4: { | ||
| // AWS SigV4 signing will be handled after build() is called |
There was a problem hiding this comment.
Maybe a dumb question, but I'm curious if its possible or reasonable for a service to require both oauth and sigv4 signing?
src/v/datalake/credential_manager.h
Outdated
Comment on lines
35
to
37
| // Get the current credentials applier, which may be updated asynchronously | ||
| // by the background refresh operation | ||
| ss::lw_shared_ptr<cloud_roles::apply_credentials> get_credentials() const; |
There was a problem hiding this comment.
WDYT about having the credential manager sign requests/payloads and having its API be something like:
ss::future<result<...>> maybe_add_token(request&);
ss::future<result<...>> maybe_sign(const std::optional<iobuf>&, request&);
...which would transparently do any background credentials fetching (i.e. the wait_for_credentials_if_needed()) if needed. As is, we're exposing a bunch of credentials details into the catalog factory (e.g. calling wait_for_credentials_if_needed(), and requiring application to call get_credentials()). It feels like if we plumb the credential_manager to the catalog_client and clients, it'd feel a bit less leaky
|
|
||
| namespace datalake { | ||
|
|
||
| credential_manager::credential_manager() = default; |
There was a problem hiding this comment.
I don't mind having something like this to encapsulate these details, but I think the public interface could be refined a bit. As it feels like we're leaking some sigv4 specific details throughout usages of the catalog clients
|
|
||
| // The background refresh op will propagate credentials. | ||
| // Static credentials will be available immediately. | ||
| while (!apply_credentials_) { |
There was a problem hiding this comment.
This needs an abort source. Or see my other suggestion about moving this into the credential manager -- if you go through with that, then there's a nice natural abort source to use
2f2d192 to
5badd6b
Compare
dotnwat
reviewed
Jun 26, 2025
andrwng
previously approved these changes
Jun 26, 2025
nvartolomei
reviewed
Jun 26, 2025
| } | ||
| } | ||
|
|
||
| ss::future<result<std::monostate>> credential_manager::maybe_sign( |
There was a problem hiding this comment.
btw result<void> is also valid; don't have a preference
|
Force-push to fix final review feedback (all changes applied to second-to-last commit). |
andrwng
previously approved these changes
Jun 27, 2025
AWS Signature V4 incorporates the AWS service as part of the string to sign. Until now the service has always been S3, but to authenticate requests to Glue our sigV4 implementation must support multiple services.
AWS signature V4 requires the service as part of the string to sign. This parameterizes the applier by service. The only service using the applier is S3. Future changes will use the generalized credentials to authenticate requests to Glue.
Credentials for signing requests to Glue will also need to be periodically refreshed. This change updates the credentials refresh machinery so later it can be used with Glue. It's a bit ad hoc, but all of this code is bit ad hoc because of how it's trying to handle multiple cloud providers and mechanisms with a uniform abstraction.
This changes sigV4 request signing so that it can use a payload hash instead of a default hash that depends on the verb. This is required to sign requests to Glue, which does not support UNSIGNED-PAYLOAD requests (empirically).
This change adds SigV4 authentication as an authentication mode for the Iceberg REST catalog clients. There are two wrinkles: 1. The request must be signed using a string to sign that depends on elements of the request headers and the request payload (if the request has one). 2. SigV4 signing credentials are usually temporary and need to be refreshed. The first issue means auth is added just before the request is sent, when it should be otherwise complete. Bearer auth, by contrast, may be added to the request earlier. The second issue is thornier. This change solves it by setting up a background refresh op like the one used by cloud storage clients. If credentials are static, they will be set up once; otherwise a background process will refresh them when they are almost expired. Because there are multiple catalogs created in multiple places, this adds a new service that manages the credentials and refreshes them in the background, like the s3 client pool does for cloud io. This change adds configuration properties parallel to cloud storage AWS credentials, and allows the AWS service to be configured, just in case a user wants to set up different credentials, or use another AWS service that provides an Iceberg rest catalog (I don't think there are any such services right now, though).
Glue does not pick a base location for you. You must specify it when creating tables. This adds a configuration property to support Glue's behavior. Most REST catalog implementations do not need this property to be set.
|
Force-push to fix test that was missing new configs from its list. |
nvartolomei
approved these changes
Jun 30, 2025
andrwng
reviewed
Jun 30, 2025
Comment on lines
+4068
to
+4122
| , iceberg_rest_catalog_aws_access_key( | ||
| *this, | ||
| "iceberg_rest_catalog_aws_access_key", | ||
| "AWS access key for Iceberg REST catalog SigV4 authentication. If not " | ||
| "set, falls back to cloud_storage_access_key when using aws_sigv4 " | ||
| "authentication mode.", | ||
| {.needs_restart = needs_restart::yes, | ||
| .visibility = visibility::user, | ||
| .gets_restored = gets_restored::no}, | ||
| std::nullopt, | ||
| &validate_non_empty_string_opt) | ||
| , iceberg_rest_catalog_aws_secret_key( | ||
| *this, | ||
| "iceberg_rest_catalog_aws_secret_key", | ||
| "AWS secret key for Iceberg REST catalog SigV4 authentication. If not " | ||
| "set, falls back to cloud_storage_secret_key when using aws_sigv4 " | ||
| "authentication mode.", | ||
| {.needs_restart = needs_restart::yes, | ||
| .visibility = visibility::user, | ||
| .secret = is_secret::yes, | ||
| .gets_restored = gets_restored::no}, | ||
| std::nullopt, | ||
| &validate_non_empty_string_opt) | ||
| , iceberg_rest_catalog_aws_region( | ||
| *this, | ||
| "iceberg_rest_catalog_aws_region", | ||
| "AWS region for Iceberg REST catalog SigV4 authentication. If not set, " | ||
| "falls back to cloud_storage_region when using aws_sigv4 authentication " | ||
| "mode.", | ||
| {.needs_restart = needs_restart::yes, | ||
| .visibility = visibility::user, | ||
| .gets_restored = gets_restored::no}, | ||
| std::nullopt, | ||
| &validate_non_empty_string_opt) | ||
| , iceberg_rest_catalog_aws_credentials_source( | ||
| *this, | ||
| "iceberg_rest_catalog_aws_credentials_source", | ||
| "Source of AWS credentials for Iceberg REST catalog SigV4 " | ||
| "authentication. " | ||
| "If not set, falls back to cloud_storage_credentials_source when using " | ||
| "aws_sigv4 authentication mode. Accepted values: config_file, " | ||
| "aws_instance_metadata, sts, gcp_instance_metadata, " | ||
| "azure_vm_instance_metadata, azure_aks_oidc_federation.", | ||
| {.needs_restart = needs_restart::yes, | ||
| .visibility = visibility::user, | ||
| .gets_restored = gets_restored::no}, | ||
| std::nullopt, | ||
| { | ||
| model::cloud_credentials_source::config_file, | ||
| model::cloud_credentials_source::aws_instance_metadata, | ||
| model::cloud_credentials_source::sts, | ||
| model::cloud_credentials_source::gcp_instance_metadata, | ||
| model::cloud_credentials_source::azure_aks_oidc_federation, | ||
| model::cloud_credentials_source::azure_vm_instance_metadata, | ||
| }) |
There was a problem hiding this comment.
Can we mark these as gets_restored::yes (the default), and revert the changes to the ducktape test? The reason the original ones are marked as not getting restored is because they need to be set in the first place in order for a cluster restore to be run, which isn't the case here
andrwng
approved these changes
Jun 30, 2025
|
@piyushredpanda will this be backported to 25.1 (please!)? |
Let me chat with the team and get back. It's a large intricate PR. |
|
@tmgstevens The engineer in charge of this, Will Berkeley, successfully backported and is just doing a sanity check of running a test sequence on the resultant build. Fundamentally the answer here is yes, backporting OK. |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds Iceberg REST data catalog support for AWS Glue.
There were broadly three obstacles:
Please see the individual commit messages for details.
As the overall change is fairly large, this is best reviewed commit-by-commit. Most commits are small; the credential refresh service commit is the most complex.
This is manually tested to work with AWS Glue, with table data readable via AWS Athena.
Implements CORE-8726.
Backports Required
Release Notes
Features
iceberg_rest_catalog_authentication_modetoaws_sigv4. Additionally, the Glue Data Catalog requires a base location for table storage, configured byiceberg_rest_catalog_base_location.