azurerm_api_management_api - support for `subscription_required` #4885

jackbatzner · 2019-11-14T22:22:53Z

Add subscription_required to data and resource for azurerm_api_management_api

(fixes ##3863 )

mbfrahry

Hey @Brunhil, thanks for opening this PR. There are a couple misspellings which cause the provider to panic which might cause us to lose users statefiles. Once those are fixed up, can you add tests to confirm this functionality works. Those tests are useful to catch these panics before review time as well.

azurerm/resource_arm_api_management_api.go

website/docs/d/api_management_api.html.markdown

jackbatzner · 2019-11-15T14:16:05Z

Good catch with the spelling @mbfrahry , it was one of those days yesterday -_-. I'll get the tests added and other changes fixed and push them up.

azurerm/resource_arm_api_management_api_test.go

mbfrahry

Hey @Brunhil, unfortunately we're still not quite there. One test panicked and the other 2 modified in this PR failed.

azurerm/resource_arm_api_management_api.go

mbfrahry · 2019-11-15T18:16:39Z

azurerm/resource_arm_api_management_api_test.go

@@ -28,6 +28,8 @@ func TestAccAzureRMApiManagementApi_basic(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "soap_pass_through", "false"),
 					resource.TestCheckResourceAttr(resourceName, "is_current", "true"),
 					resource.TestCheckResourceAttr(resourceName, "is_online", "false"),
+					resource.TestCheckResourceAttr(resourceName, "authentication_settings.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "authentication_settings.0.subscription_key_required", "true"),


This test now errors. It doesn't look like this is being set properly

------- Stdout: ------- === RUN TestAccAzureRMApiManagementApi_basic --- FAIL: TestAccAzureRMApiManagementApi_basic (2612.17s) testing.go:569: Step 0 error: Check failed: Check 6/6 error: azurerm_api_management_api.test: Attribute 'authentication_settings.0.subscription_key_required' not found FAIL

mbfrahry · 2019-11-15T18:17:07Z

azurerm/resource_arm_api_management_api_test.go

@@ -120,6 +122,33 @@ func TestAccAzureRMApiManagementApi_version(t *testing.T) {
 	})
 }

