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

Bug Fixes/Enhancements to Application Gateway #1576

Closed
tombuildsstuff opened this issue Jul 15, 2018 · 89 comments
Closed

Bug Fixes/Enhancements to Application Gateway #1576

tombuildsstuff opened this issue Jul 15, 2018 · 89 comments

Comments

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Jul 15, 2018

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Affected Resource(s)

  • azurerm_application_gateway

The azurerm_application_gateway resource is currently missing a selection of fields and also has some bugs which need resolving. Unfortunately there's a bug in the Application Gateway API where the Application Gateway isn't actually deleted which prevents us from proceeding this work, since our tests for these resources are failing around 80% of the time due to the Application Gateway not being deleted (meaning this fills up our quota's).

Rather than trying to track the status of this bug across multiple issues - I'm opening this meta-issue to keep track of these bugs and enhancements in one place. Once the bug in the API is resolved - it should be possible to add these enhancements/investigate fixing these bugs; however these are blocked at the moment.

Blocking API issues

Enhancements

Bug Fixes

@TechyMatt

This comment has been minimized.

@tombuildsstuff

This comment has been minimized.

@guitarrapc
Copy link

@thomastaylor312 can I request the support for key vault certificate integration for https pfx?
It would be great if I can use with vault id reference like azurerm_virtual_machine resource.

https://www.terraform.io/docs/providers/azurerm/r/virtual_machine.html#source_vault_id

@cdhunt

This comment has been minimized.

@thomastaylor312

This comment has been minimized.

@guitarrapc

This comment has been minimized.

@TechyMatt
Copy link
Contributor

@tombuildsstuff is this something actively being worked on now that the MS issue has been resolved? Aware that you're wanting to split up into multiple resources, I don't want to start doing PRs for any of the functionality I (selfishly) need if it's going to trip up over other people.

@tombuildsstuff tombuildsstuff removed the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Aug 14, 2018
@tombuildsstuff

This comment has been minimized.

@TechyMatt
Copy link
Contributor

TechyMatt commented Aug 14, 2018

@tombuildsstuff the two bits of functionality i'm wanting to implement are:

Tags - looks like this is now working with the recent MS fix so just a documentation update (@tombuildsstuff fixed in #2054).
Documentation - the example actually has multiple different configurations all lumped in the same (with some syntax errors) so will tidy this up. (@tombuildsstuff fixed in #2054)
SSL Ciphers - linking to #1536, #451 & #898 (@tombuildsstuff this will be tracked in #1536)

Certainly not planning on undertaking the full split out of the provider - lets keep walking before I can run 😄.

@Supermathie

This comment has been minimized.

@tombuildsstuff

This comment has been minimized.

@steve-88

This comment has been minimized.

@steve-88

This comment has been minimized.

@jstewart612
Copy link

@alert101 #1536

@Phydeauxman
Copy link

@tombuildsstuff I was attempting to use the new redirection capability this morning and got blocked almost immediately and am confused on why it was implemented the way it was. We run App Gateways using Path Based rules so that we can cover an entire solution with a single cert. We are implementing auto renewing LetsEncrypt certs that require the need to access a storage account that is reached via a redirect configuration on the App Gateway rule. The way the redirect feature is implemented in the provider, we can't do both but we can easily configure the coexistence of the 2 via the portal without breaking anything. So, confused as to why it this is not possible via the provider?

@RustyF
Copy link
Contributor

RustyF commented Apr 9, 2019

@Phydeauxman as I've just added the redirect functionality, I'm keen to help with this. However, I don't understand the coexistence you're talking about. You should be able to have path-based redirects but I guess this isn't what you mean? If you can elaborate in any way, share configs or portal screenshots, that would help. Or is this coexistence with another Gateway feature?

@Phydeauxman
Copy link

@RustyF there is a lot of confusion when it comes to the different pieces of an App Gateway. On a Request Routing Rule, there are path based maps where you can map traffic patterns to a specific backendpool/httpsetting and then there is redirection where you can send traffic to another listener, or an external URL. I am able to configure a request routing rule via the portal to have both but the code you implemented in the provider does not support this. The provider code is either one or the other.

image

@rem-aj
Copy link

rem-aj commented Apr 10, 2019

@Phydeauxman do you get an error or what happens when you try to use both those features in the code? The update documention lists 2 optional sections for both redirect rules and path based routing.
https://www.terraform.io/docs/providers/azurerm/r/application_gateway.html

@Phydeauxman
Copy link

@bostonmoto the error tells you that you can use them together...it is one or the other...which the documentation states. My question was why was it implemented this way when thru the portal, I can configure both to co-exist?

@RustyF
Copy link
Contributor

RustyF commented Apr 14, 2019

Let's assume the validation is too restrictive and I will aim (this week is the plan) to revise it. I still need to understand what the two conflicting configs are as I did experience some mutual exclusions when I was testing it. Those "Default.." settings definitely have some conflict with specific rules I'm just not sure which scenario is OK yet.

@RustyF
Copy link
Contributor

RustyF commented Apr 15, 2019

Hi @Phydeauxman - are you able to send me the fragment of redirect config you've tried to add? I will turn it into a failing test that I can then work against. Feel free to anonymise it, of course 😉

@Phydeauxman
Copy link

@RustyF below is the configuration block in my App Gateway that will throw a validation error but can be effected via the portal:

# Path-based routing rule for UI
request_routing_rule {
name                        = "${var.projectPrefix}-${var.les_naming_suffix}-rule"
rule_type                   = "PathBasedRouting"
http_listener_name          = "${var.projectPrefix}-${var.les_naming_suffix}-listener"
backend_address_pool_name   = "${data.terraform_remote_state.apps.lesapp_name}-beap"
backend_http_settings_name  = "${data.terraform_remote_state.apps.lesapp_name}"
redirect_configuration_name = "LetsEncryptChallenge"
url_path_map_name           = "${var.projectPrefix}-${var.les_naming_suffix}"
}

url_path_map {
  name = "${var.projectPrefix}-${var.les_naming_suffix}"
  default_backend_address_pool_name  = "${data.terraform_remote_state.apps.lesapp_name}-beap"
  default_backend_http_settings_name = "${data.terraform_remote_state.apps.lesapp_name}"

path_rule {
  name                       = "${data.terraform_remote_state.apps.lesapiapp_name}"
  paths                      = ["/api*"]
  backend_address_pool_name  = "${data.terraform_remote_state.apps.lesapiapp_name}-beap"
  backend_http_settings_name = "${data.terraform_remote_state.apps.lesapiapp_name}"
  }
}

This is from the documentation:
backend_http_settings_name - (Optional) The Name of the Backend HTTP Settings Collection to use for this Path Rule. Cannot be set if redirect_configuration_name is set.

redirect_configuration_name - (Optional) The Name of a Redirect Configuration to use for this Path Rule. Cannot be set if backend_address_pool_name or backend_http_settings_name is set.

@RustyF
Copy link
Contributor

RustyF commented Apr 15, 2019

Thanks for the snippet @Phydeauxman . Have you tried removing backend_address_pool_name and backend_http_settings_name from the main request_routing_rule config? I think those are not required (and hence the issue). I believe this is the purpose of the default... entries on url_path_map.

(I would do it myself but I'm at work at the moment!)

@Phydeauxman
Copy link

@RustyF I have not tried that but will.

@rem-aj
Copy link

rem-aj commented Apr 16, 2019

Thanks @Phydeauxman look forward to hearing your updates.

@Phydeauxman
Copy link

@RustyF @bostonmoto been doing some testing on this today and getting some different results than what I originally got. First off, below is an image of the redirection configuration I was trying to effect:

image

The Terraform implementation of the redirect_configuration definition does not provide a way to define the Path field.

When I start from scratch with no existing App Gateway and I attempt to apply the config below, it gives me no warnings and the apply says it is going to do what I have in the config:

image

image

This apply will fail with the following error:

image

If I remove the include_path property from the redirect_configuration block (property defaults to false), the apply error goes away and the Gateway gets created but it does not create the redirection configuration. My assumption is because a value for the required Path field is not present.

@mcharriere
Copy link
Contributor

@Phydeauxman There is no Path field in the GO SDK.

Could you try something like the following?:

  request_routing_rule {
    name               = "${var.ui_rule_name}-rule"
    rule_type          = "PathBasedRouting"
    http_listener_name = "${var.projectPrefix}-${var.ui_listener_name}-listener"
    url_path_map_name  = "${var.ui_rule_name}"
  }

  redirect_configuration {
    name          = "LetsEncryptChallenge"
    redirect_type = "Permanent"
    target_url    = "https://lechallenge.blob.core.windows.net/public"
  }

  url_path_map {
    name                               = "${var.ui_rule_name}"
    default_backend_address_pool_name  = "${data.terraform_remote_state.apps.uiapp_name}"
    default_backend_http_settings_name = "${data.terraform_remote_state.apps.uiapp_name}"

    path_rule {
      name                        = "letsencrypt"
      paths                       = ["/.well-known/challenge/*"]
      redirect_configuration_name = "LetsEncryptChallenge"
    }
  }

I haven't tested it, but I think that this is the way to achieve what you want.

@Phydeauxman
Copy link

@mcharriere appears that is not going to work either:

image

@Phydeauxman
Copy link

@mcharriere I actually went back thru this again and discovered that even though the apply throws that error...it does create the App Gateway with the rule just as I intended it to be. The by product is though...it throws that error, and you can't change or destroy the App Gateway with Terraform now. You have to manually change it or delete it.

@mcharriere
Copy link
Contributor

@Phydeauxman Yep, I've tested it myself and I've got it working without that validation placed there.
It's also odd that all those validations are inside the flatten function.
I'll test it a bit more and I'll try to send a fix for it.

@RustyF
Copy link
Contributor

RustyF commented Apr 19, 2019

Thanks for both looking at this - I’m not at my computer at the moment but I couldn’t find that path parameter in the Azure SDK docs either so the GUI is obviously mapping it onto something else. It’s quite possible that the validation isn’t quite right; the docs are a bit lacking in explaining the mutual exclusions. Btw, placing validation in the flatten function appears to be the convention but I don’t have any previous experience to validate that 🤔

@Phydeauxman
Copy link

@RustyF no worries...it takes a village. Not the first time the SDK or documentation from MS was lacking...nor will it be the last.

@timja
Copy link
Contributor

timja commented May 16, 2019

Should this be closed? Most if not all of the issues here are fixed, and would probably be better tracked as individual issues anyway?

@tombuildsstuff tombuildsstuff added this to the v1.28.0 milestone May 17, 2019
@mbfrahry
Copy link
Member

Hey all. It looks like we got all but one of the issues in this thread so I'll be closing it down. The only one we missed was #1274 which we weren't able to reproduce. If anyone has more information on that please open a new issue with additional information and steps to reproduce.

@ghost
Copy link

ghost commented May 17, 2019

This has been released in version 1.28.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.28.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

No branches or pull requests