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

Added Virtual Network Integration to the API Management service #2582

Closed
wants to merge 4 commits into from
Closed

Added Virtual Network Integration to the API Management service #2582

wants to merge 4 commits into from

Conversation

joranm
Copy link
Contributor

@joranm joranm commented Dec 29, 2018

  • Added the virtual_network_type property which allows None, Internal or External

  • Added virtual_network_configuration object with property subnet_id which references the subnet to integrate with.

See https://docs.microsoft.com/en-us/azure/api-management/api-management-using-with-vnet for details regarding API Management with Vnet integration

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.

Hi @joranm,

It looks like some of the acceptance tests are failing:
screen shot 2018-12-29 at 13 58 50

@joranm
Copy link
Contributor Author

joranm commented Dec 30, 2018

Hi @katbyte, I've found the issue why the tests were failing. Fixed the issue by adding additional checks if the virtual_network_configuration object is present. Additionally I've found that the Virtual Network Type will always return None from Azure. So I've added this as the default value of the property.

@ghost ghost removed the waiting-response label Dec 30, 2018
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.

Thanks @joranm,

We are still seeing this error:

------- Stdout: -------
=== RUN   TestAccAzureRMApiManagement_complete
=== PAUSE TestAccAzureRMApiManagement_complete
=== CONT  TestAccAzureRMApiManagement_complete
--- FAIL: TestAccAzureRMApiManagement_complete (99.17s)
	testing.go:538: Step 0 error: Error applying: 1 error(s) occurred:
		
		* azurerm_api_management.test: 1 error(s) occurred:
		
		* azurerm_api_management.test: Error creating/updating API Management Service "acctestAM-8760723735339727174" (Resource Group "acctestRG-8760723735339727174"): apimanagement.ServiceClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidParameters" Message="Invalid parameter: Virtual Network configuration not found for location(s) eastus2"
FAIL

Is this only available in select regions at the moment?

@joranm
Copy link
Contributor Author

joranm commented Jan 2, 2019

Hi @katbyte, I've found that if the additional_location object is present, the virtual_network_configuration should also be present to make this work. Otherwise it reverses to the "default" virtual_network_configuration in the first location (which fails). This requires some rework and testing from my side which I will try to perform this week.

@steve-hawkins
Copy link
Contributor

Hi @joranm, this is great

Could I ask that the private_ip_addresses attribute be added as part of this PR? Very useful when virtual_network_type is set to Internal

If not then I can look to add it after

@ghost ghost added size/L and removed size/M labels Jan 4, 2019
@joranm
Copy link
Contributor Author

joranm commented Jan 4, 2019

@tombuildsstuff and @katbyte,

I've

  • added the virtual_network_configuration to the additional locations.
  • added the private_ip_addresses to service properties and additional_locations

@ghost ghost removed the waiting-response label Jan 4, 2019
@katbyte
Copy link
Collaborator

katbyte commented Jan 6, 2019

Thanks for the updates @joranm,

I am still getting an error on our CI server, different this time however 😅

------- Stdout: -------
=== RUN   TestAccAzureRMApiManagement_complete
=== PAUSE TestAccAzureRMApiManagement_complete
=== CONT  TestAccAzureRMApiManagement_complete
--- FAIL: TestAccAzureRMApiManagement_complete (2173.23s)
	testing.go:599: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.
		
		Error: Error applying: 2 error(s) occurred:
		
		* azurerm_subnet.test1 (destroy): 1 error(s) occurred:
		
		* azurerm_subnet.test1: Error deleting Subnet "subnet-1260518037164007063" (VN "acctest-vnet-1260518037164007063" / Resource Group "acctestRG-1260518037164007063"): network.SubnetsClient#Delete: Failure sending request: StatusCode=0 -- Original Error: Code="InUseSubnetCannotBeDeleted" Message="Subnet subnet-1260518037164007063 is in use by /subscriptions/*******/providers/Microsoft.ApiManagement/service?vnetResourceGuid=8c4a597c-2d13-44ef-a935-80ae09872e1e&subnet=subnet-1260518037164007063&api-version=2016-07-07 and cannot be deleted." Details=[]
		* azurerm_subnet.test2 (destroy): 1 error(s) occurred:
		
		* azurerm_subnet.test2: Error deleting Subnet "subnet-1260518037164007063" (VN "acctest-vnet-1260518037164007063" / Resource Group "acctestRG2-1260518037164007063"): network.SubnetsClient#Delete: Failure sending request: StatusCode=0 -- Original Error: Code="InUseSubnetCannotBeDeleted" Message="Subnet subnet-1260518037164007063 is in use by /subscriptions/*******/providers/Microsoft.ApiManagement/service?vnetResourceGuid=d443303e-9acf-4b5b-8704-ba1ce8531379&subnet=subnet-1260518037164007063&api-version=2016-07-07 and cannot be deleted." Details=[]
		
		State: azurerm_resource_group.test1:
		  ID = /subscriptions/*******/resourceGroups/acctestRG-1260518037164007063
		  provider = provider.azurerm
		  location = westeurope
		  name = acctestRG-1260518037164007063
		  tags.% = 0
		azurerm_resource_group.test2:
		  ID = /subscriptions/*******/resourceGroups/acctestRG2-1260518037164007063
		  provider = provider.azurerm
		  location = eastus2
		  name = acctestRG2-1260518037164007063
		  tags.% = 0
		azurerm_subnet.test1:
		  ID = /subscriptions/*******/resourceGroups/acctestRG-1260518037164007063/providers/Microsoft.Network/virtualNetworks/acctest-vnet-1260518037164007063/subnets/subnet-1260518037164007063
		  provider = provider.azurerm
		  address_prefix = 10.0.0.0/24
		  ip_configurations.# = 0
		  name = subnet-1260518037164007063
		  resource_group_name = acctestRG-1260518037164007063
		  route_table_id = 
		  service_endpoints.# = 0
		  virtual_network_name = acctest-vnet-1260518037164007063
		
		  Dependencies:
		    azurerm_resource_group.test1
		    azurerm_virtual_network.test1
		azurerm_subnet.test2:
		  ID = /subscriptions/*******/resourceGroups/acctestRG2-1260518037164007063/providers/Microsoft.Network/virtualNetworks/acctest-vnet-1260518037164007063/subnets/subnet-1260518037164007063
		  provider = provider.azurerm
		  address_prefix = 10.0.0.0/24
		  ip_configurations.# = 0
		  name = subnet-1260518037164007063
		  resource_group_name = acctestRG2-1260518037164007063
		  route_table_id = 
		  service_endpoints.# = 0
		  virtual_network_name = acctest-vnet-1260518037164007063
		
		  Dependencies:
		    azurerm_resource_group.test2
		    azurerm_virtual_network.test2
		azurerm_virtual_network.test1:
		  ID = /subscriptions/*******/resourceGroups/acctestRG-1260518037164007063/providers/Microsoft.Network/virtualNetworks/acctest-vnet-1260518037164007063
		  provider = provider.azurerm
		  address_space.# = 1
		  address_space.0 = 10.0.0.0/16
		  dns_servers.# = 0
		  location = westeurope
		  name = acctest-vnet-1260518037164007063
		  resource_group_name = acctestRG-1260518037164007063
		  subnet.# = 1
		  subnet.2876441382.address_prefix = 10.0.0.0/24
		  subnet.2876441382.id = /subscriptions/*******/resourceGroups/acctestRG-1260518037164007063/providers/Microsoft.Network/virtualNetworks/acctest-vnet-1260518037164007063/subnets/subnet-1260518037164007063
		  subnet.2876441382.name = subnet-1260518037164007063
		  subnet.2876441382.security_group = 
		  tags.% = 0
		
		  Dependencies:
		    azurerm_resource_group.test1
		azurerm_virtual_network.test2:
		  ID = /subscriptions/*******/resourceGroups/acctestRG2-1260518037164007063/providers/Microsoft.Network/virtualNetworks/acctest-vnet-1260518037164007063
		  provider = provider.azurerm
		  address_space.# = 1
		  address_space.0 = 10.0.0.0/16
		  dns_servers.# = 0
		  location = eastus2
		  name = acctest-vnet-1260518037164007063
		  resource_group_name = acctestRG2-1260518037164007063
		  subnet.# = 1
		  subnet.2876441382.address_prefix = 10.0.0.0/24
		  subnet.2876441382.id = /subscriptions/*******/resourceGroups/acctestRG2-1260518037164007063/providers/Microsoft.Network/virtualNetworks/acctest-vnet-1260518037164007063/subnets/subnet-1260518037164007063
		  subnet.2876441382.name = subnet-1260518037164007063
		  subnet.2876441382.security_group = 
		  tags.% = 0
		
		  Dependencies:
		    azurerm_resource_group.test2
FAIL

@joranm
Copy link
Contributor Author

joranm commented Jan 6, 2019

Hi @katbyte, it looks like the Azure platform has some long time lease on the subnet. When the API Management Service is deleted there is still a lock on the subnet for 15-30 minutes. Do you have any idea how to handle this? @steve-hawkins @tombuildsstuff

annotation 2019-01-06 112745

@ghost ghost removed the waiting-response label Jan 6, 2019
@steve-hawkins
Copy link
Contributor

Hi @joranm, at a guess this is probably due to the API Manager still reliant on classic parts, there is a note in the docs when connecting to a VNET:-

image

This is further explained in the FAQ section

@tombuildsstuff I'm pretty sure this has been addressed in other areas of this provider, I just can't remember where?

@tombuildsstuff
Copy link
Member

@joranm taking a look into this I believe we should be able to run the tests for this once the fix for #2945 has been merged, I'm looking into this at the moment but there's nothing needed from your side at the moment :)

@tombuildsstuff tombuildsstuff self-assigned this Feb 25, 2019
@tombuildsstuff tombuildsstuff added this to the Blocked milestone Feb 28, 2019
@tombuildsstuff tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Feb 28, 2019
@tombuildsstuff tombuildsstuff removed their assignment Feb 28, 2019
@tombuildsstuff
Copy link
Member

hey @joranm

Just to give an update here: since we've had to downgrade to API version 2018-01-01 to work around the breaking API change - as such this PR's blocked on support for the API version 2019-01-01 being available in the Azure SDK for Go - which is being tracked in the Rest API Specs ticket mentioned above.

Rather than leaving this PR open stagnant until that's available (which I've just asked for an update in the issue) - I'm going to assign this to the 'blocked' milestone and temporarily close this PR for the moment. When the other issue's closed (and the SDK is available) we'll re-open this PR and get support for this functionality merged.

Thanks!

@keero
Copy link

keero commented May 6, 2019

Seems like the dependent ticket has been resolved. When will this PR be reopened again?

@ghost
Copy link

ghost commented May 7, 2019

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators May 7, 2019
@hashicorp hashicorp unlocked this conversation May 10, 2019
@tombuildsstuff
Copy link
Member

👋

As @keero has mentioned the upstream issue's been fixed: Azure/azure-rest-api-specs#4409 - once the SDK's been updated #3335 we should be able to go to the new version of the API Management API and look to add support for this

Thanks!

@tombuildsstuff tombuildsstuff removed the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label May 10, 2019
@tombuildsstuff tombuildsstuff modified the milestones: Blocked, v1.29.0 May 10, 2019
@katbyte
Copy link
Collaborator

katbyte commented May 10, 2019

#3335 has been merged so this is now unblocked @joranm

@tombuildsstuff tombuildsstuff modified the milestones: v1.29.0, Blocked May 22, 2019
@tombuildsstuff tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label May 22, 2019
@tombuildsstuff
Copy link
Member

hey @joranm

Sorry for the bad news - it looks like the new SDK upgrade (in #3446) is still blocked, this time on: Azure/azure-rest-api-specs#6065

Once that's fixed, we'll circle back and fix this PR up - but for the moment as this is blocked on the external/upstream issue I'm going to temporarily re-close this PR, once that's been fixed we'll re-open this and fix this up / take another look 👍

Thanks!

@ghost
Copy link

ghost commented Jun 22, 2019

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Jun 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation enhancement service/api-management size/L upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants