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.

Sign up for GitHub

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

Jump to bottom

r/azurerm_windows_virtual_machine and r/azurerm_windows_virtual_machine_scale_set - secure_boot_enabled, security_type, vtpm_enabled #13713

Merged
merged 18 commits into from
Nov 17, 2021
Merged

r/azurerm_windows_virtual_machine and r/azurerm_windows_virtual_machine_scale_set - secure_boot_enabled, security_type, vtpm_enabled #13713

merged 18 commits into from
Nov 17, 2021

Conversation

amccullough84
Copy link
Contributor

@amccullough84 amccullough84 commented Oct 13, 2021 •
edited by tombuildsstuff
Loading

Adds secure_boot_enabled and vtpm_enabled properties for windows_virtual_machine and windows_virtual_machine_scale_set.

Fixes #13039

Andy.McCullough added 3 commits October 12, 2021 17:23
Added additional SecurityProfile parameters for windows_virtual_machi…
9fba5fc 
…ne resources

Added SecurityType parameter to SecurityProfile and UefiSettings block.

Added Unit Tests
Fixup from Acceptance Tests
ed55cc8 
Resolved some mismatches from read function where a false is returned as null
refactor and add documentation
ca718da 
Added trusted platform features to windows_virtual_machine_scale_set and tests

Updated docs for new settings
@amccullough84 amccullough84 changed the title Add support for Trusted Launch features for windows_virtual_machine and windows_virtual_machine_scale_set Add support for Trusted Launch features for r/azurerm_windows_virtual_machine and r/azurerm_windows_virtual_machine_scale_set Oct 13, 2021
katbyte
katbyte approved these changes Oct 14, 2021
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @amccullough84 - LGTM 🏗️

@katbyte katbyte added this to the v2.81.0 milestone Oct 14, 2021
@katbyte katbyte changed the title Add support for Trusted Launch features for r/azurerm_windows_virtual_machine and r/azurerm_windows_virtual_machine_scale_set r/azurerm_windows_virtual_machine and r/azurerm_windows_virtual_machine_scale_set - secure_boot_enabled, security_type, security_type Oct 14, 2021
@katbyte katbyte changed the title r/azurerm_windows_virtual_machine and r/azurerm_windows_virtual_machine_scale_set - secure_boot_enabled, security_type, security_type r/azurerm_windows_virtual_machine and r/azurerm_windows_virtual_machine_scale_set - secure_boot_enabled, security_type, vtpm_enabled Oct 14, 2021
@katbyte katbyte modified the milestones: v2.81.0, v2.82.0 Oct 14, 2021
tombuildsstuff
tombuildsstuff suggested changes Oct 15, 2021
View reviewed changes
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.

hey @amccullough84

Thanks for this PR - I've taken a look through and left some comments inline but if we can fix those up then this should otherwise be good to merge 👍

Thanks!

internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_scale_set_resource.go Outdated Show resolved Hide resolved
Implement requested changes
5d778d8 
Simplified Resource Read in VM and VMSS

Updated references to TrustedLaunch to use compute.SecurityTypesTrustedLaunch constant
@katbyte katbyte modified the milestones: v2.82.0, v2.83.0 Oct 21, 2021
@github-actions
Copy link

This functionality has been released in v2.83.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!

@tombuildsstuff tombuildsstuff mentioned this pull request Oct 22, 2021
Merged
@ms-henglu
Copy link
Contributor

Hi @amccullough84 , thanks for the PR.

Sorry that I didn't notice this PR. and I'm working on the same thing,

And I found some properties may not align with the rest api spec.
https://docs.microsoft.com/en-us/rest/api/compute/virtual-machines/create-or-update#securityprofile

And I listed my concerns here: #13842 (comment)

Would you please take a look? Thanks!

@tombuildsstuff
Copy link
Contributor

@ms-henglu fwiw this intentionally differs from the API Spec - these properties should be top-level fields, so that's fine here.

tombuildsstuff
tombuildsstuff suggested changes Oct 28, 2021
View reviewed changes
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.

hey @amccullough84

Thanks for pushing those changes / apologies for the delayed re-review here.

I've taken a look through and left some comments inline regarding removing the security_type field (since there's only one value, and it's conditional on Secure Boot being enabled, so that can be implied) - and removing the update tests for both Secure Boot and VTPM (since neither of these values can be updated) - but if we can fix those up then this should be otherwise good to go 👍

Thanks!

internal/services/compute/windows_virtual_machine_resource_other_test.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource_other_test.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_scale_set_other_resource_test.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_scale_set_other_resource_test.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_scale_set_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_scale_set_resource.go Outdated Show resolved Hide resolved
@katbyte katbyte modified the milestones: v2.83.0, v2.84.0 Oct 29, 2021
amccullough84 and others added 7 commits November 2, 2021 12:51
@amccullough84 @tombuildsstuff
Update internal/services/compute/windows_virtual_machine_scale_set_ot…
660f9df 
…her_resource_test.go

Co-authored-by: Tom Harvey <[email protected]>
@amccullough84 @tombuildsstuff
Update internal/services/compute/windows_virtual_machine_resource.go
6b25033 
Co-authored-by: Tom Harvey <[email protected]>
@amccullough84 @tombuildsstuff
Update internal/services/compute/windows_virtual_machine_resource.go
137d0f2 
Co-authored-by: Tom Harvey <[email protected]>
@amccullough84 @tombuildsstuff
Update internal/services/compute/windows_virtual_machine_resource.go
bd92eaf 
Co-authored-by: Tom Harvey <[email protected]>
@amccullough84 @tombuildsstuff
Update internal/services/compute/windows_virtual_machine_scale_set_re…
7b4f7e3 
…source.go

Co-authored-by: Tom Harvey <[email protected]>
Update internal/services/compute/windows_virtual_machine_scale_set_ot…
37ef830 
…her_resource_test.go

removed update acceptance test - unnecessary with ForceNew
Remove security_type from internal/services/compute/windows_virtual_m…
31371fb 
…achine_scale_set_resource.go

security_type is inferred during create - not required
@katbyte katbyte modified the milestones: v2.84.0, v2.85.0 Nov 5, 2021
@katbyte katbyte modified the milestones: v2.85.0, v2.86.0 Nov 12, 2021
tombuildsstuff
tombuildsstuff approved these changes Nov 16, 2021
View reviewed changes
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.

hey @amccullough84

Thanks for pushing those changes - apologies for the delayed re-review here, I've spotted a couple of minor things which'll mean this doesn't build/test correctly - but if we can remove the unused lines (which I hope you don't mind but I'm going to push a commit to fix) then this should otherwise be good to go 👍

Thanks!

internal/services/compute/windows_virtual_machine_scale_set_resource.go Outdated Show resolved Hide resolved
internal/services/compute/windows_virtual_machine_resource.go Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff removed their assignment Nov 16, 2021
@tombuildsstuff
Copy link
Contributor

Tests look good 👍

@tombuildsstuff tombuildsstuff merged commit dbde9ec into hashicorp:main Nov 17, 2021
tombuildsstuff added a commit that referenced this pull request Nov 17, 2021
@tombuildsstuff
updating to include #13713
682f1ff
@github-actions
Copy link

This functionality has been released in v2.86.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

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 Dec 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

@katbyte katbyte katbyte approved these changes

Assignees
No one assigned
Labels
documentation enhancement service/virtual-machine size/XL
Projects
None yet
Milestone
v2.86.0
Development

Successfully merging this pull request may close these issues.

Support for full SecurityProfile for azurerm_windows_virtual_machine
4 participants
@amccullough84 @ms-henglu @tombuildsstuff @katbyte