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

refactor: support writable device ids #3318

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jarhodes314
Copy link
Contributor

@jarhodes314 jarhodes314 commented Jan 8, 2025

Proposed changes

This adds support for a writable device ID to aid @rina23q's work. I'm marking this as a refactoring as it doesn't really perform the task in itself, although I have modified tedge_config.rs to use the new feature for verifying it actually works as intended.

Still TODO:

  • Support a default value, so c8y.device.id defaults to device.id
  • Make the device ID logic actually do something useful
  • Check the test names are appropriate
  • Make config.device.id() private

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

fn device_id(
device: &TEdgeConfigReaderDevice,
dto_value: &OptionalConfig<String>,
) -> Result<String, ReadError> {
device_id_from_cert(&device.cert_path)
Copy link
Member

Choose a reason for hiding this comment

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

If device.id is set (= the value exists in dto), the value should be returned. It should be something like below. This applies to all functions for each cloud (c8y, az, aws).

Suggested change
device_id_from_cert(&device.cert_path)
match device.id.or_none() {
Some(id) => Ok(id.to_string()),
None => device_id_from_cert(&device.cert_path)
}

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.

I do think that the tedge config macro alone cannot solve the complexity introduced by this idea to have cloud specific device identifiers, especially when combined with identifiers that are either explicitly set by the user or derived from certificates.

The tedge cli will have to be updated to enforce that the device identifier for a given end-point is clearly defined without conflicting settings. For example, the following scenario must be prevented:

  • tedge config set device.id xxx
  • tedge config set device.cert_path cert-with-cn=yyy

Comment on lines 429 to 430
/// Identifier of the device within the fleet. It must be globally
/// unique and is derived from the device certificate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Identifier of the device within the fleet. It must be globally
/// unique and is derived from the device certificate.
/// Identifier of the device within the fleet.
/// Derived from the device certificate if there is one.
/// Used for all cloud end-points unless explicitly superseded.

To set 'device.id' to some <id>, you can use `tedge cert create --device-id <id>`.",
function = "device_id",
))]
#[tedge_config(reader(function = "device_id"))]
#[tedge_config(example = "Raspberrypi-4d18303a-6d3a-11eb-b1a6-175f6bb72665")]
#[tedge_config(note = "This setting is derived from the device certificate and is therefore read only.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[tedge_config(note = "This setting is derived from the device certificate and is therefore read only.")]

Comment on lines 484 to 485
/// Identifier of the device within the fleet. It must be globally
/// unique and is derived from the device certificate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Identifier of the device within the fleet. It must be globally
/// unique and is derived from the device certificate.
/// Identifier of the device for this cloud end-point.
/// Derived from the device certificate if there is one for this end-point.
/// Default to the global device id, unless explicitly superseded.

))]
#[tedge_config(reader(function = "c8y_device_id"))]
// TODO make this work
// #[tedge_config(default(from_optional_key = "device.id"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this default value simply encoded in the reader function (aka c8y_device_id)?

I even wonder if there is such a default value, the rules being complex:

  • derived from the cloud specific device certificate if any
  • or read from the cloud profile config if set
  • or derived from the global device certificate if any
  • or read from the config if set

Copy link
Member

Choose a reason for hiding this comment

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

I was also thinking that the default from device.id will work partially. This is the scenario which doesn't work.

c8y.device.cert_path and device.id exists, but c8y.device.id is missing. Then, I expect c8y.device.id will return the CN of the c8y.device.cert_path. However, my guess the config will return the value of device.id due to the default.

Copy link
Member

Choose a reason for hiding this comment

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

c8y.device.cert_path and device.id exists, but c8y.device.id is missing. Then, I expect c8y.device.id will return the CN of the c8y.device.cert_path. However, my guess the config will return the value of device.id due to the default.

As we discussed today, this is a corner case. It's fine that c8y.device.id returns the value of device.id instead of its CN. We just have to document that user should set c8y.device.id explicitly in that case.

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.

3 participants