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

Enable trusted launch for existing-WVD-host-pool-arm #656

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aprillc
Copy link

@aprillc aprillc commented Nov 2, 2021

Add three trusted launch parameters for the ARM template.

@philipktlin
Copy link
Contributor

@aprillc
Copy link
Author

aprillc commented Nov 3, 2021

@aprillc aprillc closed this Nov 3, 2021
@aprillc aprillc reopened this Nov 3, 2021
@aprillc
Copy link
Author

aprillc commented Nov 3, 2021

clicked wrong button

@aprillc
Copy link
Author

aprillc commented Nov 3, 2021

@@ -305,6 +305,27 @@
"description": "Session host configuration version of the host pool."
},
"defaultValue": ""
},
"securityType": {
Copy link
Member

Choose a reason for hiding this comment

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

These parameters aren't passed to the virtual machine so it doesn't do anything.

@@ -305,6 +305,27 @@
"description": "Session host configuration version of the host pool."
},
"defaultValue": ""
},
"securityType": {
Copy link
Member

Choose a reason for hiding this comment

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

These parameters aren't passed to the virtual machine so it doesn't do anything.

@@ -409,6 +409,27 @@
"description": "System data is used for internal purposes, such as support preview features."
},
"defaultValue": {}
},
"securityType": {
Copy link
Member

@lintFan lintFan Nov 3, 2021

Choose a reason for hiding this comment

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

If the only valid value for this is an empty string or TrustedLaunch then you should specify "allowedValues" with those values so the customer will get an error if they enter something else.
This applies to the other ARM templates in this PR too.

"secureBoot": {
"type": "bool",
"metadata": {
"description": "Specifies whether secure boot should be enabled on the virtual machine."
Copy link
Member

@lintFan lintFan Nov 3, 2021

Choose a reason for hiding this comment

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

Should say this is only used if securityType is set to "TrustedLaunch".
This applies to the other ARM templates in this PR too.

"vTPM": {
"type": "bool",
"metadata": {
"description": "Specifies whether vTPM (Virtual Trusted Platform Module) should be enabled on the virtual machine."
Copy link
Member

@lintFan lintFan Nov 3, 2021

Choose a reason for hiding this comment

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

Should say this is only used if securityType is set to "TrustedLaunch".
This applies to the other ARM templates in this PR too.

"securityType": {
"type": "string",
"metadata": {
"description": "Specifies the SecurityType of the virtual machine. It is set as TrustedLaunch to enable UefiSettings. Default: UefiSettings will not be enabled unless this property is set as TrustedLaunch."
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be helpful to include a link to the docs for this here https://docs.microsoft.com/en-us/azure/templates/microsoft.compute/2021-07-01/virtualmachines?tabs=bicep#securityprofile so the customer has more context about this.

@lintFan
Copy link
Member

lintFan commented Nov 3, 2021

Would you please help to remove https://github.com/Azure/RDS-Templates/blob/master/ARM-wvd-templates/nestedtemplates/unmanagedDisks-customvhdvm.json? thanks.

Removed.

It looks like unmanagedDisks-customvhdvm.json is still supported in AddVirtualMachinesTemplate.json and CreateHostpoolTemplate.json . If you want to remove support for it then I suggest taking a look at this PR https://github.com/Azure/RDS-Templates/pull/406/files where WVD classic removed support for it. You may also want to announce it as an upcoming breaking change to customer before making the change too. Although it was announced here (I think before the non-classic templates were released) so it might be ok.

This also seems un-related to the other changes in this PR so maybe make an changes around unmanagedDisks-customvhdvm.json in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants