You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "helmus (via GitHub)" <gi...@apache.org> on 2023/02/26 00:18:09 UTC
[GitHub] [arrow-rs] helmus opened a new pull request, #3766: fix aws profile
helmus opened a new pull request, #3766:
URL: https://github.com/apache/arrow-rs/pull/3766
# Which issue does this PR close?
Closes https://github.com/apache/arrow-rs/issues/3765. ( `c.expiry()` will always return `None` )
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
This sets the expiration for the temporary token to 1 hour.
Perhaps this value should be user configured trough an environment variable.
Duration timeouts for [GetSessionToken](https://docs.aws.amazon.com/STS/latest/APIReference/API_GetSessionToken.html)
```
minimum: 15 minutes
default: 12 hours
maximum: 36 hours
```
As there is no good way to know when the token in the profile was generated, I'm proposing 1 hour as a compromise.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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] helmus commented on a diff in pull request #3766: fix aws profile
Posted by "helmus (via GitHub)" <gi...@apache.org>.
helmus commented on code in PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#discussion_r1117997685
##########
object_store/src/aws/credential.rs:
##########
@@ -553,18 +553,7 @@ mod profile {
store: "S3",
source: Box::new(source),
})?;
-
- let t_now = SystemTime::now();
- let expiry = match c.expiry().and_then(|e| e.duration_since(t_now).ok()) {
Review Comment:
The expiry() method returns the [expires_after](https://github.com/awslabs/smithy-rs/blob/76ee00eb1737964ce240ae817f573980920f0607/aws/rust-runtime/aws-credential-types/src/credentials_impl.rs#L36) field, which is not loaded from the aws_profile, but instead is made optionally available to the caller as a caching ttl field. ( [reference for that here](https://github.com/awslabs/smithy-rs/blob/76ee00eb1737964ce240ae817f573980920f0607/aws/rust-runtime/aws-credential-types/src/credentials_impl.rs#L29-L36) , there is an expiry_mut() method as well, allowing the caller to modify the expiration. ).
This value will always be `None`, resulting in `Invalid expiry` Error every time
--
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] helmus commented on a diff in pull request #3766: object-store: fix handling of AWS profile credentials without expiry
Posted by "helmus (via GitHub)" <gi...@apache.org>.
helmus commented on code in PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#discussion_r1118939586
##########
object_store/src/aws/credential.rs:
##########
@@ -553,18 +553,7 @@ mod profile {
store: "S3",
source: Box::new(source),
})?;
-
- let t_now = SystemTime::now();
- let expiry = match c.expiry().and_then(|e| e.duration_since(t_now).ok()) {
Review Comment:
expiry of `None` does mean the credentials do not expire. This PR at least ensures the credentials are cached for at least 1 hour if they never expire.
<img width="776" alt="image" src="https://user-images.githubusercontent.com/1042463/221612600-73091aed-0723-4ee3-8399-a84172fe1332.png">
--
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] helmus commented on a diff in pull request #3766: object-store: fix aws profile
Posted by "helmus (via GitHub)" <gi...@apache.org>.
helmus commented on code in PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#discussion_r1117997685
##########
object_store/src/aws/credential.rs:
##########
@@ -553,18 +553,7 @@ mod profile {
store: "S3",
source: Box::new(source),
})?;
-
- let t_now = SystemTime::now();
- let expiry = match c.expiry().and_then(|e| e.duration_since(t_now).ok()) {
Review Comment:
The expiry() method returns the [expires_after](https://github.com/awslabs/smithy-rs/blob/76ee00eb1737964ce240ae817f573980920f0607/aws/rust-runtime/aws-credential-types/src/credentials_impl.rs#L36) field, which is not loaded from the aws profile in the credentials file, but instead is made optionally available to the caller as a caching ttl field. ( [reference for that here](https://github.com/awslabs/smithy-rs/blob/76ee00eb1737964ce240ae817f573980920f0607/aws/rust-runtime/aws-credential-types/src/credentials_impl.rs#L29-L36) , there is an expiry_mut() method as well, allowing the caller to modify the expiration. ).
This value will always be `None`, resulting in `Invalid expiry` Error every time
--
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 pull request #3766: object-store: fix handling of AWS profile credentials without expiry
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#issuecomment-1446734275
Thank you
--
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 #3766: object-store: fix aws profile
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#discussion_r1118705990
##########
object_store/src/aws/credential.rs:
##########
@@ -553,18 +553,7 @@ mod profile {
store: "S3",
source: Box::new(source),
})?;
-
- let t_now = SystemTime::now();
- let expiry = match c.expiry().and_then(|e| e.duration_since(t_now).ok()) {
Review Comment:
I'm not sure about this, there are definitely situations where the credentials will expire e.g. SAML.
Perhaps we could instead interpret a missing expiry as infinite expiration instead?
--
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] helmus commented on a diff in pull request #3766: object-store: fix handling of AWS profile credentials without expiry
Posted by "helmus (via GitHub)" <gi...@apache.org>.
helmus commented on code in PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#discussion_r1118939586
##########
object_store/src/aws/credential.rs:
##########
@@ -553,18 +553,7 @@ mod profile {
store: "S3",
source: Box::new(source),
})?;
-
- let t_now = SystemTime::now();
- let expiry = match c.expiry().and_then(|e| e.duration_since(t_now).ok()) {
Review Comment:
expiry of `None` does mean the credentials do not expire. This PR at least ensures the credentials are cached for at least 1 hour if they never expire.
https://github.com/awslabs/smithy-rs/blob/76ee00eb1737964ce240ae817f573980920f0607/aws/rust-runtime/aws-credential-types/src/credentials_impl.rs#L35
<img width="776" alt="image" src="https://user-images.githubusercontent.com/1042463/221612600-73091aed-0723-4ee3-8399-a84172fe1332.png">
--
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] helmus commented on a diff in pull request #3766: fix aws profile
Posted by "helmus (via GitHub)" <gi...@apache.org>.
helmus commented on code in PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#discussion_r1117997685
##########
object_store/src/aws/credential.rs:
##########
@@ -553,18 +553,7 @@ mod profile {
store: "S3",
source: Box::new(source),
})?;
-
- let t_now = SystemTime::now();
- let expiry = match c.expiry().and_then(|e| e.duration_since(t_now).ok()) {
Review Comment:
The expiry() method returns the [expires_after](https://github.com/awslabs/smithy-rs/blob/76ee00eb1737964ce240ae817f573980920f0607/aws/rust-runtime/aws-credential-types/src/credentials_impl.rs#L36) field, which is not loaded from the aws_profile, but instead is made optionally available to the caller as a caching ttl field. ( [reference for that here](https://github.com/awslabs/smithy-rs/blob/76ee00eb1737964ce240ae817f573980920f0607/aws/rust-runtime/aws-credential-types/src/credentials_impl.rs#L29-L36) , there is an expiry_mut() method as well, allowing the caller to modify the expiration. ).
This value will always be None.
--
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 #3766: object-store: fix handling of AWS profile credentials without expiry
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766
--
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 #3766: object-store: fix handling of AWS profile credentials without expiry
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#discussion_r1118941179
##########
object_store/src/aws/credential.rs:
##########
@@ -553,18 +553,7 @@ mod profile {
store: "S3",
source: Box::new(source),
})?;
-
- let t_now = SystemTime::now();
- let expiry = match c.expiry().and_then(|e| e.duration_since(t_now).ok()) {
Review Comment:
Yes, but the way it does this breaks credentials that do expire. My suggestion is just to handle the `None` case with an infinite TTL, instead of what this PR currently does which is assign an arbitrary expiry to all credentials
--
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 #3766: object-store: fix handling of AWS profile credentials without expiry
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #3766:
URL: https://github.com/apache/arrow-rs/pull/3766#issuecomment-1446752628
Benchmark runs are scheduled for baseline = 034c43fdb47389ab120c2ab84a6e61d44d0d2788 and contender = f82b704c5c38090a6ecd052b5c25bdfee7010130. f82b704c5c38090a6ecd052b5c25bdfee7010130 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/d1460ed96f4241e5b983d9cb81c02a5f...c9847c7a86c9409192973b936c924010/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/1d574828458645e9ae3d4a7b7c68ff24...1dbcc134b3f64835bc95c7f33f3a0264/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e96d26f4046849e6a8328e18f1e2785a...552d0bab3d184ed3b356351c5f3f3d00/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7a66d800f8094584af047fca7dff3ff8...f3c9ba5d7698418fb4fe00f79a4efe45/)
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