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

fix scope of name availability check in ASE hosted app services #7157

Merged
merged 1 commit into from Jun 2, 2020

Conversation

jackofallops
Copy link
Member

fixes #3003

@jackofallops jackofallops added this to the v2.13.0 milestone Jun 1, 2020
@jackofallops jackofallops requested a review from a team June 1, 2020 12:43
@ghost ghost added the size/M label Jun 1, 2020
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.

LGTM 👍

@jackofallops jackofallops merged commit 84094f0 into master Jun 2, 2020
@jackofallops jackofallops deleted the b/ase-app-name-availability branch June 2, 2020 06:40
jackofallops added a commit that referenced this pull request Jun 2, 2020
@ghost
Copy link

ghost commented Jun 4, 2020

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

@exmnewellwf
Copy link

exmnewellwf commented Jun 17, 2020

First off thanks to everyone working on this project and forgive my lack of Github etiquette if this is in the wrong place. This change does not resolve the issue for my deployments and appears to have an error in it. Looking at the code in this PR it seems that it never actually does anything with the ASE, only the App Service Plan (ASP) including using the ASP name where the ASE Name is expected to build the FQDN being passed to the availability checker, which of course then fails every time.

Here is the code in question, "aspID" (App Service Plan ID?) is being created with the details of the ASP and then aspID.Name is being used in place of <ASE name> in building of the FQDN string.

https://github.com/terraform-providers/terraform-provider-azurerm/blob/1354593cc47356da33dc0c5f1fd50bdff33996ae/azurerm/internal/services/web/resource_arm_app_service.go#L222-L240

I have verified this is what is occurring by reviewing the trace logs from my most recent attempt to deploy:

#Item# = Redacted

2020-06-16T23:59:30.505Z [DEBUG] plugin.terraform-provider-azurerm_v2.13.0_x5: [DEBUG] AzureRM Request:
2020-06-16T23:59:30.505Z [DEBUG] plugin.terraform-provider-azurerm_v2.13.0_x5: POST /subscriptions/#MySubscriptionId#/providers/Microsoft.Web/checknameavailability?api-version=2019-08-01 HTTP/1.1
...
2020-06-16T23:59:30.506Z [DEBUG] plugin.terraform-provider-azurerm_v2.13.0_x5: {"name":"#MyAppName#.#MyAppServicePlanName#.appserviceenvironment.net","type":"Microsoft.Web/sites","isFqdn":true}
2020-06-16T23:59:30.557Z [DEBUG] plugin.terraform-provider-azurerm_v2.13.0_x5: [DEBUG] AzureRM Response for https://management.azure.com/subscriptions/#MySubscriptionId#/providers/Microsoft.Web/checknameavailability?api-version=2019-08-01:
2020-06-16T23:59:30.557Z [DEBUG] plugin.terraform-provider-azurerm_v2.13.0_x5: HTTP/2.0 200 OK
...
2020-06-16T23:59:30.557Z [DEBUG] plugin.terraform-provider-azurerm_v2.13.0_x5: {"nameAvailable":false,"reason":"Invalid","message":"Hostname must be less than 64 chars and must follow allowed character guidelines at http://tools.ietf.org/html/rfc952."}

FYI - I remain unable to deploy an App Service into an ILB ASE V2 due to this availability check failing.

Also,
Forgot to add that the test used to validate this fix is not correct. The test creates the ASE via this block:

https://github.com/terraform-providers/terraform-provider-azurerm/blob/1354593cc47356da33dc0c5f1fd50bdff33996ae/azurerm/internal/services/web/tests/resource_arm_app_service_test.go#L4425-L4428

This winds up creating an external ASE and not an ILB based ASE and thus would have a FQDN of .p.azurewebsites.net. AFAIK at the moment you cannot create a "correct" ILB ASE V2 natively in terraform due to issue #5905 and must use an ARM template.

@jackofallops
Copy link
Member Author

Hi @exmnewellwf
The debug log above appears to be checking the correct scope for an ASP in an ASE (see the call using appserviceenvironment.net from which you've redacted the hostname and ASE name)? The failure appears to be in the hostname itself:

2020-06-16T23:59:30.557Z [DEBUG] plugin.terraform-provider-azurerm_v2.13.0_x5: {"nameAvailable":false,"reason":"Invalid","message":"Hostname must be less than 64 chars and must follow allowed character guidelines at http://tools.ietf.org/html/rfc952."}

which is returned by the name check API. Can you check your hostname is compatible with the RFC linked in the error message? (though this should be caught by the schema validation). If there's still a problem, could you open a new issue with a much information as you can provide and we'll look into it?

Thanks!

@exmnewellwf
Copy link

exmnewellwf commented Jun 17, 2020

@jackofallops,

I was able to cobble together a powershell script that calls Microsoft.Web/checknameavailability and did some testing and can now clearly see the issue. Only RFC valid characters for App Service Names and ASE Names are allowed, but App Service plans do not have that restriction and thus using it to build the FQDN can result in a failure. Consider the following:

App Name: MyApp
ASE Name: MyASE
AppSvcPlan Name: My_AppSvc_Plan

This code in the PR will assemble the name to be sent to checknameavailability using the name of the App Service Plan:

availabilityRequest.Name = utils.String(fmt.Sprintf("%s.%s.appserviceenvironment.net", name, aspID.Name))

Based on the example names above the code would assemble the following name:

MyApp.My_AppSvc_Plan.appserviceenvironment.net

This name fails the check because the App Service Plan name includes underscore chars, which is the case for my App Service Plan names.

The code should be assembling the following FQDN to pass to the checker:

MyApp.MyASE.appserviceenvironment.net

As the App and ASE names are limited to the RFC chars this would pass the check.

@exmnewellwf
Copy link

@jackofallops,
To verify the fault, I destroyed and recreated my App Service Plans with dashes instead of underscores and checknameavailability passed. The App Services were then all created without issue!!!!

@ghost
Copy link

ghost commented Jul 2, 2020

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!

@hashicorp hashicorp locked and limited conversation to collaborators Jul 2, 2020
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.

AppService name needs to be globally unique even in internal app service environments
3 participants