application gateway: add advanced ssl_policies

[bs-matil]

This PR should fix the Issue #1536
Feature Request: Full sslPolicy support for azurerm_application_gateway

It adds support for advanced ssl_polcies. and deprecates the old options

[jstewart612]

The English version if that helps https://docs.microsoft.com/en-us/azure/application-gateway/application-gateway-ssl-policy-overview

[bs-matil]

@jstewart612 thanks. How could that happen :P.

[jstewart612]

It is a genuine mystery to one and all! ;)

In all seriousness, I don't think this provider's documentation is localized to any other languages, and thus, I think most are, perhaps unfairly, expecting off-world links to go to sites in the same language.

It's the most minor of minor points and really anyone who knows enough how to use this ought know how to go find the English version.... but yeah.

[bs-matil]

@jstewart612 anyways fixed it :) 👍

[tombuildsstuff]

the Azure docs auto redirect to the users locale if you leave off the locale prefix, so we can make this: https://docs.microsoft.com/azure/application-gateway/application-gateway-ssl-policy-overview :)

[bs-matil]

unfortunately a tests of mine is currently broken after rebase. I'm on it. please anyways consider to check the code for its general functionality

[bs-matil]

I found the issue. Its caused due to the fact that disabled_protocols now is deprecated but still exists as option. I resolved that by creating a new ssl_policy block in parallel.
The problem here is that this does not work as both states need to be out of sync by definition. I handled this now as it has been done for fqdn and backend_ip addresses. There both blocks are marked as computed so inconsistencies are basically ignored. Actually I don't like this either. Is there a better known solution to such issues?

@jstewart612 may you had any ideas while designing your implementation?

[tombuildsstuff]

hey @bs-matil

Thanks for this PR :)

I've taken a look through and left some comments inline but this otherwise looks pretty good - if we can fix up the comments and the tests pass we should be able to get this merged 👍

Thanks!

[bs-matil]

@tombuildsstuff I now added another test wich also unfortunately showed an issue that we need also to mark the ssl_policy block with computed otherwise using disabled_ssl_proctocols would cause a changed resource in the ssl_policy block even if the inner disabled_proctocols block is marked as computed.

Are you okay with handling it like that?

[bs-matil]

Please do NOT merge right now there is something wrong in the handling with Computed inputs.
Can we have a chat @tombuildsstuff ? I found the same issue in the fqdn implementation.

f not here the textual version:
The deprecation currently is done by the "hack" to set deprecated and successor block to Computed:true. This enables the user to set only one of them while the inconsistent state of the other is ignored. That so far was expected AFAIK.

But unfortunately, there is another side effect of this.

Example fqdns vs fqdn_list in backend_address_pool:
What happens there is that everything works just fine for the new block fqdns because the block is handled with priority. What I mean by that is that every input for fqdn_list is dropped when fqdn is set.
So good so far. Now one would expect that this does not matter because if fqdns is not set the fqdn_list will work anyways. This is not the case because of the Computed:true flag the input of fqdns is always set to the current state if a state has been written once. The result is that if one writes fqdn_list once he will never be able to change any written value again with the same block because it always overridden by the existing state and the input is fully ignored till fqdns is set.

Back to our topic.

How should I proceed here? I can do the very same handling which means ignoring anything but the initial input of disable_ssl_protcols? This will allow creating a resource once with the desired setting but users will never be able to change this particular state.

Alternatively, we could "drop" disable_protocols in the ssl_policy block till 2.0 and active the new path just then.

Thanks for any help

[bs-matil]

I now implemented "plan a" which is as the fqdns handling.

[bs-matil]

@katbyte rebased against latest merges. Can be reviewed

[katbyte]

Thanks for the updates @bs-matil,

This is looking good! just a simple test failure that needs correcting now:

------- Stdout: -------
=== RUN   TestAccAzureRMApplicationGateway_disabledSslProtocols
=== PAUSE TestAccAzureRMApplicationGateway_disabledSslProtocols
=== CONT  TestAccAzureRMApplicationGateway_disabledSslProtocols
--- FAIL: TestAccAzureRMApplicationGateway_disabledSslProtocols (96.80s)
    testing.go:568: Step 0 error: errors during apply:
        
        Error: Error Creating/Updating Application Gateway "acctestag-190510215451350048" (Resource Group "acctestRG-190510215451350048"): network.ApplicationGatewaysClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ApplicationGatewayInvalidPublicIpSku" Message="Application gateway /subscriptions/8708baf2-0a54-4bb4-905b-78d21ac150da/resourceGroups/acctestRG-190510215451350048/providers/Microsoft.Network/applicationGateways/acctestag-190510215451350048 with SKU Standard_v2 can only reference public ip with Standard SKU." Details=[]
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test115327035/main.tf line 45:
          (source code not available)
        
        
FAIL

[bs-matil]

@katbyte test has been fixed.. Sorry for the inconvenience.

[katbyte]

Thanks for the changes, LGTM now @bs-matil

[ghost]

This has been released in version 1.29.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 = "~> 1.29.0"
}
# ... other configuration ...

[ghost]

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!