sunchao commented on code in PR #6059:
URL: https://github.com/apache/datafusion-comet/pull/6059#discussion_r4057249816
##########
native/core/src/parquet/objectstore/azure.rs:
##########
@@ -118,12 +133,92 @@ pub fn create_store(
.map(|(k, _)| k.as_ref())
.collect::<Vec<_>>()
);
+
+ let provider_type =
+ account_scoped_value(configs, HADOOP_OAUTH_PROVIDER_TYPE,
account.as_deref());
+ let store = build_builder(url, &translated, provider_type.as_deref(),
env_pairs()).build()?;
+ Ok((Box::new(store), path))
+}
+
+/// Process environment as UTF-8 `(key, value)` pairs, skipping entries that
are not UTF-8.
+fn env_pairs() -> impl Iterator<Item = (String, String)> {
+ std::env::vars_os()
+ .filter_map(|(k, v)| Some((k.to_str()?.to_string(),
v.to_str()?.to_string())))
+}
+
+/// Assemble the builder from the environment, the URL and the translated
Hadoop keys.
+///
+/// When the Hadoop keys configure an auth mechanism the environment is not
consulted at
+/// all, so nothing ambient can outrank, combine with or redirect the
configured identity.
+/// Otherwise the environment is read the way
`MicrosoftAzureBuilder::from_env` reads it.
+/// `provider_type` is the resolved `fs.azure.account.oauth.provider.type`, if
any.
+fn build_builder(
+ url: &Url,
+ translated: &[(AzureConfigKey, String)],
+ provider_type: Option<&str>,
+ env: impl Iterator<Item = (String, String)>,
+) -> MicrosoftAzureBuilder {
+ let mut builder = MicrosoftAzureBuilder::new();
+ if !hadoop_auth_present(translated, provider_type) {
+ builder = apply_env(builder, env);
+ }
Review Comment:
### Correctness
**[P2] Reject incomplete translated OAuth credentials before selecting IMDS**
Could we reject an incomplete translated OAuth credential before `build()`
can select IMDS? A reachable case is Hadoop's `ClientCredsTokenProvider` with
`fs.azure.account.oauth2.client.secret` in normal configuration, while the
`fs.azure.account.oauth2.client.id` and
`fs.azure.account.oauth2.client.endpoint` aliases live in a configured JCEKS
credential provider. Hadoop's ABFS reads all three through `getPassword()`, so
its credential is complete and file listing can succeed.
`NativeConfig.extractObjectStoreOptions` only copies
`Configuration.iterator()`/`get()`, so native receives the secret but neither
the client ID nor tenant.
With matching `AZURE_CLIENT_ID` and `AZURE_TENANT_ID`, the previous native
builder selects the same client-secret principal. This branch now removes those
ambient fields. In the pinned `object_store 0.13.2` builder, the incomplete
tuple falls through to `ImdsManagedIdentityProvider` without a client-ID
selector. The native read can then use the node's identity if it has access, or
fail through IMDS, instead of reporting that the configured OAuth credential
cannot be represented.
Ending environment completion is the intended policy. Could we return a
configuration error at that boundary and add a regression checking the final
provider/error selection? This only needs a fail-closed check, not native JCEKS
support. I traced this case in source and checked the native selection boundary
with the synthetic helper tests; I did not run a live Azure query.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
