`shared_image_version` - support for new block `uefi_settings` #28076

yeoldegrove · 2024-11-20T14:17:20Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Bump galleryimageversions API to support security profile and uefi settings.
For resource shared_image_version
- support new block uefi_settings
  - This new settings configure secure boot certificates and is needed if you want to boot a secure bootable image with custom keys.
- add documentation for uefi_settings
Not adding a matching block to the data source as the Azure API does not return any security profiles.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

shared_image_version - support for new block uefi_settings [https://github.com/shared_image_version - support for new block uefi_settings #28076]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #28064

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

yeoldegrove · 2024-12-04T09:51:25Z

When can I expect that somebody has a look at this?

github-actions · 2025-01-20T06:03:45Z

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

…sions to 2023-07-03

yeoldegrove · 2025-02-04T10:27:16Z

It would be great if somebody would take a look at this.

codgician · 2025-02-04T11:26:40Z

Kindly adding some active maintainers for comments: @katbyte @jackofallops @stephybun

yeoldegrove · 2025-02-17T06:33:43Z

@codgician @katbyte @jackofallops @stephybun Can anybody please start reviewing this?

ms-zhenhua

Hi @yeoldegrove,

Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍

internal/services/compute/shared_image_version_resource.go

internal/services/compute/shared_image_version_resource_test.go

website/docs/r/shared_image_version.html.markdown

yeoldegrove · 2025-02-24T11:49:46Z

@ms-zhenhua Thanks for the review! I just fixed all the issues you pointed out and force pushed to this PR.

ms-zhenhua

Hi @yeoldegrove,

Thanks for your updates - I've taken another look through and left some comments inline. Please help confirm.

internal/services/compute/shared_image_version_resource.go

website/docs/r/shared_image_version.html.markdown

internal/services/compute/shared_image_version_resource_test.go

internal/services/compute/shared_image_version_resource.go

yeoldegrove · 2025-02-26T12:47:12Z

@ms-zhenhua another round of improvements ;)

ms-zhenhua

Hi @yeoldegrove,

Thanks for your updates - I've taken another look through and left some comments inline. Please help confirm.

ms-zhenhua · 2025-02-27T07:43:36Z

internal/services/compute/shared_image_version_resource.go

+	data := input[0].(map[string]interface{})
+	return &galleryimageversions.UefiKey{
+		Type:  pointer.To(galleryimageversions.UefiKeyType(data["type"].(string))),
+		Value: &[]string{data["certificate_base64"].(string)},


since certificate_base64 is a string list, why convert it to a string ?

Removed both Single functions. Those were artifacts I missed in the refactoring.

ms-zhenhua · 2025-02-27T07:44:44Z

internal/services/compute/shared_image_version_resource.go

+
+	result := make(map[string]interface{})
+	if input.Value != nil && len(*input.Value) > 0 {
+		result["certificate_base64"] = (*input.Value)[0]


certificate_base64 is a string list, why assign the first element only?

Removed both Single functions. Those were artifacts I missed in the refactoring.

ms-zhenhua · 2025-02-27T07:47:53Z

internal/services/compute/shared_image_version_resource.go

+	return results
+}
+
+func flattenUefiKey(input *galleryimageversions.UefiKey) []interface{} {


what's the difference between flattenSingleUefiKey and flattenUefiKey ?

Removed both Single functions. Those were artifacts I missed in the refactoring.

internal/services/compute/shared_image_version_resource_test.go

ms-zhenhua · 2025-02-27T07:52:32Z

internal/services/compute/shared_image_version_resource_test.go

+				check.That(data.ResourceName).Key("uefi_settings.0.signature_template_names.#").HasValue("1"),
+				check.That(data.ResourceName).Key("uefi_settings.0.signature_template_names.0").HasValue("MicrosoftUefiCertificateAuthorityTemplate"),
+				check.That(data.ResourceName).Key("uefi_settings.0.additional_signatures.0.db.0.type").HasValue(keyType),
+				check.That(data.ResourceName).Key("uefi_settings.0.additional_signatures.0.db.0.certificate_base64.0").HasValue(certData),


why these checks still exist?

Forgot to add this to the commit 🤦

ms-zhenhua · 2025-02-27T08:22:06Z

internal/services/compute/shared_image_version_resource.go

+			if securityProfile := props.SecurityProfile; securityProfile != nil {
+				if uefiSettings := securityProfile.UefiSettings; uefiSettings != nil {
+					d.Set("uefi_settings", flattenUefiSettings(uefiSettings))
+				} else {
+					d.Set("uefi_settings", nil)
+				}
+			} else {
+				d.Set("uefi_settings", nil)
+			}


Suggested change

if securityProfile := props.SecurityProfile; securityProfile != nil {

if uefiSettings := securityProfile.UefiSettings; uefiSettings != nil {

d.Set("uefi_settings", flattenUefiSettings(uefiSettings))

} else {

d.Set("uefi_settings", nil)

}

} else {

d.Set("uefi_settings", nil)

}

if securityProfile := props.SecurityProfile; securityProfile != nil {

d.Set("uefi_settings", flattenUefiSettings(securityProfile.UefiSettings;))

}

👍 I guess I thought to complicated here.

ms-zhenhua · 2025-02-27T08:42:57Z

internal/services/compute/shared_image_version_resource_test.go

+	})
+}
+
+func TestAccSharedImageVersion_uefiSettingsUpdateKeyType(t *testing.T) {


please consider combing TestAccSharedImageVersion_uefiSettings and TestAccSharedImageVersion_uefiSettingsUpdateKeyType into one testcase as their first steps are the same.

I can remove one of those and only use the combined one, correct.

ms-zhenhua · 2025-02-27T08:45:38Z

internal/services/compute/shared_image_version_resource_test.go

+    additional_signatures {
+      db {
+        type             = "%s"
+        certificate_base64 = "%s"


since certificate_base64 is a string list in the schema, why it is a string here? Could you provide your local test result?

Actually my testing setup/cycle was provisioning resources with my built provider by hand.

I now checked out https://github.com/hashicorp/terraform-provider-azurerm/blob/main/DEVELOPER.md#developing-the-provider and managed to run make acctests SERVICE='compute' TESTARGS='-run=TestAccSharedImageVersion_uefiSettings' TESTTIMEOUT='60m'

I must say... having to create ARM_CLIENT_ID was a bit complicated as the subscription I have did not initially allow this.

But now I managed to fix most of the test cases. But now I have to figure out to boot a secure bootable VM in this test setup. This will most likely take a few hours.

@ms-zhenhua Do you think it is advisable to really boot a secure_boot_enabled and vtpm_enabled enabled VM (with real keys) inside the test suite?

ms-zhenhua · 2025-02-27T08:46:24Z

internal/services/compute/shared_image_version_resource_test.go

+    signature_template_names = [%s]
+
+    additional_signatures {
+      db {


could you also test dbx, kek and pk ?

yeoldegrove requested a review from a team as a code owner November 20, 2024 14:17

github-actions bot added enhancement documentation dependencies size/XL labels Nov 20, 2024

yeoldegrove mentioned this pull request Nov 20, 2024

opentofu/terraform resources for secure bootable images on Azure gardenlinux/gardenlinux#2490

Open

github-actions bot added the waiting-response label Dec 20, 2024

github-actions bot added the stale label Jan 20, 2025

bump go-azure-sdk/resource-manager/compute/2022-03-03/galleryimagever…

e5c7f99

…sions to 2023-07-03

github-actions bot removed the waiting-response label Jan 20, 2025

yeoldegrove force-pushed the shared_image_version_uefi_settings branch from 5cde4dd to 58ad4cf Compare January 20, 2025 10:40

yeoldegrove mentioned this pull request Feb 4, 2025

Support for uefi_settings in azurerm_shared_image_version #28064

Open

1 task

github-actions bot removed the stale label Feb 4, 2025

ms-zhenhua suggested changes Feb 21, 2025

View reviewed changes

ms-zhenhua reviewed Feb 21, 2025

View reviewed changes

website/docs/r/shared_image_version.html.markdown Outdated Show resolved Hide resolved

ms-zhenhua reviewed Feb 21, 2025

View reviewed changes

website/docs/r/shared_image_version.html.markdown Outdated Show resolved Hide resolved

ms-zhenhua reviewed Feb 21, 2025

View reviewed changes

website/docs/r/shared_image_version.html.markdown Outdated Show resolved Hide resolved

ms-zhenhua reviewed Feb 21, 2025

View reviewed changes

website/docs/r/shared_image_version.html.markdown Outdated Show resolved Hide resolved

yeoldegrove force-pushed the shared_image_version_uefi_settings branch from 58ad4cf to 57ef77c Compare February 24, 2025 11:46

yeoldegrove requested a review from ms-zhenhua February 24, 2025 11:49

ms-zhenhua suggested changes Feb 26, 2025

View reviewed changes

yeoldegrove added 2 commits February 26, 2025 13:44

shared_image_version_resource - add uefi_settings

c6a0b90

shared_image_version_resource - add uefi_settings - docs

d74f0fc

yeoldegrove force-pushed the shared_image_version_uefi_settings branch from 57ef77c to d74f0fc Compare February 26, 2025 12:45

ms-zhenhua suggested changes Feb 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`shared_image_version` - support for new block `uefi_settings` #28076

`shared_image_version` - support for new block `uefi_settings` #28076

yeoldegrove commented Nov 20, 2024 •

edited

Loading

yeoldegrove commented Dec 4, 2024

github-actions bot commented Jan 20, 2025

yeoldegrove commented Feb 4, 2025

codgician commented Feb 4, 2025

yeoldegrove commented Feb 17, 2025

ms-zhenhua left a comment

yeoldegrove commented Feb 24, 2025

ms-zhenhua left a comment

yeoldegrove commented Feb 26, 2025

ms-zhenhua left a comment

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

ms-zhenhua Feb 27, 2025

yeoldegrove Feb 27, 2025

shared_image_version - support for new block uefi_settings #28076

Are you sure you want to change the base?

shared_image_version - support for new block uefi_settings #28076

Conversation

yeoldegrove commented Nov 20, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

yeoldegrove commented Dec 4, 2024

github-actions bot commented Jan 20, 2025

yeoldegrove commented Feb 4, 2025

codgician commented Feb 4, 2025

yeoldegrove commented Feb 17, 2025

ms-zhenhua left a comment

Choose a reason for hiding this comment

yeoldegrove commented Feb 24, 2025

ms-zhenhua left a comment

Choose a reason for hiding this comment

yeoldegrove commented Feb 26, 2025

ms-zhenhua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`shared_image_version` - support for new block `uefi_settings` #28076

`shared_image_version` - support for new block `uefi_settings` #28076

yeoldegrove commented Nov 20, 2024 •

edited

Loading