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

feat: use direct c8y connection for c8y-remote-access-plugin #3006

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Jul 18, 2024

Proposed changes

Remove the c8y-remote-access-plugin connection dependency on the c8y local proxy which is launched by the tedge-mapper-c8y service. This is part one of the large ticket (#2859) which looks at creating the remote access connection which is fully independent to the tedge-mapper-c8y service, thus allowing the user to execute tedge reconnect c8y without the remote access connection from being dropped.

Note

Prior to the c8y local proxy feature, the c8y-remote-access-plugin did use a direct connection to Cumulocity IoT, however the implementation was switch to use the local proxy which adds an additional connection dependency on the tedge-mapper-c8y (e.g. if you restart the tedge-mapper-c8y, the local proxy will also restart thus stopping the connection). This PR does not change the fact that there is still a process dependency between the spawned c8y-remote-access-plugin instance and the tedge-mapper-c8y service, however this will be addressed via #3007

The PR is based on reverting the c8y local proxy commit: 845480e

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

#2859

Use a direct connection to the Cumulocity IoT URL instead of using the local c8y proxy service (as the service is maintained by the tedge-mapper-c8y service) - (technically this will be reverting the change where we switched to using the local c8y proxy)

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

@rina23q rina23q temporarily deployed to Test Pull Request July 18, 2024 14:31 — with GitHub Actions Inactive
@rina23q rina23q added improvement User value theme:c8y Theme: Cumulocity related topics labels Jul 18, 2024
Copy link
Contributor

github-actions bot commented Jul 18, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
478 0 2 478 100 1h11m13.032927999s

@didier-wenzek
Copy link
Contributor

@jarhodes314 Can you please review this PR. Thanks

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (1c68fab) to head (2d897d3).
Report is 15 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
plugins/c8y_remote_access_plugin/src/proxy.rs 32.0% <0.0%> (+0.8%) ⬆️
plugins/c8y_remote_access_plugin/src/auth.rs 0.0% <0.0%> (ø)
plugins/c8y_remote_access_plugin/src/lib.rs 24.0% <0.0%> (+2.2%) ⬆️

... and 8 files with indirect coverage changes

@rina23q rina23q added theme:plugins Theme: Plugin interfaces or general plugin topics which don't fit in any other themes theme:troubleshooting Theme: Troubleshooting and remote control labels Jul 18, 2024
@reubenmiller reubenmiller requested review from Bravo555, gligorisaev and a team as code owners July 18, 2024 19:02
@reubenmiller reubenmiller temporarily deployed to Test Pull Request July 18, 2024 19:03 — with GitHub Actions Inactive
@reubenmiller
Copy link
Contributor

I've pushed some system tests, but don't be surprised if there are a few failures as the remote access has a few dependencies (e.g. generate ssh keypair, install go-c8y-cli, calling go-c8y-cli to create the passthrough configuration, and then run the ssh command).

rina23q and others added 2 commits July 18, 2024 21:55
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller reubenmiller force-pushed the improve/2859/revert-c8y-remote-access-plugin-to-use-direct-connection branch from 6e7b5e9 to 2d897d3 Compare July 18, 2024 19:56
@reubenmiller reubenmiller temporarily deployed to Test Pull Request July 18, 2024 19:56 — with GitHub Actions Inactive
Copy link
Contributor

@gligorisaev gligorisaev left a comment

Choose a reason for hiding this comment

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

The added Test Step is OK, however having in mind that this test has additional test steps comming from #3007 I will do the more detail check in that PR

@reubenmiller
Copy link
Contributor

The added Test Step is OK, however having in mind that this test has additional test steps coming from #3007 I will do the more detail check in that PR

Though we can't yet test the independence of the connection as a socket activated service is required for this (which is provided in #3007)

Copy link
Contributor

@Bravo555 Bravo555 left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with the OG commit that's being reverted, but I don't see anything wrong.

@reubenmiller reubenmiller changed the title fix: revert c8y_remote_access_plugin to use direct connection to c8y feat: use direct c8y connection for c8y-remote-access-plugin Jul 19, 2024
@reubenmiller reubenmiller added this pull request to the merge queue Jul 19, 2024
Merged via the queue into thin-edge:main with commit c6dd0ee Jul 19, 2024
33 checks passed
@reubenmiller reubenmiller removed the theme:plugins Theme: Plugin interfaces or general plugin topics which don't fit in any other themes label Aug 1, 2024
@gligorisaev
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement User value theme:c8y Theme: Cumulocity related topics theme:troubleshooting Theme: Troubleshooting and remote control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants