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

Configure and deploy an Azure App Service #74

Merged
merged 22 commits into from
May 1, 2019
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1560daf
Add App Service to Azure using Terraform Templates
manojvazirani Apr 16, 2019
915db60
Add App Service under App Service Plan module
manojvazirani Apr 17, 2019
21f6622
Merge branch 'mavazira/features/appsvc' of github.com:Microsoft/entre…
manojvazirani Apr 17, 2019
c2ae75c
Remove additional files for app-service created
manojvazirani Apr 17, 2019
75349a7
Add public Ip and load balancer to app service
manojvazirani Apr 17, 2019
8f06b15
Merge branch 'master' of github.com:Microsoft/entref-appservice-conta…
manojvazirani Apr 22, 2019
9956e8f
Add svcplan changes and optional settings to appsvc
manojvazirani Apr 22, 2019
abbf4cb
Integrate review comments related to naming of resources
manojvazirani Apr 23, 2019
9ae78d9
Resolve merge issues with master
manojvazirani Apr 23, 2019
f0b3190
Remove Optional and Required from variables.tf
manojvazirani Apr 24, 2019
e023a40
Update variables.tf
manojvazirani Apr 24, 2019
0f58958
App service acts as load balancer
manojvazirani Apr 24, 2019
d5eee4e
Merge branch 'mavazira/features/appsvc' of github.com:Microsoft/entre…
manojvazirani Apr 24, 2019
5731e96
Add App Service as an independent module
manojvazirani Apr 24, 2019
caad189
Merge branch 'master' of github.com:Microsoft/entref-appservice-conta…
manojvazirani Apr 24, 2019
8c5160d
Fix Merge conflicts
manojvazirani Apr 24, 2019
d9a5c17
Add multiple App Service Deployment Changes
manojvazirani Apr 25, 2019
a990bea
Add type for every variable in variables.tf
manojvazirani Apr 26, 2019
cf12dea
Merge branch 'master' of github.com:Microsoft/entref-appservice-conta…
manojvazirani Apr 29, 2019
4c4348b
Update to documentation for usage
manojvazirani Apr 29, 2019
2b8c7df
Integrate review comments for Docker credentials
manojvazirani Apr 30, 2019
d2215f7
Create app service map with image tags
manojvazirani Apr 30, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions infra/modules/providers/azure/app-service/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
## Azure App Service

Build, deploy, and scale enterprise-grade web, mobile, and API apps running on any platform. Meet rigorous performance, scalability, security and compliance requirements while using a fully managed platform to perform infrastructure maintenance.

