Re: [pve-devel] [RFC PATCH installer] fix #5973: auto: first boot: allow snake- and kebabcased property names

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 05.12.24 um 15:07 schrieb Daniel Kral:
> Allow the names for the [first-boot] properties to be parsed as either
> snake_cased or kebab-cased strings, i.e. `cert_fingerprint` or
> `cert-fingerprint`, to have consistent property name casings. Currently,
> this only affects the `cert_fingerprint`.
> 
> This change does not break API as it preserves the kebabcased variant
> `cert-fingerprint`, but as this change propagates to the auto-installer,
> using the snakecased `cert_fingerprint` will result in a parse error
> during auto installation on any unpatched ISO (e.g. Proxmox VE 8.3-1).
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> I have tested this by setting up a small HTTPS server with Python's
> `http.server` and `ssl.wrap_socket()` on my local machine and creating
> two different ISOs (with the Proxmox VE 8.3-1 ISO):
> 
> - with `cert-fingerprint` (which works correctly as expected), and
> - with `cert_fingerprint` (which will fail at a parser error with the
> newest Proxmox VE 8.3-1 ISO).

This is a bit worded like that behavior would be a regression, but it
isn't AFAICT as this was always kebab-case from when being added in
commit 6526662 ("fix #5579: auto-installer: add optional first-boot hook
script"); or am I overlooking something?

> 
> I've also tested the change by booting the ISO in debug mode and copying
> over the recompiled binaries, setting up the environment and running the
> auto installation with `proxmox-fetch-answer | proxmox-auto-installer`,
> which worked as expected.
> 
>  proxmox-auto-installer/src/answer.rs | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
> index c206fcc..04c0ace 100644
> --- a/proxmox-auto-installer/src/answer.rs
> +++ b/proxmox-auto-installer/src/answer.rs
> @@ -107,7 +107,7 @@ impl FirstBootHookServiceOrdering {
>  /// Describes from where to fetch the first-boot hook script, either being baked into the ISO or
>  /// from a URL.
>  #[derive(Clone, Deserialize, Debug)]
> -#[serde(rename_all = "kebab-case", deny_unknown_fields)]
> +#[serde(deny_unknown_fields)]

But we prefer kebab-case for any public API/CLI parameter for modern code;
so shouldn't we rather to the opposite, transform all other (de)serializable
configs to use kebab-case with backward-compat aliases for the cases it
matters?

>  pub struct FirstBootHookInfo {
>      /// Mode how to retrieve the first-boot executable file, either from an URL or from the ISO if
>      /// it has been baked-in.
> @@ -118,6 +118,7 @@ pub struct FirstBootHookInfo {
>      /// Retrieve the post-install script from a URL, if source == "from-url".
>      pub url: Option<String>,
>      /// SHA256 cert fingerprint if certificate pinning should be used, if source == "from-url".
> +    #[serde(alias = "cert-fingerprint")]
>      pub cert_fingerprint: Option<String>,
>  }
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel