You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "jdockerty (via GitHub)" <gi...@apache.org> on 2024/08/07 15:53:46 UTC
[PR] feat(gcs): allow setting a token directly [opendal]
jdockerty opened a new pull request, #4978:
URL: https://github.com/apache/opendal/pull/4978
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes https://github.com/apache/opendal/issues/4876
# Rationale for this change
An OAuth2 token cannot be set directly at the moment, instead other mechanisms are required such as the `credential` or `credential_path`.
<!--
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.
-->
# 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.
-->
Inclusion of a `token` within the `GcsConfig` and corresponding methods so that a bearer token is set within the signed requests to GCP.
# Are there any user-facing changes?
Direct `token` being available as a option for authentication
<!--
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-changes` 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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1707385081
##########
core/src/services/gcs/core.rs:
##########
@@ -117,6 +119,12 @@ impl GcsCore {
}
let cred = self.load_token().await?;
+ if !self.token.is_empty() {
Review Comment:
I'm concerned that we still need to adjust the behavior here a bit, such as skipping the sign if the token is empty. I also find that `load_token` isn't handling things correctly.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1707325648
##########
core/src/services/gcs/backend.rs:
##########
@@ -74,6 +74,8 @@ pub struct GcsConfig {
pub disable_vm_metadata: bool,
/// Disable loading configuration from the environment.
pub disable_config_load: bool,
+ /// A Google Cloud OAuth2 token.
+ pub token: String,
Review Comment:
I see that `token` is used in a few other places and I've opted for a simple `String` here.
I might be oversimplifying this though too, feel free to let me know :+1:
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1712906196
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
Do you think it's a good idea to return the token in `load_token`? We can set `expires` to `usize::MAX` for now. In the future, we might introduce `token_expires_in`.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #4978:
URL: https://github.com/apache/opendal/pull/4978#issuecomment-2274846523
Hi, https://github.com/apache/opendal/pull/4979 has been merged, we can continue this one now.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1707325648
##########
core/src/services/gcs/backend.rs:
##########
@@ -74,6 +74,8 @@ pub struct GcsConfig {
pub disable_vm_metadata: bool,
/// Disable loading configuration from the environment.
pub disable_config_load: bool,
+ /// A Google Cloud OAuth2 token.
+ pub token: String,
Review Comment:
I see that `token` is used in a few other services with the `Authorization: Bearer <token>` and I've opted for a simple `String` here to follow suit.
I might be oversimplifying this though too, feel free to let me know :+1:
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1707355182
##########
core/src/services/gcs/core.rs:
##########
@@ -117,6 +119,12 @@ impl GcsCore {
}
let cred = self.load_token().await?;
+ if !self.token.is_empty() {
Review Comment:
If a token has been set, we can return it directly. This can happen before `allow_anonymous` and `load_token`.
By the way, did I overlook part of the previous PR? This `if self.allow_anonymous {}` check doesn't seem correct.
##########
core/src/services/gcs/backend.rs:
##########
@@ -74,6 +74,8 @@ pub struct GcsConfig {
pub disable_vm_metadata: bool,
/// Disable loading configuration from the environment.
pub disable_config_load: bool,
+ /// A Google Cloud OAuth2 token.
+ pub token: String,
Review Comment:
Using a string looks good to me! We can use `Option<String>` to allow the builder to distinguish between its `None` and default value. At the same time, we can provide an example of how this token appears.
We should clarify that this value will take precedence over the `credential`. If a token has been set, we will not attempt to fetch a token via the credential anymore.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1714813519
##########
core/src/services/gcs/core.rs:
##########
@@ -76,6 +79,18 @@ static BACKOFF: Lazy<ExponentialBuilder> =
impl GcsCore {
async fn load_token(&self) -> Result<Option<GoogleToken>> {
+ match (&self.token, &self.scope) {
Review Comment:
`scope` must be valid, we can store as `String` directly.
##########
core/src/services/gcs/core.rs:
##########
@@ -116,6 +131,15 @@ impl GcsCore {
}
pub async fn sign<T>(&self, req: &mut Request<T>) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
We don't need to change the logic here since `load_token` has already handled this.
##########
core/src/services/gcs/backend.rs:
##########
@@ -354,6 +364,8 @@ impl Builder for GcsBuilder {
client,
signer,
token_loader,
+ token: self.config.token,
+ scope: self.config.scope,
Review Comment:
`scope` will be `DEFAULT_GCS_SCOPE` if not set.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1707377608
##########
core/src/services/gcs/core.rs:
##########
@@ -117,6 +119,12 @@ impl GcsCore {
}
let cred = self.load_token().await?;
+ if !self.token.is_empty() {
Review Comment:
> This if self.allow_anonymous {} check doesn't seem correct.
I was wondering the same thing after I saw it here too! I can raise a separate PR to address this quickly.
If I understand rightly, this can be entirely removed? It already exists within the `load_credential` part, i.e. "allow anonymous access" not "skipping signing entirely".
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1707377608
##########
core/src/services/gcs/core.rs:
##########
@@ -117,6 +119,12 @@ impl GcsCore {
}
let cred = self.load_token().await?;
+ if !self.token.is_empty() {
Review Comment:
> This if self.allow_anonymous {} check doesn't seem correct.
I was wondering the same thing after I saw it here too! I can raise a separate PR to address this quickly.
If I understand rightly, this can be entirely removed? It already exists within the `load_credential` part, i.e. "allow anonymous access" not "skipping signing entirely".
**Edit:** I've raised https://github.com/apache/opendal/pull/4979 to address this :+1:
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1715055439
##########
core/src/services/gcs/core.rs:
##########
@@ -116,6 +131,15 @@ impl GcsCore {
}
pub async fn sign<T>(&self, req: &mut Request<T>) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
Oh, this part is a bit complex. Let me elaborate.
---
GCS's token-based authorization doesn't support signed queries. Signed queries involve credentials to build presigned URLs, similar to AWS S3. We can't build such presigned URLs using a token. So if users only set a token but not credentials, the signed query won't work.
However, I believe the changes here are the same, and we don't need to alter code `sign_xxx`. Just let `load_xxx` handle it.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1712939054
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
I don't quite understand what you mean here.
Do you mean doing something like this?
```rust
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
if let Some(token) = &self.token {
let loaded_token = self.load_token()?;
// use the GoogleToken from `loaded_token` elsewhere
}
// ...
```
I'm a little confused because I thought the idea of having `token: Option<String>` meant we could bypass the use of `load_token` since it has already been explicitly provided, so there is nothing we need to "load" with the token loader etc.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1712968860
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
> Do you mean doing something like this?
Sorry for the confusion. I'm takling about:
```rust
async fn load_token(&self) -> Result<Option<GoogleToken>> {
if let Some(token) = &self.token {
return Ok(Some(GoogleToken::new(token, usize::MAX, &self.scope)));
}
let cred = { || self.token_loader.load() }
.retry(&*BACKOFF)
.await
.map_err(new_request_credential_error)?;
if let Some(cred) = cred {
return Ok(Some(cred));
}
if self.allow_anonymous {
return Ok(None);
}
Err(Error::new(
ErrorKind::ConfigInvalid,
"no valid credential found",
))
}
```
In this way, caller of `load_token` don't need to care about the token related logic. This will also make room for us to move the token set logic in `reqsign` directly.
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
> Do you mean doing something like this?
Sorry for the confusion. I'm takling about:
```diff
async fn load_token(&self) -> Result<Option<GoogleToken>> {
+ if let Some(token) = &self.token {
+ return Ok(Some(GoogleToken::new(token, usize::MAX, &self.scope)));
+ }
let cred = { || self.token_loader.load() }
.retry(&*BACKOFF)
.await
.map_err(new_request_credential_error)?;
if let Some(cred) = cred {
return Ok(Some(cred));
}
if self.allow_anonymous {
return Ok(None);
}
Err(Error::new(
ErrorKind::ConfigInvalid,
"no valid credential found",
))
}
```
In this way, caller of `load_token` don't need to care about the token related logic. This will also make room for us to move the token set logic in `reqsign` directly.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1712990568
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
This makes sense, I had this locally, but I backtracked on it because I felt the inclusion of `scope` within the `GcsCore` was unnecessary because that is all part of `reqsign::GoogleToken` already.
I'll push what I had and let you take another look :+1:
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1712939054
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
I don't quite understand what you mean here.
Is this in the sense of doing
```rust
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
if let Some(token) = &self.token {
let loaded_token = self.load_token()?;
// use the GoogleToken from `loaded_token` elsewhere
}
// ...
```
I'm a little confused because I thought the idea of having `token: Option<String>` meant we could bypass the use of `load_token` since it has already been explicitly provided, so there is nothing we need to "load" with the token loader etc.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1715049918
##########
core/src/services/gcs/core.rs:
##########
@@ -116,6 +131,15 @@ impl GcsCore {
}
pub async fn sign<T>(&self, req: &mut Request<T>) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
Makes sense :+1:
Do I understand rightly that the same is also true for `sign_query`?
https://github.com/apache/opendal/blob/55fc7629a10fb4a17dcd7a78e2d3299efa8182b1/core/src/services/gcs/core.rs#L163-L170
I've added :point_up:, but the `sign_query` is handled through `load_credential` and _not_ `load_token`
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1715046986
##########
core/src/services/gcs/backend.rs:
##########
@@ -354,6 +364,8 @@ impl Builder for GcsBuilder {
client,
signer,
token_loader,
+ token: self.config.token,
+ scope: self.config.scope,
Review Comment:
Thanks, I've altered this now.
I skimmed over that it was being set already with a default, I'll combine this with the direct `String` usage too. Much appreciated :+1:
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1715049918
##########
core/src/services/gcs/core.rs:
##########
@@ -116,6 +131,15 @@ impl GcsCore {
}
pub async fn sign<T>(&self, req: &mut Request<T>) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
Makes sense :+1:
Do I understand rightly that the same is also true for `sign_query`? I.e. my addition here should also be removed.
https://github.com/apache/opendal/blob/55fc7629a10fb4a17dcd7a78e2d3299efa8182b1/core/src/services/gcs/core.rs#L163-L170
I've added :point_up:, but the `sign_query` is handled through `load_credential` and _not_ `load_token`
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #4978:
URL: https://github.com/apache/opendal/pull/4978
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1712990568
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
~~This makes sense, I had this locally, but I backtracked on it because I felt the inclusion of `scope` within the `GcsCore` was unnecessary because that is all part of `reqsign::GoogleToken` already.~~
**Edit:** Nevermind, I see `scope` is actually part of `GcsConfig`, but not `GcsCore` yet. I missed that. I can pass it down and this will work fine :+1:
I'll push what I had and let you take another look :+1:
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(gcs): allow setting a token directly [opendal]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #4978:
URL: https://github.com/apache/opendal/pull/4978#discussion_r1712990568
##########
core/src/services/gcs/core.rs:
##########
@@ -136,6 +147,15 @@ impl GcsCore {
}
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
+ if let Some(token) = &self.token {
Review Comment:
This makes sense, I had this locally, but I backtracked on it because I felt the inclusion of `scope` within the `GcsCore` was unnecessary because that is all part of `reqsign`.
I'll push what I had and let you take another look :+1:
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org