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

azurerm_virtual_network: support encryption #22745

Merged
merged 4 commits into from Aug 3, 2023

Conversation

wuxu92
Copy link
Contributor

@wuxu92 wuxu92 commented Jul 31, 2023

--- PASS: TestAccVirtualNetwork_complete (142.58s)
PASS

Copy link
Member

@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 @wuxu92

Thanks for this PR - I've taken a look through and left some comments inline, if we can address those then we should be able to take another look.

Thanks!

Comment on lines 112 to 115
"enabled": {
Type: pluginsdk.TypeBool,
Required: true,
},
Copy link
Member

Choose a reason for hiding this comment

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

we don't expose the enabled field, since this can be tracked by the presence of the block - can we remove this field (and make unencrypted_allowed required, since it'll be the only inner field)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -430,6 +467,21 @@ func flattenVirtualNetworkDDoSProtectionPlan(input *network.VirtualNetworkProper
}
}

func flattenVirtualNetworkEncryption(encryption *network.VirtualNetworkEncryption) interface{} {
if encryption == nil {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

this means we won't be outputting this correctly

Suggested change
return nil
return make([]interface{}, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


* `enabled` - (Required) Enable/disable encryption on Virtual Network.

* `unencrypted_allowed` - (Optional) Whether ths virtual network allos VM that does not support encryption. value `false` for `DropUnencrypted`. value `true` for `AllowUnencrypted`. Defaults to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

We should expose this as the string constant, not as a boolean, since it's likely other values will get added here in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! document updated.

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.

LGTM ☔

@katbyte katbyte merged commit 96d48fc into hashicorp:main Aug 3, 2023
23 checks passed
katbyte added a commit that referenced this pull request Aug 3, 2023
@github-actions github-actions bot added this to the v3.68.0 milestone Aug 3, 2023
Comment on lines +433 to +439
if vList := v.([]interface{}); len(vList) > 0 && vList[0] != nil {
encryptionConf := vList[0].(map[string]interface{})
properties.Encryption = &network.VirtualNetworkEncryption{
Enabled: pointer.To(true),
Enforcement: network.VirtualNetworkEncryptionEnforcement(encryptionConf["enforcement"].(string)),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@wuxu92 how would users remove this value? We'd need to send Encryption.enabled = false when len(vList) == 0?

Copy link
Contributor Author

@wuxu92 wuxu92 Aug 3, 2023

Choose a reason for hiding this comment

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

the update method will set the Encryption.Enabled to false when leave the Encryption block as null(not set).
and I submitted a PR to update the acctest for this case: #22807.

so users can just remove the encryption block in terraform configuration, then it will be set to false automatically.

image

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.

None yet

3 participants