laskoviymishka commented on code in PR #2024:
URL: https://github.com/apache/iceberg-go/pull/2024#discussion_r4075696868
##########
website/src/configuration.md:
##########
@@ -204,12 +204,13 @@ Tuning properties:
### Azure Data Lake Storage / Blob
-Authentication is selected based on the property keys present
(`io/gocloud/azure/azure.go`):
+Authentication is selected in the following order
(`io/gocloud/azure/azure.go`); the first matching option takes precedence:
Review Comment:
Small precision thing. The lead-in says "the first matching option takes
precedence," but item 5 isn't a matched condition, it's the unconditional
`else` in `azure.go` when 1-4 don't apply.
I'd tweak the lead-in to something like "the first applicable option wins;
if none of 1-4 apply, the default credential chain (5) is used," so item 5
reads as the fallback rather than another thing to match.
##########
io/config.go:
##########
@@ -54,11 +54,37 @@ const (
ADLSConnectionStringPrefix = "adls.connection-string."
ADLSSharedKeyAccountName = "adls.auth.shared-key.account.name"
ADLSSharedKeyAccountKey = "adls.auth.shared-key.account.key"
- ADLSClientID = "adls.client-id"
ADLSEndpoint = "adls.endpoint"
ADLSProtocol = "adls.protocol"
+
+ // ADLSManagedIdentityEnabled uses ManagedIdentityCredential directly
when set
+ // to "true", rather than the broader DefaultAzureCredential chain,
which already
+ // tries managed identity among its other sources. Pair it with
ADLSClientID to
+ // select a user-assigned managed identity; on its own it uses the
system-assigned
+ // managed identity.
+ //
+ // It is evaluated only after shared-key, SAS-token, and
connection-string auth:
Review Comment:
This names the same three mechanisms twice in one sentence, first as a short
list and then spelled out again, which reads a little redundant.
I'd trim to one pass, e.g. "It is evaluated only after shared-key,
SAS-token, and connection-string auth, which all take precedence over it; if
any of those match, this flag has no effect."
##########
website/src/configuration.md:
##########
@@ -204,12 +204,13 @@ Tuning properties:
### Azure Data Lake Storage / Blob
-Authentication is selected based on the property keys present
(`io/gocloud/azure/azure.go`):
+Authentication is selected in the following order
(`io/gocloud/azure/azure.go`); the first matching option takes precedence:
-1. Shared key: both `adls.auth.shared-key.account.name` and
`adls.auth.shared-key.account.key` set.
-2. Per-host SAS token: `adls.sas-token.<hostname>` (prefix-matched against the
storage account host).
-3. Per-host connection string: `adls.connection-string.<hostname>`.
-4. Managed identity: `adls.auth.managed-identity.enabled` set to a truthy
value.
+1. Shared key: a nonempty `adls.auth.shared-key.account.name` selects this
path and requires a nonempty `adls.auth.shared-key.account.key`.
+2. Per-host SAS token: `adls.sas-token.<hostname>`, where `<hostname>` exactly
matches the storage account hostname (for example,
`myaccount.dfs.core.windows.net`).
+3. Per-account connection string: `adls.connection-string.<account-name>` (for
example, `adls.connection-string.myaccount`).
+4. Managed identity: `adls.auth.managed-identity.enabled` set to exactly
`"true"` uses `ManagedIdentityCredential` directly. Set `adls.client-id` to
select a user-assigned managed identity; otherwise, the system-assigned managed
identity is used.
+5. Default credential chain: `DefaultAzureCredential`, which includes managed
identity among its credential sources.
Review Comment:
The whole point of this PR is that these two keys diverge from Java and
PyIceberg, and the godoc comments capture that really well. The website section
never says it, though, so someone reading only the docs (which is most people)
still has no signal that `adls.client-id` means service-principal auth in
PyIceberg.
There's already a pattern for this a few sections up: the "cross-client
parity" blockquote under REST metrics-reporting. I'd add a short one here
noting `adls.auth.managed-identity.enabled` has no Java/PyIceberg equivalent
and that `adls.client-id` collides in name only with PyIceberg's
service-principal key. wdyt?
##########
website/src/configuration.md:
##########
@@ -204,12 +204,13 @@ Tuning properties:
### Azure Data Lake Storage / Blob
-Authentication is selected based on the property keys present
(`io/gocloud/azure/azure.go`):
+Authentication is selected in the following order
(`io/gocloud/azure/azure.go`); the first matching option takes precedence:
-1. Shared key: both `adls.auth.shared-key.account.name` and
`adls.auth.shared-key.account.key` set.
-2. Per-host SAS token: `adls.sas-token.<hostname>` (prefix-matched against the
storage account host).
-3. Per-host connection string: `adls.connection-string.<hostname>`.
-4. Managed identity: `adls.auth.managed-identity.enabled` set to a truthy
value.
+1. Shared key: a nonempty `adls.auth.shared-key.account.name` selects this
path and requires a nonempty `adls.auth.shared-key.account.key`.
+2. Per-host SAS token: `adls.sas-token.<hostname>`, where `<hostname>` exactly
matches the storage account hostname (for example,
`myaccount.dfs.core.windows.net`).
+3. Per-account connection string: `adls.connection-string.<account-name>` (for
example, `adls.connection-string.myaccount`).
+4. Managed identity: `adls.auth.managed-identity.enabled` set to exactly
`"true"` uses `ManagedIdentityCredential` directly. Set `adls.client-id` to
select a user-assigned managed identity; otherwise, the system-assigned managed
identity is used.
Review Comment:
Reads a little oddly since "set to exactly `"true"`" ends up as the subject
of "uses." Item 1's "a nonempty X selects this path and requires Y" pattern is
cleaner, so I'd match it here:
```suggestion
4. Managed identity: `adls.auth.managed-identity.enabled` set to exactly
`"true"` selects this path and uses `ManagedIdentityCredential` directly. Set
`adls.client-id` to select a user-assigned managed identity; otherwise, the
system-assigned managed identity is used.
```
--
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]
