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

Remove ACR Webhooks; Do not manage container settings via Terraform #324

Merged
merged 5 commits into from
Sep 27, 2019
Merged
Changes from all commits
Commits
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
69 changes: 46 additions & 23 deletions infra/modules/providers/azure/app-service/main.tf
Original file line number Diff line number Diff line change
@@ -6,6 +6,16 @@ locals {
acr_webhook_name = "cdhook"
app_names = keys(var.app_service_config)
app_configs = values(var.app_service_config)

app_linux_fx_versions = [
for config in values(var.app_service_config) :
// Without specifyin a `linux_fx_version` the webapp created by the `azurerm_app_service` resource
// will be a non-container webapp.
//
// The value of "DOCKER" is a stand-in value that can be used to force the webapp created to be
// container compatible without explicitly specifying the image that the app should run.
config.image == "" ? "DOCKER" : format("DOCKER|%s/%s", var.docker_registry_server_url, config.image)
nmiodice marked this conversation as resolved.
Show resolved Hide resolved
]
}

data "azurerm_resource_group" "appsvc" {
@@ -32,40 +42,25 @@ resource "azurerm_app_service" "appsvc" {
DOCKER_REGISTRY_SERVER_PASSWORD = var.docker_registry_server_password
APPINSIGHTS_INSTRUMENTATIONKEY = var.app_insights_instrumentation_key
KEYVAULT_URI = var.vault_uri
DOCKER_ENABLE_CI = var.docker_enable_ci
}

site_config {
linux_fx_version = format("DOCKER|%s/%s", var.docker_registry_server_url, local.app_configs[count.index].image)
linux_fx_version = local.app_linux_fx_versions[count.index]
always_on = var.site_config_always_on
virtual_network_name = var.vnet_name
}

identity {
type = "SystemAssigned"
}
}

resource "null_resource" "acr_webhook_creation" {
count = var.docker_enable_ci == true && var.uses_acr ? length(local.app_names) : 0
depends_on = [azurerm_app_service.appsvc]

triggers = {
images_to_deploy = "${join(",", [for config in local.app_configs : config.image])}"
uses_acr = var.uses_acr
}

provisioner "local-exec" {
command = "az acr webhook create --registry \"$ACRNAME\" --name \"$APPNAME$WEBHOOKNAME\" --actions push --uri $(az webapp deployment container show-cd-url -n $APPNAME_URL -g $APPSVCNAME --query CI_CD_URL -o tsv)"

environment = {
ACRNAME = var.azure_container_registry_name
APPNAME = replace(lower("${var.app_service_name_prefix}${local.app_names[count.index]}"), "-", "")
APPNAME_URL = "${var.app_service_name_prefix}-${local.app_names[count.index]}"
WEBHOOKNAME = local.acr_webhook_name
APPSVCNAME = data.azurerm_resource_group.appsvc.name
}

lifecycle {
# This stanza will prevent terraform from reverting changes to the application container settings.
# These settings are how application teams deploy new containers to the app service and should not
# be overridden by Terraform deployments.
ignore_changes = [
"site_config[0].linux_fx_version"
nmiodice marked this conversation as resolved.
Show resolved Hide resolved
]
}
}

@@ -77,6 +72,34 @@ resource "azurerm_app_service_slot" "appsvc_staging_slot" {
resource_group_name = data.azurerm_resource_group.appsvc.name
app_service_plan_id = data.azurerm_app_service_plan.appsvc.id
depends_on = [azurerm_app_service.appsvc]

app_settings = {
DOCKER_REGISTRY_SERVER_URL = format("https://%s", var.docker_registry_server_url)
WEBSITES_ENABLE_APP_SERVICE_STORAGE = false
DOCKER_REGISTRY_SERVER_USERNAME = var.docker_registry_server_username
DOCKER_REGISTRY_SERVER_PASSWORD = var.docker_registry_server_password
APPINSIGHTS_INSTRUMENTATIONKEY = var.app_insights_instrumentation_key
KEYVAULT_URI = var.vault_uri
}

site_config {
linux_fx_version = local.app_linux_fx_versions[count.index]
always_on = var.site_config_always_on
virtual_network_name = var.vnet_name
}

identity {
type = "SystemAssigned"
}

lifecycle {
# This stanza will prevent terraform from reverting changes to the application container settings.
# These settings are how application teams deploy new containers to the app service and should not
# be overridden by Terraform deployments.
ignore_changes = [
"site_config[0].linux_fx_version"
]
}
}

data "azurerm_app_service" "all" {
9 changes: 2 additions & 7 deletions infra/modules/providers/azure/app-service/variables.tf
Original file line number Diff line number Diff line change
@@ -34,6 +34,7 @@ variable "resource_tags" {
variable "app_service_config" {
description = "Metadata about the app services to be created."
type = map(object({
// If "", no container configuration will be set. Otherwise, this will be used to set the container configuration for the app service.
image = string
}))
default = {}
@@ -58,7 +59,7 @@ variable "app_insights_instrumentation_key" {
}

variable "site_config_always_on" {
description = "Should the app be loaded at all times? Defaults to false."
description = "Should the app be loaded at all times? Defaults to true."
nmiodice marked this conversation as resolved.
Show resolved Hide resolved
type = string
default = true
}
@@ -98,9 +99,3 @@ variable "docker_registry_server_password" {
type = string
default = ""
}

variable "docker_enable_ci" {
description = "Enable's continuous deployment for the app service image."
type = bool
default = true
}
2 changes: 1 addition & 1 deletion infra/modules/providers/azure/provider/main.tf
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
provider "azurerm" {
version = "~>1.32.0"
version = "~>1.34.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a changelog available for this? I'm wondering if we need to be on the lookout for any new breaks related to a provider rev.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. If the upstream is following SemVer, that change should t be breaking (or else the major number would have been incremented).

But, I don’t that they’re using SemVer. I can help check tomorrow, @KeithJRome

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

@nmiodice nmiodice Sep 26, 2019

Choose a reason for hiding this comment

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

I upgraded specifically because of a bug related to the provider code that was fixed -- hashicorp/terraform-provider-azurerm#4184.

This was causing issues when trying to run apply multiple times on my test environment

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this bumps the Terraform dependency to 12.8 also (hashicorp/terraform-provider-azurerm#4341)

These changes rolled up into 1.34 look like they are good for us on balance, but I see several behavioral changes that we probably need to be aware of like how certain errors are being reported and some workarounds we have in place now may no longer be needed.

Copy link
Author

Choose a reason for hiding this comment

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

Can you elaborate on that @KeithJRome ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@KeithJRome I read that thread as they are bumping the dependency to 0.12.8. Do you think that means anyone with the 1.34 provider needs to be running on 0.12.8? or is that only for the provider team? @nmiodice did you upgrade TF to 0.12.8? I didn't see that in the commits...

Copy link
Contributor

@KeithJRome KeithJRome Sep 26, 2019

Choose a reason for hiding this comment

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

I looked more closely into each of the changes since 1.32 and the one I was most interested in happens to be the same PR that fixes what you need. The others are very unlikely to cause us problems now that I've dug deeper into the actual code changes involved.

So I don't think this should cause us any problems. Carry On :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes @iphilpot, I think that means we need to rev our direct dependency on Terraform to 0.12.8, but I don't think this will cause us any problems. A few weird error messages might become less weird, and that's probably the extent of impact for us (aside from the bug that @nmiodice referred to being fixed).

Copy link
Author

Choose a reason for hiding this comment

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

@KeithJRome @iphilpot I don't think that we should be driving the version of Terraform based off of the version of the library that the providers consume as its not currently possible to satisfy each provider.

Null Resource Provider: https://github.com/terraform-providers/terraform-provider-null/blob/b8568b9b73a19322eaf0c44982f3834ff34854ff/go.mod#L5

Azure AD Resource Provider: https://github.com/terraform-providers/terraform-provider-azuread/blob/0ed698531d3dd13fed4e51469fda2d517f98aa9d/go.mod#L10

}

provider "null" {
4 changes: 2 additions & 2 deletions infra/templates/az-isolated-service-single-region/ase.tf
Original file line number Diff line number Diff line change
@@ -65,7 +65,7 @@ module "app_service" {
app_service_config = {
for target in var.unauthn_deployment_targets :
target.app_name => {
image = "${target.image_name}:${target.image_release_tag_prefix}"
image = ""
}
}
providers = {
@@ -98,7 +98,7 @@ module "authn_app_service" {
app_service_config = {
for target in var.authn_deployment_targets :
target.app_name => {
image = "${target.image_name}:${target.image_release_tag_prefix}"
image = ""
}
}
providers = {
Original file line number Diff line number Diff line change
@@ -59,7 +59,6 @@ func asMap(t *testing.T, jsonString string) map[string]interface{} {
}

func TestTemplate(t *testing.T) {
expectedStagingSlot := asMap(t, `{"name":"staging"}`)
expectedAppDevResourceGroup := asMap(t, `{
"location": "`+region+`"
}`)
@@ -191,19 +190,24 @@ func TestTemplate(t *testing.T) {
}]
}`)
expectedAppServiceSchema := `{
"identity": [{ "type": "SystemAssigned" }],
"identity": [{ "type": "SystemAssigned" }],
"enabled": true,
"site_config": [{
"always_on": true
"always_on": true,
"linux_fx_version": "DOCKER"
}]
}`
expectedAppServiceSchema2 := `{
"identity": [{ "type": "SystemAssigned" }],
expectedAppService := asMap(t, expectedAppServiceSchema)

expectedAppServiceSlot := asMap(t, `{
"name": "staging",
"identity": [{ "type": "SystemAssigned" }],
"enabled": true,
"site_config": [{
"always_on": true
"always_on": true,
"linux_fx_version": "DOCKER"
}]
}`
expectedAppService1 := asMap(t, expectedAppServiceSchema)
expectedAppService2 := asMap(t, expectedAppServiceSchema2)
}`)

expectedAppServiceKVPolicies := asMap(t, `{
"certificate_permissions": ["get", "list"],
@@ -222,26 +226,22 @@ func TestTemplate(t *testing.T) {
TfOptions: tfOptions,
Workspace: workspace,
PlanAssertions: nil,
ExpectedResourceCount: 62,
ExpectedResourceCount: 60,
ExpectedResourceAttributeValues: infratests.ResourceDescription{
// "module.keyvault.azurerm_key_vault.keyvault": expectedKeyVault,
// "module.container_registry.azurerm_container_registry.container_registry": expectedAzureContainerRegistry,
"azurerm_resource_group.app_rg": expectedAppDevResourceGroup,
"azurerm_resource_group.admin_rg": expectedAdminResourceGroup,
"module.service_plan.azurerm_app_service_plan.svcplan": expectedAppServicePlan,
"module.app_insights.azurerm_application_insights.appinsights": expectedAppInsights,
"module.app_service.azurerm_app_service.appsvc[0]": expectedAppService1,
"module.authn_app_service.azurerm_app_service.appsvc[0]": expectedAppService2,
"module.app_service.azurerm_app_service_slot.appsvc_staging_slot[0]": expectedStagingSlot,
"module.authn_app_service.azurerm_app_service_slot.appsvc_staging_slot[0]": expectedStagingSlot,
"module.app_service.azurerm_app_service.appsvc[0]": expectedAppService,
"module.authn_app_service.azurerm_app_service.appsvc[0]": expectedAppService,
"module.app_service.azurerm_app_service_slot.appsvc_staging_slot[0]": expectedAppServiceSlot,
"module.authn_app_service.azurerm_app_service_slot.appsvc_staging_slot[0]": expectedAppServiceSlot,
"module.service_plan.azurerm_monitor_autoscale_setting.app_service_auto_scale": expectedAutoScalePlan,
"module.app_service_keyvault_access_policy.azurerm_key_vault_access_policy.keyvault[0]": expectedAppServiceKVPolicies,
"module.authn_app_service_keyvault_access_policy.azurerm_key_vault_access_policy.keyvault[0]": expectedAppServiceKVPolicies,
"module.keyvault.module.deployment_service_principal_keyvault_access_policies.azurerm_key_vault_access_policy.keyvault[0]": expectedDeploymentServicePrincipalKVPolicies,

// These are basically existence checks. Nothing interesting to inspect for the resources
"module.app_service.null_resource.acr_webhook_creation[0]": nil,
"module.authn_app_service.null_resource.acr_webhook_creation[0]": nil,
},
}

Original file line number Diff line number Diff line change
@@ -48,18 +48,14 @@ variable "ase_vnet_name" {
variable "unauthn_deployment_targets" {
description = "Metadata about apps to deploy, such as repository location, docker file metadata and image names"
type = list(object({
app_name = string
image_name = string
image_release_tag_prefix = string
app_name = string
}))
}

variable "authn_deployment_targets" {
description = "Metadata about apps to deploy that also require authentication."
type = list(object({
app_name = string
image_name = string
image_release_tag_prefix = string
app_name = string
}))
}

Original file line number Diff line number Diff line change
@@ -10,7 +10,6 @@ import (
)

var region = "eastus"
var workspace = "azsingleregion"

var tfOptions = &terraform.Options{
TerraformDir: "../../",
@@ -35,6 +34,8 @@ func asMap(t *testing.T, jsonString string) map[string]interface{} {

func TestTemplate(t *testing.T) {
expectedAppServicePlan := asMap(t, `{
"enabled": true,
"site_config": [{ "always_on": true }]
}`)

expectedAppGatewayPlan := asMap(t, `{
@@ -100,17 +101,20 @@ func TestTemplate(t *testing.T) {
testFixture := infratests.UnitTestFixture{
GoTest: t,
TfOptions: tfOptions,
Workspace: workspace,
PlanAssertions: nil,
ExpectedResourceCount: 37,
ExpectedResourceCount: 36,
ExpectedResourceAttributeValues: infratests.ResourceDescription{
"azurerm_resource_group.svcplan": map[string]interface{}{
"location": region,
},
"module.app_gateway.data.azurerm_resource_group.appgateway": map[string]interface{}{},
"module.app_insight.data.azurerm_resource_group.appinsights": map[string]interface{}{},
"module.app_service.azurerm_app_service_slot.appsvc_staging_slot[0]": map[string]interface{}{
"name": "staging",
"name": "staging",
"enabled": true,
"site_config": []interface{}{
map[string]interface{}{"always_on": true},
},
},
"module.app_service.azurerm_app_service.appsvc[0]": expectedAppServicePlan,
"module.app_gateway.azurerm_application_gateway.appgateway": expectedAppGatewayPlan,