Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r/azurerm_linux_virtual_machine and r/azurerm_linux_virtual_machine_scale_set - secure_boot_enabled, vtpm_enabled #13842

Merged

Conversation

ms-henglu
Copy link
Contributor

@ms-henglu ms-henglu commented Oct 22, 2021

=== RUN   TestAccLinuxVirtualMachine_otherUefi
=== PAUSE TestAccLinuxVirtualMachine_otherUefi
=== CONT  TestAccLinuxVirtualMachine_otherUefi
--- PASS: TestAccLinuxVirtualMachine_otherUefi (863.74s)

=== RUN   TestAccLinuxVirtualMachineScaleSet_otherUefi
=== PAUSE TestAccLinuxVirtualMachineScaleSet_otherUefi
=== CONT  TestAccLinuxVirtualMachineScaleSet_otherUefi
--- PASS: TestAccLinuxVirtualMachineScaleSet_otherUefi (750.46s)


=== RUN   TestAccWindowsVirtualMachine_otherUefi
=== PAUSE TestAccWindowsVirtualMachine_otherUefi
=== CONT  TestAccWindowsVirtualMachine_otherUefi
--- PASS: TestAccWindowsVirtualMachine_otherUefi (813.99s)

=== RUN   TestAccWindowsVirtualMachineScaleSet_otherUefi
=== PAUSE TestAccWindowsVirtualMachineScaleSet_otherUefi
=== CONT  TestAccWindowsVirtualMachineScaleSet_otherUefi
--- PASS: TestAccWindowsVirtualMachineScaleSet_otherUefi (781.65s)
PASS

Process finished with exit code 0

Fixes #13039

@tombuildsstuff
Copy link
Contributor

hey @ms-henglu

Thanks for this PR - although this looks to be a duplicate of #13713? Since we're flattening the properties in that PR - but this PR is also adding support for the Linux VM/VMSS resources, would you mind using the same approach for the Linux VM/VMSS (and reverting the Windows VM/VMSS changes here)?

Thanks!

@ms-henglu
Copy link
Contributor Author

Hi @tombuildsstuff ,

Thanks for the information, I didn't notice there's an existing PR. And yes, I'll change it.

@ms-henglu
Copy link
Contributor Author

ms-henglu commented Oct 22, 2021

Hi @tombuildsstuff ,

I checked that #13713, I think it may not align with rest api spec.
https://docs.microsoft.com/en-us/rest/api/compute/virtual-machines/create-or-update#securityprofile

  1. security_type is not a computed property, if users use secure_boot_enabled or vtpm_enabled, then user should set security_type to TrustLaunch. And I think we don't need to expose this property.
  2. secure_boot_enabled, vtpm_enabled should not be force new, actually only security_type needs to be force new. So I think the schema in this PR is better. Because when user add uefi block, it will force the resource to recreate, but changing secure_boot_enabled and v_tpm_enabled won't.
  uefi {
    secure_boot_enabled = true
    v_tpm_enabled = true
  }

What do you think?

@ms-henglu ms-henglu force-pushed the branch-211022-vm-supports-trust-launch branch from 04eb27d to 46b9539 Compare October 22, 2021 08:25
@ms-henglu
Copy link
Contributor Author

HI @tombuildsstuff , I've updated this PR to use same schema as 13713, please take another look, thanks!

@ms-henglu ms-henglu changed the title virtual machine supports uefi r/azurerm_linux_virtual_machine and r/azurerm_linux_virtual_machine_scale_set - secure_boot_enabled, vtpm_enabled Oct 22, 2021
@ms-henglu
Copy link
Contributor Author

Hi @tombuildsstuff , I've updated this PR, would you please take another look? And would you please also remove the duplicate label? Thanks!

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @ms-henglu

Thanks for pushing those changes - I've taken a look through and left a few comments inline, if we can fix those up then this should otherwise be good to go 👍

Thanks!

name = "acctestVM-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
size = "Standard_DS3_v2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a machine with 4 cores and 14GB of ram.. an F1 will be more than sufficient here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried F1, but got "The selected VM size 'Standard_F1' cannot boot Hypervisor Generation '2'"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need to use the Gen1 image too (see the other VM/VMSS tests for examples)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this feature can only be enabled in Gen2 image.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in which case can we pick a smaller vm size then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to Standard_B1ls, please take another look, thanks!

@ms-henglu
Copy link
Contributor Author

Hi @tombuildsstuff ,

I've added a commit according to your suggestions, would you please take another look? thanks!

@ms-henglu
Copy link
Contributor Author

Hi @tombuildsstuff ,

I changed them to use a smaller vm size, please take another look, thanks!

@katbyte katbyte removed this from the v2.86.0 milestone Nov 19, 2021
@katbyte katbyte added this to the v2.87.0 milestone Nov 19, 2021
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for making those changes @ms-henglu

@jackofallops jackofallops modified the milestones: v2.87.0, v2.88.0 Nov 26, 2021
@katbyte katbyte merged commit 979672d into hashicorp:main Nov 26, 2021
katbyte added a commit that referenced this pull request Nov 26, 2021
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

This functionality has been released in v2.88.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented Jan 2, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for full SecurityProfile for azurerm_windows_virtual_machine
4 participants