Add role assignments for ingress application gateway and corresponding example by lonegunmanb · Pull Request #426 · Azure/terraform-azurerm-aks

lonegunmanb · 2023-08-16T13:02:52Z

Describe your changes

This pr added azurerm_role_assignment resources so the existing application gateway could work immediately. It also added a working example.

This pr contains breaking change.

Issue number

#223

Checklist before requesting a review

The pr title can be used to describe what this pr did in CHANGELOG.md file
I have executed pre-commit on my machine
I have passed pr-check on my machine

Thanks for your cooperation!

github-actions · 2023-08-16T13:10:19Z

Potential Breaking Changes in d1ea167:
[delete] "Variables.ingress_application_gateway_id.Name" from 'ingress_application_gateway_id' to ''

github-actions · 2023-11-16T07:14:10Z

Potential Breaking Changes in 2b1ff07:
[delete] "Variables.ingress_application_gateway_id.Name" from 'ingress_application_gateway_id' to ''

github-actions · 2023-11-22T08:06:16Z

Potential Breaking Changes in fb538b0:
[delete] "Variables.ingress_application_gateway_id.Name" from 'ingress_application_gateway_id' to ''

lonegunmanb · 2023-11-22T09:54:37Z

Hi @zioproto, would you please give this pr a review? Thanks!

zioproto · 2023-11-23T09:59:47Z

This PR fixes the roles assignments when the Application Gateway is created by user with an external to the module Terraform resource.

The scenario depicted in issue #223 is when we expect the AGIC controller running in the cluster to create the Application Gateway on behalf of the user.

I tried with the following config:

 ingress_application_gateway_enabled   = true
 ingress_application_gateway_subnet_id = module.network-westeurope.vnet_subnets[2]
 ingress_application_gateway_name = "aks-agw-westeurope"

And I still see in my kube-system namespace the Pod ingress-appgw-deployment-c5d78b4f8-6dgtx failing to create the Application Gateway

ingress-appgw-deployment-c5d78b4f8-6dgtx ingress-appgw-container I1123 09:54:13.013152       1 client.go:314] Deploying Gateway
ingress-appgw-deployment-c5d78b4f8-6dgtx ingress-appgw-container I1123 09:54:13.030598       1 client.go:321] Using resource group: MC_istio-aks-westeurope_westeurope-aks_westeurope
ingress-appgw-deployment-c5d78b4f8-6dgtx ingress-appgw-container I1123 09:54:13.030631       1 client.go:324] Starting ARM template deployment: aks-agw-westeurope
ingress-appgw-deployment-c5d78b4f8-6dgtx ingress-appgw-container F1123 09:54:44.072080       1 main.go:168] Failed in deploying Application GatewayCode="DeploymentFailed" Message="At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-deployment-operations for usage details." Details=[{"code":"Conflict","message":"{\r\n  \"error\": {\r\n    \"code\": \"ApplicationGatewayInsufficientPermissionOnSubnet\",\r\n    \"message\": \"Client with object id d0ba9abb-427c-450b-9bfb-30689a03c3d4 does not have permission on the Virtual Network resource /subscriptions/9b70acd9-975f-44ba-bad6-255a2c8bda37/resourceGroups/istio-aks-westeurope/providers/Microsoft.Network/virtualNetworks/istio-aks-westeurope/subnets/user to perform action Microsoft.Network/virtualNetworks/subnets/join/action. For details on the required permissions, please visit https://aka.ms/agsubnetjoin.\",\r\n    \"details\": []\r\n  }\r\n}"}]

zioproto · 2023-11-23T08:57:35Z

