-
Notifications
You must be signed in to change notification settings - Fork 478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(gcs): allow setting a token directly #4978
Conversation
Hi, #4979 has been merged, we can continue this one now. |
core/src/services/gcs/backend.rs
Outdated
@@ -354,6 +364,8 @@ impl Builder for GcsBuilder { | |||
client, | |||
signer, | |||
token_loader, | |||
token: self.config.token, | |||
scope: self.config.scope, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope
will be DEFAULT_GCS_SCOPE
if not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
core/src/services/gcs/core.rs
Outdated
@@ -76,6 +79,18 @@ static BACKOFF: Lazy<ExponentialBuilder> = | |||
|
|||
impl GcsCore { | |||
async fn load_token(&self) -> Result<Option<GoogleToken>> { | |||
match (&self.token, &self.scope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope
must be valid, we can store as String
directly.
core/src/services/gcs/core.rs
Outdated
@@ -116,6 +131,15 @@ impl GcsCore { | |||
} | |||
|
|||
pub async fn sign<T>(&self, req: &mut Request<T>) -> Result<()> { | |||
if let Some(token) = &self.token { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to change the logic here since load_token
has already handled this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
Do I understand rightly that the same is also true for sign_query
? I.e. my addition here should also be removed.
opendal/core/src/services/gcs/core.rs
Lines 163 to 170 in 55fc762
if let Some(token) = &self.token { | |
req.headers_mut().remove(HOST); | |
let header_value = format!("Bearer {}", token); | |
req.headers_mut() | |
.insert(header::AUTHORIZATION, header_value.parse().unwrap()); | |
return Ok(()); | |
} |
I've added ☝️, but the sign_query
is handled through load_credential
and not load_token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
Which issue does this PR close?
Closes #4876
Rationale for this change
An OAuth2 token cannot be set directly at the moment, instead other mechanisms are required such as the
credential
orcredential_path
.What changes are included in this PR?
Inclusion of a
token
within theGcsConfig
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