Skip to content

Open Service Mesh addon #132

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.

Sign up for GitHub

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

Merged
lonegunmanb merged 6 commits into from
Jul 8, 2022
Merged

Open Service Mesh addon #132

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aab437b
Add: open service mesh addon
nlamirault Nov 26, 2021
bce3b8d
Fix: typo
nlamirault Nov 26, 2021
68bb6c1
Merge branch 'master' into feat/addon-osm
nlamirault Jan 26, 2022
a187376
Merge branch 'master' into feat/addon-osm
nlamirault Jun 17, 2022
891769b
Fix: typo
nlamirault Jun 17, 2022
81d1fc8
Fix: description and default value for OSM addon
nlamirault Jul 8, 2022
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
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ module "aks" {
private_cluster_enabled = true # default value
enable_http_application_routing = true
enable_azure_policy = true
enable_open_service_mesh = true
enable_auto_scaling = true
enable_host_encryption = true
agents_min_count = 1
Expand Down
4 changes: 4 additions & 0 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ resource "azurerm_kubernetes_cluster" "main" {
log_analytics_workspace_id = var.enable_log_analytics_workspace ? azurerm_log_analytics_workspace.main[0].id : null
}

open_service_mesh {
enabled = var.enable_open_service_mesh
}

Copy link
Copy Markdown
Contributor

@zioproto zioproto Jun 15, 2022

Choose a reason for hiding this comment

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

Looking at the documentation this is not a block:
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/kubernetes_cluster#open_service_mesh_enabled

It should be a oneliner:

open_service_mesh_enabled = var.enable_open_service_mesh

Copy link
Copy Markdown
Contributor

@zioproto zioproto Jun 15, 2022

Choose a reason for hiding this comment

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

I actually tested this and it works with provider registry.terraform.io/hashicorp/azurerm v2.99.0. The change I proposed will be needed once the module will use the version 3 of the provider.

Copy link
Copy Markdown
Member

@lonegunmanb lonegunmanb Jun 15, 2022

Choose a reason for hiding this comment

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

@zioproto you're right. For now this module works with provider 2.x so this patch is correct, but we're close to merge a pr that upgrade provider to 3.x so I'm not sure whether we should continue work on 2.x. My thought is we upgrade to 3.x first then let's see what we can do for this pr.

Copy link
Copy Markdown
Contributor

@zioproto zioproto Jun 17, 2022

Choose a reason for hiding this comment

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

@nlamirault you introduced a line with empty spaces at line 102. We are going to introduce very soon a CI that will lint the code, and this file will not pass the linting. Could you please remove the empty spaces ?

To avoid this pain in the future you can set your editor to automatically trim empty spaces at the end of lines when you save the files. Thank you

Copy link
Copy Markdown
Contributor Author

@nlamirault nlamirault Jun 17, 2022

Choose a reason for hiding this comment

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

Sorry 👎 I made the fix using Github UI ... I fix that asap.

Copy link
Copy Markdown
Member

@lonegunmanb lonegunmanb Jul 1, 2022

Choose a reason for hiding this comment

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

Hi @zioproto agreed but I think we can accept this pr for this time since we'll submit a single pr to fix all the naming issues and style issues.

dynamic "ingress_application_gateway" {
for_each = var.enable_ingress_application_gateway == null ? [] : ["ingress_application_gateway"]
content {
Expand Down
6 changes: 6 additions & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,12 @@ variable "enable_azure_policy" {
default = false
}

variable "enable_open_service_mesh" {
description = "Enable Open Service Mesh Addon."
Copy link
Copy Markdown
Member

@lonegunmanb lonegunmanb Jul 8, 2022

Choose a reason for hiding this comment

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

Can we use the same description as provider's document?

type = bool
default = false
Copy link
Copy Markdown
Member

@lonegunmanb lonegunmanb Jul 8, 2022

Choose a reason for hiding this comment

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

Would the default value null be better? A false default value will cause changes in plan when the users upgrade module's value.

}

variable "sku_tier" {
description = "The SKU Tier that should be used for this Kubernetes Cluster. Possible values are Free and Paid"
type = string
Expand Down
⚡