raulcd commented on code in PR #46837:
URL: https://github.com/apache/arrow/pull/46837#discussion_r2168230039
##########
python/pyarrow/_azurefs.pyx:
##########
@@ -107,7 +128,32 @@ cdef class AzureFileSystem(FileSystem):
if account_key and sas_token:
raise ValueError("Cannot specify both account_key and sas_token.")
- if account_key:
+ if (tenant_id or client_id or client_secret):
+ if tenant_id and client_id and client_secret:
Review Comment:
I think the previous iteration was clearer:
- If there's no client_id fail early without requiring more checks
- If there's no tenant and no client_secret -->
ConfigureManagedIdentityCredential
- If there's tenant and client_secret --> ConfigureClientSecretCredential
- Otherwise fail
##########
python/pyarrow/_azurefs.pyx:
##########
@@ -107,7 +128,32 @@ cdef class AzureFileSystem(FileSystem):
if account_key and sas_token:
raise ValueError("Cannot specify both account_key and sas_token.")
- if account_key:
+ if (tenant_id or client_id or client_secret):
+ if tenant_id and client_id and client_secret:
+ options.ConfigureClientSecretCredential(
+ tobytes(tenant_id), tobytes(client_id),
tobytes(client_secret)
+ )
+ self.tenant_id = tobytes(tenant_id)
+ self.client_id = tobytes(client_id)
+ self.client_secret = tobytes(client_secret)
+ elif client_id and not tenant_id and not client_secret:
+ options.ConfigureManagedIdentityCredential(tobytes(client_id))
+ self.client_id = tobytes(client_id)
+ elif not client_id:
+ raise ValueError(
+ "client_id must be specified for both
ClientSecretCredential and ManagedIdentityCredential. "
+ "For ClientSecretCredential, provide tenant_id, client_id,
and client_secret. "
+ "For ManagedIdentityCredential, provide only client_id."
+ )
+ else:
+ if client_id and (tenant_id or client_secret):
+ raise ValueError(
+ "Ambiguous Azure credential configuration: "
Review Comment:
`For ClientSecretCredential all client_id, client_secret and tenant_id must
be specified`
##########
python/pyarrow/_azurefs.pyx:
##########
@@ -107,7 +128,32 @@ cdef class AzureFileSystem(FileSystem):
if account_key and sas_token:
raise ValueError("Cannot specify both account_key and sas_token.")
- if account_key:
+ if (tenant_id or client_id or client_secret):
+ if tenant_id and client_id and client_secret:
+ options.ConfigureClientSecretCredential(
+ tobytes(tenant_id), tobytes(client_id),
tobytes(client_secret)
+ )
+ self.tenant_id = tobytes(tenant_id)
+ self.client_id = tobytes(client_id)
+ self.client_secret = tobytes(client_secret)
+ elif client_id and not tenant_id and not client_secret:
+ options.ConfigureManagedIdentityCredential(tobytes(client_id))
+ self.client_id = tobytes(client_id)
+ elif not client_id:
+ raise ValueError(
+ "client_id must be specified for both
ClientSecretCredential and ManagedIdentityCredential. "
+ "For ClientSecretCredential, provide tenant_id, client_id,
and client_secret. "
+ "For ManagedIdentityCredential, provide only client_id."
Review Comment:
I don't think we require that much verbosity (this is already specified on
the API), let's maybe just say what is the problem?
`client_id must be specified if client_secret or tenant_id are provided`?
##########
python/pyarrow/_azurefs.pyx:
##########
@@ -107,7 +128,32 @@ cdef class AzureFileSystem(FileSystem):
if account_key and sas_token:
raise ValueError("Cannot specify both account_key and sas_token.")
- if account_key:
+ if (tenant_id or client_id or client_secret):
+ if tenant_id and client_id and client_secret:
+ options.ConfigureClientSecretCredential(
+ tobytes(tenant_id), tobytes(client_id),
tobytes(client_secret)
+ )
+ self.tenant_id = tobytes(tenant_id)
+ self.client_id = tobytes(client_id)
+ self.client_secret = tobytes(client_secret)
+ elif client_id and not tenant_id and not client_secret:
+ options.ConfigureManagedIdentityCredential(tobytes(client_id))
+ self.client_id = tobytes(client_id)
+ elif not client_id:
+ raise ValueError(
+ "client_id must be specified for both
ClientSecretCredential and ManagedIdentityCredential. "
+ "For ClientSecretCredential, provide tenant_id, client_id,
and client_secret. "
+ "For ManagedIdentityCredential, provide only client_id."
+ )
+ else:
+ if client_id and (tenant_id or client_secret):
Review Comment:
this should be unnecessary `if (tenant_id or client_id or client_secret):`
then this is the only case left, right?
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
