New Resource & Data Source: `azurerm_app_service_virtual_network_connection_gateway` #4458

njuCZ · 2019-09-30T07:50:00Z

Add new resource and data source for Gateway Required App Service Virtual Network Connection
(fix [#4333, #4332])

katbyte

I started reviewing this and have left a bunch of comments inline. While going through the create function it appears we are calling a bunch of different APIs. Would this resource make more sense a a few individual ones that combine for the same affect? possible a virtual_network_gateway_connection and a virtual_network_gateway_connection_app_service_association resource?

azurerm/data_source_app_service_virtual_network_connection.go

azurerm/data_source_app_service_virtual_network_connection_test.go

azurerm/resource_arm_app_service_virtual_network_connection.go

website/docs/r/app_service_virtual_network_connection.html.markdown

azurerm/resource_arm_app_service_virtual_network_connection.go

njuCZ · 2019-11-06T06:15:01Z

Hi @njuCZ,

I started reviewing this and have left a bunch of comments inline. While going through the create function it appears we are calling a bunch of different APIs. Would this resource make more sense a a few individual ones that combine for the same affect? possible a virtual_network_gateway_connection and a virtual_network_gateway_connection_app_service_association resource?

this resource is to intergrate an app service with a virtual network. It's one whole function in the azure portal, though it needs call two different api. there is no need to separate into two different resources

njuCZ · 2019-11-06T06:27:31Z

@katbyte I have modified my codes according to your suggestion, could you kindly review my codes once more?

katbyte

Thanks for the revisions @njuCZ,

In addition to the comments i've left i'm wondering if it makes sense to export all those properties? are they available via the app service or vnet resource/data sources? because if they are it might make sense to leave them out with a comment stating as such and users can just instead use those, wdyt?

azurerm/data_source_app_service_virtual_network_connection.go

azurerm/resource_arm_app_service_virtual_network_connection.go

azurerm/resource_arm_app_service_virtual_network_connection_test.go

website/docs/d/app_service_virtual_network_connection.html.markdown

website/docs/r/app_service_virtual_network_connection.html.markdown

katbyte · 2019-11-15T01:26:20Z

Also we are getting test failures:

------- Stdout: -------
=== RUN   TestAccDataSourceAzureRMAppServiceVirtualNetworkConnection_basic
=== PAUSE TestAccDataSourceAzureRMAppServiceVirtualNetworkConnection_basic
=== CONT  TestAccDataSourceAzureRMAppServiceVirtualNetworkConnection_basic
--- FAIL: TestAccDataSourceAzureRMAppServiceVirtualNetworkConnection_basic (2946.44s)
    testing.go:569: Step 0 error: errors during apply:
        
        Error: error add certificate for gateway "example": error adding cerfiticate for gateway "example" (Resource Group "acctestRG-191114233037998613"): network.VirtualNetworkGatewaysClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="AnotherOperationInProgress" Message="Another operation on this or dependent resource is in progress. To retrieve status of the operation use uri: https://management.azure.com/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/providers/Microsoft.Network/locations/westeurope/operations/a672b118-fb92-4de0-911a-2bdd5a3c3050?api-version=2019-06-01." Details=[]

njuCZ · 2019-11-15T11:20:19Z

@katbyte thank you for your suggestion, I have fixed them

katbyte

thanks for the updates @njuCZ, i've left some more comments inline that need to be addressed before merge.

azurerm/data_source_app_service_virtual_network_connection.go

azurerm/resource_arm_app_service_virtual_network_connection.go

website/docs/r/app_service_virtual_network_connection.html.markdown

azurerm/resource_arm_app_service_virtual_network_connection.go

website/docs/r/app_service_virtual_network_connection.html.markdown

njuCZ · 2019-12-20T05:12:29Z

hi @katbyte I have refactored to azurerm_app_service_virtual_network_connection_gateway

katbyte

Hi @njuCZ,

I've given this another review and left my comments inline. I'd really like some additional detail in comments about the workflow of the create function and the steps its taking. Thanks!

katbyte · 2019-12-26T05:36:01Z

azurerm/internal/services/web/resource_arm_app_service_virtual_network_connection_gateway.go

+		return fmt.Errorf("this gateways %q under vnet %q (Resource Group %q) does not have a Point-to-site Address Range. Please specify one in CIDR notation, e.g. 10.0.0.0/8", gatewayName, vnetName, vnetResGroup)
+	}
+
+	isRelated, err := checkGatewayInVirtualNetwork(virtualNetworkGateway, vnetId)


Could we get some comments explaining what the workflow is here and these function calls aredoing?

I have left some comments in the codes for the steps. At first I want to find the virtual network gateway from the virtual network, now we have both parameters: virtual_network_id and virtual_network_gateway_id, so I first check whether they are related. (thus virtual network gateway should located in a gateway subnet in the virtual network)

azurerm/internal/services/web/data_source_app_service_virtual_network_connection_gateway.go

azurerm/internal/services/web/resource_arm_app_service_virtual_network_connection_gateway.go

...ernal/services/web/tests/resource_arm_app_service_virtual_network_connection_gateway_test.go

katbyte · 2020-03-03T21:23:28Z

...ernal/services/web/tests/resource_arm_app_service_virtual_network_connection_gateway_test.go

+
+  lifecycle {
+    ignore_changes = [
+      vpn_client_configuration.0.root_certificate,


Why are we ignoring lifecycle changes on this property? it might want to be computed if its being updated externally now

Because the steps are

CreateOrUpdateVnetConnection

result of step 1 contains cert infomation, we should set the cert to virtual network gateway (check duplicate)

generate vpn package uri

CreateOrUpdateVnetConnectionGateway using step 3's result

a gateway required virtual network integration needs these four steps. In this process, we need update certificate of the virtual network gateway.
I think there is no means to split this process.

I have added computed: true in the newest codes for virtual network gateway

website/docs/r/app_service_virtual_network_connection_gateway.html.markdown

katbyte · 2020-03-03T21:29:40Z

website/docs/r/app_service_virtual_network_connection_gateway.html.markdown

+* `routes` - (Array) One or more `route` block defined below.
+
+---
+
+A `route` block supports the following:
+* `name` - Resource Name.
+* `route_type` - The type of route this is: DEFAULT - By default, every app has routes to the local address ranges specified by RFC1918 INHERITED - Routes inherited from the real Virtual Network routes STATIC - Static route set on the app only. Valid values are `DEFAULT`, `INHERITED`, `STATIC`
+* `start_address` The starting address for the route.
+* `end_address` - The ending address for the route.


these are exported so could we word them as such?

website/docs/r/app_service_virtual_network_connection_gateway.html.markdown

…itual Network Connection

njuCZ · 2020-03-10T14:59:18Z

@katbyte sorry for late reply. I have refined my codes.
The logic is a little complex for this function, you can mainly refer to this script
https://gallery.technet.microsoft.com/scriptcenter/Connect-an-app-in-Azure-ab7527e3

Thanks again for your patience reviewing this PR!

jackofallops · 2020-03-25T16:34:12Z

Hi @njuCZ
I've been working through this PR to see how we can get it progressed.

It feels like things are confused (and confusing) as there should be two resources (with data sources) here, azurerm_app_service_network_connection and azurerm_app_service_network_connection_gateway?

I think a redesign into 2 will simplify things, give better focus to each part, and help us get this through. WDYT?

tombuildsstuff · 2020-06-05T15:34:39Z

Based on this comment there's some design questions which need to be addressed here - rather than leaving this open whilst that's happening I'm going to close this PR for the moment, but once those have been resolved we can circle back around and take another look here

ghost · 2020-07-06T14:50:37Z

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!

ghost added size/XXL documentation labels Sep 30, 2019

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch from 87bfcc6 to 5259d19 Compare September 30, 2019 08:09

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch from 2a91ccb to aac30f1 Compare October 8, 2019 03:03

This was referenced Oct 8, 2019

Web Apps - Virtual Network Connection #4332

Closed

Web Apps - Virtual Network Connection Gateway #4333

Closed

tombuildsstuff assigned katbyte Oct 31, 2019

katbyte requested changes Nov 1, 2019

View reviewed changes

katbyte added the waiting-response label Nov 1, 2019

katbyte added this to the v1.37.0 milestone Nov 1, 2019

ghost removed the waiting-response label Nov 6, 2019

njuCZ requested a review from katbyte November 6, 2019 06:28

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch from f0adece to cbcb1bb Compare November 8, 2019 08:06

katbyte changed the title ~~Add new resource and data source for Gateway Required App Service Connection~~ New Resource & Data Source: azurerm_app_service_virtual_network_connection Nov 15, 2019

katbyte requested changes Nov 15, 2019

View reviewed changes

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch from b0b358d to 36c8020 Compare November 18, 2019 06:32

tombuildsstuff modified the milestones: v1.37.0, v1.38.0 Nov 21, 2019

tombuildsstuff modified the milestones: v1.38.0, v1.39.0 Dec 3, 2019

katbyte requested changes Dec 11, 2019

View reviewed changes

katbyte added the service/app-service label Dec 11, 2019

tombuildsstuff removed this from the v1.39.0 milestone Dec 12, 2019

katbyte added the waiting-response label Dec 12, 2019

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch from 7c66d8f to ce14e3f Compare December 17, 2019 08:54

ghost removed the size/XXL label Dec 17, 2019

ghost removed the waiting-response label Dec 20, 2019

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch 2 times, most recently from ff6f8c9 to 4c97eac Compare December 23, 2019 10:58

njuCZ changed the title ~~New Resource & Data Source: azurerm_app_service_virtual_network_connection~~ New Resource & Data Source: azurerm_app_service_virtual_network_connection_gateway Dec 23, 2019

katbyte requested changes Mar 3, 2020

View reviewed changes

katbyte added the waiting-response label Mar 3, 2020

njuCZ and others added 5 commits March 10, 2020 16:22

Add new resource and data source for Gateway Required App Service Vir…

eab3760

…itual Network Connection

rebase and update

4dbc1ba

update

3f77b6d

update test case

4079cbc

update

6c12bca

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch from e44f494 to 2f5a478 Compare March 10, 2020 14:34

update

65716b0

njuCZ force-pushed the app-service-gateway-required-virtual-network-connection branch from 2f5a478 to 65716b0 Compare March 10, 2020 14:52

ghost added size/XL and removed size/XXL labels Mar 10, 2020

ghost removed the waiting-response label Mar 10, 2020

update

12703aa

katbyte assigned jackofallops Mar 24, 2020

fix ci error

9dccde4

ghost added size/XXL and removed size/XL labels Mar 26, 2020

jackofallops added the waiting-response label Mar 27, 2020

tombuildsstuff closed this Jun 5, 2020

ghost locked and limited conversation to collaborators Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource & Data Source: `azurerm_app_service_virtual_network_connection_gateway` #4458

New Resource & Data Source: `azurerm_app_service_virtual_network_connection_gateway` #4458

njuCZ commented Sep 30, 2019 •

edited

Loading

katbyte left a comment

njuCZ commented Nov 6, 2019

njuCZ commented Nov 6, 2019

katbyte left a comment

katbyte commented Nov 15, 2019

njuCZ commented Nov 15, 2019

katbyte left a comment

njuCZ commented Dec 20, 2019

katbyte left a comment

katbyte Dec 26, 2019

njuCZ Mar 10, 2020

katbyte Mar 3, 2020

njuCZ Mar 10, 2020

katbyte Mar 3, 2020

njuCZ commented Mar 10, 2020

jackofallops commented Mar 25, 2020

tombuildsstuff commented Jun 5, 2020

ghost commented Jul 6, 2020

New Resource & Data Source: azurerm_app_service_virtual_network_connection_gateway #4458

New Resource & Data Source: azurerm_app_service_virtual_network_connection_gateway #4458

Conversation

njuCZ commented Sep 30, 2019 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

njuCZ commented Nov 6, 2019

njuCZ commented Nov 6, 2019

katbyte left a comment

Choose a reason for hiding this comment

katbyte commented Nov 15, 2019

njuCZ commented Nov 15, 2019

katbyte left a comment

Choose a reason for hiding this comment

njuCZ commented Dec 20, 2019

katbyte left a comment

Choose a reason for hiding this comment

katbyte Dec 26, 2019

Choose a reason for hiding this comment

njuCZ Mar 10, 2020

Choose a reason for hiding this comment

katbyte Mar 3, 2020

Choose a reason for hiding this comment

njuCZ Mar 10, 2020

Choose a reason for hiding this comment

katbyte Mar 3, 2020

Choose a reason for hiding this comment

njuCZ commented Mar 10, 2020

jackofallops commented Mar 25, 2020

tombuildsstuff commented Jun 5, 2020

ghost commented Jul 6, 2020

New Resource & Data Source: `azurerm_app_service_virtual_network_connection_gateway` #4458

New Resource & Data Source: `azurerm_app_service_virtual_network_connection_gateway` #4458

njuCZ commented Sep 30, 2019 •

edited

Loading