+func TestAccAzureRMApiManagementApi_authenticationSettings(t *testing.T) {


This test is also failing

=== RUN TestAccAzureRMApiManagementApi_authenticationSettings --- FAIL: TestAccAzureRMApiManagementApi_authenticationSettings (2825.43s) testing.go:569: Step 0 error: Check failed: Check 3/3 error: azurerm_api_management_api.test: Attribute 'authentication_settings.0.subscription_key_required' not found FAIL

tombuildsstuff

hey @Brunhil

Thanks for pushing those changes - running the tests again it looks like the AuthenticationSettings block may not be being returned from the API here (which is why this shows as missing):

Out of interest are you able to confirm if this is present in the response for a provisioned APIM API? It's generally easiest to see this in the resource response via resources.azure.com (or by proxing Terraform via Charles/Fiddler depending on your OS)

Thanks!

tombuildsstuff · 2019-11-17T14:59:41Z

azurerm/resource_arm_api_management_api.go

+			"authentication_settings": {
+				Type:     schema.TypeList,
+				Optional: true,
+				Computed: true,


Running the tests for this - both the basic and authenticationRequired tests fail:

=== RUN TestAccAzureRMApiManagementApi_authenticationSettings --- FAIL: TestAccAzureRMApiManagementApi_authenticationSettings (2264.83s) testing.go:569: Step 0 error: Check failed: Check 3/3 error: azurerm_api_management_api.test: Attribute 'authentication_settings.0.subscription_key_required' not found FAIL

and

------- Stdout: ------- === RUN TestAccAzureRMApiManagementApi_basic --- FAIL: TestAccAzureRMApiManagementApi_basic (2276.86s) testing.go:569: Step 0 error: Check failed: Check 6/6 error: azurerm_api_management_api.test: Attribute 'authentication_settings.0.subscription_key_required' not found FAIL

is it possible this information isn't returned from the API? If so, we may need to update the flatten function to set a default value here: https://github.com/terraform-providers/terraform-provider-azurerm/pull/4885/files#diff-e28c81f44cf2e8523cae5b19081cf837R494

e.g. something like this:

func flattenApiManagementAuthenticationSettings(input *apimanagement.AuthenticationSettingsContract) []interface{} { subscriptionKeyRequired := false if input != nil && input.SubscriptionKeyRequired != nil { subscriptionKeyRequired = *paramNames.SubscriptionKeyRequired } return []interface{}{ map[string]interface{}{ "subscription_key_required": subscriptionKeyRequired, }, } }

So this actually is a misunderstanding on my part...I'll explain more in the message on this PR.

jackbatzner · 2019-11-20T18:01:46Z

@tombuildsstuff , after digging further into this, it turns out we have to wait for the new API Management API Version.

The 'AuthenticationSettings' field is a 2019-01-01 API Version property. So these changes are actually for 2019-01-01. I was implementing this when looking at the REST API here. I had modified the try-it option to point at 2018-01-1 and assumed the documentation on the left updated, it did not :).

After testing/verifying in 2018-01-01, the response body for GET is as follows:

{
  "id": "...",
  "type": "Microsoft.ApiManagement/service/apis",
  "name": "test-api-name",
  "properties": {
    "displayName": "test-api-name",
    "apiRevision": "1",
    "description": "",
    "subscriptionRequired": false,

Then attempting to set the value in the body for subscriptionRequired in the REST API led to,

{
  "error": {
    "code": "ValidationError",
    "message": "SubscriptionRequired property is not supported for versions before 20180601.",
    "details": null
  }
}

As a result, I'm marking this PR as WIP. We would need to update to 2019-01-01 API version to support this field, and this is blocked by the upstream issues that #2613 is blocked by.

tombuildsstuff · 2019-12-18T20:55:42Z

Assigning this to the "blocked" milestone since this is blocked on upgrading to 2019-01-01, which is blocked on Azure/azure-rest-api-specs#6372

katbyte · 2020-03-24T22:45:36Z

Looks like this should be close, from the blocking issue

just yesterday we got signed all required PRs and now preparing the nuget package. Should be ready in nearest days.
``

usamaahmadkhan · 2020-04-06T12:49:09Z

@Brunhil can we have an estimated date when this will get released? Blocked as we want to consume this resource from terraform in our module

markti · 2020-04-23T16:08:58Z

what about linking to an authorization_server (oauth / openidc) ??? is this included in this PR?

katbyte

hey @Brunhil, thanks for the PR! however it looks like some tests are failing:

est Failed

------- Stdout: -------
=== RUN   TestAccAzureRMApiManagementApi_basic
    TestAccAzureRMApiManagementApi_basic: testing.go:640: Step 0 error: Check failed: Check 5/5 error: azurerm_api_management_api.test: Attribute 'subscription_required' expected "true", got "false"
--- FAIL: TestAccAzureRMApiManagementApi_basic (2574.11s)

are you sure the default is true for the property?

jackbatzner · 2020-05-15T14:20:42Z

@katbyte , when this was initially implemented Subscription Required was turned on by default. They may have changed their behavior in the new API version.

b-twis · 2020-06-04T15:50:42Z

@Brunhil Do you have any update on when this might be ready?

danderbury · 2020-06-16T10:08:53Z

We're also blocked with this current issue. Do you have an ETA?

- Fixes hashicorp#4884

jackbatzner · 2020-06-16T12:56:35Z

Sorry all, forgot to hit push! The test should be fixed if it defaults to false. Let me know if there's any other requested changes. Thanks for reviewing!

3mard · 2020-06-23T12:28:38Z

Thanks you all for this, can't wait for this to get merged

katbyte

thanks @Brunhil! LGTM now 🚀

ghost · 2020-06-25T17:39:38Z

This has been released in version 2.16.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.16.0"
}
# ... other configuration ...

…hicorp#4885) Add subscription_required to data and resource for azurerm_api_management_api (fixes hashicorp#3863 )

ghost · 2020-07-25T14: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!

ghost added size/M documentation labels Nov 14, 2019

mbfrahry self-assigned this Nov 14, 2019

mbfrahry requested changes Nov 14, 2019

View reviewed changes

azurerm/resource_arm_api_management_api.go Outdated Show resolved Hide resolved

azurerm/resource_arm_api_management_api.go Outdated Show resolved Hide resolved

website/docs/d/api_management_api.html.markdown Outdated Show resolved Hide resolved

jackbatzner commented Nov 15, 2019

View reviewed changes

azurerm/resource_arm_api_management_api_test.go Outdated Show resolved Hide resolved

jackbatzner requested a review from mbfrahry November 15, 2019 17:01

mbfrahry requested changes Nov 15, 2019

View reviewed changes

tombuildsstuff added service/api-management enhancement labels Nov 17, 2019

tombuildsstuff reviewed Nov 17, 2019

View reviewed changes

tombuildsstuff added the waiting-response label Nov 17, 2019

katbyte changed the title ~~Apim authentication settings~~ azurerm_api_management_api - support for authentication settings Nov 17, 2019

jackbatzner changed the title ~~azurerm_api_management_api - support for authentication settings~~ [WIP] azurerm_api_management_api - support for authentication settings Nov 20, 2019

ghost removed the waiting-response label Nov 20, 2019

This was referenced Nov 20, 2019

azurerm_api_management_api lacks ability to set "Subscription required" flag to false #3863

Closed

Unable to disable subscription for azure.apimanagement.Api pulumi/pulumi-azure#381

Closed

mbfrahry removed their assignment Dec 9, 2019

tombuildsstuff added this to the Blocked milestone Dec 18, 2019

tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Dec 18, 2019

jackbatzner mentioned this pull request Mar 13, 2020

Add ability to turn off subscription key in API Management API #4884

Closed

jackbatzner closed this Mar 14, 2020

jackbatzner deleted the apim_authentication_settings branch March 14, 2020 14:48

jackbatzner restored the apim_authentication_settings branch March 14, 2020 14:48

jackbatzner reopened this Mar 14, 2020

ghost added size/S and removed size/M labels Apr 24, 2020

jackbatzner changed the title ~~[WIP] azurerm_api_management_api - support for subscription_required~~ azurerm_api_management_api - support for subscription_required Apr 24, 2020

katbyte requested changes May 7, 2020

View reviewed changes

katbyte added the waiting-response label May 7, 2020

ghost removed the waiting-response label May 15, 2020

Jack Batzner and others added 6 commits June 16, 2020 07:52

Add ability to turn off subscription key in API Management API

84a245a

- Fixes hashicorp#4884

Fix verbiage

08a39f1

Add new field to resource documentation

4edecc1

Remove 'authentication_settings' block

4e5a855

Fix test formatting

d89dd04

subscription_required defaults to false?

b968fff

jackbatzner force-pushed the apim_authentication_settings branch from 1c2bdb8 to b968fff Compare June 16, 2020 12:55

jackbatzner requested review from katbyte and mbfrahry June 16, 2020 12:55

katbyte approved these changes Jun 24, 2020

View reviewed changes

katbyte added this to the v2.16.0 milestone Jun 24, 2020

katbyte merged commit 3511639 into hashicorp:master Jun 24, 2020

katbyte added a commit that referenced this pull request Jun 24, 2020

update CHANGELOG.md for #4885

a20ceeb

jrauschenbusch pushed a commit to jrauschenbusch/terraform-provider-azurerm that referenced this pull request Jun 29, 2020

update CHANGELOG.md for hashicorp#4885

50c9b95

ghost locked and limited conversation to collaborators Jul 25, 2020

jackbatzner deleted the apim_authentication_settings branch March 2, 2021 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_api_management_api - support for `subscription_required` #4885

azurerm_api_management_api - support for `subscription_required` #4885

jackbatzner commented Nov 14, 2019 •

edited by katbyte

Loading

mbfrahry left a comment

jackbatzner commented Nov 15, 2019

mbfrahry left a comment

mbfrahry Nov 15, 2019

mbfrahry Nov 15, 2019

tombuildsstuff left a comment

tombuildsstuff Nov 17, 2019

jackbatzner Nov 20, 2019

jackbatzner commented Nov 20, 2019 •

edited

Loading

tombuildsstuff commented Dec 18, 2019

katbyte commented Mar 24, 2020 •

edited

Loading

usamaahmadkhan commented Apr 6, 2020

markti commented Apr 23, 2020

katbyte left a comment

jackbatzner commented May 15, 2020

b-twis commented Jun 4, 2020

danderbury commented Jun 16, 2020

jackbatzner commented Jun 16, 2020

3mard commented Jun 23, 2020

katbyte left a comment

ghost commented Jun 25, 2020

ghost commented Jul 25, 2020

azurerm_api_management_api - support for subscription_required #4885

azurerm_api_management_api - support for subscription_required #4885

Conversation

jackbatzner commented Nov 14, 2019 • edited by katbyte Loading

mbfrahry left a comment

Choose a reason for hiding this comment

jackbatzner commented Nov 15, 2019

mbfrahry left a comment

Choose a reason for hiding this comment

mbfrahry Nov 15, 2019

Choose a reason for hiding this comment

mbfrahry Nov 15, 2019

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Nov 17, 2019

Choose a reason for hiding this comment

jackbatzner Nov 20, 2019

Choose a reason for hiding this comment

jackbatzner commented Nov 20, 2019 • edited Loading

tombuildsstuff commented Dec 18, 2019

katbyte commented Mar 24, 2020 • edited Loading

usamaahmadkhan commented Apr 6, 2020

markti commented Apr 23, 2020

katbyte left a comment

Choose a reason for hiding this comment

jackbatzner commented May 15, 2020

b-twis commented Jun 4, 2020

danderbury commented Jun 16, 2020

jackbatzner commented Jun 16, 2020

3mard commented Jun 23, 2020

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Jun 25, 2020

ghost commented Jul 25, 2020

azurerm_api_management_api - support for `subscription_required` #4885

azurerm_api_management_api - support for `subscription_required` #4885

jackbatzner commented Nov 14, 2019 •

edited by katbyte

Loading

jackbatzner commented Nov 20, 2019 •

edited

Loading

katbyte commented Mar 24, 2020 •

edited

Loading