nocloud: add support for dmi variable expansion for seedfrom URL

[blackboxsw]

Proposed Commit Message

nocloud: add support for dmi variable expansion for seedfrom URL

NoCloud meta-data seedfrom (or kernel commandline seedfrom) URL can
now provide variable expansion for system-specific DMI values as seen
in /sys/class/dmi/id on Linux or kenv on FreeBSD platforms.

Variable names of the format __dmi.SOME_VAR__ will be replaced when
determining the URL from which NoCloud datasource GETs its user-data
and meta-data.

This allows for a common templated seedfrom URL which can be reused
for mass deployments, but can allow for unique URLs based on classes
of DMI system characteristics such as chassis serial, product name,
UUID etc.

LP: #1994980

Additional Context

Test Steps

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[blackboxsw]

Based on the original intent and discussion brought up by @ITJamie in #1808 . Added some unittests and restructured logic to live over in cloudinit.dmi

[ITJamie]

One thing i noticed, We probably want to add a note about when this feature was added in docs. Since it might cause confusion when people use older images that dont have support for it.

Otherwise LGTM

[TheRealFalcon]

Nice work. I left some comments inline.

[TheRealFalcon]

Oh, also docs linting doesn't like a few line lengths.

[holmanb]

There is a possibility that this breaks someone out there using a similar URL scheme in their current deployments, right?

For example if someone has deployments that pass in http://10.10.0.1:8000/%dmi.chassis-serial-number%/ with the expectation that this requests to http://10.10.0.1:8000/%dmi.chassis-serial-number%/meta-data and not whatever that expands to under this new scheme, then they will have been broken in a way that would require server-side changes I think.

It would be a weird coincidence, but we have no way of knowing someone isn't doing this.

There is the case for encoding / special characters in URLs and the fact that this PR hasn't settled on whether to use % or :, each of which has special meaning in URLs, but I do think that even with encoding and limitations that a collision is still possible.

[igalic]

in an environment like libvirt, how would i set these variables for testing this code?

[ITJamie]

So i just spoke with a friend in another company today about this. Auto adding the / is going to break some existing usecases.

His seedfrom url is something like
Http://some.tld/seedmaker?template=somename&filename=

In this case adding a slash automatically is going to at minimum need him to update his code to strip a prefixing slash from the filename param value but is effectively a breaking change to his usecase. I doubt hes the only one

I guess we could add a check to see if the last character is an = and if so dont add the slash?

[igalic]

I guess we could add a check to see if the last character is an = and if so dont add the slash?

or more generally: is there's a query string in the URI, there's no need to append a /

[blackboxsw]

@ITJamie thanks for the input here. We should also check if the URL provided has is a query_string with URLParse. Then we'd know if we have a trailing query string and we would not want to append the trialing slash there. Good catch!.

Oops and just refreshed to see @igalic 's comment. Yes +1

[blackboxsw]

There is a possibility that this breaks someone out there using a similar URL scheme in their current deployments, right?

For example if someone has deployments that pass in http://10.10.0.1:8000/%dmi.chassis-serial-number%/ with the expectation that this requests to http://10.10.0.1:8000/%dmi.chassis-serial-number%/meta-data and not whatever that expands to under this new scheme, then they will have been broken in a way that would require server-side changes I think.

It would be a weird coincidence, but we have no way of knowing someone isn't doing this.
There is the case for encoding / special characters in URLs and the fact that this PR hasn't settled on whether to use % or :, each of which has special meaning in URLs, but I do think that even with encoding and limitations that a collision is still possible.

Thanks @holmanb for the thoughtful suggestion here.

Through the course of this PR, we ended up dropping support for other kenv keys which happened to contain underscores _ in the key names, I think I can revert the bookend variable expansion characters from % to __ which was closer to @ITJamie 's first suggestion because the supported characters of our dmi variable are now limited to [.-a-z] which means:

1. __ is a both URL-safe and a more unique unique marker as the end or beginning and end of a __dmi.varname__.
2. It can then be used without any negative impact to url escaping or unescaping, python's %-formatting in cloud-init functions and GRUB-related custom variable expansion

By making this a dunder marker instead of just a single character. I think we've probably insulated ourselves from most usefuls corner cases for use of a seedfrom URL. But, if we still think this is a potential regression risk, we can add RELEASE_BLOCKER comment and avoid this nocloud behavior on stable releases. I have added the RELEASE_BLOCKER comment around the automatic append of a trailing forward slash because I feel that's probably more likely to have more cases in the wild that we have to be wary of.

[TheRealFalcon]

Left two inline comments, but overall LGTM. If we're going to patch out the trailing slash in our SRUs, I'll plug going the feature flag route again, which I think would make the most sense to add in this PR.

[TheRealFalcon]

Since rtd is faling anyway, one small nit left that somehow didn't get posted yesterday.