New Resource: `azurerm_api_management` #1516

torresdal · 2018-07-08T09:17:58Z

Added a new resource for API Management as requested in #1177. Contains the resource, tests and documentation.

ISSUE: It's marked as WIP since there is currently an issue with how the azure-sdk-for-go (or more likely Azure itself) handles deletion of the service when there are associated subnets. Basically the subnets fail to delete because it's still in use by the API Management service, even though it is deleted. I will create a separate issue for this and link back, but any pointers would be appreciated 👍

Because of the issue mentioned above, the integration test TestAccAzureRMApiManagement_complete currently fail.

torresdal · 2018-07-09T15:16:56Z

Regarding the issue (#1520) where subnet used by azurerm_api_management cannot be deleted, when azurerm_api_management is in fact destroyed and should no longer have a dependency to the subnet.

Has this happend with other Azure Terraform resources? It's strange that it takes 30-50 minutes to create the API Mangement resource in Azure, but only seconds to delete it...

torresdal · 2018-07-09T16:00:24Z

As requested by @tombuildsstuff I've registered an issue for the subnet issue at Azure/azure-sdk-for-go#2199

tombuildsstuff · 2018-07-09T16:36:01Z

@torresdal awesome, thanks - I'm taking a look through this at the moment :)

tombuildsstuff · 2018-07-09T18:08:56Z

@torresdal

Regarding the issue (#1520) where subnet used by azurerm_api_management cannot be deleted, when azurerm_api_management is in fact destroyed and should no longer have a dependency to the subnet.
Has this happend with other Azure Terraform resources? It's strange that it takes 30-50 minutes to create the API Mangement resource in Azure, but only seconds to delete it...

We've had this in a few cases, Virtual Network Gateway's and Application Gateway's - in both of these cases the API affects the entire resource and as such we were blocked from shipping them until they were fixed*. However in this instance this is an optional field, as such I think we should be able to ship the rest of the resource and then add the subnet support back in when the API is fixed; what do you think?

[* Unfortunately we didn't realise the scale of the Application Gateway bug until after we'd shipped it - and now we've shipped it so it's hard to pull from the provider (it's causing us a lot of pain internally, since it's breaking our acceptance tests nightly / leaving test resources kicking around which fill our quota's).]

tombuildsstuff

hey @torresdal

Thanks for opening this PR :)

I've taken a look and left some comments inline, but this is off to a good start - whilst I've added a number of comments most of them are pretty similar.

The main thing we need to think about in this case is going to be the Subnet issue you've referenced. Given this is an optional field I think it'd be best to remove this field for the moment and then add it back in once the API's fixed (thanks for raising that as a bug in the SDK repository too!)

Thanks!

tombuildsstuff · 2018-07-09T15:28:34Z

azurerm/data_source_api_management.go

+				Computed: true,
+			},
+
+			"sku": apiManagementDataSourceSkuSchema(),


can we in-line this function?

Sure, but just to make sure I understand your preference, since I see you have not mentioned apiManagementDataSourceCertificateInfoSchema. What I've done is create funcs for schemas that are used multiple locations. Both apiManagementDataSourceSkuSchema and apiManagementDataSourceCertificateInfoSchema is used twice in the overall data source schema.

If this is not a good practice here (or in general 😃 ) I'll inline them.

tbh in this case I think we could probably remove the second SKU in the data source, since we've also removed it from the resource (which is why I originally posted this)

(in general we tend to follow the Go guidelines of duplicating code the first time, and then refactoring it into a method - but it's mostly a gut feeling based on where something's likely to diverge, in this case I'd argue it probably wouldn't since it's the same object in both cases)

tombuildsstuff · 2018-07-09T15:31:34Z

azurerm/data_source_api_management.go

+		d.Set("publisher_name", props.PublisherName)
+
+		d.Set("notification_sender_email", props.NotificationSenderEmail)
+		d.Set("created", props.CreatedAtUtc.Local().Format(time.RFC3339))


I think we can remove .Local() from this?

tombuildsstuff · 2018-07-09T15:32:06Z

azurerm/data_source_api_management.go

+		d.Set("vnet_type", string(props.VirtualNetworkType))
+		d.Set("custom_properties", &props.CustomProperties)
+
+		err, hostnameConfigurations := flattenApiManagementHostnameConfigurations(d, props.HostnameConfigurations)


in general we invert these (e.g. hostnameConfigurations, err := ...) can we flip this to match?

also can we duplicate this method for the Data Source, since they can diverge over time

tombuildsstuff · 2018-07-09T15:32:18Z

azurerm/data_source_api_management.go

+			return fmt.Errorf("Error setting `additional_location`: %+v", err)
+		}
+
+		err, certificates := flattenApiManagementCertificates(d, props.Certificates)


(same here)

also can we duplicate this method for the Data Source, since they can diverge over time

tombuildsstuff · 2018-07-09T15:33:00Z

azurerm/data_source_api_management.go

+			return fmt.Errorf("Error setting `hostname_configuration`: %+v", err)
+		}
+
+		additionalLocations := flattenApiManagementAdditionalLocations(props.AdditionalLocations)


can we duplicate this method for the Data Source, since they can diverge over time

tombuildsstuff · 2018-07-09T16:40:33Z

website/azurerm.erb

+              <a href="#">API Management Resources</a>
+              <ul class="nav nav-visible">
+
+                <li<%= sidebar_current("docs-azurerm-datasource-azurerm_api_management-x") %>>


minor can we change the underscores to dashes here?

tombuildsstuff · 2018-07-09T16:41:15Z

website/docs/d/api_management.html.markdown

+
+* `vnet_subnet_id` - The full resource ID of a subnet in a virtual network where the API Management service is deployed.
+
+* `vnet_type` - The type of VPN in which API Managemet service needs to be configured in. None (Default Value) means the API Management service is not part of any Virtual Network, External means the API Management deployment is set up inside a Virtual Network having an Internet Facing Endpoint, and Internal means that API Management deployment is setup inside a Virtual Network having an Intranet Facing Endpoint only. Possible values include: `VirtualNetworkTypeNone`, `VirtualNetworkTypeExternal`, `VirtualNetworkTypeInternal`.


can we change VirtualNetworkTypeNone -> None, VirtualNetworkTypeExternal -> External and VirtualNetworkTypeInternal -> Internal?

tombuildsstuff · 2018-07-09T16:43:12Z

website/docs/r/api_management.html.markdown

+---
+layout: "azurerm"
+page_title: "Azure Resource Manager: azurerm_api_management"
+sidebar_current: "docs-azurerm-resource-api-management"


can we add the suffix -x to match the sidebar?

tombuildsstuff · 2018-07-09T16:43:49Z

website/docs/r/api_management.html.markdown

+
+Create a Api Management component.
+
+-> **Note:** When `azurerm_api_management` service has dependencies on one or more `azurerm_subnet` resources, referenced using `vnet_subnet_id` or `additional_location.*.vnet_subnet_id` - the `azurerm_subnet`s will fail to be destroyed because of an issue in Azure where a reference to the `azurerm_api_management` service is left behind after its destruction. The `azurerm_api_management` service will eventually be released from `azurerm_subnet`, but it could take hours.


So as mentioned on the main page, we're going to need to remove the ability for API Management Instances to be attached to Virtual Networks until this is fixed by Azure (unfortunately) - as such can we remove this for the moment?

Should we replace this with an explanation for why Virtual Networks is not supported at the moment?

tombuildsstuff · 2018-07-09T16:44:31Z

website/docs/r/api_management.html.markdown

+
+* `vnet_subnet_id` - (Optional) The full resource ID of a subnet in a virtual network where the API Management service is deployed.
+
+* `vnet_type` - (Optional) The type of VPN in which API Managemet service needs to be configured in. None (Default Value) means the API Management service is not part of any Virtual Network, External means the API Management deployment is set up inside a Virtual Network having an Internet Facing Endpoint, and Internal means that API Management deployment is setup inside a Virtual Network having an Intranet Facing Endpoint only. Possible values include: `VirtualNetworkTypeNone`, `VirtualNetworkTypeExternal`, `VirtualNetworkTypeInternal`.


can we update VirtualNetworkTypeNone -> None, VirtualNetworkTypeExternal -> External and VirtualNetworkTypeInternal -> Internal?

torresdal · 2018-07-09T23:22:24Z

Thanks for pointing out lots of useful improvements in your review @tombuildsstuff! I believe everything should now be up to date.

In addition:

I removed everything related to Virtual Networks as you suggested and retested to make sure destroy now works - and it does
Removed sku from additional_location after verifying that is was not needed as you pointed out

I left you a few comments on a couple of your suggestions (inlining schema function and if we should document why we currently don't support Virtual Networks).

torresdal · 2018-07-14T07:12:29Z

Anything else I can do before this get merged @tombuildsstuff?

Also, what is your preference regarding changes in master that needs to be resolved in the pull-request branch - merge or rebase?

tombuildsstuff · 2018-07-30T11:37:08Z

hey @torresdal

Sorry for the delay in re-reviewing this - I'd started reviewing this and forgot to hit submit; let me check if the comments are still valid and re-submit this; apologies for the delay here!

Also, what is your preference regarding changes in master that needs to be resolved in the pull-request branch - merge or rebase?

Rebasing is generally preferred from our side if possible, but we're fairly flexible here :)

Thanks!

tombuildsstuff · 2018-07-30T12:03:20Z

@torresdal actually I've just noticed the vendoring here, so I'm going to rebase this & push a commit to resolve that; hope you don't mind!

tombuildsstuff

hey @torresdal

Thanks for pushing the latest updates - apologies for the delay in re-reviewing this! I've pushed a commit to fix a vendoring issue I noticed and to rebase this on top of master (I hope you don't mind!)

In general this is looking pretty good - there's a few common issues throughout this (mostly to do with not starting at the 0 index for arrays) which I've tried to highlight at each instance - if we can fix up the comments then we should be able to run the tests :)

