-
Notifications
You must be signed in to change notification settings - Fork 67
fix: support HTTP proxy in c8y remote access plugin #3600
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
fix: support HTTP proxy in c8y remote access plugin #3600
Conversation
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
1a8cd28
to
957f742
Compare
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Robot Results
|
let stream = TcpStream::connect(&host_port).await.into_diagnostic()?; | ||
let mut stream = match address.scheme() { | ||
ProxyScheme::Https => { | ||
let connector: TlsConnector = config.clone().unwrap().into(); |
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.
Could the config
be None
?
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.
Not in practice. In the unit tests, we set config to None
in order to avoid TLS on the server it's hosting, but for the plugin code that calls this, we always have this value set
@@ -77,6 +77,7 @@ assert-json-diff = "2.0" | |||
assert_cmd = "2.0" | |||
assert_matches = "1.5" | |||
async-compat = "0.2.1" | |||
async-http-proxy = "1.2" |
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.
Being surprised to see that this has not been required sooner, notably for c8y_auth_proxy
to connect c8y, I added a test 4391f1e. Indeed, this dependency is not required before because c8y_auth_proxy
's connection is opened by reqwest
which is properly configured with the proxy when it applies.
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.
Approved
I've pushed a system test to verify the remote access access. |
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.
Approved (and tested it locally)
Signed-off-by: reubenmiller <reuben.d.miller@gmail.com>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
651d642
to
136e46e
Compare
Proposed changes
Support the HTTP proxy configuration in the c8y remote access plugin. This was an accidental omission with the proxy feature.
Types of changes
Paste Link to the issue
Checklist
just prepare-dev
once)just format
as mentioned in CODING_GUIDELINESjust check
as mentioned in CODING_GUIDELINESFurther comments