fix: Honor c8y.mqtt setting on tedge connect #2789

albinsuresh · 2024-03-18T16:54:23Z

Proposed changes

Use the port specified in c8y.mqtt setting while establishing direct connection to c8y on tedge connect c8y
Refactor BridgeConfig to use HostPort instead of separate host and port settings

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

Custom MQTT port is not respected when connecting using tedge connect c8y #2787

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy 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

github-actions · 2024-03-18T17:56:57Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
428	0	3	428	100	0s

crates/core/tedge/src/cli/connect/command.rs

crates/core/tedge/src/bridge/aws.rs

codecov · 2024-03-20T15:39:33Z

Codecov Report

Attention: Patch coverage is 53.96825% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 76.6%. Comparing base (d55ddd2) to head (2fa7266).
Report is 5 commits behind head on main.

Additional details and impacted files

Files	Coverage Δ
.../common/tedge_config/src/tedge_config_cli/error.rs	`21.0% <ø> (ø)`
...ge_config/src/tedge_config_cli/models/host_port.rs	`76.5% <100.0%> (+9.1%)`	⬆️
...n/tedge_config/src/tedge_config_cli/models/port.rs	`75.0% <100.0%> (ø)`
crates/core/tedge/src/bridge/aws.rs	`98.1% <50.0%> (-1.0%)`	⬇️
crates/core/tedge/src/bridge/azure.rs	`98.1% <66.6%> (-1.0%)`	⬇️
crates/core/tedge/src/bridge/c8y.rs	`88.1% <33.3%> (-0.9%)`	⬇️
crates/core/tedge/src/cli/certificate/upload.rs	`61.8% <0.0%> (ø)`
crates/core/c8y_api/src/http_proxy.rs	`71.6% <73.6%> (-0.2%)`	⬇️
crates/core/tedge/src/bridge/config.rs	`81.5% <28.5%> (-2.8%)`	⬇️
...ore/tedge/src/cli/connect/c8y_direct_connection.rs	`0.0% <0.0%> (ø)`
... and 1 more

... and 6 files with indirect coverage changes

tests/RobotFramework/README.md

albinsuresh · 2024-03-22T09:05:04Z

crates/core/c8y_api/src/http_proxy.rs

+        // The configured `c8y.http` setting may have a port value specified,
+        // but the incoming URL is less likely to have any port specified.
+        // Hence just matching the host prefix.


The other option was to change the C8yEndPoint::c8y_host field into a HostPort and just validate the host part of it against the incoming host. But, that would be have been a huge change needing the generic port parameter <P> to be added to C8yEndPoint as well, since HostPort<P> is generic.

didier-wenzek

The system test "tedge-agent should terminate on SIGINT while downloading file" is failing, because the url http://localhost/speedlimit/10MB is erroneously rewritten http://127.0.0.1:8001/c8y/speedlimit/10MB, preventing the download to start.

Bravo555

Found one thing I'm unsure of, otherwise LGTM

crates/core/c8y_api/src/http_proxy.rs

didier-wenzek · 2024-03-28T10:18:06Z

crates/core/c8y_api/src/http_proxy.rs

        let url = Url::parse(url).ok()?;
        let url_host = url.domain()?;

        let (_, host) = url_host.split_once('.').unwrap_or((url_host, ""));
-        let (_, c8y_host) = tenant_uri.split_once('.').unwrap();
+        let (_, c8y_host) = tenant_host.split_once('.').unwrap();


Not sure this unwrap is safe. What's about a C8Y Edge tenant known only by its host name?

Yeah, this is something that @reubenmiller and I were discussing in the morning and the conclusion was that this whole subdomain based check is fragile and must be replaced with a proper check that retrieves the real tenant URL behind any custom domain and validates the incoming URL against that. But, since that work is a bit more involved, we decided to create a separate issue for that, and just resolve the port aspect in this PR (keeping the existing fragile mechanism as-is for now).

So, let's go with this.

Here is ticket for the custom domain issue: #2804

In the mean time, I've just updated the unwrap logic to at least avoid a panic.

didier-wenzek

Approved

albinsuresh requested review from didier-wenzek, jarhodes314 and rina23q as code owners March 18, 2024 16:54

albinsuresh temporarily deployed to Test Pull Request March 18, 2024 16:54 — with GitHub Actions Inactive

albinsuresh requested a review from Bravo555 March 18, 2024 16:54

albinsuresh had a problem deploying to Test Auto March 18, 2024 17:27 — with GitHub Actions Failure

didier-wenzek reviewed Mar 19, 2024

View reviewed changes

crates/core/tedge/src/cli/connect/command.rs Show resolved Hide resolved

reubenmiller changed the title ~~fix: Honor c8y.mqtt setting on tedge connect #2787~~ fix: Honor c8y.mqtt setting on tedge connect Mar 20, 2024

reubenmiller added theme:mqtt Theme: mqtt and mosquitto related topics theme:c8y Theme: Cumulocity related topics labels Mar 20, 2024

reubenmiller reviewed Mar 20, 2024

View reviewed changes

crates/core/tedge/src/bridge/aws.rs Show resolved Hide resolved

albinsuresh requested a review from a team as a code owner March 20, 2024 15:31

albinsuresh temporarily deployed to Test Pull Request March 20, 2024 15:31 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto March 20, 2024 15:38 — with GitHub Actions Failure

reubenmiller reviewed Mar 20, 2024

View reviewed changes

tests/RobotFramework/README.md Outdated Show resolved Hide resolved

albinsuresh force-pushed the fix/2787/use-c8y-mqtt-port branch from b37d514 to 845a49e Compare March 22, 2024 08:15

albinsuresh temporarily deployed to Test Pull Request March 22, 2024 08:15 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto March 22, 2024 08:22 — with GitHub Actions Failure

albinsuresh temporarily deployed to Test Pull Request March 22, 2024 08:57 — with GitHub Actions Inactive

albinsuresh requested review from reubenmiller and didier-wenzek March 22, 2024 08:57

albinsuresh commented Mar 22, 2024

View reviewed changes

albinsuresh had a problem deploying to Test Auto March 22, 2024 10:25 — with GitHub Actions Failure

albinsuresh temporarily deployed to Test Pull Request March 25, 2024 12:52 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto March 25, 2024 12:52 — with GitHub Actions Failure

didier-wenzek reviewed Mar 25, 2024

View reviewed changes

Bravo555 reviewed Mar 26, 2024

View reviewed changes

crates/core/c8y_api/src/http_proxy.rs Outdated Show resolved Hide resolved

albinsuresh temporarily deployed to Test Pull Request March 28, 2024 09:39 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto March 28, 2024 09:53 — with GitHub Actions Inactive

albinsuresh requested review from didier-wenzek and Bravo555 March 28, 2024 10:13

didier-wenzek reviewed Mar 28, 2024

View reviewed changes

didier-wenzek approved these changes Mar 28, 2024

View reviewed changes

albinsuresh added 2 commits March 28, 2024 11:29

fix: Honor c8y.mqtt setting on tedge connect thin-edge#2787

a175eaa

Update system test CA cert generation instruction

2fa7266

albinsuresh force-pushed the fix/2787/use-c8y-mqtt-port branch from 282bfb8 to 2fa7266 Compare March 28, 2024 11:30

albinsuresh temporarily deployed to Test Pull Request March 28, 2024 11:30 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto March 28, 2024 11:36 — with GitHub Actions Inactive

albinsuresh added this pull request to the merge queue Mar 28, 2024

Merged via the queue into thin-edge:main with commit 92f140d Mar 28, 2024
32 checks passed

albinsuresh deleted the fix/2787/use-c8y-mqtt-port branch April 17, 2024 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Honor c8y.mqtt setting on tedge connect #2789

fix: Honor c8y.mqtt setting on tedge connect #2789

albinsuresh commented Mar 18, 2024 •

edited by reubenmiller

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

albinsuresh Mar 22, 2024 •

edited

Loading

didier-wenzek left a comment

Bravo555 left a comment

didier-wenzek Mar 28, 2024

albinsuresh Mar 28, 2024 •

edited

Loading

didier-wenzek Mar 28, 2024

albinsuresh Mar 28, 2024

didier-wenzek left a comment

fix: Honor c8y.mqtt setting on tedge connect #2789

fix: Honor c8y.mqtt setting on tedge connect #2789

Conversation

albinsuresh commented Mar 18, 2024 • edited by reubenmiller Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

github-actions bot commented Mar 18, 2024 • edited Loading

Robot Results

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

albinsuresh Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Bravo555 left a comment

Choose a reason for hiding this comment

didier-wenzek Mar 28, 2024

Choose a reason for hiding this comment

albinsuresh Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

didier-wenzek Mar 28, 2024

Choose a reason for hiding this comment

albinsuresh Mar 28, 2024

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

albinsuresh commented Mar 18, 2024 •

edited by reubenmiller

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

albinsuresh Mar 22, 2024 •

edited

Loading

albinsuresh Mar 28, 2024 •

edited

Loading