feat: add profile support to the `tedge` cli and fix mosquitto configuration by jarhodes314 · Pull Request #3262 · thin-edge/thin-edge.io

jarhodes314 · 2024-11-25T11:02:04Z

Proposed changes

This changes tedge connect etc. to use the <cloud>@<profile> syntax. For example,

tedge connect c8y --profile edge

becomes

tedge connect c8y@edge

It also fixes some issues that were causing mosquitto to fail to start when multiple cloud profiles are configured.

Additionally it adds the missing systemd service definitions, so you can now start tedge-mapper-c8y@profile to spawn a mapper instance with a profile selected.

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

codecov · 2024-11-25T11:12:37Z

Codecov Report

Attention: Patch coverage is 48.40580% with 356 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge/src/cli/connect/command.rs	0.00%	95 Missing ⚠️
crates/core/tedge/src/cli/common.rs	17.24%	47 Missing and 1 partial ⚠️
crates/core/tedge_mapper/src/lib.rs	25.49%	34 Missing and 4 partials ⚠️
.../tedge_config/src/tedge_config_cli/tedge_config.rs	76.76%	24 Missing and 9 partials ⚠️
crates/core/tedge/src/cli/certificate/cli.rs	11.76%	25 Missing and 5 partials ⚠️
...ig/src/system_services/managers/general_manager.rs	15.38%	22 Missing ⚠️
crates/core/tedge/src/cli/connect/cli.rs	0.00%	15 Missing ⚠️
crates/core/tedge/src/cli/refresh_bridges.rs	0.00%	13 Missing ⚠️
plugins/c8y_remote_access_plugin/src/lib.rs	0.00%	13 Missing ⚠️
...ommon/tedge_config/src/system_services/services.rs	18.18%	9 Missing ⚠️
... and 12 more

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2024-11-25T11:24:27Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
529	0	2	529	100	1h29m34.666915s

didier-wenzek

The UX is really nice and it's a real plus to have a certificate per cloud.

$ tedge config set c8y@pki.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key
$ tedge config set c8y@pki.device.cert_path /etc/tedge/device-certs/demo-c8y-ca.pem
$ tedge cert create --device-id demo-c8y-ca c8y@pki
Certificate was successfully created
$ tedge cert show c8y@pki
Device certificate: /etc/tedge/device-certs/demo-c8y-ca.pem
...
$ tedge cert show c8y    
Device certificate: /etc/tedge/device-certs/demo-device-888.pem
... untouched! nice!

However, error messages might be improved regarding this new CLOUD parameter on many tedge commands. Could the error message list the clouds?

$ tedge cert show c8y@foo
Error: missing configuration parameter

Caused by:
    Unknown profile `foo` for the multi-profile property c8y

But:

$ tedge cert show foo@pki
error: invalid value 'foo@pki' for '[CLOUD]': Unrecognised cloud type: "foo@pki"

For more information, try '--help'.

crates/core/tedge/src/cli/common.rs

reubenmiller · 2024-12-02T14:53:52Z

@jarhodes314 Overall it looks good, though I just have the following questions/observations:

Can users set profile specific settings using environment variables?

For example the following is rejected by zsh/bash:
```
$ export TEDGE_C8Y@HELLO_URL=hello.com
export: not valid in this context: TEDGE_C8Y@HELLO_URL
```

There might be still merit to supporting the --profile argument as it is in some cases easier for users, mainly for the tedge connect command

tedge config set c8y@pki.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key
tedge config set c8y@tenant1.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key
tedge config set c8y@tenant2.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key

# vs.
tedge config set c8y.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key --profile pki
tedge config set c8y.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key --profile tenant1
tedge config set c8y.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key --profile tenant2

The latter is a bit easier for users to manage, and it would be open to support the following (if the --profile could also be set via env variable). Though the --profile argument could be syntaxial sugar for the c8y@<profile> syntax (or vice versa)

export TEDGE_C8Y_PROFILE=pki
tedge config set c8y.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key

…files

jarhodes314 · 2024-12-02T15:21:51Z

Can users set profile specific settings using environment variables?

@reubenmiller Not currently, although I'm not convinced of the value of such a feature, given the point of profiles is to allow us to store separate cloud configurations inside tedge.toml. If you're using environment variables to configure tedge, then surely you can just set the right variables per process to manually manage your cloud configurations? I guess for temporarily overriding a value it might make sense.

There might be still merit to supporting the --profile argument as it is in some cases easier for users, mainly for the tedge connect command

I agree, and I'll have a look into how easy this is to achieve.

export TEDGE_C8Y_PROFILE=pki
tedge config set c8y.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key

I'm slightly worried allowing this to be controlled by an environment variable might lead to confusion/difficult to resolve scenarios. As far as I'm aware, it can be quite difficult to unset an environment variable, so if this was set and you wanted to modify the default profile, you'd have a hard time doing so. I don't think environment variables provide the same value when they're replacing a command line argument

Signed-off-by: James Rhodes <jarhodes314@gmail.com>

didier-wenzek

Approved.

jarhodes314 · 2024-12-02T17:20:21Z

Can users set profile specific settings using environment variables?

@reubenmiller Not currently, although I'm not convinced of the value of such a feature, given the point of profiles is to allow us to store separate cloud configurations inside tedge.toml. If you're using environment variables to configure tedge, then surely you can just set the right variables per process to manually manage your cloud configurations? I guess for temporarily overriding a value it might make sense.

There might be still merit to supporting the --profile argument as it is in some cases easier for users, mainly for the tedge connect command

I agree, and I'll have a look into how easy this is to achieve.
export TEDGE_C8Y_PROFILE=pki
tedge config set c8y.device.key_path /etc/tedge/device-certs/demo-c8y-ca.key
I'm slightly worried allowing this to be controlled by an environment variable might lead to confusion/difficult to resolve scenarios. As far as I'm aware, it can be quite difficult to unset an environment variable, so if this was set and you wanted to modify the default profile, you'd have a hard time doing so. I don't think environment variables provide the same value when they're replacing a command line argument

I've decided to put these changes in a separate PR as this one is already getting quite large and makes sense in itself. As neither of these suggestions are breaking changes, it will be easier to deal with these separately.

albinsuresh

LGTM. Just some cosmetic comments and test/doc improvements.

albinsuresh · 2024-12-02T14:06:22Z

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

+                    The device id is read from the device certificate and cannot be set directly.\n\
+                    To set 'device.id' to some <id>, you can use `tedge cert create --device-id <id>`.",


Suggested change

The device id is read from the device certificate and cannot be set directly.\n\

To set 'device.id' to some <id>, you can use `tedge cert create --device-id <id>`.",

The device id that is used to connect to this cloud instance,

which defaults to the main `device.id` setting, but can be overridden for this cloud.

The device id is read from the device certificate and cannot be set directly.\n\

To override the main `device.id` value for this cloud instance,

create a new certificate at a different location using `tedge cert create --device-id <id> --config-dir /some/other/location`."

and update the `key_path` and `cert_path` for this cloud instance using

`tedge config set c8y@<instance>.device.key_path` and `tedge config set c8y@<instance>.device.cert_path` respectively

albinsuresh · 2024-12-02T14:23:09Z

crates/common/tedge_config/src/system_services/managers/general_manager.rs


 #[derive(Debug, Copy, Clone)]