+
+provider "kubernetes" {
+  host                   = module.aks.admin_host
+  client_certificate     = base64decode(module.aks.admin_client_certificate)


Is using this output from the AKS module safe ?

The most reliable way to configure the Kubernetes provider is to ensure that the cluster itself and the Kubernetes provider resources can be managed with separate apply operations. Data-sources can be used to convey values between the two stages as needed.

https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs#stacking-with-managed-kubernetes-cluster-resources

Just double checking because we don't want to introduce random CI failures

zioproto · 2023-11-23T09:01:24Z

@@ -0,0 +1,65 @@
+resource "azurerm_log_analytics_workspace" "main" {


How this change about azurerm_log_analytics_workspace is related with the role assignments for ingress ? Should this go to a different PR ?

I understand now you are moving this resource to a different file. You could do this cleanup in a different PR for readability.

zioproto · 2023-11-23T09:02:24Z

-    for_each = var.api_server_authorized_ip_ranges != null || var.api_server_subnet_id != null ? [
-      "api_server_access_profile"
-    ] : []
+    for_each = var.api_server_authorized_ip_ranges != null || var.api_server_subnet_id != null ? ["api_server_access_profile"] : []


Is this a change in formatting that was applied automatically by terraform fmt ?

zioproto · 2023-11-23T09:04:23Z

-      for_each = var.load_balancer_profile_enabled && var.load_balancer_sku == "standard" ? [
-        "load_balancer_profile"
-      ] : []
+      for_each = var.load_balancer_profile_enabled && var.load_balancer_sku == "standard" ? ["load_balancer_profile"] : []


Change in formatting, why ?

zioproto · 2023-11-23T09:49:59Z

  description = "(Optional) The ID of the Subnet where the API server endpoint is delegated to."
 }

+variable "application_gateway_for_ingress" {


This is used when the Application Gateway is created with a resource outside of the module.

This change is not related to the issue #223 linked in this PR, where it is the AGIC controller running in the AKS cluster that is in charge of creating the Application Gateway.

zioproto · 2023-11-23T10:03:38Z

-  create_analytics_workspace = var.log_analytics_workspace_enabled && var.log_analytics_workspace == null
+  create_analytics_solution                       = var.log_analytics_workspace_enabled && var.log_analytics_solution == null
+  create_analytics_workspace                      = var.log_analytics_workspace_enabled && var.log_analytics_workspace == null
+  create_role_assignments_for_application_gateway = try(var.application_gateway_for_ingress.create_role_assignments, false)


The role assignments should be created if var.ingress_application_gateway_enabled = true

zioproto · 2023-11-23T10:18:17Z

In my testing scenario each nodepool has a dedicated subnet. I have a system and a user nodepool in a system and a user subnet:

https://github.com/zioproto/istio-aks-example/blob/main/multicluster-istio-on-aks/nodepools.tf
https://github.com/zioproto/istio-aks-example/blob/main/multicluster-istio-on-aks/network.tf#L8
https://github.com/zioproto/istio-aks-example/blob/main/multicluster-istio-on-aks/main.tf#L15

When the AGIC creates the Application Gateway I see the error ApplicationGatewayInsufficientPermissionOnSubnet.

ingress-appgw-deployment-c5d78b4f8-6dgtx ingress-appgw-container F1123 10:09:20.881644       1 main.go:168] Failed in deploying Application GatewayCode="DeploymentFailed" Message="At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-deployment-operations for usage details." Details=[{"code":"Conflict","message":"{\r\n  \"error\": {\r\n    \"code\": \"ApplicationGatewayInsufficientPermissionOnSubnet\",\r\n    \"message\": \"Client with object id d0ba9abb-427c-450b-9bfb-30689a03c3d4 does not have permission on the Virtual Network resource /subscriptions/9b70acd9-975f-44ba-bad6-255a2c8bda37/resourceGroups/istio-aks-westeurope/providers/Microsoft.Network/virtualNetworks/istio-aks-westeurope/subnets/user to perform action Microsoft.Network/virtualNetworks/subnets/join/action. For details on the required permissions, please visit https://aka.ms/agsubnetjoin.\",\r\n    \"details\": []\r\n  }\r\n}"}]

The problem seems to be there only for the user subnet.

zioproto · 2023-11-24T08:45:36Z

Next steps:

The application_gateway_for_ingress object should have Optional ID.
The old variables ingress_application_gateway_enabled ingress_application_gateway_name ingress_application_gateway_subnet_cidr ingress_application_gateway_subnet_id will be removed and will go into the application_gateway_for_ingress object
The role assignments will be created both in the scenario where the customer brings his own application gateway with an existing ID, and in the scenario where the customer only gives a name, and the application gateway is created by the AGIC Pod in the AKS control plane.

github-actions · 2023-11-30T02:30:01Z

Potential Breaking Changes in d5a2de4:
[delete] "Variables.ingress_application_gateway_subnet_cidr.Name" from 'ingress_application_gateway_subnet_cidr' to ''
[delete] "Variables.ingress_application_gateway_name.Name" from 'ingress_application_gateway_name' to ''
[delete] "Variables.ingress_application_gateway_enabled.Name" from 'ingress_application_gateway_enabled' to ''
[delete] "Variables.ingress_application_gateway_subnet_id.Name" from 'ingress_application_gateway_subnet_id' to ''
[delete] "Variables.ingress_application_gateway_id.Name" from 'ingress_application_gateway_id' to ''
[update] "Variables.maintenance_window.Type" from 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), []),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})' to 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), [
]),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})'

github-actions · 2023-12-01T14:10:54Z

Potential Breaking Changes in e388792:
[delete] "Variables.ingress_application_gateway_subnet_id.Name" from 'ingress_application_gateway_subnet_id' to ''
[delete] "Variables.ingress_application_gateway_name.Name" from 'ingress_application_gateway_name' to ''
[delete] "Variables.ingress_application_gateway_id.Name" from 'ingress_application_gateway_id' to ''
[delete] "Variables.ingress_application_gateway_subnet_cidr.Name" from 'ingress_application_gateway_subnet_cidr' to ''
[delete] "Variables.ingress_application_gateway_enabled.Name" from 'ingress_application_gateway_enabled' to ''
[update] "Variables.maintenance_window.Type" from 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), []),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})' to 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), [
]),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})'

github-actions · 2023-12-04T03:21:28Z

Potential Breaking Changes in b577a56:
[delete] "Variables.ingress_application_gateway_id.Name" from 'ingress_application_gateway_id' to ''
[delete] "Variables.ingress_application_gateway_subnet_id.Name" from 'ingress_application_gateway_subnet_id' to ''
[delete] "Variables.ingress_application_gateway_enabled.Name" from 'ingress_application_gateway_enabled' to ''
[delete] "Variables.ingress_application_gateway_name.Name" from 'ingress_application_gateway_name' to ''
[delete] "Variables.ingress_application_gateway_subnet_cidr.Name" from 'ingress_application_gateway_subnet_cidr' to ''
[update] "Variables.maintenance_window.Type" from 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), []),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})' to 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), [
]),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})'

lonegunmanb · 2023-12-05T09:23:18Z

Hi @zioproto would you please give this pr another review? Thanks!

github-actions · 2023-12-05T09:26:37Z

Potential Breaking Changes in 399ab35:
[delete] "Variables.ingress_application_gateway_enabled.Name" from 'ingress_application_gateway_enabled' to ''
[delete] "Variables.ingress_application_gateway_subnet_cidr.Name" from 'ingress_application_gateway_subnet_cidr' to ''
[delete] "Variables.ingress_application_gateway_subnet_id.Name" from 'ingress_application_gateway_subnet_id' to ''
[delete] "Variables.ingress_application_gateway_id.Name" from 'ingress_application_gateway_id' to ''
[delete] "Variables.ingress_application_gateway_name.Name" from 'ingress_application_gateway_name' to ''
[update] "Variables.maintenance_window.Type" from 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), []),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})' to 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), [
]),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})'

lonegunmanb added the breaking-change label Aug 16, 2023

lonegunmanb added this to the 8.0.0 milestone Aug 16, 2023

lonegunmanb temporarily deployed to acctests August 17, 2023 04:07 — with GitHub Actions Inactive

lonegunmanb force-pushed the f-223 branch from d1ea167 to 2b1ff07 Compare November 16, 2023 07:08

