Custom UEFI certificates - API change #5247

benjamreis · 2023-11-17T08:41:12Z

Solves: #5245

ocaml/idl/datamodel_pool.ml

ocaml/xapi/xapi_globs.ml

lindig · 2023-11-17T16:24:28Z

ocaml/xapi/xapi_pool.ml

+let get_uefi_certificates ~__context ~self =
+  let custom_certs = Db.Pool.get_custom_uefi_certificates ~__context ~self in
+  match (!Xapi_globs.allow_custom_uefi_certs, custom_certs) with
+  | false, _ | true, "" ->


This suggests the empty string has a special meaning. Is this obvious for the user?

In the design clearing the custom UEFI certificates would default back to using the default ones.

Some docs can be added clarifying this clearing. An alternative could be to add an api function called Pool.clear_custom_uefi_certs

If the empty string has special meaning, I would like to see that documented in the function that sets the string. Usually we try to avoid giving an empty string special meaning and use option values instead: it is more obvious that None has a special meaning.

Note that this special meaning of an empty value is already present currently in Db.Pool.{set,}uefi_certificates. We just moved this to the new field, to keep the amount of changes to a minimum.

I've added in the doc of the setter the defaulting back when empty behavior. :)

ocaml/idl/datamodel_pool.ml

lindig · 2023-11-20T09:33:47Z

ocaml/xapi/xapi_pool.ml

+let get_uefi_certificates ~__context ~self =
+  let custom_certs = Db.Pool.get_custom_uefi_certificates ~__context ~self in
+  match (!Xapi_globs.allow_custom_uefi_certs, custom_certs) with
+  | false, _ | true, "" ->


If the empty string has special meaning, I would like to see that documented in the function that sets the string. Usually we try to avoid giving an empty string special meaning and use option values instead: it is more obvious that None has a special meaning.

stormi · 2023-12-06T15:59:58Z

Late remarks:

I initially misunderstood Pau's comment regarding ocaml/xapi-cli-server/records.ml, so we still need to update it.
To fully implement my design, pool.uefi_certificates needs to be updated at XAPI start regardless of the value of allow-custom-uefi-certs.

Don't merge yet :)

benjamreis · 2023-12-07T09:56:31Z

I've added the xe cli part for this PR and an helper to get the correct uefi certificates in Helpers.ml

benjamreis · 2023-12-19T10:37:15Z

last force push was to fix typos in the commit messages, everything is good and validated on our side so good for merge according to us! :)

benjamreis · 2023-12-19T10:43:52Z

The format error does not concern my changes, not sure what to do 🤔

benjamreis · 2023-12-19T12:08:02Z

rebased on master for format change.

stormi · 2023-12-21T09:24:44Z

How about a merge of this as a Christmas gift? 😉

lindig · 2023-12-21T09:33:51Z

ocaml/idl/datamodel_pool.ml

+  call ~name:"set_custom_uefi_certificates" ~lifecycle:[]
+    ~doc:
+      "Set custom UEFI certificates for a pool and all its hosts. Need \
+       `allow-custom-uefi-verts` set to true in conf. If empty: default back \


typo? "verts"?

indeed! Fixed 😇

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

- `Pool.set/get_custom_uefi_certificates` - `Pool/Host.set_uefi_certificates` deprecated - `Pool.get_uefi_certificates` return the certificates used by the pool Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

They'are also needed to fallback when custom are empty Signed-off-by: Benjamin Reis <benjamin.reis@vates.tech>

psafont · 2023-12-21T10:10:11Z

ocaml/idl/datamodel_pool.ml

+    ~lifecycle:
+      [
+        (Published, "22.16.0", "")
+      ; (Deprecated, "23.32.0", "use set_custom_uefi_certificates instead")


This is the latest version

benjamreis force-pushed the custom-uefi-cert-api-change branch 9 times, most recently from ae2452a to 49880ea Compare November 17, 2023 12:53

lindig reviewed Nov 17, 2023

View reviewed changes

benjamreis force-pushed the custom-uefi-cert-api-change branch 3 times, most recently from bd6a757 to 7b54eb2 Compare November 20, 2023 09:23

psafont reviewed Nov 20, 2023

View reviewed changes

ocaml/idl/datamodel_pool.ml Outdated Show resolved Hide resolved

psafont reviewed Nov 20, 2023

View reviewed changes

ocaml/idl/datamodel_pool.ml Outdated Show resolved Hide resolved

benjamreis force-pushed the custom-uefi-cert-api-change branch 3 times, most recently from 06adb4b to 693ac7a Compare November 20, 2023 09:47

psafont reviewed Nov 20, 2023

View reviewed changes

ocaml/idl/datamodel_pool.ml Outdated Show resolved Hide resolved

benjamreis force-pushed the custom-uefi-cert-api-change branch 4 times, most recently from 9e420bd to 7bdc699 Compare November 20, 2023 15:00

lindig reviewed Nov 20, 2023

View reviewed changes

benjamreis force-pushed the custom-uefi-cert-api-change branch 6 times, most recently from 4822862 to f0857b0 Compare November 22, 2023 09:55

benjamreis mentioned this pull request Dec 6, 2023

Custom UEFI certificates - API change xcp-ng-rpms/xapi#52

Merged

benjamreis force-pushed the custom-uefi-cert-api-change branch from 73447b6 to d6eaf75 Compare December 6, 2023 14:54

benjamreis force-pushed the custom-uefi-cert-api-change branch 3 times, most recently from 8fef70d to ffd3d8b Compare December 7, 2023 09:54

benjamreis force-pushed the custom-uefi-cert-api-change branch from ffd3d8b to ce56312 Compare December 7, 2023 09:57

psafont approved these changes Dec 7, 2023

View reviewed changes

benjamreis force-pushed the custom-uefi-cert-api-change branch from 46572aa to a52663f Compare December 11, 2023 17:54

benjamreis force-pushed the custom-uefi-cert-api-change branch from a52663f to 97473dd Compare December 19, 2023 10:36

stormi approved these changes Dec 19, 2023

View reviewed changes

benjamreis requested a review from lindig December 19, 2023 10:42

benjamreis force-pushed the custom-uefi-cert-api-change branch from 97473dd to 88c89e8 Compare December 19, 2023 12:06

lindig approved these changes Dec 21, 2023

View reviewed changes

benjamreis and others added 6 commits December 21, 2023 10:58

Rename 'override_uefi_certs' with 'allow_custom_uefi_certs'

3073e1e

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Introduce new methods for custom uefi certs

568382b

- `Pool.set/get_custom_uefi_certificates` - `Pool/Host.set_uefi_certificates` deprecated - `Pool.get_uefi_certificates` return the certificates used by the pool Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Do not break symlink when no custom certificates are set

2587c90

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Do not fail when fullpath is given to extract certificates

5196f53

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Check for used UEFI certificates when updating vm platform

dfa9b94

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Always set pool.uefi_certificates

70e07d0

They'are also needed to fallback when custom are empty Signed-off-by: Benjamin Reis <benjamin.reis@vates.tech>

benjamreis force-pushed the custom-uefi-cert-api-change branch from 88c89e8 to 70e07d0 Compare December 21, 2023 09:58

psafont reviewed Dec 21, 2023

View reviewed changes

psafont approved these changes Dec 21, 2023

View reviewed changes

psafont merged commit 5724962 into xapi-project:master Dec 21, 2023

psafont deleted the custom-uefi-cert-api-change branch December 21, 2023 10:12

Uh oh!

Custom UEFI certificates - API change #5247

Custom UEFI certificates - API change #5247

Uh oh!

Conversation

benjamreis commented Nov 17, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stormi commented Dec 6, 2023

Uh oh!

benjamreis commented Dec 7, 2023

Uh oh!

benjamreis commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamreis commented Dec 19, 2023

Uh oh!

benjamreis commented Dec 19, 2023

Uh oh!

stormi commented Dec 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benjamreis commented Dec 19, 2023 •

edited

Loading