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

Proc macro for tedge config #1936

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
375bc6c
POC proc macro for tedge config
jarhodes314 Apr 28, 2023
fb59fda
Make example realistic
jarhodes314 Apr 28, 2023
5e9b80d
Refactor tests and example into appropriate places
jarhodes314 May 2, 2023
f4c67a1
Ensure missing key error contains the field name
jarhodes314 May 2, 2023
1aab677
Store the key regardless of success/failure
jarhodes314 May 2, 2023
771db4c
Replace all or nothing logic in macro with library function
jarhodes314 May 3, 2023
80fe910
Add doc comments and support for read only configurations
jarhodes314 May 3, 2023
ffff8d1
Begin to use new macro in tedge_config
jarhodes314 May 9, 2023
8d59006
Migrate old accessors to new DTOs
jarhodes314 May 9, 2023
c7490e9
Fix test failures in tedge
jarhodes314 May 9, 2023
1868294
Merge remote-tracking branch 'upstream/main' into feat/tedge-config-r…
jarhodes314 May 9, 2023
86af093
Update tedge config set to use the new dto
jarhodes314 May 9, 2023
28dcee3
Migrate `tedge config unset` to new dto
jarhodes314 May 9, 2023
8cd46ad
Fix clippy warning
jarhodes314 May 9, 2023
6471e1f
Migrate `tedge config list` to new dto/reader
jarhodes314 May 9, 2023
98f4a85
Run formatter
jarhodes314 May 9, 2023
6e2e3d0
Actually support renaming
jarhodes314 May 9, 2023
6556d1c
Update references to renamed keys
jarhodes314 May 9, 2023
98e5201
Migrate `tedge cert` to new dto
jarhodes314 May 10, 2023
5b513a0
Fix integration tests and add some tests for backwards compatibility
jarhodes314 May 10, 2023
4696954
Merge remote-tracking branch 'upstream/main' into feat/tedge-config-r…
jarhodes314 May 11, 2023
5d3e4e4
Finish implementing private reader fields and optional paths
jarhodes314 May 11, 2023
81b7193
Support renaming groups
jarhodes314 May 11, 2023
ce77b22
Log only if we actually migrate tedge.toml
jarhodes314 May 11, 2023
b2ef8fa
Add more documentation and tests for the macro, fix clippy errors
jarhodes314 May 11, 2023
3b95375
Add documentation
jarhodes314 May 12, 2023
501b756
Fix unused dependency error
jarhodes314 May 15, 2023
d2063d5
Merge remote-tracking branch 'upstream/main' into feat/tedge-config-r…
jarhodes314 May 15, 2023
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
105 changes: 99 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions crates/common/tedge_config_macros/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
[package]
name = "tedge_config_macros"
version = "0.1.0"
edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
camino = { version = "*", features = ["serde1"] }
doku = "0.21"
once_cell = "*"
serde = "*"
tedge_config_macros-macro = { path = "macro" }
thiserror = "1.0"
tracing = "*"
url = "*"
178 changes: 178 additions & 0 deletions crates/common/tedge_config_macros/examples/macro.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
use camino::Utf8PathBuf;
use std::net::IpAddr;
use std::net::Ipv4Addr;
use std::num::NonZeroU16;
use std::path::PathBuf;
use tedge_config_macros::*;

static DEFAULT_ROOT_CERT_PATH: &str = "/etc/ssl/certs";

define_tedge_config! {
device: {
#[tedge_config(readonly(
write_error = "Device id is inferred from the certificate",
function = "device_id",
))]
#[doku(as = "String")]
id: Result<String, String>,

/// Path where the device's private key is stored
#[tedge_config(example = "/etc/tedge/device-certs/tedge-private-key.pem", default(function = "default_device_key"))]
#[doku(as = "PathBuf")]
key_path: Utf8PathBuf,

/// Path where the device's certificate is stored
#[tedge_config(example = "/etc/tedge/device-certs/tedge-certificate.pem", default(function = "default_device_cert"))]
#[doku(as = "PathBuf")]
cert_path: Utf8PathBuf,

/// The default device type
#[tedge_config(example = "thin-edge.io")]
#[tedge_config(rename = "type")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this just proxy to #[serde(rename)]? Because e.g. on line 84 we use #[serde(alias = ...)], so when should we use one or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does proxy to #[serde(rename)]. Long story short, I couldn't find a nice way of parsing a (strict) subset of serde attributes that we do something special with and allowing the rest to just pass through to the underlying structs.

For rename specifically, this not only affects the TOML deserialisation, but it also affects the key used in tedge config commands, and this is why tedge_config cares about renaming. This generally is only needed for fields like type, where we want to avoid conflicting with Rust keywords as raw identifiers are difficult to read. The macro invocation will fail to compile if you try and use #[serde(rename)] to prevent confusion. Looking back on how much effort goes into establishing if this exists, we could just as easily parse #[serde(rename)] instead.

