CompositionalIT / farmer

Repeatable Azure deployments with ARM templates - made easy!
https://compositionalit.github.io/farmer
MIT License
526 stars 159 forks source link

Create a WebApp in an existing ServicePlan #285

Closed TheRSP closed 4 years ago

TheRSP commented 4 years ago

Hi I've been playing around with trying to create a webApp in an existing ASP. The ASP will be shared and configured manually so I don't want the ARM template I generate to modify it or try to deploy it in any way.

Is there a good way to do this? If not, have there been discussions thoughts on how this might be possible and would it be something you would be interested in including? (I may, depending on work schedule, and subject to an acceptable design be able to contribute if so)

I have been playing around and the following script produces something very close to what I want.

let webAppName :string = ...
let aspName :string = ...

let sharedServicePlan = 
    sprintf "[concat(resourceGroup().id,'/providers/Microsoft.Web/serverfarms/%s')]" aspName
    |> ResourceName
    |> External

let webApp = webApp {
    name "codat-my-web-app"
    service_plan_name sharedServicePlan
    app_insights_off
}

let deployment = arm{
    location Location.UKSouth
    add_resource webApp
}

However, this outputs an ARM template with an unmet dependency which causes

InvalidTemplate - Long running operation failed with status 'Failed'. Additional Info:'Deployment template validation
failed: 'The resource 'Microsoft.Web/serverfarms/...' is not defined in the template. 

If, however, I remove the single entry in the dependsOn array, it does exactly what I want.

      "dependsOn": [
-        "[concat(resourceGroup().id,'/providers/Microsoft.Web/serverfarms/...')]" 
      ],

Obviously having to include an ARM expression is sub par and it would be good to make this something similar to the following

let sharedServicePlan : ResourceName = ExternalResource.ServicePlan ResourceGroup.Current aspName
isaacabraham commented 4 years ago

@rjwpope This is good feedback. Up until now we've treated the Farmer template as a kind of "owner" of the entire topology rather than the ability to "inject" new resources into an existing set of resources.

There is actually a resourceId function to build resources in a slightly nicer way than sprintf etc., but something smells wrong with that error, so needs to be looked at.

If the service plan lives in the same resource group as the web app you're deploying, you can actually ignore the fully qualified name and simply provide the service plan name itself. Could you try this whilst we look into the issue itself?

TheRSP commented 4 years ago

Thank you for your response. Unfortunately in my case, the app service plan lives in separate resource group and is used for apps from different resource groups. (I used resourceGroup().id to simplify the example).

Was I correct in thinking that ResourceRef.External was intended for this purpose or is that for resources which are "external" in some other way?

Trying the same example above but using only the simple name results in the same error

let sharedServicePlan = 
    aspName
    |> ResourceName
    |> External

let webApp = webApp {
    name "my-web-app"
    service_plan_name sharedServicePlan
    app_insights_off
}

let deployment = arm{
    location Location.UKSouth
    add_resource webApp
}

deployment
|> Writer.quickWrite "MyWebApp"
> New-AzResourceGroupDeployment -ResourceGroupName ... -TemplateFile .\MyWebApp.json -WhatIf
New-AzResourceGroupDeployment :
InvalidTemplate - Long running operation failed with status 'Failed'. Additional Info:'Deployment template validation
failed: 'The template reference 'codat-dev1-asp' is not valid: could not find template resource or resource copy with
this name.

I note that in the WebApp builder, it unconditionally outputs the service plan name in the dependencies list which I think is the cause of this.

isaacabraham commented 4 years ago

Yes, External was meant exactly for this purpose - a resource defined outside of the builder. There shouldn't be an issue with having the dependency on the service plan if it lives in another resource group, as far as I'm aware - we need that dependency in case the service plan is still being defined within Farmer but external to the builder.

Using a simple name will fail unless it is being defined within the template - apologies for the incorrect statement above!

I've just noticed you're using service_plan_name - this still creates a service plan, rather than linking to an existing one. You should use the link_to_service_plan keyword instead - this will stop creating a new service plan and simply reference the existing one.

We still need to check on what format the name is, though.

TheRSP commented 4 years ago

Thanks again. Unfortunately using link_to_service_plan results in the same error. Because the ASP doesn't exist in the generated ARM template, I think this will fail as long as the ASP name is in the dependsOn array. I was wondering if a new ResourceRef.PreExisting union case or similar might solve this? This would be treated the same as External except in the Dependencies list where it would be omitted.

isaacabraham commented 4 years ago

Mmmm. Looks like you're right - you can't define dependencies on resources that are defined outside of a template:

Resources that must be deployed before this resource is deployed. Resource Manager evaluates the dependencies between resources and deploys them in the correct order. When resources aren't dependent on each other, they're deployed in parallel. The value can be a comma-separated list of a resource names or resource unique identifiers. Only list resources that are deployed in this template. Resources that aren't defined in this template must already exist. Avoid adding unnecessary dependencies as they can slow your deployment and create circular dependencies.

This suggests that we would need to add OOTB support for linking resources that truly exist outside of the Farmer template. This is something that is a cross-cutting concern i.e. not just about app service but SQL and several other resources.

For now there are a few options to work around this:

  1. Make a "raw" Site yourself (Farmer.Arm.Web.Site) which maps 1:1 with the App Service. However, there is no fancy builder syntax, you create a raw record which in the case of a Site has a lot of properties.
  2. You can run the Builder that you've made yourself to get to the automatically created Site and then modify it. This is diving a bit deeper into the guts of Farmer but does work (just tried myself):
open Farmer
open Farmer.Builders
open Farmer.CoreTypes

// Create a reference to the server farm in a more-or-less type-safe manner
let serverFarm = ArmExpression.resourceId(Arm.Web.serverFarms, ResourceName "server-farm-name", "resource-group-name").Eval()

// Make my web app
let webApp = webApp {
    name "isaac-web-app"
    link_to_service_plan serverFarm
    app_insights_off
}

// Create a simple Builder function that, given a location, creates a set of ARM resources
let webAppResource : Builder =
    fun location -> [
        // Manually "run" the builder to get all the raw ARM resources.
        let resources = (webApp :> IBuilder).BuildResources location
        // We know that the first resource is the site
        let rawWebsite = resources.[0] :?> Arm.Web.Site
        // Remove all dependencies from it and return back out
        { rawWebsite with Dependencies = [] }
    ]

// Add the raw, cleaned app to the arm template
let deployment = arm {
    add_resource webAppResource
}

This should work but as you can see, it's pushing to the limits the Farmer API - using an unsafe cast, manually modifying dependencies etc.

  1. Generate the ARM template and manually "fix it" before deploying (undesirable).

Proper solutions

We have a few options here:

  1. Add a fourth type of resource e.g. ExternalTemplate or something - indicating that the resource is not just defined outside the builder but outside Farmer as well. This would tell Farmer to never include it in the dependencies list. This complicates the type system and api as we would need to expose this to the user to indicate this. Perhaps we could / should revisit that whole type anyway as I think the abstraction as it currently exists doesn't fit to be honest.

  2. Come up with a way of manually "dropping" dependencies (or perhaps a more generalised "customisation" option) that we could put into the Farmer pipeline.

TheRSP commented 4 years ago

Thanks for this. I may be able to use your workaround. As a third proper solution, when generating the arm template could Farmer remove any dependencies which are not in the template being generated? This should always be safe as not doing so would lead to an invalid template. The risk I see with this is that if a resource is declared but is accidentally not included in the deployment it could lead to a confusing situation. This might be a bigger-than-desirable change though as it would require two passes over the resources, one to collate the names and a second pass to generate the ARM with/without dependencies.

isaacabraham commented 4 years ago

That's a great idea - will have a think about that!

isaacabraham commented 4 years ago

@rjwpope As you thought, it's a bit more work - it'd involve adding new methods to the IArmResource interface which I really want to avoid. I think the best thing to do would be to say that if you pick "External" as any resource, Farmer will never add it as a dependency - it's up to you to do that as you're effectively taking over control of the provisioning; if you want Farmer to set up the dependency, either you need to do it yourself or let Farmer control the provisioning.

TheRSP commented 4 years ago

That sounds like a good solution to me. Am I right in thinking that the depends_on keyword could be used to achieve that?

isaacabraham commented 4 years ago

Exactly. So for a "truly" external dependency, you wouldn't call it, but for an service plan that you've created inside farmer "separately", you'd have to do it. What we could do is optimise for both paths based on overloads:

link_to_service_plan "my-service-plan" <- optimise for first case, don't add a dependency link_to_service_plan myServicePlan <- optimise for second case, add the dependency as this is linked to a service plan builder / arm resource.

TheRSP commented 4 years ago

That sounds brilliant!

isaacabraham commented 4 years ago

Hey. I've spent a bit of time playing around with alternatives. I think I've settled on this code:

Existing Code - Links to "farmer managed" resources

 // dependencies created with links
let wa = webApp {
    name "test"
    link_to_app_insights "myAi"
    link_to_service_plan "mySp" }

New behaviour - Links to "unmanaged" resources

 // no dependencies created - just links
let wa = webApp {
    name "test"
    link_to_unmanaged_app_insights "myAi"
    link_to_unmanaged_service_plan "mySp" }

This is an additional set of keywords rather than slotting into the existing ones. I tried that way (as I had suggested previously) but I think it would have been too easy to get it "wrong". This way it's much more explicit, and from a coding point of view it's easier as well.

What do you think?

TheRSP commented 4 years ago

This is definitely the clearest option and likely the easiest to document as well. It certainly meets my requirement nicely. 👍

isaacabraham commented 4 years ago

@rjwpope Great. I have a pull request ready to go and have applied the same changes across other "linkable" resources. In the end I indeed redesigned the types so we effectively have four cases:

TheRSP commented 4 years ago

@isaacabraham, Thanks for getting this sorted so quickly. I was wondering when you were thinking of releasing this? if you're holding off for a bigger release in future, I can work from source but if you're going to release this soon I'd rather wait for the release.

Thanks.

isaacabraham commented 4 years ago

@TheRSP Apologies, just saw this. I'll push out a release today.