feat(azure): Add Azure Ignition fragment generation#1264
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for generating Azure-specific Ignition fragments. The implementation is well-structured, adding a new CLI command, a systemd service for execution, and the core logic for fetching data from IMDS and OVF. The refactoring to share SSH key parsing logic is a good improvement.
I've found one critical issue regarding the systemd service activation which would cause the feature to not work as intended, and one medium-severity suggestion to improve code efficiency and readability. Please see the detailed comments.
6af9c2e to
50a81df
Compare
|
@peytonr18, I have started looking into the changes, I will add my feedback in the next day or so. |
prestist
left a comment
There was a problem hiding this comment.
Overall this looks good! just a few concerns around the CLI design and how the IMDS client is built.
|
@prestist I should have addressed all of your comments - thank you so much for these! I agreed with all of your points and adjusted the logic accordingly. |
|
@peytonr18 I will take another pass today! thank you for the quick changes! |
|
@peytonr18 we do have some failing lints in the CI and you would need to add an entry to the release notes file in the "upcoming" release notes section. https://github.com/coreos/afterburn/blob/main/docs/release-notes.md |
b2da779 to
af9bd12
Compare
9d71b9b to
aef7dca
Compare
prestist
left a comment
There was a problem hiding this comment.
Quick look over, a few small questions/nits
3786e89 to
c555f66
Compare
d2bb19f to
9d6be5c
Compare
prestist
left a comment
There was a problem hiding this comment.
This looks wonderful thank you @peytonr18. I dont see anything that needs to change outside of a small nit about the commit structure. Since we merge directly into the main branch lets squash and rename some of the commits to follow the repo convention of : (lowercase subsystem, imperative tense). Something
like:
- azure: Add render-ignition subcommand for Ignition fragment generation
- dracut: Add afterburn-ignition-fragment.service ordered between fetch and disks
Or if you'd prefer to keep more granularity, each commit should at minimum get a subsystem prefix (e.g. azure:, dracut:, cli:).
e40335b to
68762e9
Compare
|
@prestist Should have changed the git commit history based on that suggestion, thanks! |
| .context("fetching metadata from provider")?; | ||
|
|
||
| if hostname { | ||
| crate::providers::microsoft::azure::config::generate_hostname_fragment( |
There was a problem hiding this comment.
this is azure-specific call in cli code. this should be generic to provider
There was a problem hiding this comment.
Quick design question: after our offline discussion, ignition.rs holds the Ignition config structs + serialization (with the write_to() method on IgnitionConfig), and the render-ignition CLI keeps small generate_hostname_fragment / generate_user_fragment functions that pull data off the provider and write the fragments.
An alternative would be to move that provider→fragment orchestration onto the MetadataProvider trait as default methods, mirroring the existing write_hostname / write_network_units helpers — e.g.:
// src/providers/mod.rs
fn write_hostname_ignition_fragment(&self, output_dir: &str) -> Result<()> {
let Some(hostname) = self.hostname()? else { return Ok(()); };
let path = Path::new(output_dir).join("hostname.ign");
IgnitionConfig::hostname_fragment(&hostname).write_to(&path)
}
fn write_user_ignition_fragment(&self, output_dir: &str) -> Result<()> {
// admin_username / ssh_keys / admin_password_hash -> IgnitionConfig::user_fragment(..).write_to(..)
}Do you think the trait-default-method approach would be cleaner/preferred here, or is the current split fine? I'm happy to switch the approach if the trait method seems better - it's a small change either way.
There was a problem hiding this comment.
I think defaults defined in the trait would be preferable but I would like to hear @prestist's thoughts on the subject
Move the Ignition fragment types and rendering from the Azure provider into render_ignition and source platform data through the MetadataProvider trait (admin_username, admin_password_hash), and switch the feature flags to opt-out (--disable-hostname-fragment / --disable-user-fragment).
Move the Ignition config structs and serialization into src/ignition.rs with a write_to() method on IgnitionConfig, leaving the provider-to-fragment orchestration in the render-ignition CLI. Signed-off-by: peytonr18 <peytonr18@sbcglobal.net>
|
@peytonr18, this still has my stamp :) from my perspective we should be able to merge. |
Rolv-Apneseth
left a comment
There was a problem hiding this comment.
Thanks for working on this @peytonr18 . Mostly LGTM but I have a couple questions and suggestions. Realistically though most are nits and not essential.
| let platform_user = !self.disable_user_fragment; | ||
|
|
||
| if !hostname && !platform_user { | ||
| slog_scope::warn!("render-ignition: all fragments disabled, nothing to do"); |
There was a problem hiding this comment.
| slog_scope::warn!("render-ignition: all fragments disabled, nothing to do"); | |
| warn!("render-ignition: all fragments disabled, nothing to do"); |
| } | ||
|
|
||
| fn generate_hostname_fragment(provider: &dyn MetadataProvider, output_dir: &str) -> Result<()> { | ||
| let hostname = match provider.hostname()? { |
There was a problem hiding this comment.
| let hostname = match provider.hostname()? { | |
| let Some(hostname) = provider.hostname()? else { | |
| warn!("hostname requested, but not available from this provider"); | |
| return Ok(()); | |
| }; |
| .admin_username() | ||
| .context("failed to query admin username from provider")?; | ||
| let username = match username { | ||
| Some(u) => u, |
There was a problem hiding this comment.
| Some(u) => u, | |
| let Some(username) = username else { | |
| warn!("platform-user requested, but admin username not available from this provider"); | |
| return Ok(()); | |
| }; |
| let password_hash = provider | ||
| .admin_password_hash() | ||
| .context("failed to query admin password hash from provider")?; | ||
|
|
There was a problem hiding this comment.
Might be worth checking if any credentials are actually available before writing, WDYT?
| if ssh_keys.is_empty() && password_hash.is_none() { | |
| warn!("admin username present but no SSH keys or password; skipping user fragment"); | |
| return Ok(()); | |
| } |
|
|
||
| /// Minimal generic provider used to exercise the fragment generators | ||
| /// without any platform-specific plumbing. | ||
| struct FakeProvider { |
There was a problem hiding this comment.
Some of the boilerplate below could potentially be cut down with a #[derive(Default)], e.g.
let provider = FakeProvider {
hostname: Some("myhost".into()),
..Default::default()
};
// Or, if no differing fields are required
let provider = FakeProvider::default();| .send::<String>() | ||
| .context("failed to query IMDS for adminUsername")?; | ||
|
|
||
| match username { |
There was a problem hiding this comment.
| match username { | |
| match username.map(|u| u.trim().to_owned()) { | |
| Some(u) if !u.is_empty() => Ok(Some(u)), | |
| _ => Ok(None), | |
| } |
| @@ -0,0 +1,334 @@ | |||
| // Copyright 2017 CoreOS, Inc. | |||
There was a problem hiding this comment.
| // Copyright 2017 CoreOS, Inc. | |
| // Copyright 2026 CoreOS, Inc. |
| storage: Some(Storage { | ||
| files: vec![StorageFile { | ||
| path: "/etc/hostname".into(), | ||
| mode: 420, |
There was a problem hiding this comment.
I think this is clearer in octal:
| mode: 420, | |
| mode: 0o644, |
| } | ||
| let json = | ||
| serde_json::to_string_pretty(self).context("failed to serialize ignition config")?; | ||
| fs::write(path, json.as_bytes()) |
There was a problem hiding this comment.
Would it be worth writing to a temp file and renaming after the permissions are set to keep this atomic in case of a failure / crash?
| storage: Some(Storage { | ||
| files: vec![StorageFile { | ||
| path: "/etc/hostname".into(), | ||
| mode: 420, |
There was a problem hiding this comment.
| mode: 420, | |
| mode: 0o644, |
Summary
This PR adds a
render-ignitionCLI subcommand to Afterburn that generates Ignition config fragment files from Azure IMDS and OVF metadata. These fragments are written to/etc/ignition/base.platform.d/azure/during initrd, allowing Ignition to natively merge platform-provided user data (hostname, SSH keys, password) without requiring a custom Ignition config.Changes
New
render-ignitionsubcommandsrc/cli/render_ignition.rswith clap-derived argument parsing.--provider/--cmdline,--render-ignition-dir, and per-feature flags (--hostname,--platform-user,--platform-extensions).Azure fragment generation (
src/providers/microsoft/azure/config.rs)generate_hostname_fragment(): fetches hostname viaMetadataProvider::hostname(), writeshostname.ignwith astorage.filesentry for/etc/hostnameusing adata:,<hostname>URI.generate_user_fragment(): fetches admin username and SSH keys from IMDS viaMetadataProvider, optionally readsadminPasswordfrom OVF (ovf-env.xmlon CD-ROM), hashes it with SHA-512, and writesuser.ignwith apasswd.usersentry.OVF / IMDS source rules
adminPasswordis the only field read from OVF. If present and non-empty, it is SHA-512 hashed and included aspasswordHashin the Ignition user fragment. If absent or empty, it is omitted.Systemd / Dracut integration
afterburn-ignition-fragment.serviceunit (dracut module) runs during initrd.ignition.platform.id=azure.network-online.target, beforeignition-kargs.service.afterburn render-ignition --cmdline --render-ignition-dir /etc/ignition/base.platform.d/azure/ --platform-extensions.CLI usage
--hostnamehostname.ign(storage.filesentry for/etc/hostname)--platform-useruser.ign(passwd.usersentry with username + SSH keys + optional password hash)--platform-extensions--hostname --platform-userEach feature writes its own standalone fragment file. Missing data (e.g., no hostname available) logs a warning and skips, matching existing Afterburn behavior.
Existing functionality is untouched
The
multisubcommand's--hostname=<path>direct-file-write path is unchanged. Theafterburn-hostname.service(including its Azure/AzureStack conditions) is not modified.The CLI design is up for discussion — would love feedback on the flag names, subcommand structure, and overall approach.