Skip to content
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: use MQTT as well as HTTP host to determine possible tenant url #2977

Merged

Conversation

jarhodes314
Copy link
Contributor

Proposed changes

Determine whether URLs are Cumulocity URLs based on both the HTTP and MQTT host, not just the HTTP host.

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)
  • 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

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 87.30159% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (4a805f3) to head (1c68fab).
Report is 16 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
...rates/extensions/c8y_firmware_manager/src/tests.rs 92.6% <100.0%> (+<0.1%) ⬆️
crates/extensions/c8y_http_proxy/src/actor.rs 54.1% <100.0%> (+0.4%) ⬆️
crates/extensions/c8y_http_proxy/src/tests.rs 93.4% <100.0%> (+<0.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs 84.1% <100.0%> (+<0.1%) ⬆️
...es/extensions/c8y_mapper_ext/src/operations/mod.rs 88.0% <100.0%> (+<0.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs 92.7% <100.0%> (+<0.1%) ⬆️
crates/core/c8y_api/src/http_proxy.rs 75.5% <96.7%> (+2.9%) ⬆️
crates/extensions/c8y_mapper_ext/src/config.rs 48.5% <66.6%> (+0.2%) ⬆️
...ates/extensions/c8y_firmware_manager/src/config.rs 60.7% <50.0%> (-1.1%) ⬇️
crates/extensions/c8y_http_proxy/src/lib.rs 52.9% <0.0%> (-2.2%) ⬇️

... and 4 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jul 5, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
476 0 3 476 100 1h13m3.111446s

Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

The logic is pretty straightforward for those having separate c8y.http and c8y.mqtt settings, and implemented as described in the ticket.

But, my understanding was that the end user sometimes may not even be aware of the internal URLs and they are only exposed to the external custom domains. So, they probably wouldn't even know that they need to configure both settings separately. This is the reason why querying the https://your.custom.domain/tenant/loginOptions endpoint was proposed as a better fix approach in #2804.

But, if it is guaranteed that custom domain users would configure their tedge like this with both the settings, then I'm happy to approve this PR once the tests pass (a rebase required).

@reubenmiller
Copy link
Contributor

The logic is pretty straightforward for those having separate c8y.http and c8y.mqtt settings, and implemented as described in the ticket.

But, my understanding was that the end user sometimes may not even be aware of the internal URLs and they are only exposed to the external custom domains. So, they probably wouldn't even know that they need to configure both settings separately. This is the reason why querying the https://your.custom.domain/tenant/loginOptions endpoint was proposed as a better fix approach in #2804.

But, if it is guaranteed that custom domain users would configure their tedge like this with both the settings, then I'm happy to approve this PR once the tests pass (a rebase required).

We can address the "easy of configuration" aspect to a separate PR. This PR is purely focusing on the case where the user knows that they have to use two separate URLs for the MQTT and HTTP traffic, however thin-edge.io may receive binaries either using the internal URL OR the custom domain name (as the user can manually use the custom domain when uploading binaries to their tenant).

@reubenmiller reubenmiller added the theme:c8y Theme: Cumulocity related topics label Jul 10, 2024
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

@jarhodes314 jarhodes314 changed the title Use MQTT as well as HTTP host to determine possible tenant url fix: use MQTT as well as HTTP host to determine possible tenant url Jul 18, 2024
@jarhodes314 jarhodes314 added this pull request to the merge queue Jul 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2024
@didier-wenzek
Copy link
Contributor

There is a flaky test (unrelated to this PR) failing the merge queue :

test tests::test_cli_pub_basic::none_expects ... FAILED

failures:

---- tests::test_cli_pub_basic::none_expects stdout ----
thread 'tests::test_cli_pub_basic::none_expects' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/assert_cmd-2.0.13/src/output.rs:65:25:
command="/home/runner/work/thin-edge.io/thin-edge.io/target/llvm-cov-target/debug/tedge" "--config-dir" "/tmp/.tmp5EPpVm" "mqtt" "pub" "topic" "message"
code=1
stdout=""
stderr=
ERROR: the message has not been published
Error: failed to publish the message \"message\" on the topic \"topic\" with QoS \"AtMostOnce\".

Caused by:
    0: I/O: connection closed by peer
    1: connection closed by peer

@didier-wenzek didier-wenzek added this pull request to the merge queue Jul 18, 2024
Merged via the queue into thin-edge:main with commit 952b379 Jul 18, 2024
33 checks passed
@jarhodes314 jarhodes314 deleted the bug/custom-domain-tenant-matching branch July 18, 2024 19:06
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants