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
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 16 additions & 20 deletions crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,15 +428,9 @@ define_tedge_config! {
device: {
/// Identifier of the device within the fleet. It must be globally
/// unique and is derived from the device certificate.
Comment on lines 429 to 430
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.

#[tedge_config(readonly(
write_error = "\
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>`.",
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.")]

#[tedge_config(reader(private))]
#[doku(as = "String")]
id: Result<String, ReadError>,

Expand Down Expand Up @@ -489,14 +483,10 @@ define_tedge_config! {
device: {
/// Identifier of the device within the fleet. It must be globally
/// unique and is derived from the device certificate.
Comment on lines 484 to 485
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(readonly(
write_error = "\
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>`.",
function = "c8y_device_id",
))]
#[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.

#[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.")]
#[doku(as = "String")]
id: Result<String, ReadError>,

Expand Down Expand Up @@ -1355,26 +1345,32 @@ fn default_http_bind_address(dto: &TEdgeConfigDto) -> IpAddr {

fn device_id_from_cert(cert_path: &Utf8Path) -> Result<String, ReadError> {
let pem = PemCertificate::from_pem_file(cert_path)
.map_err(|err| cert_error_into_config_error(ReadOnlyKey::DeviceId.to_cow_str(), err))?;
.map_err(|err| cert_error_into_config_error(ReadableKey::DeviceId.to_cow_str(), err))?;
let device_id = pem
.subject_common_name()
.map_err(|err| cert_error_into_config_error(ReadOnlyKey::DeviceId.to_cow_str(), err))?;
.map_err(|err| cert_error_into_config_error(ReadableKey::DeviceId.to_cow_str(), err))?;
Ok(device_id)
}

fn device_id(device: &TEdgeConfigReaderDevice) -> Result<String, ReadError> {
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)
}

}

fn c8y_device_id(device: &TEdgeConfigReaderC8yDevice) -> Result<String, ReadError> {
fn c8y_device_id(
device: &TEdgeConfigReaderC8yDevice,
dto_value: &OptionalConfig<String>,
) -> Result<String, ReadError> {
device_id_from_cert(&device.cert_path)
}

fn az_device_id(device: &TEdgeConfigReaderAzDevice) -> Result<String, ReadError> {
fn az_device_id(device: &TEdgeConfigReaderAzDevice, _: &()) -> Result<String, ReadError> {
device_id_from_cert(&device.cert_path)
}

fn aws_device_id(device: &TEdgeConfigReaderAwsDevice) -> Result<String, ReadError> {
fn aws_device_id(device: &TEdgeConfigReaderAwsDevice, _: &()) -> Result<String, ReadError> {
device_id_from_cert(&device.cert_path)
}

Expand Down
2 changes: 1 addition & 1 deletion crates/common/tedge_config_macros/examples/macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ define_tedge_config! {
}
}

fn device_id(_reader: &TEdgeConfigReaderDevice) -> Result<String, ReadError> {
fn device_id(_reader: &TEdgeConfigReaderDevice, _: &()) -> Result<String, ReadError> {
Ok("dummy-device-id".to_owned())
}

Expand Down
148 changes: 147 additions & 1 deletion crates/common/tedge_config_macros/impl/src/dto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use quote::quote_spanned;
use syn::parse_quote_spanned;
use syn::spanned::Spanned;

use crate::error::extract_type_from_result;
use crate::input::FieldOrGroup;
use crate::prefixed_type_name;

Expand All @@ -21,7 +22,17 @@ pub fn generate(
for item in items {
match item {
FieldOrGroup::Field(field) => {
if !field.dto().skip && field.read_only().is_none() {
if field.reader_function().is_some() {
let ty = match extract_type_from_result(field.ty()) {
Some((ok, _err)) => ok,
None => field.ty(),
};
idents.push(field.ident());
tys.push(parse_quote_spanned!(ty.span() => Option<#ty>));
sub_dtos.push(None);
preserved_attrs.push(field.attrs().iter().filter(is_preserved).collect());
extra_attrs.push(quote! {});
} else if !field.dto().skip && field.read_only().is_none() {
idents.push(field.ident());
tys.push({
let ty = field.ty();
Expand Down Expand Up @@ -106,7 +117,11 @@ fn is_preserved(attr: &&syn::Attribute) -> bool {

#[cfg(test)]
mod tests {
use proc_macro2::Span;
use syn::parse_quote;
use syn::Ident;
use syn::Item;
use syn::ItemStruct;

use super::*;

Expand Down Expand Up @@ -137,4 +152,135 @@ mod tests {
#[unknown_attribute = "some value"]
)))
}

#[test]
fn dto_is_generated() {
let input: crate::input::Configuration = parse_quote!(
c8y: {
url: String,
},
sudo: {
enable: bool,
},
);

let generated = generate_test_dto(&input);
let expected = parse_quote! {
#[derive(Debug, Default, ::serde::Deserialize, ::serde::Serialize, PartialEq)]
#[non_exhaustive]
pub struct TEdgeConfigDto {
#[serde(default)]
#[serde(skip_serializing_if = "TEdgeConfigDtoC8y::is_default")]
pub c8y: TEdgeConfigDtoC8y,
#[serde(default)]
#[serde(skip_serializing_if = "TEdgeConfigDtoSudo::is_default")]
pub sudo: TEdgeConfigDtoSudo,
}

impl TEdgeConfigDto {
#[allow(unused)]
fn is_default(&self) -> bool {
self == &Self::default()
}
}

#[derive(Debug, Default, ::serde::Deserialize, ::serde::Serialize, PartialEq)]
#[non_exhaustive]
pub struct TEdgeConfigDtoC8y {
pub url: Option<String>,
}

impl TEdgeConfigDtoC8y {
#[allow(unused)]
fn is_default(&self) -> bool {
self == &Self::default()
}
}

#[derive(Debug, Default, ::serde::Deserialize, ::serde::Serialize, PartialEq)]
#[non_exhaustive]
pub struct TEdgeConfigDtoSudo {
pub enable: Option<bool>,
}

impl TEdgeConfigDtoSudo {
#[allow(unused)]
fn is_default(&self) -> bool {
self == &Self::default()
}
}
};

assert_eq(&generated, &expected);
}

#[test]
fn ok_type_is_extracted_from_reader_function_if_relevant() {
let input: crate::input::Configuration = parse_quote!(
device: {
#[tedge_config(reader(function = "c8y_device_id"))]
id: Result<String, ReadError>,
}
);

let mut generated = generate_test_dto(&input);
generated
.items
.retain(only_struct_named("TEdgeConfigDtoDevice"));

let expected = parse_quote! {
#[derive(Debug, Default, ::serde::Deserialize, ::serde::Serialize, PartialEq)]
#[non_exhaustive]
pub struct TEdgeConfigDtoDevice {
pub id: Option<String>,
}
};

assert_eq(&generated, &expected);
}

#[test]
fn reader_function_type_is_used_verbatim_in_dto_if_not_result() {
let input: crate::input::Configuration = parse_quote!(
device: {
#[tedge_config(reader(function = "c8y_device_id"))]
id: String,
}
);

let mut generated = generate_test_dto(&input);
generated
.items
.retain(only_struct_named("TEdgeConfigDtoDevice"));

let expected = parse_quote! {
#[derive(Debug, Default, ::serde::Deserialize, ::serde::Serialize, PartialEq)]
#[non_exhaustive]
pub struct TEdgeConfigDtoDevice {
pub id: Option<String>,
}
};

assert_eq(&generated, &expected);
}

fn generate_test_dto(input: &crate::input::Configuration) -> syn::File {
let tokens = super::generate(
Ident::new("TEdgeConfigDto", Span::call_site()),
&input.groups,
"",
);
syn::parse2(tokens).unwrap()
}

fn assert_eq(actual: &syn::File, expected: &syn::File) {
pretty_assertions::assert_eq!(
prettyplease::unparse(&actual),
prettyplease::unparse(&expected),
)
}

fn only_struct_named<'a>(target: &'a str) -> impl Fn(&Item) -> bool + 'a {
move |i| matches!(i, Item::Struct(ItemStruct { ident, .. }) if ident == target)
}
}
3 changes: 2 additions & 1 deletion crates/common/tedge_config_macros/impl/src/input/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

// FIXME: if let can be simplified with `.unwrap_or_default()`
// for all `#[darling(default)]`
#![allow(clippy::manual_unwrap_or_default)]
// #![allow(clippy::manual_unwrap_or_default)]

use darling::util::SpannedValue;
use darling::FromAttributes;
Expand Down Expand Up @@ -198,6 +198,7 @@ pub struct FieldDtoSettings {
pub struct ReaderSettings {
#[darling(default)]
pub private: bool,
pub function: Option<syn::Path>,
#[darling(default)]
pub skip: bool,
}
Expand Down
58 changes: 58 additions & 0 deletions crates/common/tedge_config_macros/impl/src/input/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use syn::parse_quote_spanned;
use syn::spanned::Spanned;

use crate::error::combine_errors;
use crate::error::extract_type_from_result;
use crate::optional_error::OptionalError;
use crate::optional_error::SynResultExt;
use crate::reader::PathItem;
Expand Down Expand Up @@ -207,6 +208,40 @@ impl ReadOnlyField {
}
}

impl ReadWriteField {
pub fn lazy_reader_name(&self, parents: &[PathItem]) -> syn::Ident {
format_ident!(
"LazyReader{}{}",
parents
.iter()
.filter_map(PathItem::as_static)
.map(|p| p.to_string().to_upper_camel_case())
.collect::<Vec<_>>()
.join(""),
self.rename()
.map(<_>::to_owned)
.unwrap_or_else(|| self.ident.to_string())
.to_upper_camel_case()
)
}

pub fn parent_name(&self, parents: &[PathItem]) -> syn::Ident {
format_ident!(
"TEdgeConfigReader{}",
parents
.iter()
.filter_map(PathItem::as_static)
.map(|p| p.to_string().to_upper_camel_case())
.collect::<Vec<_>>()
.join(""),
)
}

pub fn rename(&self) -> Option<&str> {
Some(self.rename.as_ref()?.as_str())
}
}

#[derive(Debug)]
pub struct ReadWriteField {
pub attrs: Vec<syn::Attribute>,
Expand Down Expand Up @@ -301,6 +336,21 @@ impl ConfigurableField {
}
}

pub fn reader_function(&self) -> Option<(&syn::Path, &ReadWriteField)> {
match self {
Self::ReadWrite(
field @ ReadWriteField {
reader:
ReaderSettings {
function: Some(f), ..
},
..
},
) => Some((f, field)),
_ => None,
}
}

pub fn deprecated_keys(&self) -> impl Iterator<Item = &str> {
let keys = match self {
Self::ReadOnly(field) => &field.deprecated_keys,
Expand Down Expand Up @@ -357,6 +407,14 @@ impl TryFrom<super::parse::ConfigurableField> for ConfigurableField {
.push(parse_quote_spanned!(span=> #[serde(rename = #literal)]))
}

if value.reader.function.is_some() && value.from.is_none() {
value.from = Some(
extract_type_from_result(&value.ty)
.map_or(&value.ty, |tys| tys.0)
.clone(),
);
}

for name in value.deprecated_names {
let name_str = name.as_str();
if name.contains('.') {
Expand Down
Loading
Loading