azurerm_batch_pool - support for new block security_profile
#28069
+158
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
enhancement
documentation
service/batch
size/M
and removed
enhancement
labels
liuwuliuyun
changed the title
azurerm_batch_pool - support new block security_profileazurerm_batch_pool - support for new block security_profile
catriona-m
requested changes
Dec 9, 2024
There was a problem hiding this comment.
Thanks for this @liuwuliuyun - I've had a look through and left some suggestions inline but once those are addressed I can take another look. Thanks!
| @@ -740,6 +740,24 @@ func TestAccBatchPool_interNodeCommunicationWithTaskSchedulingPolicy(t *testing. | |||
| }) | |||
| } | |||
|
|
|||
| func TestAccBatchPool_securityProfileWithUEFISettings(t *testing.T) { | |||
There was a problem hiding this comment.
could we add additional steps to this test to test updating the properties in the block and to test adding/removing the block?
There was a problem hiding this comment.
Oh I have just recalled that the security_profile can only be specified during creation and does not support updates. I will ensure this information is added to the documentation.
There was a problem hiding this comment.
Plus I will mark this block as forceNew
Comment on lines
1283
to
1296
| if config.SecurityProfile != nil { | ||
| securityProfile := make([]interface{}, 0) | ||
| securityConfig := make(map[string]interface{}) | ||
| securityConfig["host_encryption_enabled"] = pointer.ToBool(config.SecurityProfile.EncryptionAtHost) | ||
| if config.SecurityProfile.SecurityType != nil { | ||
| securityConfig["security_type"] = string(*config.SecurityProfile.SecurityType) | ||
| } | ||
| if config.SecurityProfile.UefiSettings != nil { | ||
| securityConfig["secure_boot_enabled"] = pointer.ToBool(config.SecurityProfile.UefiSettings.SecureBootEnabled) | ||
| securityConfig["vtpm_enabled"] = pointer.ToBool(config.SecurityProfile.UefiSettings.VTpmEnabled) | ||
| } | ||
| securityProfile = append(securityProfile, securityConfig) | ||
| d.Set("security_profile", securityProfile) | ||
| } |
There was a problem hiding this comment.
should we move this into a flatten func?
Co-authored-by: catriona-m <[email protected]>
|
New testing evidence: |
catriona-m
requested changes
Dec 11, 2024
There was a problem hiding this comment.
Thanks for updating this @liuwuliuyun - I left a couple more suggestions inline but can take another look after those are addressed. Thanks!
Comment on lines
418
to
420
| if configProfile.EncryptionAtHost != nil { | ||
| securityConfig["host_encryption_enabled"] = *configProfile.EncryptionAtHost | ||
| } |
There was a problem hiding this comment.
Suggested change
| if configProfile.EncryptionAtHost != nil { | |
| securityConfig["host_encryption_enabled"] = *configProfile.EncryptionAtHost | |
| } | |
| securityConfig["host_encryption_enabled"] = pointer.From(configProfile.EncryptionAtHost) |
Comment on lines
422
to
424
| if configProfile.SecurityType != nil { | ||
| securityConfig["security_type"] = string(*configProfile.SecurityType) | ||
| } |
There was a problem hiding this comment.
Suggested change
| if configProfile.SecurityType != nil { | |
| securityConfig["security_type"] = string(*configProfile.SecurityType) | |
| } | |
| securityConfig["security_type"] = pointer.From(*configProfile.SecurityType) |
Comment on lines
427
to
429
| if configProfile.UefiSettings.SecureBootEnabled != nil { | ||
| securityConfig["secure_boot_enabled"] = pointer.ToBool(configProfile.UefiSettings.SecureBootEnabled) | ||
| } |
There was a problem hiding this comment.
Suggested change
| if configProfile.UefiSettings.SecureBootEnabled != nil { | |
| securityConfig["secure_boot_enabled"] = pointer.ToBool(configProfile.UefiSettings.SecureBootEnabled) | |
| } | |
| securityConfig["secure_boot_enabled"] = pointer.From(configProfile.UefiSettings.SecureBootEnabled) |
Comment on lines
430
to
432
| if configProfile.UefiSettings.VTpmEnabled != nil { | ||
| securityConfig["vtpm_enabled"] = pointer.ToBool(configProfile.UefiSettings.VTpmEnabled) | ||
| } |
There was a problem hiding this comment.
Suggested change
| if configProfile.UefiSettings.VTpmEnabled != nil { | |
| securityConfig["vtpm_enabled"] = pointer.ToBool(configProfile.UefiSettings.VTpmEnabled) | |
| } | |
| securityConfig["vtpm_enabled"] = pointer.From(configProfile.UefiSettings.VTpmEnabled) |
| } | ||
|
|
||
| if v, ok := item["host_encryption_enabled"]; ok { | ||
| securityProfile.EncryptionAtHost = pointer.FromBool(v.(bool)) |
There was a problem hiding this comment.
Suggested change
| securityProfile.EncryptionAtHost = pointer.FromBool(v.(bool)) | |
| securityProfile.EncryptionAtHost = pointer.To(v.(bool)) |
| } | ||
|
|
||
| if v, ok := item["secure_boot_enabled"]; ok { | ||
| securityProfile.UefiSettings.SecureBootEnabled = pointer.FromBool(v.(bool)) |
There was a problem hiding this comment.
Suggested change
| securityProfile.UefiSettings.SecureBootEnabled = pointer.FromBool(v.(bool)) | |
| securityProfile.UefiSettings.SecureBootEnabled = pointer.To(v.(bool)) |
| } | ||
|
|
||
| if v, ok := item["vtpm_enabled"]; ok { | ||
| securityProfile.UefiSettings.VTpmEnabled = pointer.FromBool(v.(bool)) |
There was a problem hiding this comment.
Suggested change
| securityProfile.UefiSettings.VTpmEnabled = pointer.FromBool(v.(bool)) | |
| securityProfile.UefiSettings.VTpmEnabled = pointer.To(v.(bool)) |
| @@ -165,6 +165,8 @@ The following arguments are supported: | |||
|
|
|||
| * `os_disk_placement` - (Optional) Specifies the ephemeral disk placement for operating system disk for all VMs in the pool. This property can be used by user in the request to choose which location the operating system should be in. e.g., cache disk space for Ephemeral OS disk provisioning. For more information on Ephemeral OS disk size requirements, please refer to Ephemeral OS disk size requirements for Windows VMs at <https://docs.microsoft.com/en-us/azure/virtual-machines/windows/ephemeral-os-disks#size-requirements> and Linux VMs at <https://docs.microsoft.com/en-us/azure/virtual-machines/linux/ephemeral-os-disks#size-requirements>. The only possible value is `CacheDisk`. | |||
|
|
|||
| * `security_profile` - (Optional) A `security_profile` block that describes the security settings for the Batch pool as defined below. | |||
There was a problem hiding this comment.
can we add here that Changing this forces a new resource to be created.?
Comment on lines
+510
to
+516
| * `host_encryption_enabled` - (Optional) Whether to enable host encryption for the Virtual Machine or Virtual Machine Scale Set. This will enable the encryption for all the disks including Resource/Temp disk at host itself. Possible values are `true` and `false`. Changing this forces a new resource to be created. | ||
|
|
||
| * `security_type` - (Optional) The security type of the Virtual Machine. Possible values are `confidentialVM` and `trustedLaunch`. Changing this forces a new resource to be created. | ||
|
|
||
| * `secure_boot_enabled` - (Optional) Whether to enable secure boot for the Virtual Machine or Virtual Machine Scale Set. Possible values are `true` and `false`. Changing this forces a new resource to be created. | ||
|
|
||
| * `vtpm_enabled` - (Optional) Whether to enable virtual trusted platform module (vTPM) for the Virtual Machine or Virtual Machine Scale Set. Possible values are `true` and `false`. Changing this forces a new resource to be created. |
There was a problem hiding this comment.
if the properties within the block cannot be updated once it's been created as well, we should add ForceNew to these properties in the schema too
catriona-m
added a commit
that referenced
this pull request
Dec 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community Note
Description
For resource
azurerm_batch_poolsecurity_profilesecurity_profilePR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_batch_pool- support for new blocksecurity_profile[azurerm_batch_pool- support for new blocksecurity_profile#28069]This is a (please select all that apply):
Related Issue(s)
Fixes #27952