New Resource: azurerm_virtual_hub_connection #5951

neil-yechenwei · 2020-03-02T08:01:05Z

This PR is to improve the code as the issue #5613 is saying.

tombuildsstuff · 2020-03-04T00:47:21Z

Thanks for this PR.

Due to the way the Networking API's work, the last time we looked at this it appeared that this functionality wanted splitting out into it's own virtual resource (azurerm_virtual_hub_connection) rather than being nested within the azurerm_virtual_hub resource. There's a branch tracking this functionality pushed here however the API was broken the last time we took a look at this - would you be able to confirm if this has been fixed?

If so rather than nesting this within the azurerm_virtual_hub resource could we instead switch to using a Virtual Resource instead as in the other branch? This approach allows us to better represent the behaviour of the Networking API's.

Thanks!

neil-yechenwei · 2020-03-04T07:49:13Z

hi @tombuildsstuff , seems I misunderstood your meaning before.
Previously I assume we should make "virtual hub connection" as separate resource like "resource_arm_virtual_network_gateway_connection.go" with independent "connection client" like "https://github.com/terraform-providers/terraform-provider-azurerm/blob/9aa32daf9a440fa94996d255e056f11edb8415d1/azurerm/internal/services/network/resource_arm_virtual_network_gateway_connection.go#L257". So I found the independent "HubVirtualNetworkConnectionClient" (https://github.com/Azure/azure-sdk-for-go/blob/090dc0ee4d8d2d60e2a9525774d967a4111a2b0c/services/network/mgmt/2019-11-01/network/hubvirtualnetworkconnections.go#L50) for virtual hub doesn't support create/update operation. According by your commit "6b49836", seems that I should still use "VirtualHubClient" to update "virtual hub connection" in independent virtual resource. Thanks for your suggestion. I will update this PR to make "virtual hub connection" as separate virtual resource following the commit 6b49836 .

neil-yechenwei · 2020-03-05T10:06:56Z

@tombuildsstuff , I've updated this PR according by your commit 6b49836

katbyte

thanks for the PR @neil-yechenwei,

i've given this a quick review and most of my comments are minor or around naming.

katbyte · 2020-03-05T13:21:08Z

azurerm/internal/services/network/resource_arm_virtual_hub_connection.go

+				ForceNew: true,
+			},
+
+			"enable_internet_security": {


Could we change this to

Suggested change

"enable_internet_security": {

"internet_security_enabled": {

katbyte · 2020-03-05T13:22:40Z

website/docs/r/virtual_hub_connection.html.markdown

+
+* `allow_remote_vnet_to_use_hub_vnet_gateways` - (Optional) Allow the Remote Virtual Network to transit use the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.
+
+* `enable_internet_security` - (Optional) Should Internet Security be enabled? Changing this forces a new resource to be created.


COuld we expand on wha internet security is a little here?

katbyte · 2020-03-05T13:23:12Z

website/docs/r/virtual_hub_connection.html.markdown

+
+* `allow_hub_to_remote_vnet_transit` - (Optional) Allow the Virtual Hub to transit traffic via the Remote Virtual Network? Changing this forces a new resource to be created. 
+
+* `allow_remote_vnet_to_use_hub_vnet_gateways` - (Optional) Allow the Remote Virtual Network to transit use the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.


Suggested change

* `allow_remote_vnet_to_use_hub_vnet_gateways` - (Optional) Allow the Remote Virtual Network to transit use the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.

* `allow_remote_vnet_to_use_hub_gateways` - (Optional) Is Remote Virtual Network traffic allowed to transit the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.

website/docs/r/virtual_hub_connection.html.markdown

katbyte · 2020-03-05T13:46:50Z

azurerm/internal/services/network/tests/resource_arm_virtual_hub_connection_test.go

+		resourceGroup := id.ResourceGroup
+		hubName := id.Name


We don't need to assign these to locals?

katbyte · 2020-03-05T13:47:31Z

azurerm/internal/services/network/resource_arm_virtual_hub_connection.go

+	resourceGroup := id.ResourceGroup
+	virtualHubName := id.Name


I don't think we need to assign these to locals?

katbyte · 2020-03-05T13:50:01Z

azurerm/internal/services/network/tests/resource_arm_virtual_hub_connection_test.go

+  allow_remote_vnet_to_use_hub_vnet_gateways = false
+  enable_internet_security                   = true
+}
+`, template, data.RandomInteger)


could we define two virtual hubs for the complete test, and then have another update test where we go basic -> compelte -> basic to ensure we correctly handel multiple?

katbyte · 2020-03-05T13:51:08Z

azurerm/internal/services/network/tests/resource_arm_virtual_hub_connection_test.go

+			return fmt.Errorf("Virtual Hub Connection not found: %s", resourceName)
+		}
+
+		virtualHubId := rs.Primary.Attributes["virtual_hub_id"]


We should be able to get the primary ID and parse it as a virtual_hub_connection id?

Here is getting virtual_hub_id not virtual hub connection id. so won't fix.

You should be able to get everything require from ParseVirtualHubConnectionID as we do on read/delete

katbyte · 2020-03-05T13:51:13Z

azurerm/internal/services/network/tests/resource_arm_virtual_hub_connection_test.go

+			continue
+		}
+
+		virtualHubId := rs.Primary.Attributes["virtual_hub_id"]


Here is getting virtual_hub_id not virtual hub connection id. so won't fix.

neil-yechenwei · 2020-03-06T09:04:19Z

@katbyte , Thanks for your comments. I've updated code.

katbyte

Thanks for the revisions @neil-yechenwei, i have left a number of formatting and style comments inline that should be adressed

azurerm/internal/services/network/resource_arm_virtual_hub_connection.go

katbyte · 2020-03-09T20:43:27Z

azurerm/internal/services/network/tests/resource_arm_virtual_hub_connection_test.go

+			return fmt.Errorf("Virtual Hub Connection not found: %s", resourceName)
+		}
+
+		virtualHubId := rs.Primary.Attributes["virtual_hub_id"]


You should be able to get everything require from ParseVirtualHubConnectionID as we do on read/delete

katbyte · 2020-03-09T20:43:31Z

azurerm/internal/services/network/tests/resource_arm_virtual_hub_connection_test.go

+			continue
+		}
+
+		virtualHubId := rs.Primary.Attributes["virtual_hub_id"]


katbyte · 2020-03-09T20:49:48Z

azurerm/internal/services/network/resource_arm_virtual_hub_connection.go

+	if virtualHub.VirtualHubProperties == nil {
+		return fmt.Errorf("Error retrieving Virtual Hub %q (Resource Group %q): `properties` was nil", id.Name, resourceGroup)
+	}
+	if virtualHub.VirtualHubProperties.VirtualNetworkConnections == nil {
+		return fmt.Errorf("Error retrieving Virtual Hub %q (Resource Group %q): `properties.VirtualNetworkConnections` was nil", id.Name, resourceGroup)
+	}
+
+	connections = *virtualHub.VirtualHubProperties.VirtualNetworkConnections
+	newConnection, _ := findVirtualHubConnection(name, connections)


I think if you moved the checks into the find function and took the vhub this would be much more clear

Suggested change

if virtualHub.VirtualHubProperties == nil {

return fmt.Errorf("Error retrieving Virtual Hub %q (Resource Group %q): `properties` was nil", id.Name, resourceGroup)

}

if virtualHub.VirtualHubProperties.VirtualNetworkConnections == nil {

return fmt.Errorf("Error retrieving Virtual Hub %q (Resource Group %q): `properties.VirtualNetworkConnections` was nil", id.Name, resourceGroup)

}

connections = *virtualHub.VirtualHubProperties.VirtualNetworkConnections

newConnection, _ := findVirtualHubConnection(name, connections)

newConnection, _ := findVirtualHubConnection(name, virtualHub)

katbyte · 2020-03-09T20:51:23Z

azurerm/internal/services/network/resource_arm_virtual_hub_connection.go

+	if connection.HubVirtualNetworkConnectionProperties == nil {
+		return fmt.Errorf("Error retrieving Connection %q (Virtual Hub %q / Resource Group %q): `properties` was nil`", id.Name, virtualHubName, resourceGroup)
+	}
+	props := *connection.HubVirtualNetworkConnectionProperties
+
+	d.Set("name", id.Name)
+	d.Set("virtual_hub_id", virtualHub.ID)
+	d.Set("hub_to_vitual_network_traffic_allowed", props.AllowHubToRemoteVnetTransit)
+	d.Set("vitual_network_to_hub_gateways_traffic_allowed", props.AllowRemoteVnetToUseHubVnetGateways)
+	d.Set("internet_security_enabled", props.EnableInternetSecurity)
+
+	remoteVirtualNetworkId := ""
+	if props.RemoteVirtualNetwork != nil && props.RemoteVirtualNetwork.ID != nil {
+		remoteVirtualNetworkId = *props.RemoteVirtualNetwork.ID
+	}
+	d.Set("remote_virtual_network_id", remoteVirtualNetworkId)


Could we be consistent with other resources

Suggested change

if connection.HubVirtualNetworkConnectionProperties == nil {

return fmt.Errorf("Error retrieving Connection %q (Virtual Hub %q / Resource Group %q): `properties` was nil`", id.Name, virtualHubName, resourceGroup)

}

props := *connection.HubVirtualNetworkConnectionProperties

d.Set("name", id.Name)

d.Set("virtual_hub_id", virtualHub.ID)

d.Set("hub_to_vitual_network_traffic_allowed", props.AllowHubToRemoteVnetTransit)

d.Set("vitual_network_to_hub_gateways_traffic_allowed", props.AllowRemoteVnetToUseHubVnetGateways)

d.Set("internet_security_enabled", props.EnableInternetSecurity)

remoteVirtualNetworkId := ""

if props.RemoteVirtualNetwork != nil && props.RemoteVirtualNetwork.ID != nil {

remoteVirtualNetworkId = *props.RemoteVirtualNetwork.ID

}

d.Set("remote_virtual_network_id", remoteVirtualNetworkId)

if connection.HubVirtualNetworkConnectionProperties == nil {

return fmt.Errorf("Error retrieving Connection %q (Virtual Hub %q / Resource Group %q): `properties` was nil`", id.Name, virtualHubName, resourceGroup)

}

d.Set("name", id.Name)

d.Set("virtual_hub_id", virtualHub.ID)

if props := connection.HubVirtualNetworkConnectionProperties; props != nil {

d.Set("hub_to_vitual_network_traffic_allowed", props.AllowHubToRemoteVnetTransit)

d.Set("vitual_network_to_hub_gateways_traffic_allowed", props.AllowRemoteVnetToUseHubVnetGateways)

d.Set("internet_security_enabled", props.EnableInternetSecurity)

remoteVirtualNetworkId := ""

if props.RemoteVirtualNetwork != nil && props.RemoteVirtualNetwork.ID != nil {

remoteVirtualNetworkId = *props.RemoteVirtualNetwork.ID

}

d.Set("remote_virtual_network_id", remoteVirtualNetworkId)

}

katbyte · 2020-03-09T20:53:09Z

azurerm/internal/services/network/resource_arm_virtual_hub_connection.go

+	return nil
+}
+
+func findVirtualHubConnection(name string, connections []network.HubVirtualNetworkConnection) (conn *network.HubVirtualNetworkConnection, index int) {


I didn't notice the index used anywhere

Suggested change

func findVirtualHubConnection(name string, connections []network.HubVirtualNetworkConnection) (conn *network.HubVirtualNetworkConnection, index int) {

func findVirtualHubConnection(name string, connections []network.HubVirtualNetworkConnection) *network.HubVirtualNetworkConnection {

katbyte · 2020-03-09T20:53:30Z

azurerm/internal/services/network/resource_arm_virtual_hub_connection.go

+			conn = &connection
+			index = i
+			return


Suggested change

conn = &connection

index = i

return

return &connection

neil-yechenwei · 2020-03-10T16:22:02Z

@katbyte , I've updated code. Thanks for your comments.

katbyte

Thanks for the updates @neil-yechenwei! LGTM now 👍

ghost · 2020-03-19T00:00:22Z

This has been released in version 2.2.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.2.0"
}
# ... other configuration ...

ghost · 2020-04-16T14:50:25Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

neil-yechenwei added 2 commits March 2, 2020 15:53

Add more support for virtual hub

e5840fa

Update code

0728838

ghost added size/L documentation labels Mar 2, 2020

tombuildsstuff added question service/virtual-hub waiting-response labels Mar 4, 2020

ghost removed the waiting-response label Mar 4, 2020

Update code

403fdab

ghost added size/XL and removed size/L labels Mar 5, 2020

katbyte changed the title ~~Add support for hub virtual network connection - virtual hub~~ New Resource: azurerm_virtual_hub_connection Mar 5, 2020

katbyte requested changes Mar 5, 2020

View reviewed changes

katbyte added waiting-response and removed question labels Mar 5, 2020

katbyte added this to the v2.1.0 milestone Mar 5, 2020

Update code

de1b29c

neil-yechenwei requested a review from katbyte March 6, 2020 09:03

ghost removed the waiting-response label Mar 6, 2020

katbyte requested changes Mar 9, 2020

View reviewed changes

katbyte added the waiting-response label Mar 9, 2020

Update code

522d4a9

neil-yechenwei requested a review from katbyte March 10, 2020 16:21

ghost removed the waiting-response label Mar 10, 2020

tombuildsstuff removed this from the v2.1.0 milestone Mar 11, 2020

tombuildsstuff added this to the v2.2.0 milestone Mar 11, 2020

katbyte approved these changes Mar 13, 2020

View reviewed changes

katbyte merged commit 27686d5 into hashicorp:master Mar 17, 2020

katbyte added a commit that referenced this pull request Mar 17, 2020

update CHANGELOG.md to include #5951

690d64e

ghost locked and limited conversation to collaborators Apr 16, 2020


		* `allow_remote_vnet_to_use_hub_vnet_gateways` - (Optional) Allow the Remote Virtual Network to transit use the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.

		* `enable_internet_security` - (Optional) Should Internet Security be enabled? Changing this forces a new resource to be created.


		* `allow_hub_to_remote_vnet_transit` - (Optional) Allow the Virtual Hub to transit traffic via the Remote Virtual Network? Changing this forces a new resource to be created.

		* `allow_remote_vnet_to_use_hub_vnet_gateways` - (Optional) Allow the Remote Virtual Network to transit use the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.

	* `allow_remote_vnet_to_use_hub_vnet_gateways` - (Optional) Allow the Remote Virtual Network to transit use the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.
	* `allow_remote_vnet_to_use_hub_gateways` - (Optional) Is Remote Virtual Network traffic allowed to transit the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.

	func findVirtualHubConnection(name string, connections []network.HubVirtualNetworkConnection) (conn *network.HubVirtualNetworkConnection, index int) {
	func findVirtualHubConnection(name string, connections []network.HubVirtualNetworkConnection) *network.HubVirtualNetworkConnection {

New Resource: azurerm_virtual_hub_connection #5951

New Resource: azurerm_virtual_hub_connection #5951

Conversation

neil-yechenwei commented Mar 2, 2020 • edited Loading

tombuildsstuff commented Mar 4, 2020

neil-yechenwei commented Mar 4, 2020

neil-yechenwei commented Mar 5, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Mar 6, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Mar 10, 2020

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Mar 19, 2020

ghost commented Apr 16, 2020

neil-yechenwei commented Mar 2, 2020 •

edited

Loading