Aliasing fields, on the other hand, is used to migrate a key from one name to another, e.g. to add or remove underscores. This is used, via doku, to work out if we have renamed the field in the past to allow tedge config to parse the key successfully (with a warning that the key is deprecated). Because the define_tedge_config macro doesn't interact with this attribute directly, it doesn't need to parse it so we can leave it as #[serde(alias = ...)].

I think this question points to a larger possibly-faulty design decision I took. I assumed that it would be simple to be transparent and just use serde attributes where possible, but maybe it might be clearer to replace the common cases (i.e. these two) with tedge_config specific attributes, and give them names more relevant to our specific use case (e.g. #[tedge_config(deprecated_name = "cafile")] instead of #[serde(alias = "cafile)]). Alternatively, the existing solution might be the way, but with "which attribute do I use where" (more clearly) documented.

@Bravo555 @didier-wenzek, what are your opinions on those possible solutions?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jarhodes314 The need for a specific #[tedge_config(rename)] vs #[serde(rename)] makes sense.

About alias, I think the confusion is beyond just a lack of documentation. It's not obvious that #[serde(alias = ...)] has a specific meaning for the tedge_config and that this meaning is added thanks to doku.

So if things were free I would vote for #[tedge_config(deprecated_name = "cafile")].

Copy link
Contributor Author

@jarhodes314 jarhodes314 May 11, 2023

Choose a reason for hiding this comment

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

I've now modified this to match your suggestion. I've also added #[tedge_config(deprecated_key = "mqtt.port")]. The difference between deprecated_name and deprecated_key is that deprecated_name is for renaming a field/group, but deprecated_key is for mapping one key to a different structure (e.g. mqtt.port -> mqtt.bind.port), which cannot just translate to #[serde(alias)]. The macro will emit compiler errors if the user uses the wrong one (by checking if the input contains ., if it does, it can only be a key, and if it doesn't, it can only be a (field/group) name as all fields are nested in at least one group).

I think having different attributes and a compiler error is easier to grasp than one attribute that changes its behaviour dynamically depending on whether the input contains ., but I'm happy to change the behaviour if you disagree with this.

device_type: String,
},

#[serde(alias = "azure")] // for 0.1.0 compatibility
az: {
/// Endpoint URL of Azure IoT tenant
#[tedge_config(example = "myazure.azure-devices.net")]
url: ConnectUrl,

/// The path where Azure IoT root certificate(s) are stared
#[tedge_config(note = "The value can be a directory path as well as the path of the direct certificate file.")]
#[tedge_config(example = "/etc/tedge/az-trusted-root-certificates.pem", default(variable = "DEFAULT_ROOT_CERT_PATH"))]
#[doku(as = "PathBuf")]
root_cert_path: Utf8PathBuf,

mapper: {
/// Whether the Azure IoT mapper should add a timestamp or not
#[tedge_config(example = "true")]
#[tedge_config(default(value = true))]
timestamp: bool,
}
},

mqtt: {
bind: {
/// The address mosquitto binds to for internal use
#[tedge_config(example = "127.0.0.1", default(variable = "Ipv4Addr::LOCALHOST"))]
address: IpAddr,

/// The port mosquitto binds to for internal use
#[tedge_config(example = "1883", default(function = "default_mqtt_port"))]
#[doku(as = "u16")]
// This was originally u16, but I can't think of any way in which
// tedge could actually connect to mosquitto if it bound to a random
// free port, so I don't think 0 is *really* valid here
port: NonZeroU16,
},

client: {
/// The host that the thin-edge MQTT client should connect to
#[tedge_config(example = "localhost", default(value = "localhost"))]
host: String,

/// The port that the thin-edge MQTT client should connect to
#[tedge_config(default(from_path = "mqtt.bind.port"))]
#[doku(as = "u16")]
port: NonZeroU16,

auth: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm affraid I don't completely understand how optional settings work in this proposal. Can only final settings (mqtt.client.auth.cafile, mqtt.client.auth.certfile, etc.) be made optional and entire groups like mqtt.client.auth cannot? How are settings marked as optional? I understand that when #[tedge_config(default = ...)] is used, a default can be used in case a setting is not set, but what if there is no reasonable default?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. My understanding is that will have to be manage at the application layer using functions like all_or_nothing to interpret (None,None) as None.

Copy link
Contributor Author

@jarhodes314 jarhodes314 May 5, 2023

Choose a reason for hiding this comment

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

If there is no default, the field in the reader is optional. It possibly makes the code slightly less clear here than if you provide an explicit Option<_> for each field that needs it, but I originally tried to implement it that way, and it's frankly just a pain to deal with in the macro, compared to deciding whether a field is optional based on the presence of a default or not.

Groups are never optional (but they are only serialized if they contain values for one or more fields) because this drastically increases the complexity involved in reading and updating the configuration. all_or_nothing is a tool to try and deal with this problem. My intention is that where we need custom logic, like mqtt_config() at the moment, we should just add that as methods on the reader as, like with much of this stuff, most of the configurations don't need especially complex handling.

/// Path to the CA certificate used by MQTT clients to use when authenticating the MQTT broker
#[tedge_config(example = "/etc/mosquitto/ca_certificates/ca.crt")]
#[doku(as = "PathBuf")]
#[serde(alias = "cafile")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really related to the macro stuff, but I'd rather see #[serde(rename = ...)] used here and for other options with _ in rust name but no _ in the actual TOML

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'm using alias here to cope with the existing keys (i.e. to accept them in tedge config), but I was trying to make the keys more consistent than they were previously (mainly putting _ anywhere we have words that should be delimited but not by .. If we want all of cafile, certfile etc. to remain as is, we can just modify the field name directly to match tedge config.

ca_file: Utf8PathBuf,

/// Path to the directory containing the CA certificates used by MQTT
/// clients when authenticating the MQTT broker
#[tedge_config(example = "/etc/mosquitto/ca_certificates")]
#[doku(as = "PathBuf")]
#[serde(alias = "capath")]
ca_path: Utf8PathBuf,

/// Path to the client certficate
#[doku(as = "PathBuf")]
#[serde(alias = "certfile")]
cert_file: Utf8PathBuf,

/// Path to the client private key
#[doku(as = "PathBuf")]
#[serde(alias = "keyfile")]
key_file: Utf8PathBuf,
}
},

external: {
bind: {
/// The port mosquitto binds to for external use
#[tedge_config(example = "8883")]
port: u16,

/// The address mosquitto binds to for external use
#[tedge_config(example = "0.0.0.0")]
address: IpAddr,

/// Name of the network interface which mosquitto limits incoming connections on
#[tedge_config(example = "wlan0")]
interface: String,
},

/// Path to a file containing the PEM encoded CA certificates that are
/// trusted when checking incoming client certificates
#[tedge_config(example = "/etc/ssl/certs")]
#[doku(as = "PathBuf")]
#[serde(alias = "capath")]
ca_path: Utf8PathBuf,

/// Path to the certificate file which is used by the external MQTT listener
#[tedge_config(note = "This setting shall be used together with `mqtt.external.key_file` for external connections.")]
#[tedge_config(example = "/etc/tedge/device-certs/tedge-certificate.pem")]
#[doku(as = "PathBuf")]
#[serde(alias = "certfile")]
cert_file: Utf8PathBuf,

/// Path to the key file which is used by the external MQTT listener
#[tedge_config(note = "This setting shall be used together with `mqtt.external.cert_file` for external connections.")]
#[tedge_config(example = "/etc/tedge/device-certs/tedge-private-key.pem")]
#[doku(as = "PathBuf")]
#[serde(alias = "keyfile")]
key_file: Utf8PathBuf,
}
}
}

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

fn default_device_key(location: &TEdgeConfigLocation) -> Utf8PathBuf {
location
.tedge_config_root_path()
.join("device-certs")
.join("tedge-private-key.pem")
}

fn default_device_cert(location: &TEdgeConfigLocation) -> Utf8PathBuf {
location
.tedge_config_root_path()
.join("device-certs")
.join("tedge-certificate.pem")
}

fn default_mqtt_port() -> NonZeroU16 {
NonZeroU16::try_from(1883).unwrap()
}

fn main() {
let dto = TEdgeConfigDto::default();
let config = TEdgeConfigReader::from_dto(&dto, &TEdgeConfigLocation);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add this to the example, as this is that straight use the generated structs that let me prefer this proposal.

Suggested change
let dto = TEdgeConfigDto::default();
let config = TEdgeConfigReader::from_dto(&dto, &TEdgeConfigLocation);
let mut dto = TEdgeConfigDto::default();
dto.mqtt.bind.address = Some(IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4)));
let config = TEdgeConfigReader::from_dto(&dto, &TEdgeConfigLocation);
let host: IpAddress = config.mqtt.bind.address;
let port: NonZeroU16 = config.mqtt.bind.port;
println!("mqtt = {}:{}", host, port);

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't lines 172 and 173 move values out of config? Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't lines 172 and 173 move values out of config? Is this intended?

No, because IpAddress and NonZeroU16 both implement Copy.

println!(
"Device id is {}.",
// We have to pass the config into read to avoid TEdgeConfigReader being
// self-referential
config.device.id.read(&config).as_ref().unwrap()
);
assert_eq!(u16::from(config.mqtt.bind.port), 1883);
assert_eq!(config.mqtt.external.bind.port.or_none(), None);
}
13 changes: 13 additions & 0 deletions crates/common/tedge_config_macros/impl/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[package]
name = "tedge_config_macros-impl"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
darling = "0.20"
heck = "0.4.1"
proc-macro2 = "1"
quote = "1"
syn = { version = "2", features = ["full", "extra-traits"] }
Loading