Thanks!

tombuildsstuff · 2018-07-11T13:01:38Z

azurerm/data_source_api_management.go

+	}
+
+	if sku := resp.Sku; sku != nil {
+		d.Set("sku", flattenDataSourceApiManagementServiceSku(sku))


given this is a complex object, can we check for errors when setting this? e.g.

if err := d.Set("sku", flattenDataSourceApiManagementServiceSku(sku)); err != nil { return fmt.Errorf("Error flattening `sku`: %+v", err) }

tombuildsstuff · 2018-07-11T13:05:34Z

azurerm/data_source_api_management.go

+		}
+
+		certificate["thumbprint"] = *cert.Thumbprint
+		certificate["subject"] = *cert.Subject


there's a potential crash on these two properties, can we nil-check around them?

tombuildsstuff · 2018-07-11T13:09:43Z

azurerm/data_source_api_management.go

+
+	if profile != nil {
+		sku["name"] = string(profile.Name)
+		sku["capacity"] = *profile.Capacity


can we wrap this in a nil-check to work around the crash here?

tombuildsstuff · 2018-07-30T11:39:00Z

azurerm/data_source_api_management.go

+}
+
+func flattenDataSourceApiManagementAdditionalLocations(props *[]apimanagement.AdditionalLocation) []interface{} {
+	additional_locations := make([]interface{}, 0, 1)


can we remove the , 1 here? as-is this'll add a blank item at the start of the locations

tombuildsstuff · 2018-07-30T11:39:37Z

azurerm/data_source_api_management.go

+
+func flattenDataSourceApiManagementCertificate(cert *apimanagement.CertificateInformation) []interface{} {
+	certificate := make(map[string]interface{}, 2)
+	certInfos := make([]interface{}, 0, 1)


can we remove the , 1 here, since this'll add a blank item to the start of the array (technically you're setting the starting point of the array to position 1, which will be the second item in the array)

tombuildsstuff · 2018-07-30T12:06:58Z

azurerm/resource_arm_api_management.go

+		d.Set("management_api_url", props.ManagementAPIURL)
+		d.Set("scm_url", props.ScmURL)
+		d.Set("static_ips", props.StaticIps)
+		d.Set("custom_properties", &props.CustomProperties)


rather than setting these directly (which will result in a set error) - could we add a flatten function for this (as below) and convert this list to a map[string]interface{} rather than setting map[string]string here?

tombuildsstuff · 2018-07-30T12:07:35Z

azurerm/resource_arm_api_management.go

+		certificates := flattenApiManagementCertificates(d, props.Certificates)
+
+		if err != nil {
+			return err


could we wrap this to explain what failed, e.g.

return fmt.Errorf("Error flattening `certificate`: %+v", err)

tombuildsstuff · 2018-07-30T12:08:04Z

azurerm/resource_arm_api_management.go

+		hostnameConfigurations, err := flattenApiManagementHostnameConfigurations(d, props.HostnameConfigurations)
+
+		if err != nil {
+			return err


could we wrap this to explain what failed, e.g.

return fmt.Errorf("Error flattening `hostname_configuration `: %+v", err)

@torresdal,

Could we get this done?

tombuildsstuff · 2018-07-30T12:12:21Z

azurerm/resource_arm_api_management.go

+		d.Set("publisher_name", props.PublisherName)
+
+		d.Set("notification_sender_email", props.NotificationSenderEmail)
+		d.Set("created", props.CreatedAtUtc.Format(time.RFC3339))


can we remove the creation date, I can't think when this would be likely to be used within Terraform?

tombuildsstuff · 2018-07-30T12:12:37Z

azurerm/data_source_api_management.go

+
+			"created": {
+				Type:     schema.TypeString,
+				Computed: true,


can we remove the creation date, I can't think when this would be likely to be used within Terraform?

torresdal · 2018-08-23T22:13:03Z

I believe I've addressed all the requested changes by @tombuildsstuff now (thanks for a thorough review btw!), so hopefully this should be ready to move on.

torresdal · 2018-08-31T07:31:57Z

@jeffreyCline Any chance you could do a final review? Thanks.

katbyte

Hi @torresdal,

Thank you for all the updates and patience with the review process. I have left a fair number of comments inline with the major ones being:

moving the import tests to the standard ones
changing how your passing along the ert and cert password values. I don't think using the index is wise as that can change outside of terraform.
there are multiple tests failing that need to be fixed

I'll do my best to review this asap after these changes have been made.

azurerm/config.go

azurerm/data_source_api_management.go

azurerm/resource_arm_api_management.go

azurerm/resource_arm_api_management_test.go

website/docs/r/api_management.html.markdown

… than proxy

- refactoring to make the hostname configurations & security block parsing clearer - default values for the security block - making name and capacity in the sku block required - moving the validation test to where the validate function is - removing crash points & ensuring we set an empty list where required - registering the `Microsoft.ApiManagement` resource provider

- Moving validation into the validation package - Renaming `hostname_configurations` to `hostname_configuration` to better match the Terraform structure - Ensuring we always set hostname_configurations - Setting default values for the `security` block - Fixing the conflicts_with. rewriting the docs

… issues

tombuildsstuff

hey @torresdal

Thanks for all the work on this PR - I've taken a look through and left a few remaining comments (which I'm going to push a commit to fix) - but this now otherwise LGTM, I'll kick off the tests shortly 👍

Thanks!

azurerm/data_source_api_management.go

azurerm/provider.go

website/docs/r/api_management.html.markdown

dismissing since changes have been pushed / this is now out of date

tombuildsstuff · 2018-10-01T07:34:50Z

Data Source tests pass:

tombuildsstuff · 2018-10-01T09:39:22Z

Resource Tests pass:

kumlien · 2018-10-15T15:45:03Z

Nice work! Is this one already available for us 'normal users'? I dont' see it on the terraform site and my idea tf-plugin doesn't recognize it... (running terraform-provider-azurerm_v1.16.0_x4).

torresdal · 2018-10-16T11:52:04Z

@kumlien Looks like its scheduled to be in version 1.17.0.

https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/CHANGELOG.md

ghost · 2019-03-06T14:22:31Z

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!

torresdal mentioned this pull request Jul 8, 2018

Feature Request: API Management support #1177

Closed

torresdal changed the title ~~[WIP] API Management service~~ [WIP] New Resource: API Management service Jul 8, 2018

torresdal mentioned this pull request Jul 9, 2018

Subnet used by destroyed azurerm_api_management cannot be deleted #1520

Closed

torresdal mentioned this pull request Jul 9, 2018

Subnet previously used by deleted api management service cannot be deleted Azure/azure-sdk-for-go#2199

Closed

tombuildsstuff self-assigned this Jul 9, 2018

tombuildsstuff added new-resource service/api-management labels Jul 9, 2018

tombuildsstuff suggested changes Jul 9, 2018

View reviewed changes

torresdal force-pushed the api-management branch from 1609203 to bc4d7d6 Compare July 10, 2018 22:23

tombuildsstuff force-pushed the api-management branch from eb0a81e to c9c9df2 Compare July 30, 2018 12:04

tombuildsstuff suggested changes Jul 30, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jul 30, 2018

metacpp requested a review from WodansSon August 3, 2018 00:52

ghost added the size/XXL label Aug 23, 2018

torresdal changed the title ~~[WIP] New Resource: API Management service~~ New Resource: API Management service Aug 30, 2018

ghost added the size/XXL label Aug 31, 2018

torresdal force-pushed the api-management branch from 94ab8b6 to 771afcf Compare September 1, 2018 08:55

ghost added the size/XXL label Sep 1, 2018

katbyte requested changes Sep 12, 2018

View reviewed changes

torresdal and others added 17 commits October 1, 2018 19:43

Added support for Managed Service Identity

e8b1b1e

Support ref certs using key_vault_id

524b947

Fixed issue where default ssl binding was set for other hotname types…

2157d98

… than proxy

Updated docs for API Management to reflect recent changes. Code cleanup

e8ebdb3

Cleaned up data source for api-management

93ecec6

Fixed API Management example to reflect recent changes

d9811ea

d/api_management: refactoring

5ea1c27

r/api_management: refactoring to remove the customizeDiff

4b94092

Fixing the sidebar highlight

d893782

Fixing the broken tests

a7f2dfb

Running tests against different regions to get around the concurrency…

05db8ab

… issues

Removed duplicate section spacing

1029ee0

Changed static_ips to public_ip_addresses

29d4360

Fixing an incorrect assignment bug

9e3bbf9

Updating to v21.1.0 of the Azure SDK

c69342b

tombuildsstuff force-pushed the api-management branch from 249404a to c69342b Compare October 1, 2018 06:44

tombuildsstuff approved these changes Oct 1, 2018

View reviewed changes

azurerm/data_source_api_management.go Outdated Show resolved Hide resolved

azurerm/provider.go Outdated Show resolved Hide resolved

website/docs/r/api_management.html.markdown Outdated Show resolved Hide resolved

website/docs/r/api_management.html.markdown Outdated Show resolved Hide resolved

Resolving PR comments

6c35627

Ensuring that different resource groups are provisioned

24d1f14

tombuildsstuff merged commit caca04d into hashicorp:master Oct 1, 2018

tombuildsstuff added a commit that referenced this pull request Oct 1, 2018

Updating to include #1516

8a833a6

ghost locked and limited conversation to collaborators Mar 6, 2019


		* `vnet_subnet_id` - The full resource ID of a subnet in a virtual network where the API Management service is deployed.

		* `vnet_type` - The type of VPN in which API Managemet service needs to be configured in. None (Default Value) means the API Management service is not part of any Virtual Network, External means the API Management deployment is set up inside a Virtual Network having an Internet Facing Endpoint, and Internal means that API Management deployment is setup inside a Virtual Network having an Intranet Facing Endpoint only. Possible values include: `VirtualNetworkTypeNone`, `VirtualNetworkTypeExternal`, `VirtualNetworkTypeInternal`.


		Create a Api Management component.

		-> Note: When `azurerm_api_management` service has dependencies on one or more `azurerm_subnet` resources, referenced using `vnet_subnet_id` or `additional_location.*.vnet_subnet_id` - the `azurerm_subnet`s will fail to be destroyed because of an issue in Azure where a reference to the `azurerm_api_management` service is left behind after its destruction. The `azurerm_api_management` service will eventually be released from `azurerm_subnet`, but it could take hours.


		* `vnet_subnet_id` - (Optional) The full resource ID of a subnet in a virtual network where the API Management service is deployed.

		* `vnet_type` - (Optional) The type of VPN in which API Managemet service needs to be configured in. None (Default Value) means the API Management service is not part of any Virtual Network, External means the API Management deployment is set up inside a Virtual Network having an Internet Facing Endpoint, and Internal means that API Management deployment is setup inside a Virtual Network having an Intranet Facing Endpoint only. Possible values include: `VirtualNetworkTypeNone`, `VirtualNetworkTypeExternal`, `VirtualNetworkTypeInternal`.

New Resource: azurerm_api_management #1516

New Resource: azurerm_api_management #1516

Conversation

torresdal commented Jul 8, 2018 • edited Loading

torresdal commented Jul 9, 2018

torresdal commented Jul 9, 2018

tombuildsstuff commented Jul 9, 2018

tombuildsstuff commented Jul 9, 2018

tombuildsstuff 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

torresdal commented Jul 9, 2018

torresdal commented Jul 14, 2018 • edited Loading

tombuildsstuff commented Jul 30, 2018

tombuildsstuff commented Jul 30, 2018

tombuildsstuff 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

torresdal commented Aug 23, 2018

torresdal commented Aug 31, 2018 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Oct 1, 2018

tombuildsstuff commented Oct 1, 2018

kumlien commented Oct 15, 2018

torresdal commented Oct 16, 2018 • edited Loading

ghost commented Mar 6, 2019

New Resource: `azurerm_api_management` #1516

New Resource: `azurerm_api_management` #1516

torresdal commented Jul 8, 2018 •

edited

Loading

torresdal commented Jul 14, 2018 •

edited

Loading

torresdal commented Aug 31, 2018 •

edited

Loading

torresdal commented Oct 16, 2018 •

edited

Loading