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

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

[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).

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)]
 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>,
 }
 
-- 
2.39.5



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

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

[Thomas Lamprecht]

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

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

[Christoph Heiss]

On Tue Jan 28, 2025 at 3:38 PM CET, Thomas Lamprecht wrote:
> Am 05.12.24 um 15:07 schrieb Daniel Kral:
[..]
> > - 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?

To provide context; since I actually introduced this option/code: That
was indeed intentionally kebab-cased, as we prefer that style for new
code - as explained below.

[..]
> > -#[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?

I'd prefer that variant too, FWIW.


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

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

[Daniel Kral]

On 1/28/25 15:38, Thomas Lamprecht wrote:
> 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'm sorry that the commit message came across like this, I didn't intend 
to word it as a regression, but I can see why it did. I wasn't aware 
that we prefer kebab-case for newer property names in the answer file 
and I'll keep that in mind for future patches.

> 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?

I also like that solution and that is more in line with the motivation 
behind the patch. I could queue up a patch for the next ISO release, so 
that it's indifferent to the user whether they write the config 
parameter names in their answer files in the old snake case or new 
kebab-case format.

I'd prefer a single serde attribute for that rather than rename_all + 
alias at every property, if that's possible in a few lines, as it would 
be cleaner and we don't have to look at every property whether it's 
missing a alias attribute or not.


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

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

[Thomas Lamprecht]

Am 30.01.25 um 10:11 schrieb Daniel Kral:
> I also like that solution and that is more in line with the motivation 
> behind the patch. I could queue up a patch for the next ISO release, so 
> that it's indifferent to the user whether they write the config 
> parameter names in their answer files in the old snake case or new 
> kebab-case format.

Yes, please do.
 
> I'd prefer a single serde attribute for that rather than rename_all + 
> alias at every property, if that's possible in a few lines, as it would 
> be cleaner and we don't have to look at every property whether it's 
> missing a alias attribute or not.

FWIW, you need to check out all structs that get serialized or are
member of such a struct anyway, and most are relatively small, so looking
at every property is not a big problem I think.

In the long term we might go back to a single variant to reduce complexity
for us and users. I'd target a relatively slow deprecation period for that
though, like e.g., allow both for Bookworm (e.g. PVE 8) and Trixie (e.g.
PVE 9) based releases but print a deprecation notice starting with Trixie
based releases and then drop support for the snake_case with Forky (e.g.
PVE 10) based releases.

Would be also good to know if there is any usage of snake_case for things
we send out, like the system info or post installation hook stuff.


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

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

[Daniel Kral]

On 12/5/24 15:07, Daniel Kral wrote:
> 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>

This has been superseded by another patch series:

https://lore.proxmox.com/pve-devel/20250217121748.117222-1-d.kral@proxmox.com/T/


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