Open stormi opened 1 year ago
This looks like the design we discussed last week. It's adding a new field, which is not used when the per-host setting is false, and it even simplifies the case. This looks good to me, and I'd be glad to review PRs implementing this, @mg12 might want to add comments before implementation starts
If you add a new field then is the xapi.conf entry still needed, can it be completely removed to reduce the combinatorial matrix of codepaths to test? By default that new field would be empty, and as long as the user doesn't override it, they'll be fine.
If they make an API call (or 'xe' cli call) then we'll apply the semantics of that field (maybe for a short while we need to do something on upgrades to still recognize the old xapi.conf field). i.e. treat 'allow_custom_uefi_certs' as always true even in XenServer, it is just that in xenserver we'd leave the field to be empty.
That seems to match the semantics you wanted? i.e. these 2 options are the same
If allow_custom_uefi_certs is false: Ensure /var/lib/varstored is a symlink to /usr/share/varstored (unchanged from previous behaviour) If allow_custom_uefi_certs is true AND Pool.custom_uefi_certificates is empty or is not a valid value: Ensure /var/lib/varstored is a symlink to /usr/share/varstored (no custom certs: keep/restore the symlink)
@edwintorok Yes. The reason why this setting exist is because XenServer didn't want to allow installing custom certificates and XCP-ng needed to. I'm fine with either keeping or removing the setting.
I thought the difference was that we installed some certificates by default (and the method that we used to install them changed over time), whereas xcp-ng got 0 certificates by default and the user had to install them.
I don't think that having the ability to install custom certificates in XenServer would be bad, it might be a useful feature, but it isn't something we'd advertise in the UI, or (initially) support customers to do, and because we don't strictly need it, we likely wouldn't test it. But just like with all the other experimental and unsupported features in XAPI if someone from the CLI does it then if they break it is their responsibility to fix.
If we can keep the code simpler by having just 2 code paths (XenServer default and Xcp-ng default) instead of 3 or 4, then that might be better
cc @mg12 who worked on the original flag in XS for this.
maybe for a short while we need to do something on upgrades to still recognize the old xapi.conf field
If it's just ignored when present, without causing XAPI to fail, that's enough for me. Only beta-testers of XCP-ng 8.3 are concerned on our side, and we override xapi.conf when the defaults change in it (this is because defaults in XAPI code are tailored for XenServer, so xapi.conf is our way to set our XCP-ng settings. Users are redirected to xapi.conf.d if they need to add or change a setting).
Requirements
In XCP-ng, we would like to offer the ability to install custom UEFI certificates directly through XAPI, as was the case in 8.2, without requiring to modify xapi.conf on all hosts. This is for user convenience on one hand, but also because we are not sure yet whether we can distribute the UEFI certificates in our RPMs without adding restrictions of use and/or of distribution to the XCP-ng as a whole, which would make XCP-ng non-free by FLOSS standards.
When no custom certificates are set, then the default certificates should be used, exactly as what would happen if support for custom certificates were not enabled. Users would be able to set custom certificates, but also to remove them, which would go back to using the defaults.
The current implementation almost fulfills these requirements, but has a few drawbacks for us:
override-uefi-certs=false
tooverride-uefi-certs=true
in xapi.conf, thePool.uefi_certificates
field begins with a non-empty value (written there by XAPI based on the master host's disk contents). Thus, XAPI believes that there already are custom certificates in the DB, and won't sync again with the defaults, were them to be updated in the future. However, as we intend to enableoverride-uefi-certs
at installation time, we will likely avoid this issue, unless a user changes the value tofalse
and then back totrue
. And we will have to tell existing testers of XCP-ng 8.3 that they must clear the custom certificates, and it's almost guaranteed that some will not see the message./var/lib/varstored
contains a copy of the default certificates, not a direct symlink to them. Thus, if the default certificates would get updated, the contents of/var/lib/varstored
would get out of sync.override-uefi-certs
is true (and this was the default value we intended for XCP-ng 8.3), the contents ofPool.uefi_certificates
doesn't not always describe the actual certificates in use. When the default certificates are used, that is no custom certificates are set, the getter will return an empty value, which means "no custom certificates", but doesn't allow to check the value of the default certificates that are used instead.These issues all stem from the fact that there is only one DB field (Pool.uefi_certificates) for several different needs.
Here's a proposed update to the API which builds on the existing, but separates the values for custom certificates and default certificates in the database. I believe it would not change the behaviour in XenServer's context.
xapi.conf
override-uefi-certs
parameter toallow-custom-uefi-certs
, which better describes what we need it to do when it's true.override_uefi_certs
toallow_custom_uefi_certs
in the source code.Proposed API changes
On the Pool object:
Pool.uefi_certificates
(contains a tarball or is empty).Pool.get_custom_uefi_certificates
.Pool.set_custom_uefi_certificates
. Returns an error ifallow_custom_uefi_certs
isfalse
.Pool.uefi_certificates
toPool.default_uefi_certificates
. However, renaming database fields is not a simple operation in XAPI, so we'll keep the original field.Pool.get_uefi_certificates
:allow_custom_uefi_certs
isfalse
ORPool.custom_uefi_certificates
is empty: return the contents ofPool.uefi_certificates
allow_custom_uefi_certs
istrue
ANDPool.custom_uefi_certificates
is not emptyPool.set_uefi_certificates
: always returns an error (or removed if doable. We don't need to keep it for backward compatibility in XCP-ng)As far as XenServer is concerned, where
allow_custom_uefi_certs
isfalse
, the API doesn't change:Pool.get_uefi_certificates
keeps the same signature and returns the same data.Pool.set_uefi_certificates
can be kept if necessary (and return an error like it does today), or completely dropped.Proposed relation between XAPI and the filesystem
Pool.uefi_certificates
is updated based on the contents of/usr/share/varstored
, regardless of the value ofallow_custom_uefi_certs
. This is simplified when compared with the previous behaviour, because there now is a dedicated field in the database to contain the custom certificates, if any.Pool.set_custom_uefi_certificates
is called ANDallow_custom_uefi_certs
istrue
):allow_custom_uefi_certs
isfalse
:/var/lib/varstored
is a symlink to/usr/share/varstored
(unchanged from previous behaviour)allow_custom_uefi_certs
istrue
ANDPool.custom_uefi_certificates
is empty or is not a valid value:/var/lib/varstored
is a symlink to/usr/share/varstored
(no custom certs: keep/restore the symlink)allow_custom_uefi_certs
istrue
ANDPool.custom_uefi_certificates
is a valid value:/var/lib/varstored
directory if needed/var/lib/varstored
/var/lib/varstored
Database upgrade
Just creation of the new field.