You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "roeap (via GitHub)" <gi...@apache.org> on 2023/02/11 22:57:00 UTC
[GitHub] [arrow-rs] roeap opened a new pull request, #3698: object_store: azure cli authorization
roeap opened a new pull request, #3698:
URL: https://github.com/apache/arrow-rs/pull/3698
# Which issue does this PR close?
closes #3696
closes #3697
# Rationale for this change
see #3697
# What changes are included in this PR?
Fix passing bearer token in azure store directly to authorization header. Add a new `TokenCredential` that use4s the azure CLI for acquiring an access token. One problem with this PR is the lack of additional tests. I validated that is works locally, but was unsure how to write a meaningful test to use in CI.
P.S: In the meantime I also validated that the workload identity credential works as expected.
# Are there any user-facing changes?
users can now use bearer tokens and azure cli for authorization against azure object stores.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #3698: object_store: azure cli authorization
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #3698:
URL: https://github.com/apache/arrow-rs/pull/3698
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] roeap commented on a diff in pull request #3698: object_store: azure cli authorization
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #3698:
URL: https://github.com/apache/arrow-rs/pull/3698#discussion_r1103867215
##########
object_store/src/azure/credential.rs:
##########
@@ -540,6 +548,113 @@ impl TokenCredential for WorkloadIdentityOAuthProvider {
}
}
+mod az_cli_date_format {
+ use chrono::{DateTime, TimeZone};
+ use serde::{self, Deserialize, Deserializer};
+
+ pub fn deserialize<'de, D>(
+ deserializer: D,
+ ) -> Result<DateTime<chrono::Local>, D::Error>
+ where
+ D: Deserializer<'de>,
+ {
+ let s = String::deserialize(deserializer)?;
+ // expiresOn from azure cli uses the local timezone
+ let date = chrono::NaiveDateTime::parse_from_str(&s, "%Y-%m-%d %H:%M:%S.%6f")
+ .map_err(serde::de::Error::custom)?;
+ chrono::Local
+ .from_local_datetime(&date)
+ .single()
+ .ok_or(serde::de::Error::custom(
+ "azure cli returned ambiguous expiry date",
+ ))
+ }
+}
+
+#[derive(Debug, Clone, Deserialize)]
+#[serde(rename_all = "camelCase")]
+struct AzureCliTokenResponse {
Review Comment:
makes sense, so I added a check to validate we have the right token type.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3698: object_store: azure cli authorization
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3698:
URL: https://github.com/apache/arrow-rs/pull/3698#discussion_r1103848388
##########
object_store/src/azure/mod.rs:
##########
@@ -887,6 +900,12 @@ impl MicrosoftAzureBuilder {
self
}
+ /// Set if the Azure Cli should be used for acquiring access token
+ pub fn with_use_azure_cli(mut self, use_azure_cli: bool) -> Self {
Review Comment:
It would be nice to include a link to some sort of reference here, I struggled to find something that clearly documented this CLI interface :sweat_smile:
##########
object_store/src/azure/credential.rs:
##########
@@ -540,6 +548,113 @@ impl TokenCredential for WorkloadIdentityOAuthProvider {
}
}
+mod az_cli_date_format {
+ use chrono::{DateTime, TimeZone};
+ use serde::{self, Deserialize, Deserializer};
+
+ pub fn deserialize<'de, D>(
+ deserializer: D,
+ ) -> Result<DateTime<chrono::Local>, D::Error>
+ where
+ D: Deserializer<'de>,
+ {
+ let s = String::deserialize(deserializer)?;
+ // expiresOn from azure cli uses the local timezone
+ let date = chrono::NaiveDateTime::parse_from_str(&s, "%Y-%m-%d %H:%M:%S.%6f")
+ .map_err(serde::de::Error::custom)?;
+ chrono::Local
+ .from_local_datetime(&date)
+ .single()
+ .ok_or(serde::de::Error::custom(
+ "azure cli returned ambiguous expiry date",
+ ))
+ }
+}
+
+#[derive(Debug, Clone, Deserialize)]
+#[serde(rename_all = "camelCase")]
+struct AzureCliTokenResponse {
Review Comment:
Is it worth deserializing `tokenType` also and ensuring it matches `Bearer`?
##########
object_store/src/azure/client.rs:
##########
@@ -169,6 +169,18 @@ impl AzureClient {
CredentialProvider::AccessKey(key) => {
Ok(AzureCredential::AccessKey(key.to_owned()))
}
+ CredentialProvider::BearerToken(token) => {
+ Ok(AzureCredential::AuthorizationToken(
+ // we do the conversion to a HeaderValue here, since it is fallible
+ // and we wna to use it in an infallible function
Review Comment:
```suggestion
// and we want to use it in an infallible function
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3698: object_store: azure cli authorization
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #3698:
URL: https://github.com/apache/arrow-rs/pull/3698#issuecomment-1428054851
Benchmark runs are scheduled for baseline = d82298f855a1bc8c99fc635292cbf55675807c46 and contender = e37e379f158c644fd3bed63dfc9acc23b49aaf4d. e37e379f158c644fd3bed63dfc9acc23b49aaf4d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8e81a153a0a7449489d59312818cdd43...d7b6b295b9a049bbb535193881e670e2/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a2eddd4a8c044176b5d8583f3ae96d06...a7dcdf9b716c478fa96834d789972d9d/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b45fb11ccfbc437f920d906a9c2769f2...7e4d2db2e6614243b01a8fb4348505fe/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/f1edefdf574143798fc0fb068fca4c17...267d96e42152405ea811bb3441853a2b/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org