Skip to content

Conversation

jarhodes314
Copy link
Contributor

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

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314 jarhodes314 force-pushed the bug/remote-access-respect-proxy branch from 1a8cd28 to 957f742 Compare May 8, 2025 13:44
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request May 8, 2025 13:44 — with GitHub Actions Inactive
Copy link

codecov bot commented May 8, 2025

Codecov Report

Attention: Patch coverage is 87.95181% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
plugins/c8y_remote_access_plugin/src/lib.rs 0.00% 6 Missing ⚠️
plugins/c8y_remote_access_plugin/src/proxy.rs 94.80% 0 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented May 8, 2025

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
631 0 3 631 100 1h49m40.672333s

@reubenmiller reubenmiller added theme:troubleshooting Theme: Troubleshooting and remote control theme:c8y Theme: Cumulocity related topics labels May 8, 2025
let stream = TcpStream::connect(&host_port).await.into_diagnostic()?;
let mut stream = match address.scheme() {
ProxyScheme::Https => {
let connector: TlsConnector = config.clone().unwrap().into();
Copy link
Contributor

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?

Copy link
Contributor Author

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

@reubenmiller reubenmiller requested a review from a team as a code owner May 9, 2025 08:32
@reubenmiller reubenmiller temporarily deployed to Test Pull Request May 9, 2025 08:32 — with GitHub Actions Inactive
@@ -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"
Copy link
Contributor

@didier-wenzek didier-wenzek May 9, 2025

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.

@reubenmiller reubenmiller temporarily deployed to Test Pull Request May 9, 2025 08:46 — with GitHub Actions Inactive
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@reubenmiller
Copy link
Contributor

I've pushed a system test to verify the remote access access.

Copy link
Contributor

@reubenmiller reubenmiller left a 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)

reubenmiller and others added 2 commits May 9, 2025 15:30
Signed-off-by: reubenmiller <reuben.d.miller@gmail.com>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@reubenmiller reubenmiller force-pushed the bug/remote-access-respect-proxy branch from 651d642 to 136e46e Compare May 9, 2025 13:30
@reubenmiller reubenmiller temporarily deployed to Test Pull Request May 9, 2025 13:30 — with GitHub Actions Inactive
@reubenmiller reubenmiller enabled auto-merge May 9, 2025 13:43
@reubenmiller reubenmiller added this pull request to the merge queue May 9, 2025
@reubenmiller reubenmiller added the theme:connectivity Generic connectivity related stuff like HTTP proxy etc. label May 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 9, 2025
@jarhodes314 jarhodes314 added this pull request to the merge queue May 9, 2025
Merged via the queue into thin-edge:main with commit 261085b May 9, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:c8y Theme: Cumulocity related topics theme:connectivity Generic connectivity related stuff like HTTP proxy etc. theme:troubleshooting Theme: Troubleshooting and remote control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants