Firewall support #470

Dev25 · 2020-03-31T19:37:14Z

Closes #452

Add the 2 firewall rules raised in that issue, which can be opted out using var.firewall_enabled.

Common inbound ports are also white listed by default (8443/9443 and 15017 for istio 1.5) for a better UX (port list controlled by var.firewall_inbound_ports)

Refactoring:

Save "gke-${var.name}" network tag format into locals, this module already adds that network tag to all node pools
Move networks.tf to autogen/ so module variants have access to data.google_compute_subnetwork.gke_subnetwork which can be used to determine pod CIDR
Misc formatting fixes to templates so the latest docker image is happy for make build docker_test_lint

TODO:

README update, this requires the terraform runner to have necessary IAM permissions to make firewall changes in the VPC project

Signed-off-by: Dev <Dev25@users.noreply.github.com>

autogen/main/firewall.tf.tmpl

morgante

Thanks for submitting this, just a few suggestions.

autogen/main/README.md

morgante · 2020-04-15T23:15:24Z

autogen/main/networks.tf

+
+{{ autogeneration_note }}
+
+data "google_compute_network" "gke_network" {


Note this might raise some issues with networks created inline. Could we make these data sources conditional on the firewall_enabled var?

Will look into doing that.

I think the regular count conditional approach should work on datasources too

I think this is the only thing blocking merge now.

Should be fixed in 8ffa5e8
Also removed the top level network datasource since it seems unused

Plus ceef395 to fix attribute errors, not pretty but it works/keeps it all conditional.

Error: Unsupported attribute on .terraform/modules/blue_gke.gke/main.tf line 75, in locals: 75: cluster_alias_ranges_cidr = { for range in toset(data.google_compute_subnetwork.gke_subnetwork.*.secondary_ip_range) : range.range_name => range.ip_cidr_range }

autogen/main/variables.tf.tmpl

Signed-off-by: Dev <Dev25@users.noreply.github.com>

morgante · 2020-04-21T20:35:36Z

@Dev25 This is the error which shows up for that test:

       Error: project: required field is not set
       
         on ../../../main.tf line 22, in data "google_compute_zones" "available":
         22: data "google_compute_zones" "available" {
       
       
       
       Error: project: required field is not set
       
         on ../../../main.tf line 125, in data "google_container_engine_versions" "region":
        125: data "google_container_engine_versions" "region" {

bharathkkb · 2020-04-21T22:44:01Z

That does seem weird, let me try re running - svpc example seems fine to me

morgante · 2020-04-22T20:23:02Z

autogen/main/networks.tf

+
+{{ autogeneration_note }}
+
+data "google_compute_network" "gke_network" {


I think this is the only thing blocking merge now.

Signed-off-by: Dev <Dev25@users.noreply.github.com>

Dev25 · 2020-04-22T20:41:54Z

btw a old CI job seems to be stuck, you may want to explicitly cancel it: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/runs/605516376

Google Cloud Build / Trigger: 10ee403a-235b-42d6-8acc-5ebfe78cfcae
started 1d 6h 13m 32s ago

bharathkkb · 2020-04-22T20:46:51Z

@Dev25 that succeeded on cloud build, for some reason it didnt report back to GH

Signed-off-by: Dev <Dev25@users.noreply.github.com>

erichaase · 2020-07-22T20:40:29Z

modules/beta-private-cluster-update-variant/main.tf

@@ -71,6 +71,9 @@ locals {
  // auto upgrade by defaults only for regional cluster as long it has multiple masters versus zonal clusters have only have a single master so upgrades are more dangerous.
  default_auto_upgrade = var.regional ? true : false

+  cluster_subnet_cidr       = var.add_cluster_firewall_rules ? data.google_compute_subnetwork.gke_subnetwork[0].ip_cidr_range : null
+  cluster_alias_ranges_cidr = var.add_cluster_firewall_rules ? { for range in toset(data.google_compute_subnetwork.gke_subnetwork[0].secondary_ip_range) : range.range_name => range.ip_cidr_range } : {}


I'm getting the following error on terraform plan (also applies to apply, refresh, etc.) thrown by this line:

Error: Iteration over null value on .terraform/modules/corp-dev-us-east1-c-01.gke/terraform-google-kubernetes-engine-10.0.0/modules/beta-private-cluster-update-variant/main.tf line 72, in locals: 72: cluster_alias_ranges_cidr = var.add_cluster_firewall_rules ? { for range in toset(data.google_compute_subnetwork.gke_subnetwork[0].secondary_ip_range) : range.range_name => range.ip_cidr_range } : {} |---------------- | data.google_compute_subnetwork.gke_subnetwork[0].secondary_ip_range is null A null value cannot be used as the collection in a 'for' expression.

I'm having a hard time troubleshooting this on our end and would appreciate any feedback y'all have on this. cc: @bharathkkb @morgante

Could you open a new issue w/ your Terraform config?

Yeah, sure: #608

…s#470) * Squash Commits Signed-off-by: Dev <Dev25@users.noreply.github.com> * Fix example Signed-off-by: Dev <Dev25@users.noreply.github.com> * Rename var + update README Signed-off-by: Dev <Dev25@users.noreply.github.com> * Set to false as default Signed-off-by: Dev <Dev25@users.noreply.github.com> * Enable firewall support in shared_vpc example Signed-off-by: Dev <Dev25@users.noreply.github.com> * Remove network datasource and make subnetwork conditional on firewall Signed-off-by: Dev <Dev25@users.noreply.github.com> * Fix attribute error Signed-off-by: Dev <Dev25@users.noreply.github.com>

Dev25 requested review from adrienthebo, Jberlinsky and morgante as code owners March 31, 2020 19:37

Dev25 force-pushed the firewall branch from 88330d6 to c4b357f Compare April 10, 2020 11:05

Dev25 requested review from bharathkkb and a team as code owners April 10, 2020 11:05

Squash Commits

5ef4044

Signed-off-by: Dev <Dev25@users.noreply.github.com>

Dev25 force-pushed the firewall branch from a4fbc49 to 5ef4044 Compare April 10, 2020 18:52

Fix example

33bf1e8

Signed-off-by: Dev <Dev25@users.noreply.github.com>

bharathkkb reviewed Apr 15, 2020

View reviewed changes

autogen/main/firewall.tf.tmpl Show resolved Hide resolved

morgante suggested changes Apr 15, 2020

View reviewed changes

Dev25 added 4 commits April 21, 2020 15:26

Merge branch 'master' into firewall

cfb97c4

Rename var + update README

2629100

Signed-off-by: Dev <Dev25@users.noreply.github.com>

Set to false as default

517cb2f

Signed-off-by: Dev <Dev25@users.noreply.github.com>

Enable firewall support in shared_vpc example

3f06a83

Signed-off-by: Dev <Dev25@users.noreply.github.com>

morgante suggested changes Apr 22, 2020

View reviewed changes

Remove network datasource and make subnetwork conditional on firewall

8ffa5e8

Signed-off-by: Dev <Dev25@users.noreply.github.com>

Fix attribute error

ceef395

Signed-off-by: Dev <Dev25@users.noreply.github.com>

morgante approved these changes Apr 23, 2020

View reviewed changes

morgante merged commit 16bdd6e into terraform-google-modules:master Apr 23, 2020

erichaase reviewed Jul 22, 2020

View reviewed changes

fstr mentioned this pull request Apr 29, 2021

Webhook firewall rule not targeting correct source range #859

Closed

tomasgareau mentioned this pull request Jan 12, 2022

intra-egress firewall rule depends on google_container_cluster.primary #1124

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firewall support #470

Firewall support #470

Dev25 commented Mar 31, 2020 •

edited

Loading

morgante left a comment

morgante Apr 15, 2020

Dev25 Apr 21, 2020

bharathkkb Apr 22, 2020

morgante Apr 22, 2020

Dev25 Apr 22, 2020

Dev25 Apr 23, 2020

morgante commented Apr 21, 2020

bharathkkb commented Apr 21, 2020 •

edited

Loading

morgante Apr 22, 2020

Dev25 commented Apr 22, 2020

bharathkkb commented Apr 22, 2020

erichaase Jul 22, 2020 •

edited

Loading

morgante Jul 22, 2020

erichaase Jul 22, 2020 •

edited

Loading


		{{ autogeneration_note }}

		data "google_compute_network" "gke_network" {

Firewall support #470

Firewall support #470

Conversation

Dev25 commented Mar 31, 2020 • edited Loading

morgante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgante commented Apr 21, 2020

bharathkkb commented Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Dev25 commented Apr 22, 2020

bharathkkb commented Apr 22, 2020

erichaase Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichaase Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Dev25 commented Mar 31, 2020 •

edited

Loading

bharathkkb commented Apr 21, 2020 •

edited

Loading

erichaase Jul 22, 2020 •

edited

Loading

erichaase Jul 22, 2020 •

edited

Loading