laskoviymishka commented on code in PR #1062:
URL: https://github.com/apache/iceberg-go/pull/1062#discussion_r3288505003
##########
io/config.go:
##########
@@ -43,12 +43,14 @@ const (
// Constants for Azure configuration options
const (
- ADLSSasTokenPrefix = "adls.sas-token."
- ADLSConnectionStringPrefix = "adls.connection-string."
- ADLSSharedKeyAccountName = "adls.auth.shared-key.account.name"
- ADLSSharedKeyAccountKey = "adls.auth.shared-key.account.key"
- ADLSEndpoint = "adls.endpoint"
- ADLSProtocol = "adls.protocol"
+ ADLSSasTokenPrefix = "adls.sas-token."
+ ADLSConnectionStringPrefix = "adls.connection-string."
+ ADLSSharedKeyAccountName = "adls.auth.shared-key.account.name"
+ ADLSSharedKeyAccountKey = "adls.auth.shared-key.account.key"
+ ADLSEndpoint = "adls.endpoint"
+ ADLSProtocol = "adls.protocol"
+ ADLSManagedIdentityEnabled = "adls.auth.managed-identity.enabled"
+ ADLSManagedIdentityClientID = "adls.auth.managed-identity.client-id"
Review Comment:
These property names don't exist in Java's `AzureProperties`, PyIceberg's
`pyiceberg/io/__init__.py`, or iceberg-rust's `azdls.rs` — they're Go-only. A
REST catalog vending creds from Java will never emit them, so this branch is
effectively unreachable in any cross-client deployment.
For the client-id case, both PyIceberg and Rust already use
`adls.client-id`. I'd reuse that key rather than introduce
`adls.auth.managed-identity.client-id` as a second name for the same field.
##########
io/gocloud/azure_test.go:
##########
@@ -73,6 +73,26 @@ func TestCreateAzureBucketDefaultCredentialEmptyBucketName(t
*testing.T) {
"Expected container name error but got: %v", err)
}
+func TestCreateAzureBucketManagedIdentityCredentialCalled(t *testing.T) {
+ ctx := context.Background()
+
+ parsedURL, err :=
url.Parse("abfs://[email protected]/path")
+ assert.NoError(t, err)
+
+ // NewManagedIdentityCredential never fails at construction, so bucket
creation always succeeds.
+ bucket, err := createAzureBucket(ctx, parsedURL, map[string]string{
+ "adls.auth.managed-identity.enabled": "true",
+ })
+ assert.NoError(t, err)
+ assert.NotNil(t, bucket)
+
+ iter := bucket.List(nil)
+ _, err = iter.Next(ctx)
+ assert.Error(t, err)
+ assert.Contains(t, err.Error(), "ManagedIdentityCredential",
Review Comment:
`"ManagedIdentityCredential"` appears in errors from both
`ManagedIdentityCredential` and `DefaultAzureCredential` — DAC contains MI in
its chain and propagates the credential name through. So this test (and its
`TestCreateAzureBucketDefaultCredentialCalled` neighbour) passes whether the
new branch was taken or not.
The cleanest fix is a small test seam — accept an optional credential
constructor in `createAzureBucket` so the test can inject a stub and assert on
the type. Short of that, at minimum assert the error does **not** contain
`"DefaultAzureCredential"` so the two tests can't both pass on the same code
path.
##########
io/gocloud/azure.go:
##########
@@ -159,6 +159,28 @@ func createAzureBucket(ctx context.Context, parsed
*url.URL, props map[string]st
if err != nil {
return nil, fmt.Errorf("failed
container.NewClientFromConnectionString: %w", err)
}
+ } else if props[io.ADLSManagedIdentityEnabled] == "true" {
Review Comment:
Since `azidentity` v1.8, setting
`AZURE_TOKEN_CREDENTIALS=ManagedIdentityCredential` makes
`DefaultAzureCredential` use only the MI credential in its chain (with `dac:
false`), and `AZURE_CLIENT_ID` already drives user-assigned identity selection.
That's runtime-equivalent to this branch, with no new code and no Go-only
properties.
If the production case is "on AKS, skip the IMDS-probe slowness of DAC", the
env-var path already solves it. If the real driver is per-table identity
isolation in the same process, keeping the code is justified — but I'd want
that motivation stated explicitly. wdyt?
--
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]
