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

application gateway: add advanced ssl_policies #3360

Conversation

bs-matil
Copy link
Contributor

@bs-matil bs-matil commented May 2, 2019

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

@bs-matil bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch 4 times, most recently from f29caee to 8331d9a Compare May 2, 2019 14:42
For `policy_type`=`Predefined`:

* `policy_name` - (Optional) The Name of the Policy e.g AppGwSslPolicy20170401S. Required if `policy_type` is set to `Predefined`. Possible values can chang over time and
are published here https://docs.microsoft.com/de-de/azure/application-gateway/application-gateway-ssl-policy-overview. Not compatible with `disabled_protocols`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jstewart612 thanks. How could that happen :P.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jstewart612 anyways fixed it :) 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch from 8331d9a to f504358 Compare May 3, 2019 05:07
@bs-matil
Copy link
Contributor Author

bs-matil commented May 3, 2019

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
Copy link
Contributor Author

bs-matil commented May 3, 2019

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?

@bs-matil bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch 2 times, most recently from 4f9051b to 1da0216 Compare May 3, 2019 11:58
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway_test.go Outdated Show resolved Hide resolved
website/docs/r/application_gateway.html.markdown Outdated Show resolved Hide resolved
website/docs/r/application_gateway.html.markdown Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
@bs-matil bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch 6 times, most recently from 80b3073 to a740044 Compare May 6, 2019 12:12
@bs-matil
Copy link
Contributor Author

bs-matil commented May 6, 2019

@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?

@ghost ghost removed the waiting-response label May 6, 2019
@katbyte katbyte added this to the v1.28.0 milestone May 6, 2019
@tombuildsstuff tombuildsstuff self-assigned this May 7, 2019
@bs-matil
Copy link
Contributor Author

bs-matil commented May 7, 2019

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
Copy link
Contributor Author

bs-matil commented May 7, 2019

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

@bs-matil bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch from 491bb2f to b6906bd Compare May 7, 2019 07:41
@ghost ghost removed the waiting-response label May 8, 2019
@bs-matil bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch 6 times, most recently from a2a0347 to 93dd859 Compare May 10, 2019 20:15
@bs-matil bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch 2 times, most recently from 53737c5 to 017a494 Compare May 10, 2019 20:31
@bs-matil bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch from 017a494 to 88a98dd Compare May 10, 2019 20:43
@bs-matil
Copy link
Contributor Author

@katbyte rebased against latest merges. Can be reviewed

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 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 bs-matil force-pushed the application-gateway-add-advanced-ssl-policies-with-deprecation branch from c4b1140 to f0a839a Compare May 12, 2019 20:23
@bs-matil
Copy link
Contributor Author

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

@ghost ghost removed the waiting-response label May 13, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.28.0, v1.29.0 May 17, 2019
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 for the changes, LGTM now @bs-matil

katbyte added a commit that referenced this pull request May 17, 2019
@katbyte katbyte merged commit 0f47674 into hashicorp:master May 17, 2019
@ghost
Copy link

ghost commented May 26, 2019

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
Copy link

ghost commented Jun 18, 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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Jun 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants