-
Notifications
You must be signed in to change notification settings - Fork 518
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
Changes from 9 commits
5278aaf
9e1badf
b236f5b
3adbe47
91887b8
adbd57a
afb7ef0
b36ca3b
55fc762
32d1b1d
3346244
2a98679
f1a78d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -24,6 +24,7 @@ use std::time::Duration; | |||||||||||||||||
use backon::ExponentialBuilder; | ||||||||||||||||||
use backon::Retryable; | ||||||||||||||||||
use bytes::Bytes; | ||||||||||||||||||
use http::header; | ||||||||||||||||||
use http::header::CONTENT_LENGTH; | ||||||||||||||||||
use http::header::CONTENT_TYPE; | ||||||||||||||||||
use http::header::HOST; | ||||||||||||||||||
|
@@ -53,6 +54,8 @@ pub struct GcsCore { | |||||||||||||||||
pub client: HttpClient, | ||||||||||||||||||
pub signer: GoogleSigner, | ||||||||||||||||||
pub token_loader: GoogleTokenLoader, | ||||||||||||||||||
pub token: Option<String>, | ||||||||||||||||||
pub scope: Option<String>, | ||||||||||||||||||
pub credential_loader: GoogleCredentialLoader, | ||||||||||||||||||
|
||||||||||||||||||
pub predefined_acl: Option<String>, | ||||||||||||||||||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||
(Some(token), Some(scope)) => { | ||||||||||||||||||
return Ok(Some(GoogleToken::new(token, usize::MAX, scope))) | ||||||||||||||||||
} | ||||||||||||||||||
(Some(_), None) => { | ||||||||||||||||||
return Err(Error::new( | ||||||||||||||||||
ErrorKind::ConfigInvalid, | ||||||||||||||||||
"Scope is required when token is set", | ||||||||||||||||||
)) | ||||||||||||||||||
} | ||||||||||||||||||
_ => {} | ||||||||||||||||||
} | ||||||||||||||||||
let cred = { || self.token_loader.load() } | ||||||||||||||||||
.retry(&*BACKOFF) | ||||||||||||||||||
.await | ||||||||||||||||||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to change the logic here since There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 opendal/core/src/services/gcs/core.rs Lines 163 to 170 in 55fc762
I've added ☝️, but the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||
req.headers_mut().remove(HOST); | ||||||||||||||||||
|
||||||||||||||||||
let header_value = format!("Bearer {}", token); | ||||||||||||||||||
req.headers_mut() | ||||||||||||||||||
.insert(header::AUTHORIZATION, header_value.parse().unwrap()); | ||||||||||||||||||
return Ok(()); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if let Some(cred) = self.load_token().await? { | ||||||||||||||||||
self.signer | ||||||||||||||||||
.sign(req, &cred) | ||||||||||||||||||
|
@@ -136,6 +160,15 @@ impl GcsCore { | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> { | ||||||||||||||||||
if let Some(token) = &self.token { | ||||||||||||||||||
Xuanwo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
req.headers_mut().remove(HOST); | ||||||||||||||||||
|
||||||||||||||||||
let header_value = format!("Bearer {}", token); | ||||||||||||||||||
req.headers_mut() | ||||||||||||||||||
.insert(header::AUTHORIZATION, header_value.parse().unwrap()); | ||||||||||||||||||
return Ok(()); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if let Some(cred) = self.load_credential()? { | ||||||||||||||||||
self.signer | ||||||||||||||||||
.sign_query(req, duration, &cred) | ||||||||||||||||||
|
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 beDEFAULT_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 👍