Iceberg other creds - #30284
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Iceberg REST catalog credential refresh to use the correct credential source and avoid incorrectly applying the cloud storage credentials host override to non–cloud-storage credential refreshes.
Changes:
- Route Iceberg REST catalog background credential refresh to use
iceberg_rest_catalog_credentials_source(with fallback) rather thancloud_storage_credentials_source. - Add and wire
iceberg_rest_catalog_credentials_hostso the REST catalog can use a distinct credentials host override. - Plumb host override through the auth refresh configuration pipeline (cloud storage + cloud roles), removing the previous implicit global override behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/datalake/credential_manager.cc | Passes Iceberg-specific credentials source + host override into the refresh credentials source builder. |
| src/v/config/configuration.h | Declares new iceberg_rest_catalog_credentials_host config property. |
| src/v/config/configuration.cc | Defines and documents iceberg_rest_catalog_credentials_host. |
| src/v/cloud_storage_clients/credential_manager.cc | Ensures cloud storage refresh uses cloud_storage_credentials_host via the new builder signature. |
| src/v/cloud_storage_clients/configuration.h | Extends build_refresh_credentials_source API to accept an optional host override. |
| src/v/cloud_storage_clients/configuration.cc | Threads host override into auth_refresh_bg_op::s3_compat_config. |
| src/v/cloud_roles/refresh_credentials.h | Adds host override parameter to the refresh-credentials factory template and removes implicit global host override. |
| src/v/cloud_roles/refresh_credentials.cc | Propagates host override through make_refresh_credentials dispatch. |
| src/v/cloud_roles/auth_refresh_bg_op.h | Extends s3_compat_config with an optional host override field. |
| src/v/cloud_roles/auth_refresh_bg_op.cc | Plumbs host override into make_refresh_credentials invocation. |
CI test resultstest results on build#83612
test results on build#84304 |
start_auth_refresh_if_needed was passing cloud_storage_credentials_source to build_refresh_credentials_source, ignoring iceberg_rest_catalog_aws_credentials_source. Use get_credentials_source() which already prefers the iceberg-specific config with fallback to cloud storage.
cc6fab6 to
7ff48de
Compare
| "cloud_storage_credentials_source is set to sts with a custom " | ||
| "credentials " |
There was a problem hiding this comment.
This kind of alludes to this property being connected with cloud_storage_credentials_source in some way. Does it make sense in the many cases for them to be the same? Wondering if the user of this should be doing some fallback onto cloud_storage_credentials_source if this is null?
There was a problem hiding this comment.
Clarified in the description. It's only relevant when specifically setting up cloud storage and the rest catalog credentials differently, using different sources, so we wouldn't want fallback in that case.
| {.needs_restart = needs_restart::yes, .visibility = visibility::user}, | ||
| std::nullopt, | ||
| &validate_non_empty_string_opt) | ||
| , iceberg_rest_catalog_credentials_host( |
There was a problem hiding this comment.
a bit worried this will be confused with iceberg_rest_catalog_oauth2_server_uri. consider copying the help text from the cloud storage config
Only required when using IAM role based access
and maybe also specifying that this does not apply to oauth schemes / applies only to aws
When cloud_storage_credentials_host is set (e.g. to an STS endpoint), it was being applied to Iceberg REST catalog credential requests too, regardless of the catalog's credentials source. This caused failures when the catalog uses aws_instance_metadata (which expects 169.254.169.254) but the cloud storage host override points elsewhere. Add iceberg_rest_catalog_credentials_host to allow the catalog credentials host to be set independently. Thread host_override through build_refresh_credentials_source and s3_compat_config down to make_refresh_credentials, replacing the direct config read in the template with an explicit parameter.
7ff48de to
b49b35a
Compare
Retry command for Build#84304please wait until all jobs are finished before running the slash command |
|
/ci-repeat 1 |
|
/backport v26.1.x |
|
/backport v25.3.x |
|
/backport v25.2.x |
|
Failed to create a backport PR to v25.2.x branch. I tried: |
|
Failed to create a backport PR to v25.3.x branch. I tried: |
The Iceberg REST catalog credential refresh background operation was incorrectly using cloud storage configuration in two ways: it ignored iceberg_rest_catalog_credentials_source in favour of cloud_storage_credentials_source when selecting the credentials provider, and it unconditionally applied cloud_storage_credentials_host as a host override regardless of which subsystem was refreshing credentials. This caused failures when the two subsystems use different credential sources — for example, cloud storage using STS with a custom host override while the REST catalog uses EC2 instance metadata, resulting in IMDS requests being sent to the STS endpoint and timing out.
Backports Required
Release Notes
Bug Fixes
Features
iceberg_rest_catalog_credentials_host: The hostname to connect to for retrieving role-based credentials for the Iceberg REST catalog. May be required when the REST catalog uses a different credentials source than cloud storage — for example, if cloud storage is configured to use STS with a custom credentials host, but the REST catalog uses aws_instance_metadata, this should be set to 169.254.169.254 to prevent the cloud storage host override from applying to catalog credential requests.