-
Notifications
You must be signed in to change notification settings - Fork 95
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
Adjusted the handling of URLs to support more types of hosts #173
Conversation
cc @sameo |
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 @jialez0 LGTM! Just some minor naming suggestion.
@@ -17,7 +17,7 @@ pub mod types; | |||
const KBS_REQ_TIMEOUT_SEC: u64 = 60; | |||
const KBS_GET_RESOURCE_MAX_ATTEMPT: u64 = 3; | |||
|
|||
pub const KBS_URL_PREFIX: &str = "kbs/v0"; | |||
pub const KBS_URL_PREFIX: &str = "/kbs/v0"; |
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.
maybe KBS_RESOURCE_PREFIX
is better?
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.
The prefix here is not only used to obtain resources, but also to obtain tokens in the passport model. Perhaps we can just call it KBS_PREFIX
.
@@ -93,10 +93,10 @@ impl KbsProtocolWrapper { | |||
&mut self.http_client | |||
} | |||
|
|||
async fn attestation(&mut self, kbs_host_url: String) -> Result<String> { | |||
async fn attestation(&mut self, kbs_root_url: String) -> Result<String> { |
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.
maybe use kbs_url
directly which contains the scheme://domain-name[:port][/user]
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.
In many domain name allocation scenario, the root domain name assigned to KBS may have a more complex form. I believe we should leave enough flexibility for the form of the root domain name. Therefore, for now, I suggest not defining and resolving the format and content of the root domain name.
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.
maybe use
kbs_url
directly which contains thescheme://domain-name[:port][/user]
If we use kbs_url
here, it may be confused with the whole resource url.
Currently, our default host URL format for KBS is http://IP:PORT, But in productization, we may give host URL more forms. For example: http://my-kbs.com , http://my-kbs.com/user-alice Therefore, it is necessary to expand the supported KBS host URL format, More precisely, it should no longer be referred to as the "host URL", but rather as the "root URL". Signed-off-by: Jiale Zhang <zhangjiale@linux.alibaba.com>
Currently, our default host URL format for KBS is http://IP:PORT, But in productization, we may give host URL more forms. For example: http://my-kbs.com , http://my-kbs.com/user-alice Therefore, it is necessary to expand the supported KBS host URL format, More precisely, it should no longer be referred to as the "host URL", but rather as the "root URL".