More information for Azure App Services can be found [here](https://azure.microsoft.com/en-us/services/app-service/)

Copy link
Contributor

Choose a reason for hiding this comment

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

General note, please update the usage section of the readme for the service plan module so that cobalt users have more context on how to define and configure there module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me open a new PR with the documentation changes for service plan.

Cobalt gives ability to specify following settings for App Service based on the requirements:
- name : The name of the App Services to be created.
- service_plan_resource_group_name : The Name of the Resource Group where the Service Plan exists.
- app_service_plan_id : The ID of the App Service Plan within which the App Service exists is populated automatically based on service plan.
- tags : A mapping of tags to assign to the resource.
- app_settings : A key-value pair of App Settings. Settings for private Container Registries
- DOCKER_REGISTRY_SERVER_URL : The docker registry server URL for app service to be created
- DOCKER_REGISTRY_SERVER_USERNAME : The docker registry server usernames for app services to be created
- DOCKER_REGISTRY_SERVER_PASSWORD : The docker registry server passwords for app services to be created
- site_config :
- linux_fx_version : Linux App Frameworks and versions for the App Services created. Possible options are a Docker container (DOCKER|<user/image:tag>), a base-64 encoded Docker Compose file (COMPOSE|${base64encode(file("compose.yml"))}) or a base-64 encoded Kubernetes Manifest (KUBE|${base64encode(file("kubernetes.yml"))}).
- always_on : Should the app be loaded at all times? Defaults to false.

Please click the [link](https://www.terraform.io/docs/providers/azurerm/d/app_service.html) to get additional details on settings in Terraform for Azure App Service.

## Usage
erikschlegel marked this conversation as resolved.
Show resolved Hide resolved

```
data "azurerm_resource_group" "appsvc" {
name = "${var.service_plan_resource_group_name}"
}

data "azurerm_app_service_plan" "appsvc" {
name = "${var.service_plan_name}"
resource_group_name = "${data.azurerm_resource_group.appsvc.name}"
}

resource "azurerm_app_service" "appsvc" {
Copy link
Contributor

@erikschlegel erikschlegel Apr 25, 2019

Choose a reason for hiding this comment

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

The documented usage should be aligned with how we're using these modules in our own templates. That being said, we should reference our own modules as terraform modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The module is being defined here. And the module usage is defined below.

Copy link
Contributor

@erikschlegel erikschlegel Apr 26, 2019

Choose a reason for hiding this comment

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

the goal of these modules is for developers to avoid explicitly defining resource like this resource "azurerm_app_service" "appsvc"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the actual usage is after the definition. A couple more lines down, the example usage specifies how it has to be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

my comment was not addressed. The documentation seems a bit confusing as this section represents how a developer would use the app_service module. Can you please remove the module definition from the readme and reference the reader to the location of the definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an usage documentation example for how the load balancer module is defined https://github.com/Azure/terraform-azurerm-loadbalancer

name = "${var.app_service_name[count.index]}"
resource_group_name = "${data.azurerm_resource_group.appsvc.name}"
location = "${data.azurerm_resource_group.appsvc.location}"
app_service_plan_id = "${data.azurerm_app_service_plan.appsvc.id}"
tags = "${var.resource_tags}"
count = "${length(var.app_service_name)}"

app_settings {
DOCKER_REGISTRY_SERVER_URL = "${var.docker_registry_server_url[count.index]}"
DOCKER_REGISTRY_SERVER_USERNAME = "${var.docker_registry_server_username[count.index]}"
DOCKER_REGISTRY_SERVER_PASSWORD = "${var.docker_registry_server_password[count.index]}"
}

site_config {
linux_fx_version = "${var.site_config_linux_fx_version[count.index]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these variables defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Variables are defined in variables.tf file

Copy link
Contributor

Choose a reason for hiding this comment

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

both the app service name and container exec command should be driven off the same variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an ideal situation yes but that seems too rigid to push developers to give same names. Many containers will have additional versions that might not be part of the app service name.

Copy link
Contributor

@erikschlegel erikschlegel Apr 26, 2019

Choose a reason for hiding this comment

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

ok.... if we go with multiple variables, how are you accounting for situations where there's an array length mismatch between site_config_linux_fx_version and app_service_name variables?

always_on = "${var.site_config_always_on[count.index]}"
}
}

Example Usage:

module "app_service" {
service_plan_resource_group_name = "${azurerm_resource_group.cluster_rg.name}"
app_service_name = ["app_service_name1", "app_service_name2"]
}
```
28 changes: 28 additions & 0 deletions infra/modules/providers/azure/app-service/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
data "azurerm_resource_group" "appsvc" {
name = "${var.service_plan_resource_group_name}"
}

data "azurerm_app_service_plan" "appsvc" {
name = "${var.service_plan_name}"
resource_group_name = "${data.azurerm_resource_group.appsvc.name}"
}

resource "azurerm_app_service" "appsvc" {
name = "${var.app_service_name[count.index]}"
resource_group_name = "${data.azurerm_resource_group.appsvc.name}"
location = "${data.azurerm_resource_group.appsvc.location}"
app_service_plan_id = "${data.azurerm_app_service_plan.appsvc.id}"
tags = "${var.resource_tags}"
count = "${length(var.app_service_name)}"

app_settings {
DOCKER_REGISTRY_SERVER_URL = "${var.docker_registry_server_url[count.index]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels rigid as in most cases developers will be using the same docker registry. In the instance of deploying multiple app services, one would have to redefine the docker credentials each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be rigid, if we don't give developers the option. That is the reason each app-service has these settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

my point is that if you're using the same registry across 5 app services then someone is going to have to define the docker registry 5 times(all containing the same values)... Also, this seems like an unnecessary step for images hosted publicly on docker hub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but a developer now has the option of adding it for an app service. If not they are optional and can be kept empty. Presuming that the docker registry is going to be same is against best practices which kind of takes away the option of flexibility for the developer.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this was marked as resolved as this approach would force me to provide my docker credentials for public images hosted on docker hub.

Copy link
Contributor

Choose a reason for hiding this comment

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

supporting multiple docker hosts is out of scope for this user story as the technical design assumes the customer is using a single docker registry(either local or public)

DOCKER_REGISTRY_SERVER_USERNAME = "${var.docker_registry_server_username[count.index]}"
DOCKER_REGISTRY_SERVER_PASSWORD = "${var.docker_registry_server_password[count.index]}"
}

site_config {
linux_fx_version = "${var.site_config_linux_fx_version[count.index]}"
always_on = "${var.site_config_always_on[count.index]}"
manojvazirani marked this conversation as resolved.
Show resolved Hide resolved
}
}
5 changes: 5 additions & 0 deletions infra/modules/providers/azure/app-service/output.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
output "app_service_uri" {
description = "The URL of the app service created"
value = "${azurerm_app_service.appsvc.*.default_site_hostname}"
}

46 changes: 46 additions & 0 deletions infra/modules/providers/azure/app-service/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
variable "service_plan_resource_group_name" {
description = "The name of the resource group in which the service plan was created."
type = "string"
}


variable "service_plan_name" {
description = "The name of the service plan"
erikschlegel marked this conversation as resolved.
Show resolved Hide resolved
type = "string"
}

variable "resource_tags" {
description = "Map of tags to apply to taggable resources in this module. By default the taggable resources are tagged with the name defined above and this map is merged in"
type = "map"
default = {}
}

variable "app_service_name" {
description = "The name of the app service to be created"
default = ["appsvc11", "appsvc12"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn’t have a default value.

}

variable "docker_registry_server_url" {
description = "The docker registry server URL for app service to be created"
default = ["", ""]
}

variable "docker_registry_server_username" {
description = "The docker registry server username for app service to be created"
default = ["", ""]
Copy link
Contributor

Choose a reason for hiding this comment

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

can you provide more clarity on the rationale for defaulting this to an enpty array of 2 elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as discussed in the meeting

}

variable "docker_registry_server_password" {
description = "The docker registry server password for app service to be created"
default = ["", ""]
}

variable "site_config_linux_fx_version" {
description = "The Linux App Framework and version for the App Service to be created"
manojvazirani marked this conversation as resolved.
Show resolved Hide resolved
default = ["", ""]
}

variable "site_config_always_on" {
description = "Should the app be loaded at all times? Defaults to false."
default = ["", ""]
}
2 changes: 1 addition & 1 deletion infra/modules/providers/azure/provider/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,4 @@ resource "null_resource" "example" {
command = "execute shell script"
}
}
```
```
2 changes: 1 addition & 1 deletion infra/modules/providers/azure/provider/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ provider "null" {

provider "azuread" {
version = "~>0.1"
}
}
13 changes: 7 additions & 6 deletions infra/modules/providers/azure/service-plan/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ variable "resource_group_name" {

variable "resource_group_location" {
description = "Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created. The full list of Azure regions can be found at https://azure.microsoft.com/regions"
type = "string"
}

variable "resource_tags" {
Expand All @@ -19,18 +20,18 @@ variable "service_plan_name" {
}

variable "service_plan_tier" {
description = "The tier under which the service plan is created. Details can be found at https://docs.microsoft.com/en-us/azure/app-service/overview-hosting-plans"
default = "Isolated"
description = "The tier under which the service plan is created. Details can be found at https://docs.microsoft.com/en-us/azure/app-service/overview-hosting-plans"
manojvazirani marked this conversation as resolved.
Show resolved Hide resolved
default = "Isolated"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not default to isolated as some developers will not need to run this on a private vnet. If a developer needs to run this in a vnet then we should provide a template to automate that and add this default in the relevant TF template(s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what we had discussed in the meeting on call with @code4clouds. I did not have a default value before but we agreed to add this default for Service plans inside vnet based on the Game plan document.

Copy link
Contributor

@erikschlegel erikschlegel Apr 26, 2019

Choose a reason for hiding this comment

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

we discussed adding this default in the template variables.tf that stands up a vnet, not the module. Most users of this module will not need to run this in isolated mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a design decision that should be approved by @James-Nance @day-jeff . If we are building an Enterprise level App Reference then Isolated should be mandatory as this is the most secure way to protect the information flowing between the services. Enterprise App Reference in my head = PII, information that shouldn't be tampered or exposed outside of a VNET.

Copy link
Contributor

@erikschlegel erikschlegel Apr 27, 2019

Choose a reason for hiding this comment

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

This isn’t a design decision as vnets are out of scope for this particular user story🙂. Generally speaking, we still need to do more research to double check whether isolated app service is required for private vnets / app services. This particular module will be used by developers for app service plans which don’t necessarily require a private vnet. Isolated app services may make sense in certain scenarios but are expensive Instance. This decision is specific to the deployment environment(template) as this module will be used for azure customer / developer(s) interested in deploying app services.

}

variable "service_plan_size" {
description = "The compute and storage needed for the service plan to be deployed. Details can be found at https://azure.microsoft.com/en-us/pricing/details/app-service/windows/"
default = "S1"
description = "The compute and storage needed for the service plan to be deployed. Details can be found at https://azure.microsoft.com/en-us/pricing/details/app-service/windows/"
default = "S1"
}

variable "service_plan_kind" {
description = "The kind of Service Plan to be created. Possible values are Windows/Linux/FunctionApp/App"
default = "Linux"
description = "The kind of Service Plan to be created. Possible values are Windows/Linux/FunctionApp/App"
erikschlegel marked this conversation as resolved.
Show resolved Hide resolved
default = "Linux"
}

variable "service_plan_capacity" {
Expand Down