lonegunmanb had a problem deploying to acctests November 16, 2023 07:15 — with GitHub Actions Failure

lonegunmanb temporarily deployed to acctests November 17, 2023 01:23 — with GitHub Actions Inactive

lonegunmanb mentioned this pull request Nov 17, 2023

Application Gateway is NOT Created #223

Closed

lonegunmanb force-pushed the f-223 branch from 2b1ff07 to fb538b0 Compare November 22, 2023 07:59

lonegunmanb temporarily deployed to acctests November 22, 2023 08:03 — with GitHub Actions Inactive

lonegunmanb requested a review from zioproto November 22, 2023 09:52

zioproto suggested changes Nov 23, 2023

View reviewed changes

zioproto reviewed Nov 23, 2023

View reviewed changes

lonegunmanb had a problem deploying to acctests November 27, 2023 04:09 — with GitHub Actions Failure

lonegunmanb force-pushed the f-223 branch from a906d9c to e72c4d4 Compare November 29, 2023 08:11

lonegunmanb temporarily deployed to acctests November 29, 2023 08:12 — with GitHub Actions Inactive

lonegunmanb temporarily deployed to acctests November 30, 2023 02:23 — with GitHub Actions Inactive

lonegunmanb force-pushed the f-223 branch from a406a26 to 8b0ab33 Compare December 1, 2023 08:12

try to fix 223

14605bf

lonegunmanb force-pushed the f-223 branch from 8b0ab33 to 14605bf Compare December 1, 2023 09:04

revert way to config kubernetes provider

cd7df27

lonegunmanb had a problem deploying to acctests December 1, 2023 09:39 — with GitHub Actions Error

add version upgrade test

556944c

lonegunmanb had a problem deploying to acctests December 1, 2023 09:44 — with GitHub Actions Failure

fix incorrect condition

2f89634

lonegunmanb had a problem deploying to acctests December 1, 2023 09:57 — with GitHub Actions Failure

fix incorrect expression

0328cbf

lonegunmanb had a problem deploying to acctests December 1, 2023 10:14 — with GitHub Actions Failure

fix syntax error

e388792

lonegunmanb had a problem deploying to acctests December 1, 2023 14:05 — with GitHub Actions Failure

fix syntax error

b577a56

lonegunmanb temporarily deployed to acctests December 4, 2023 03:25 — with GitHub Actions Inactive

zioproto suggested changes Dec 4, 2023

View reviewed changes

Comment thread main.tf Outdated

Comment thread variables.tf

update due code review

399ab35

lonegunmanb temporarily deployed to acctests December 5, 2023 09:22 — with GitHub Actions Inactive

lonegunmanb requested a review from zioproto December 5, 2023 09:22

zioproto approved these changes Dec 6, 2023

View reviewed changes

lonegunmanb merged commit bc905a5 into Azure:main Dec 8, 2023

		@@ -0,0 +1,65 @@
		resource "azurerm_log_analytics_workspace" "main" {

Conversation

lonegunmanb commented Aug 16, 2023

Describe your changes

Issue number

Checklist before requesting a review

Uh oh!

github-actions Bot commented Aug 16, 2023

Uh oh!

github-actions Bot commented Nov 16, 2023

Uh oh!

github-actions Bot commented Nov 22, 2023

Uh oh!

lonegunmanb commented Nov 22, 2023

Uh oh!

zioproto commented Nov 23, 2023

Uh oh!

zioproto Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

zioproto Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

zioproto Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

zioproto Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

zioproto Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

zioproto Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

zioproto Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

zioproto commented Nov 23, 2023

Uh oh!

zioproto commented Nov 24, 2023

Uh oh!

github-actions Bot commented Nov 30, 2023

Uh oh!

github-actions Bot commented Dec 1, 2023

Uh oh!

github-actions Bot commented Dec 4, 2023

Uh oh!

Uh oh!

Uh oh!

lonegunmanb commented Dec 5, 2023

Uh oh!

github-actions Bot commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants