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: Support multiple cloud profiles #3278

Merged
merged 17 commits into from
Dec 13, 2024

Conversation

jarhodes314
Copy link
Contributor

@jarhodes314 jarhodes314 commented Dec 3, 2024

Proposed changes

Following the merging of #3262 early to limit conflicts, there are some outstanding tasks:

Current status:

  • Allow profiled configurations to be overridden with environment variables
  • Change the bridge local client ID to be backwards compatible, to avoid lost messages
  • Test the conflicting configuration validations for tedge connect (unit tests now added, will add a system test too)
  • Clean up the system test that was largely copied from the existing configuration operation test file
  • Support a --profile argument as an alternative to cloud@profile syntax to make scripting simpler

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

@@ -79,17 +75,13 @@ Setup Second Device
Execute Command tedge config set [email protected]_prefix c8y-second
Execute Command tedge config set [email protected] "$(tedge config get c8y.url)"

Execute Command tedge cert create c8y@second --device-id ${child_sn}
Execute Command tedge cert create c8y@second --device-id ${second_device_sn}
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@albinsuresh pointed out doing this will bypass the automatic cleanup we get for the devices we create. I'm not sure exactly how best to perform this clean-up. @reubenmiller are you able to help with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can ignore the cleanup for now (for this PR). We can easily add a new keyword to register a device for cleanup once the suite has finished.

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
545 0 2 545 100 1h37m27.122302s

@didier-wenzek
Copy link
Contributor

$ invoke flake-finder --test-name "Test non-mosquitto-bridge service status mapping" --iterations 10 --outputdir output_ff --clean
------------------------------
Overall: PASSED
Results: 10 iterations, 10 passed, 0 failed

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.


pub type Cloud = MaybeBorrowedCloud<'static>;
#[derive(clap::Args, PartialEq, Eq, Debug, Clone)]
pub struct CloudArgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment would help to understand when to use CloudArgs vs OptionalCloudArgs vs Option<ProfileName> as a clap argument.

  • tedge cert create uses OptionalCloudArgs letting the user creates a certificate for a specific cloud profile as well as a generic one not specifically attached to some cloud profile.
  • tedge connect uses CloudArgs because a cloud is required, possibly with a specific profile.
  • tedge cert upload c8y uses Option<ProfileName> because this feature is only available for Cumulocity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now fixed this by changing this to be a subcommand, so we can wrap the result is in Option. I guess the point around cert upload is still relevant, but I think it's fairly obvious why that is a special case.

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.

Really nice feature. Just a few open todos:

Summary

  • Item 1: Help for tedge cert create help text has duplicate --device-id entries
  • Item 4: Add TEDGE_CLOUD_PROFILE variable to the command's help text
  • Item 6: Remove references to @ syntax in error message and config list

In Follow-up PR

  • Item 2: include mapper config when testing the cloud connection (same as tedge connect c8y)
  • Item 3: add cloud profile information into the tedge connect output
  • Item 5: Add maintainer script snippets to handle systemd template certificates - @reubenmiller

See the following sections for the addition details.

Action item details

Item 1: Help for tedge cert create help text has duplicate --device-id entries

tedge cert create c8y --help

Usage: tedge cert create --device-id <ID> c8y [OPTIONS] --device-id <ID>

Ideally the following should be supported:

tedge cert create c8y [OPTIONS] --device-id <ID>

tedge cert create c8y --device-id bar0001 --profile staging-latest

Item 2: include mapper config when testing the cloud connection (same as tedge connect c8y)

The test command would be more useful if it showed the configuration details as well during the test.

The following only shows that the connect to the "cloud" is ok, but not the url of the cloud (and not the tenant).

$ tedge connect c8y --profile staging-latest --test

Verifying device is connected to cloud... ✓
Checking Cumulocity is connected to intended tenant... ✓
Connection check to c8y cloud is successful.

Item 3: add cloud profile information into the tedge connect output

tedge reconnect c8y --profile staging-latest
Disconnecting from Cumulocity
Removing bridge config file... ✓
Restarting mosquitto to apply configuration... ✓
Disabling tedge-mapper-c8y@staging-latest... ✓
Reconnecting with config:
        device id: TST_develop_refined_assurance
        cloud host: iot.latest.stage.c8y.io:8883
        certificate file: /etc/tedge/device-certs/tedge-certificate.pem
        bridge: mosquitto
        service manager: systemd
        mosquitto version: 2.0.11

For example:

Reconnecting with config:
        device id: TST_develop_refined_assurance
        cloud host: iot.latest.stage.c8y.io:8883
        cloud profile: staging-latest
        cloud type: c8y
        certificate file: /etc/tedge/device-certs/tedge-certificate.pem
        bridge: mosquitto
        service manager: systemd
        mosquitto version: 2.0.11

Item 4: Add TEDGE_CLOUD_PROFILE variable to the command's help text

For example, add the env: section to the flag's help text (you may have to do this manually).

$ tedge connect c8y --help
Usage: tedge connect c8y [OPTIONS]

Options:
      --profile <PROFILE>
          The cloud profile you wish to use
          [env: TEDGE_CLOUD_PROFILE]

If it is too much effort, then we can add this in the next release (as we're having a bit of feature creep)

Item 5: Add maintainer script snippets to handle systemd template services

The systemd template services (e.g. [email protected]) are not correctly stopped/started/enabled in deb and rpm maintainer scripts (e.g. preinst, prerm, postinst).

Example snippet:

systemctl show "tedge-mapper-c8y@*" --state=loaded --property=Id --value --no-pager | while read -r UNIT; do
        echo "Restart service: $UNIT" >&2
        systemctl restart "$UNIT"
done

Item 6: Remove references to @ syntax in error message and config list

Remove guidance using the @ suffix syntax from error messages.

$ tedge connect c8y --profile staging-latest
error: The configurations: c8y.bridge.topic_prefix, [email protected]_prefix should be set to different values before connecting, but are currently set to the same value

Why is the @ syntax shown in the config list

$ tedge config list 'c8y@'

Why can't it following the tedge.toml data format?

For example, given:

[c8y.profiles.staging-latest]
url = "iot.latest.stage.c8y.io"

Then just support the syntax as follows:

c8y.profiles.staging-latest.url

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.

Thank you for your sustained effort on improving the UX experience of users and devs using tedge config.

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.

Retested the changes and everything looks great. I'll create a follow up PR to address the packaging changes (e.g. maintainer script changes regarding the service management).

@jarhodes314 jarhodes314 force-pushed the feat/profiles-ux-improvments branch from 1a3782e to c077ab0 Compare December 13, 2024 11:43
@jarhodes314 jarhodes314 added this pull request to the merge queue Dec 13, 2024
Merged via the queue into thin-edge:main with commit be7cd20 Dec 13, 2024
33 checks passed
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.

LGTM.

Comment on lines +68 to +73
if display_name == format!("mosquitto-{prefix}-bridge") && status == Status::Up {
// Receiving this message indicates mosquitto has reconnected (following a
// disconnection) to the cloud. We need to re-request operations in case any
// were triggered while we were down
value.push(create_get_pending_operations_message(prefix));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, it would have been better if this was not done from this simple "conversion" function, but done from the calling process_health_status_message function. But I understand that doing this from here, when you have access to display_name simplifies the code.

# Get configuration
#

*** Test Cases ***
Copy link
Contributor

@albinsuresh albinsuresh Dec 13, 2024

Choose a reason for hiding this comment

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

Suggested change
*** Test Cases ***
*** Test Cases ***
# Since the test framework does not have access to multiple cloud tenants,
# multi cloud profile support is tested by connecting the same device to the same cloud
# using different external identities configured using different profiles.
# This setup results in a single "device" having two device twin identities in the cloud.
# That same "device" can be controlled via either twins.

Adding some commentary would be nice to help others looking at this test and going: "where are those multiple cloud connections?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants