-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add TLS support for http #39
Add TLS support for http #39
Conversation
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
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.
PTAL @Andrewmatilde
# Conflicts: # Cargo.lock # Cargo.toml # chaos-tproxy-controller/Cargo.toml # chaos-tproxy-proxy/Cargo.toml # chaos-tproxy-proxy/src/proxy/http/server.rs
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
derivative = "2.2.0" | ||
rustls-pemfile = "1.0.0" | ||
webpki-roots = "0.22" | ||
hyper-rustls = { git = "https://github.com/Andrewmatilde/hyper-rustls.git", features = ["http2"] } |
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 we can use dependency override to replace hyper globally instead of modifying it in each dependency.
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.
I think this is not the common use case of dependency override
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.
Lucky, something we need just merged rust-lang/cargo#10497
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.
I know what's the problem , I will fix 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.
It seems dependency override not fix the problem.
tracing::debug!(target : "Accept streaming", "remote={:?}, local={:?}", | ||
addr_remote, addr_local); | ||
let service = HttpService::new(addr_remote, | ||
addr_local, |
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.
Please format the code, there should be some idents.
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.
That didn't auto fmt becuase the code are inside the macro.
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.
I will fix 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.
Fixed.
} | ||
} | ||
|
||
loop { |
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.
Why not merge the two loops by a bit of abstraction?
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.
It seems there are only three lines code are same , so I think we do not need give an abstrction here.
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.
Fixed.
@@ -89,15 +153,15 @@ pub async fn serve_http_with_error_return( | |||
}, | |||
Err(e) => { | |||
return if e.is_parse() { | |||
tracing::debug!("{}:Turn into tcp transfer.", log_key); | |||
debug!("{}:Turn into tcp transfer.", log_key); |
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.
Please use the span instead of log_key
.
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.
Fixed.
parts.scheme = Some(Scheme::HTTP); | ||
} | ||
|
||
*request.uri_mut() = Uri::from_parts(parts)?; | ||
|
||
let client = Client::builder().build(HttpConnector::new(self.remote)); | ||
let mut response = if let Some(tls_client_config) = &self.tls_client_config { |
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.
There is duplicated code in two cases of if-else, please reuse them.
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.
Fixed.
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
Signed-off-by: andrewmatilde <[email protected]>
PTAL @Hexilee |
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.
LGTM
related issue :
#42
Signed-off-by: andrewmatilde [email protected]