Upgrade to azurerm 3.0.0+, major improvements, security fixes, Terraform logic and spelling error fixes #164

rdvansloten · 2022-05-09T20:02:28Z

Added

node_taints to default node pool

Changed

Upgraded to Terraform 1.1.9 from 0.13.0
Upgraded to azurerm provider 3.x from 2.x
Split prefix and dns_prefix, as they might not be the same
Separated Log Analytics Workspace and Log Analytics Solutions, for users who do not want both resources
Renamed most variables to be more consistent with resources
Required azurerm provider to >= 3.0.0 from ~> 2.46
Split tests into Kubenet and Azure CNI AKS clusters
Reduced test outputs to bare necessities

Fixed

Fixed typos and mistakes in variables descriptions
role_based_access_control has been deprecated in favor of azure_active_directory_role_based_access_control
addon_profile block has been deprecated in azurerm 3.0
Duplicate default_node_pool was removed and replaced with in-block ternary
Renamed CHANGLOG.md to CHANGELOG.md

Security

In-repo ssh module. This generated a local file on the agent with a private key (highly unsafe!) and was unused. Replaced by a tls_private_key block without a file output

Removed

max_node, min_node and node_count are not mutually exclusive.
agent_ particle in variables.tf. AKS uses node pools and nodes in their terminology

ghost · 2022-05-09T20:02:40Z

All CLA requirements met.

henworth · 2022-05-10T13:25:29Z

main.tf

-  count               = var.enable_log_analytics_workspace ? 1 : 0
-  name                = var.cluster_log_analytics_workspace_name == null ? "${var.prefix}-workspace" : var.cluster_log_analytics_workspace_name
+  count               = var.log_analytics_workspace_enabled ? 1 : 0
+  name                = var.log_analytics_workspace_name == null ? "${try(var.prefix, var.cluster_name)}-workspace" : var.log_analytics_workspace_name


When I was testing this, I ran into an issue here:

│ Error: Invalid template interpolation value │ │ on .terraform/modules/aks/main.tf line 108, in resource "azurerm_log_analytics_workspace" "main": │ 108: name = var.log_analytics_workspace_name == null ? "${try(var.prefix, var.cluster_name)}-workspace" : var.log_analytics_workspace_name │ ├──────────────── │ │ var.cluster_name is "test-aks" │ │ var.prefix is null │ │ The expression result is null. Cannot include a null value in a string template.

What fixed it for me--and what I'd recommend--is to use coalesce(var.cluster_name, var.prefix) instead of try().

caruccio · 2022-05-11T20:02:25Z

Hey @rdvansloten. Would you be willing to add changes from caruccio@25a99a4?
Feel free to copy+paste as you wish.

llabake · 2022-05-11T22:55:16Z

@rdvansloten, are you able to add this change Attaching a container registry to a Kubernetes cluster.

rdvansloten · 2022-05-13T10:12:53Z

@caruccio Added Key Vault and maintenance support.

key_vault_secrets_provider_enabled = false
allowed_maintenance_windows = [
    {
      day   = "Saturday"
      hours = [01]
    },
    {
      day   = "Sunday"
      hours = [01]
    }
  ]

@henworth Added your coalesce() fix.

var.log_analytics_workspace_name == null ? "${coalesce(var.cluster_name, var.prefix)}-workspace" : var.log_analytics_workspace_name

@llabake Container registry can be added:

azure_container_registry_id        = azurerm_container_registry.main.id
azure_container_registry_enabled   = false

rdvansloten · 2022-05-16T10:52:01Z

@lonegunmanb @yupwei68

lonegunmanb · 2022-05-16T23:26:43Z

Hi all, @yupwei68 is no longer maintaining this repo, and I'll take over. We're working on a new Azure module ci pipeline so we can merge further pull requests in a high velocity and keep us safe from common issues and breaking changes. The first version with this new ci pipeline won't touch module's code, and all remain pull requests will be tested by this pipeline.

The feature branch with pipeline is here, and a demo pull request is here, anyone who's interesting in how this pipeline works is welcomed to give it a review.

Our plan is to release a new major version first, with new pipeline, and NO change to the module's code. It'll make things easier. After that, we'll merge some pull requests that has no breaking changes, and no surprise changes (means no change in tfplan by default). Then another new major version with all these pr with breaking changes will be released.

As for this pull request, I think it's very large to review. The smaller and more cohesion the pr is, the easier it can be reviewed and merged.

I'm testing some small pr with our new pipeline now, as for this pr, since it'll introduce breaking changes, I'll review it later after I've merged all no-breaking pull requests. Thank you all for your contribution to this module, please give us some time.

rdvansloten · 2022-05-18T12:36:16Z

Hi @lonegunmanb, thank you for the feedback. Yeah, it's a big overhaul, but a lot of features that are required to use this in an enterprise setting were more or less missing or not added yet.

lonegunmanb · 2022-05-19T08:53:46Z

Hi @rdvansloten , we'd like to upgrade this module to v5.0.0 first. In v5 we won't merge any breaking change into the module, only introduce the ci pipeline and acc tests, maybe some non-breaking changes too. After that, we'll upgrade to v6, which I think we'll merge your pr. Would you please rebase to v5 version and update your pr when we start upgrade work to v6? Thanks.

caruccio · 2022-05-19T14:34:51Z

outputs.tf

-output "system_assigned_identity" {
-  value = azurerm_kubernetes_cluster.main.identity
+output "identity" {
+  value = {


IMHO, returning attributes one-by-one is prune to breaking changes from the resource provider.
Maybe just returning value = azurerm_kubernetes_cluster.main.identity[0] is a better approach.
WYT?

lonegunmanb · 2022-06-02T09:07:23Z

main.tf

+    enable_node_public_ip  = var.enable_node_public_ip
+    zones                  = var.zones
+    node_labels            = var.node_labels
+    node_taints            = var.node_taints


According to terraform-azurerm-provider pr #8982, since the v2.35.0, the node_taints argument is no longer be configured.

I will change this field.

github-actions · 2022-09-13T03:06:09Z

MAIN BRANCH PUSH DETECTED DUE TO #241, THIS PR WILL BE UPDATED.

github-actions · 2022-09-13T03:21:56Z

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR WILL BE UPDATED.

github-actions · 2022-09-13T03:22:53Z

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR WILL BE UPDATED.

github-actions · 2022-09-13T03:26:52Z

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

lonegunmanb · 2022-09-13T05:52:11Z

I'm closing this pr since we've upgraded azurerm to 3.0.0+. Thanks for opening this pr @rdvansloten .

rdvansloten added 6 commits May 9, 2022 19:13

Drastically improved Terraform code

e0e3de8

Syntax fies, Readme update, changelog update

edca3ee

Syntax fixes

156b428

Updated changelog

dc99dac

Update Dockerfile tf version

9f4610e

Revert Dockerfile version

a1c6c19

rdvansloten added 2 commits May 9, 2022 22:04

Fixed Changelog

033033b

Added fix to README

d1f687f

henworth reviewed May 10, 2022

View reviewed changes

Added suggested fixed

a14d743

lonegunmanb mentioned this pull request May 17, 2022

docs(README): add terraform-docs generated module specification #165

Closed

Fix typo in output

bcc529d

caruccio reviewed May 19, 2022

View reviewed changes

lonegunmanb requested changes Jun 2, 2022

View reviewed changes

lonegunmanb closed this Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to azurerm 3.0.0+, major improvements, security fixes, Terraform logic and spelling error fixes #164

Upgrade to azurerm 3.0.0+, major improvements, security fixes, Terraform logic and spelling error fixes #164

rdvansloten commented May 9, 2022

ghost commented May 9, 2022 •

edited by ghost

Loading

henworth May 10, 2022

caruccio commented May 11, 2022

llabake commented May 11, 2022

rdvansloten commented May 13, 2022

rdvansloten commented May 16, 2022

lonegunmanb commented May 16, 2022 •

edited

Loading

rdvansloten commented May 18, 2022

lonegunmanb commented May 19, 2022

caruccio May 19, 2022

lonegunmanb Jun 2, 2022

rdvansloten Jun 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

lonegunmanb commented Sep 13, 2022

Upgrade to azurerm 3.0.0+, major improvements, security fixes, Terraform logic and spelling error fixes #164

Upgrade to azurerm 3.0.0+, major improvements, security fixes, Terraform logic and spelling error fixes #164

Conversation

rdvansloten commented May 9, 2022

Added

Changed

Fixed

Security

Removed

ghost commented May 9, 2022 • edited by ghost Loading

henworth May 10, 2022

Choose a reason for hiding this comment

caruccio commented May 11, 2022

llabake commented May 11, 2022

rdvansloten commented May 13, 2022

rdvansloten commented May 16, 2022

lonegunmanb commented May 16, 2022 • edited Loading

rdvansloten commented May 18, 2022

lonegunmanb commented May 19, 2022

caruccio May 19, 2022

Choose a reason for hiding this comment

lonegunmanb Jun 2, 2022

Choose a reason for hiding this comment

rdvansloten Jun 13, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

lonegunmanb commented Sep 13, 2022

ghost commented May 9, 2022 •

edited by ghost

Loading

lonegunmanb commented May 16, 2022 •

edited

Loading