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

feat: mesh_certificates support #1712

Conversation

bgvdiscord
Copy link
Contributor

@bgvdiscord bgvdiscord commented Aug 17, 2023

Implements feature request from #1691 (quoted below) to add enable_mesh_certificates configuration option, including to safer-cluster.

Defaults to false / not enabled, which is the default behavior for clusters without it set. Not sure whether to call this a breaking change due to overwriting if value was set to true with another module or gcloud command.

Happy to take feedback.

TL;DR

In order to use mTLS service security with Traffic Director "Mesh certificates" must be enabled on the cluster. Add the ability for all cluster modules in this repository to enable this functionality with a variable.

Terraform Resources

The documentation for google_container_cluster with the mesh_certificates block
https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/container_cluster#enable_certificates

Instructions for --enable-mesh-certificates on GKE cluster in the Traffic Director documentation for setting up mTLS certificates
https://cloud.google.com/traffic-director/docs/security-envoy-setup#create-cluster

Detailed design

This could look like exposing an module variable to configure the underlying google_container_cluster terraform resource.

mesh_certificates {
  enable_certificates = var.enable_mesh_certificates
}

@bgvdiscord bgvdiscord changed the title [feat] enable_mesh_certificates for safer-cluster and beta-private-cluster [feat] enable_mesh_certificates support Aug 17, 2023
@bgvdiscord bgvdiscord changed the title [feat] enable_mesh_certificates support [feat] mesh_certificates support Aug 17, 2023
@bgvdiscord bgvdiscord force-pushed the feat/safer-cluster-mesh-certificates branch 2 times, most recently from e2b2fcb to 6a5093b Compare August 17, 2023 16:01
@bgvdiscord bgvdiscord marked this pull request as ready for review August 17, 2023 16:03
@bgvdiscord bgvdiscord requested review from Jberlinsky, ericyz and a team as code owners August 17, 2023 16:03
@ericyz
Copy link
Collaborator

ericyz commented Aug 23, 2023

Hi @bgvdiscord , thank you for your contribution. Please help to resolve the conflicts and I will help to review this

@bgvdiscord bgvdiscord force-pushed the feat/safer-cluster-mesh-certificates branch from 6a5093b to 55e447d Compare August 29, 2023 16:15
@bgvdiscord bgvdiscord changed the title [feat] mesh_certificates support feat: mesh_certificates support Aug 29, 2023
@bgvdiscord
Copy link
Contributor Author

Hi @bgvdiscord , thank you for your contribution. Please help to resolve the conflicts and I will help to review this

@ericyz : conflicts are resolved. Thanks!

Copy link
Contributor

@g-awmalik g-awmalik left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Please take a look at a few nits.

autogen/main/main.tf.tmpl Outdated Show resolved Hide resolved
@g-awmalik
Copy link
Contributor

/gcbrun

@@ -185,6 +185,9 @@ module "gke" {
// We enable Workload Identity by default.
identity_namespace = "${var.project_id}.svc.id.goog"

// Enabling mesh certificates requires Workload Identity
enable_mesh_certificates = var.enable_mesh_certificates
Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap this in {% if autopilot_cluster != true %} so it doesn't render for autopilot clusters.

@bgvdiscord bgvdiscord force-pushed the feat/safer-cluster-mesh-certificates branch from 91145cd to ba8641c Compare September 7, 2023 15:30
@bgvdiscord bgvdiscord force-pushed the feat/safer-cluster-mesh-certificates branch from ba8641c to 00878e5 Compare September 7, 2023 15:33
@@ -219,6 +219,12 @@ locals {
cluster_workload_identity_config = ! local.workload_identity_enabled ? [] : var.identity_namespace == "enabled" ? [{
workload_pool = "${var.project_id}.svc.id.goog" }] : [{ workload_pool = var.identity_namespace
}]
{% if autopilot_cluster != true %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: cluster_mesh_certificates_config no longer in autopilot clusters.

@@ -219,6 +219,12 @@ locals {
cluster_workload_identity_config = ! local.workload_identity_enabled ? [] : var.identity_namespace == "enabled" ? [{
workload_pool = "${var.project_id}.svc.id.goog" }] : [{ workload_pool = var.identity_namespace
}]
{% if autopilot_cluster != true %}
cluster_mesh_certificates_config = local.workload_identity_enabled ? [{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied readability suggestion - thanks!

@bgvdiscord
Copy link
Contributor Author

@g-awmalik those changes are made; thanks!

@g-awmalik
Copy link
Contributor

/gcbrun

@g-awmalik
Copy link
Contributor

Thanks for the updates @bgvdiscord. There still seem to be a few references in the autopilot modules. Please take a look at the lint check and resolve them.

@@ -170,6 +170,17 @@ output "identity_namespace" {
google_container_cluster.primary
]
}

{% if autopilot_cluster != true %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped to fix lint

@g-awmalik
Copy link
Contributor

/gcbrun

@g-awmalik g-awmalik merged commit 8913ef2 into terraform-google-modules:master Sep 11, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants