Rebase of #90: Add Secure Boot support #130

sebastian-de · 2022-12-01T18:31:28Z

In preparation of Cloud-Init related additions I'd like to make, I have to update to the latest proxmox-api-go, which includes a fix I committed there: Telmate/proxmox-api-go#216

Since updating the api module requires work already done in #90 I rebased this PR and added a final commit that bumps proxmox-api-go to the latest version. I hope this approach is OK and @alex-sector doesn't mind me taking over.

The updated proxmox-api-go closes #66
Added support for HTTP heades might also fix #81

sebastian-de · 2022-12-02T14:04:43Z

I currently can't figure out why go test -v ./... -run TestLogin fails, I'm not familiar enough with net/http/httptest. Login and Token-auth on a real PVE work though.
I suppose this is because proxmox-api-go added support for HTTP headers ( Telmate/proxmox-api-go#202 ).

sebastian-de · 2022-12-02T16:08:52Z

OK, so the error seems to be http: named cookie not present upon req.Cookie("PVEAuthCookie")

packer-plugin-proxmox/builder/proxmox/common/client_test.go

Line 67 in d4a1864

    
           if val, err := req.Cookie("PVEAuthCookie"); err != nil || val.Value != "dummy-ticket" {

@mraerino do you have an idea why?

sebastian-de · 2022-12-02T17:39:23Z

I figued it out myself, the condition now needs to be req.Header["Authorization"][0] != "PVEAuthCookie=dummy-ticket".

sebastian-de · 2022-12-11T21:22:59Z

I also did a rebase of #93 on this PR: https://github.com/sebastian-de/packer-plugin-proxmox/tree/replace
If desired, I can open another PR, should this one get merged.

mabeett · 2022-12-11T23:22:38Z

Hello @sebastian-de,
during this week my PR which solves #66 was merged.
In case of updating the references to the PR we might solve the referenced issue =)

sebastian-de · 2022-12-12T06:14:48Z

Hi @mabeett I bumped proxmox-api-go to include your fix.

sebastian-de · 2023-01-09T15:30:31Z

Hi @nywilken or @lbajolet-hashicorp (I don't know who else from the Packer-team is currently active here) is there interest from your side to get this merged?
I'd like to make other contributions to this plugin, which are all based on this PR, but I don't know how to proceed.
Should I open PRs which inlude this one, or should I wait until it's merged?

I can totally understand that your resources are limited, but I'm very grateful if anyone can find time for a short feedback.

lbajolet-hashicorp

Hi @sebastian-de,

Sorry for letting this one slide, taking a look at the code right now.
It looks good to me, however I'm slightly worried that the change is breaking, I assume the current code cannot be used with the updated library since there may be some missing information, but I'd be cautious when changing the type of an already existing attribute.

To avoid breaking existing templates, may I suggest adding the new structure as a new attribute in the configs? Something like efi_config, and maybe if the idea is to supplant the existing efidisk attribute, add a note to the documentation stating that it is deprecated and that the new method is preferred.

That is, unless the current attribute cannot be used with the new structure. If this is the case, we can probably proceed with the changes you propose.

Other than that, the code looks OK to me, we can consider merging this when the PR is amended.

Thanks for the contribution!

sebastian-de · 2023-01-09T19:02:57Z

To avoid breaking existing templates, may I suggest adding the new structure as a new attribute in the configs? Something like efi_config, and maybe if the idea is to supplant the existing efidisk attribute, add a note to the documentation stating that it is deprecated and that the new method is preferred.

That is, unless the current attribute cannot be used with the new structure. If this is the case, we can probably proceed with the changes you propose.

That's a good point. The current attribute cannot be reused - here is the PR for proxmox-api-go that introduced the change: Telmate/proxmox-api-go#179

I'll change the attribute to efi_config as you suggested and add the relevant documentation.

lbajolet-hashicorp

Second pass here: regarding the breaking change, I've taken a look at the API code and the related PRs you link in the description here and in #90. It seems to me that we can't really avoid a breaking change here, so I would proceed with removing the old code, and from the docs.
We can add a note in the efi_config section to document that we replace efidisk by this from now on.

Regarding the code itself, I have some remaining concerns regarding the default values for this attribute, once this is addressed we can do another pass and hopefully merge this later.

Thanks!

lbajolet-hashicorp · 2023-01-09T19:43:32Z

docs/builders/iso.mdx

@@ -347,7 +347,19 @@ in the image's Cloud-Init settings for provisioning.

 - `bios` - (string) - Set the machine bios. This can be set to ovmf or seabios. The default value is seabios.

- `efidisk` - (string) - Set the efidisk storage location. This need to be set if you use ovmf uefi boot
+- `efidisk` - (string) - This option is deprecated, please use `efi_config` instead (see below).


Pardon me if I'm wrong, but it looks like the latest reroll removes the option entirely, so I think we can remove this from the docs, I fear leaving this here may be confusing to users.
I presume leaving a note in the efi_config section to state that it supersedes efidisk would be enough for users that did specify it in their configs.

This change will however need to be documented in the changelog for the plugin in the next release, if we move on with this.

Thanks a lot for your review!
Sorry, I misunderstood your last comment - I removed the efidisk section.

Do you create changelogs automatically, or should I add a line to CHANGELOG.md with this PR?

Changelogs are generated, but we can edit them to add a note if something is breaking for example, so that's probably what will happen when we fold this in, so users are aware of the breaking change.

builder/proxmox/common/config.go

lbajolet-hashicorp · 2023-01-09T20:57:12Z

builder/proxmox/common/config.go

+type efiConfig struct {
+	Storage         string `mapstructure:"storage"`
+	PreEnrolledKeys int    `mapstructure:"pre_enrolled_keys"`
+	EfiType         string `mapstructure:"efitype"`


Regarding those attributes, could you add some documentation/examples so users have an idea which values these attributes accept, and what they describe?

Also since we rely on them to generate the efiConfig during the start VM step, we should probably sanity-check them during the Prepare step.
I wonder, what happens if a user does not define efi_config at all? From the looks of it, it seems that there will be a QemuDevice created, with 0 for the pre-enrolled-keys, is it expected?

I ended up rewriting this part - thanks a lot for your input. Seems that while doing the rebase I didn't spend enough time checking the code code written by the original author.
PreEnrolledKeys is now a bool and it's ensured that no device will be created when efi_config isn't defined.

lbajolet-hashicorp

Hi @sebastian-de,

With the latest reroll, the code looks good to me, thanks for being that responsive, much appreciated!

Given this is a breaking change, I'll wait until someone else from the team approves it as well, as soon as it is approved (unless they have something else to report), we can merge this.

Thanks again!

sebastian-de · 2023-01-10T19:58:23Z

Hey @lbajolet-hashicorp,
that's great to hear!

And thanks to you for the helpful review!

sebastian-de · 2023-01-14T09:46:22Z

I had yet another look at this and found a way to implement the change without breaking existing templates.

The problem was that the old attribute wasn't properly documented anywhere. So I actually searched for templates in the wild that use efidisk and found that one, for example: https://github.com/dandyrow/packer-template-proxmox-archlinux/blob/4211ea0b1f96b4db35bd47d45a8253346ce1bfc8/archlinux.source.pkr.hcl#L54

With efidisk being simply efi_storage_pool it's easy to generate a full efi_config, since efi_storage_pool is its only mandatory option anyway.

While testing I found that the EFI disk doesn't get created reliably during initial VM creation, so I added an additional step in stepStartVM.

Sorry for changing this PR yet again, but avoiding a breaking change makes this worth it, think.

Tested on a PVE 7.3

lbajolet-hashicorp

Hi @sebastian-de,

No need to apologise, it's great that you found a way to not break existing configs :)

I just left a last nit regarding the phrasing for the error when both efi_config and efidisk are set at the same time, other than that the code looks good to me.

When the nit is fixed, we can merge this, no need for two approvals since we're not breaking the existing configs now.

Thanks for your perseverance, hopefully we can merge this soon!

lbajolet-hashicorp · 2023-01-16T15:06:50Z

builder/proxmox/common/config.go

@@ -346,6 +352,24 @@ func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []st
 			errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("one of iso_file, iso_url, or a combination of cd_files and cd_content must be specified for AdditionalISO file %s", c.AdditionalISOFiles[idx].Device))
 		}
 	}
+	if c.EFIDisk != "" {
+		if c.EFIConfig != (efiConfig{}) {
+			errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("both efi_config and efidisk set, remove deprecated efidisk"))


Nit regarding phrasing here:

Suggested change

errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("both efi_config and efidisk set, remove deprecated efidisk"))

errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("both efi_config and efidisk cannot be set at the same time, consider defining only efi_config as efidisk is deprecated"))

Done, thanks!

To avoid breaking existing templates. Improve naming of other attributes and add sanity checks.

lbajolet-hashicorp · 2023-01-16T15:26:09Z

Perfect, thanks for the speedy reroll, merging this!

alex-sector added 2 commits December 1, 2022 19:19

Add new EFI parameters syntax

ed65b3f

Update EFI documentation

a39557a

sebastian-de requested a review from a team as a code owner December 1, 2022 18:31

Update struct to last proxmox-api-go API

576c27d

sebastian-de mentioned this pull request Dec 4, 2022

[Proxmox-clone] add cloud-init config #7

Closed

Update to latest github.com/Telmate/proxmox-api-go

efbe678

lbajolet-hashicorp requested changes Jan 9, 2023

View reviewed changes

sebastian-de requested a review from lbajolet-hashicorp January 9, 2023 19:29

lbajolet-hashicorp requested changes Jan 9, 2023

View reviewed changes

lbajolet-hashicorp approved these changes Jan 10, 2023

View reviewed changes

sebastian-de mentioned this pull request Jan 11, 2023

Do not upload an existing iso_url #79

Closed

sebastian-de requested a review from lbajolet-hashicorp January 14, 2023 09:46

lbajolet-hashicorp approved these changes Jan 16, 2023

View reviewed changes

Add efi_config attribute

8e07454

To avoid breaking existing templates. Improve naming of other attributes and add sanity checks.

lbajolet-hashicorp merged commit e1bd001 into hashicorp:main Jan 16, 2023

sebastian-de deleted the efi branch January 16, 2023 17:11

lbajolet-hashicorp mentioned this pull request Jan 16, 2023

Add Secure Boot support #90

Closed

lbajolet-hashicorp added the enhancement label Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebase of #90: Add Secure Boot support #130

Rebase of #90: Add Secure Boot support #130

sebastian-de commented Dec 1, 2022 •

edited

Loading

sebastian-de commented Dec 2, 2022

sebastian-de commented Dec 2, 2022 •

edited

Loading

sebastian-de commented Dec 2, 2022

sebastian-de commented Dec 11, 2022

mabeett commented Dec 11, 2022

sebastian-de commented Dec 12, 2022

sebastian-de commented Jan 9, 2023

lbajolet-hashicorp left a comment

sebastian-de commented Jan 9, 2023

lbajolet-hashicorp left a comment

lbajolet-hashicorp Jan 9, 2023

sebastian-de Jan 10, 2023

lbajolet-hashicorp Jan 10, 2023

lbajolet-hashicorp Jan 9, 2023

sebastian-de Jan 10, 2023

lbajolet-hashicorp left a comment

sebastian-de commented Jan 10, 2023

sebastian-de commented Jan 14, 2023

lbajolet-hashicorp left a comment

lbajolet-hashicorp Jan 16, 2023

sebastian-de Jan 16, 2023

lbajolet-hashicorp commented Jan 16, 2023

	errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("both efi_config and efidisk set, remove deprecated efidisk"))
	errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("both efi_config and efidisk cannot be set at the same time, consider defining only efi_config as efidisk is deprecated"))

Rebase of #90: Add Secure Boot support #130

Rebase of #90: Add Secure Boot support #130

Conversation

sebastian-de commented Dec 1, 2022 • edited Loading

sebastian-de commented Dec 2, 2022

sebastian-de commented Dec 2, 2022 • edited Loading

sebastian-de commented Dec 2, 2022

sebastian-de commented Dec 11, 2022

mabeett commented Dec 11, 2022

sebastian-de commented Dec 12, 2022

sebastian-de commented Jan 9, 2023

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

sebastian-de commented Jan 9, 2023

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp Jan 9, 2023

Choose a reason for hiding this comment

sebastian-de Jan 10, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp Jan 10, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp Jan 9, 2023

Choose a reason for hiding this comment

sebastian-de Jan 10, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

sebastian-de commented Jan 10, 2023

sebastian-de commented Jan 14, 2023

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp Jan 16, 2023

Choose a reason for hiding this comment

sebastian-de Jan 16, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp commented Jan 16, 2023

sebastian-de commented Dec 1, 2022 •

edited

Loading

sebastian-de commented Dec 2, 2022 •

edited

Loading