-enum ServiceCommand {
+enum ServiceCommand<'a> {


One of the reasons why I hate adding lifetime parameters to structs (esp existing ones), unless absolutely necessary, as it just litters the whole call stack with that parameter.

Could we have used owned ProfileNames in SystemService as well, as done for the Cloud enum or BridgeConfigXyzParams structs, since the cost of having those owned copies here is very minimal in the grand scheme of things? Or was there some other reason other than efficiency that forced you to use references here?

albinsuresh · 2024-12-02T17:57:11Z

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

+            local_clientid: if let Some(profile) = &profile_name {
+                format!("c8y-bridge@{profile}")
+            } else {
+                "c8y-bridge".into()


Suggested change

"c8y-bridge".into()

"Cumulocity".into()

If we don't use the same old client id, wouldn't that lead to message loses when thin-edge is self-updated, as the broker won't send any queued messages for the older client-id to the new one?

albinsuresh · 2024-12-02T18:10:28Z

crates/core/tedge/src/cli/common.rs

-    C8y,
-    Azure,
-    Aws,
+    C8y(Option<Cow<'a, ProfileName>>),


You really love Cows, don't you? 😉

Not a request for change, but, in the context of these commands that are only executed just a handful of times during the entire lifetime of a device, I wonder if it's really worth the added complexity, unless I'm missing some hidden benefits.

albinsuresh · 2024-12-02T18:29:04Z

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

+            .collect::<Vec<_>>()
+            .join(", ");
+
+        bail!("The configurations: {keys} should be set to diffrent values, but are currently set to the same value");


Suggested change

bail!("The configurations: {keys} should be set to diffrent values, but are currently set to the same value");

bail!("The configurations: {keys} should be set to different values, but are currently set to the same value");

albinsuresh · 2024-12-02T19:56:30Z

tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot

+
+Test Setup
+    Customize Operation Workflows    ${PARENT_SN}
+    Copy Configuration Files    ${PARENT_SN}


Even this step can be performed just once at the Suite Setup level itself, as repeating this step at Test Setup level just repeats the same steps on the same device.

albinsuresh · 2024-12-02T19:59:30Z

tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot

+# Get configuration
+#
+
+Get Configuration from Main Device


Suggested change

Get Configuration from Main Device

Get Configuration from Primary Cloud Connection

albinsuresh · 2024-12-02T19:59:44Z

tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot

+    Text file    topic_prefix=c8y    external_id=${PARENT_SN}    config_type=CONFIG1    device_file=/etc/config1.json
+    Binary file    topic_prefix=c8y    external_id=${PARENT_SN}    config_type=CONFIG1_BINARY    device_file=/etc/binary-config1.tar.gz
+
+Get Configuration from Second Device


Suggested change

Get Configuration from Second Device

Get Configuration from Profiled Cloud Connection

albinsuresh · 2024-12-02T20:01:23Z

tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot

+
+
+*** Variables ***
+${PARENT_SN}            ${EMPTY}


Suggested change

${PARENT_SN} ${EMPTY}

${DEVICE_SN} ${EMPTY}

Just because there's no parent or child devices in this test.

albinsuresh · 2024-12-02T20:02:20Z

configuration/init/systemd/tedge-mapper-aws@.service

@@ -0,0 +1,14 @@
+[Unit]
+Description=tedge-mapper-aws checks Thin Edge JSON measurements and forwards to AWS IoT Hub.


Suggested change

Description=tedge-mapper-aws checks Thin Edge JSON measurements and forwards to AWS IoT Hub.

Description=tedge-mapper-aws checks Thin Edge JSON measurements and forwards to AWS IoT Core.

You've only inherited this from the existing non-templated version, which was wrong.

albinsuresh · 2024-12-02T20:22:28Z

tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot

+    Execute Command
+    ...    cmd=sudo env C8Y_USER='${C8Y_CONFIG.username}' C8Y_PASSWORD='${C8Y_CONFIG.password}' tedge cert upload c8y@second
+
+    Execute Command    tedge connect c8y@second


One unforseen side effect of these "secondary device twins" are that they're never auto-deleted from the connected tenant.

jarhodes314 temporarily deployed to Test Pull Request November 25, 2024 11:02 — with GitHub Actions Inactive

jarhodes314 changed the title ~~Add profile support to the tedge cli and fix mosquitto configuration~~ Add profile support to the tedge cli and fix mosquitto configuration Nov 25, 2024

jarhodes314 had a problem deploying to Test Auto November 25, 2024 11:07 — with GitHub Actions Failure

jarhodes314 force-pushed the feat/profile-support-in-tedge branch from bc8fd93 to 8f9ab76 Compare November 25, 2024 17:21

jarhodes314 had a problem deploying to Test Pull Request November 25, 2024 17:21 — with GitHub Actions Failure

jarhodes314 force-pushed the feat/profile-support-in-tedge branch from 8f9ab76 to dba0000 Compare November 26, 2024 10:13

jarhodes314 temporarily deployed to Test Pull Request November 26, 2024 10:13 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Auto November 26, 2024 12:52 — with GitHub Actions Inactive

jarhodes314 had a problem deploying to Test Pull Request November 27, 2024 14:12 — with GitHub Actions Failure

jarhodes314 had a problem deploying to Test Pull Request November 28, 2024 09:51 — with GitHub Actions Failure

jarhodes314 had a problem deploying to Test Pull Request November 28, 2024 18:33 — with GitHub Actions Failure

jarhodes314 marked this pull request as ready for review November 29, 2024 10:36

jarhodes314 requested review from a team, albinsuresh, didier-wenzek, gligorisaev, reubenmiller and rina23q as code owners November 29, 2024 10:36

jarhodes314 force-pushed the feat/profile-support-in-tedge branch from 8081bf8 to f9abca3 Compare November 29, 2024 11:23

jarhodes314 had a problem deploying to Test Pull Request November 29, 2024 11:23 — with GitHub Actions Failure

jarhodes314 temporarily deployed to Test Pull Request November 29, 2024 11:29 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Auto November 29, 2024 12:07 — with GitHub Actions Inactive

jarhodes314 had a problem deploying to Test Pull Request November 29, 2024 17:31 — with GitHub Actions Failure

jarhodes314 force-pushed the feat/profile-support-in-tedge branch from 2883409 to 4acb430 Compare November 29, 2024 17:31

jarhodes314 had a problem deploying to Test Pull Request November 29, 2024 17:31 — with GitHub Actions Failure

didier-wenzek reviewed Nov 29, 2024

View reviewed changes

crates/core/tedge/src/cli/common.rs Outdated Show resolved Hide resolved

Add profile support to the tedge cli and fix mosquitto configuration

d41ca30

jarhodes314 added 4 commits December 2, 2024 14:59

Add a copy of device configuration to cloud config and test cloud pro…

6574bee

…files

Update doctest

b3edce6

Fix clippy issues

a86a30b

Validate configuration doesn't conflict

79d60eb

jarhodes314 force-pushed the feat/profile-support-in-tedge branch from 4acb430 to 822cc8e Compare December 2, 2024 15:00

jarhodes314 had a problem deploying to Test Pull Request December 2, 2024 15:00 — with GitHub Actions Failure

jarhodes314 force-pushed the feat/profile-support-in-tedge branch from 822cc8e to 677eb5b Compare December 2, 2024 15:12

jarhodes314 had a problem deploying to Test Pull Request December 2, 2024 15:12 — with GitHub Actions Failure

Show valid clouds on command failure

ec86468

Signed-off-by: James Rhodes <jarhodes314@gmail.com>

jarhodes314 force-pushed the feat/profile-support-in-tedge branch from 677eb5b to ec86468 Compare December 2, 2024 15:24

jarhodes314 temporarily deployed to Test Pull Request December 2, 2024 15:24 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Auto December 2, 2024 15:31 — with GitHub Actions Inactive

didier-wenzek approved these changes Dec 2, 2024

View reviewed changes

jarhodes314 changed the title ~~Add profile support to the tedge cli and fix mosquitto configuration~~ feat: add profile support to the tedge cli and fix mosquitto configuration Dec 2, 2024

jarhodes314 added this pull request to the merge queue Dec 2, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 2, 2024

jarhodes314 added theme:c8y Theme: Cumulocity related topics theme:aws Theme: AWS cloud related topics theme:az Theme: Azure IoT related topics theme:cli Theme: cli related topics labels Dec 2, 2024

jarhodes314 added this pull request to the merge queue Dec 2, 2024

Merged via the queue into thin-edge:main with commit 985c980 Dec 2, 2024

albinsuresh reviewed Dec 2, 2024

View reviewed changes

albinsuresh mentioned this pull request Dec 3, 2024

feat: tedge upload c8y #3274

Merged

11 tasks

jarhodes314 mentioned this pull request Dec 3, 2024

feat: Support multiple cloud profiles #3278

Merged

16 tasks

reubenmiller added the skip-release-notes Don't include the ticket in the auto generated release notes label Dec 16, 2024

		The device id is read from the device certificate and cannot be set directly.\n\
		To set 'device.id' to some <id>, you can use `tedge cert create --device-id <id>`.",

-                    The device id is read from the device certificate and cannot be set directly.\n\
-                    To set 'device.id' to some <id>, you can use `tedge cert create --device-id <id>`.",
+                    The device id that is used to connect to this cloud instance,
+                    which defaults to the main `device.id` setting, but can be overridden for this cloud.
+                    The device id is read from the device certificate and cannot be set directly.\n\
+                    To override the main `device.id` value for this cloud instance,
+                    create a new certificate at a different location using `tedge cert create --device-id <id> --config-dir /some/other/location`."
+                    and update the `key_path` and `cert_path` for this cloud instance using
+                    `tedge config set c8y@<instance>.device.key_path` and `tedge config set c8y@<instance>.device.cert_path` respectively

	bail!("The configurations: {keys} should be set to diffrent values, but are currently set to the same value");
	bail!("The configurations: {keys} should be set to different values, but are currently set to the same value");

	Get Configuration from Main Device
	Get Configuration from Primary Cloud Connection

	Get Configuration from Second Device
	Get Configuration from Profiled Cloud Connection

		@@ -0,0 +1,14 @@
		[Unit]
		Description=tedge-mapper-aws checks Thin Edge JSON measurements and forwards to AWS IoT Hub.

Conversation

jarhodes314 commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

codecov bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reubenmiller commented Dec 2, 2024

Uh oh!

jarhodes314 commented Dec 2, 2024

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

jarhodes314 commented Dec 2, 2024

Uh oh!

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albinsuresh Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jarhodes314 commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

albinsuresh Dec 2, 2024 •

edited

Loading