laskoviymishka commented on code in PR #2024:
URL: https://github.com/apache/iceberg-go/pull/2024#discussion_r4060142001
##########
io/config.go:
##########
@@ -54,11 +54,27 @@ 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 opts in to authenticating with an Azure
managed
Review Comment:
I think this oversimplifies a bit. `ADLSManagedIdentityEnabled` is actually
fourth in the auth if/else chain in `azure.go`: shared-key, then SAS token,
then connection string, then this. So if any of those are still set for the
same account or host, setting this flag silently no-ops and you never reach the
managed-identity path.
Since the point here is documenting the behavior accurately, I'd add a line
on precedence, something like "evaluated after shared-key, SAS-token, and
connection-string auth; if any of those are configured they take precedence and
this has no effect." wdyt?
##########
io/config.go:
##########
@@ -54,11 +54,27 @@ 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 opts in to authenticating with an Azure
managed
+ // identity instead of the default credential chain. Pair it with
ADLSClientID to
Review Comment:
"instead of the default credential chain" reads like managed identity is off
the table unless you set this, but `DefaultAzureCredential` already includes
`ManagedIdentityCredential` in its cascade, so managed identity works without
this flag too, just not exclusively.
I'd reword to something like "uses `ManagedIdentityCredential` directly
rather than the broader `DefaultAzureCredential` chain" so it's clear what this
flag actually changes.
##########
io/config.go:
##########
@@ -54,11 +54,27 @@ 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 opts in to authenticating with an Azure
managed
+ // identity instead of the default credential chain. Pair it with
ADLSClientID to
+ // select a user assigned identity; on its own it uses the system
assigned identity.
Review Comment:
Small one: "user assigned" and "system assigned" want hyphens as compound
adjectives: "user-assigned" / "system-assigned". And the `ADLSClientID` comment
says "user-assigned managed identity" while this one drops the "managed", so
worth using the full phrase in both for consistency.
##########
io/config.go:
##########
@@ -54,11 +54,27 @@ 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 opts in to authenticating with an Azure
managed
+ // identity instead of the default credential chain. Pair it with
ADLSClientID to
+ // select a user assigned identity; on its own it uses the system
assigned identity.
+ //
+ // This property is specific to this implementation. Neither the Java
nor the
+ // PyIceberg implementation defines an equivalent property, so catalog
properties
+ // that set it will not carry over to them.
ADLSManagedIdentityEnabled = "adls.auth.managed-identity.enabled"
+ // ADLSClientID selects a user assigned managed identity by client ID.
It only
+ // takes effect when ADLSManagedIdentityEnabled is "true"; on its own
it is ignored.
+ //
+ // PyIceberg recognizes the same "adls.client-id" key but uses it for
service
Review Comment:
Cross-client claims like this age badly without a citation. Could we pin the
PyIceberg behavior to a docs URL or a specific version so it stays auditable?
While we're here: Java's `AzureProperties` defines no `adls.client-id` at
all (no service-principal or client-ID concept), so a one-liner noting that
would round out the divergence picture. Non-blocking.
--
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]
