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

@wuxu92 wuxu92 force-pushed the vnet/encryption branch from c59f1e7 to deec543 Compare July 31, 2023 06:51
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 @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
Contributor

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
Contributor

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
Contributor

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
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
Contributor

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

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 May 19, 2024
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.

3 participants