Merge pull request #31 from anyscale/brent/firewallfix

fix: VPC Firewall when using CIDR Ingress Range

modules/google-anyscale-vpc-firewall/README.md

@@ -5,7 +5,7 @@
 
 This sub-module builds Google VPC Firewalls that will work with Anyscale. It should be used from the [root module](../../README.md).
 
-<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
+<!-- BEGIN_TF_DOCS -->
 ## Requirements
 
 | Name | Version |
@@ -38,19 +38,19 @@ No modules.
 
 | Name | Description | Type | Default | Required |
 |------|-------------|------|---------|:--------:|
-| <a name="input_anyscale_project_id"></a> [anyscale\_project\_id](#input\_anyscale\_project\_id) | (Optional) The ID of the project to create the resource in.<br><br>If it is not provided, the provider project is used.<br><br>ex:<pre>anyscale_project_id = "project-1234567890"</pre> | `string` | `null` | no |
-| <a name="input_default_ingress_cidr_range"></a> [default\_ingress\_cidr\_range](#input\_default\_ingress\_cidr\_range) | (Optional) List of IPv4 cidr ranges to default to if a specific mapping isn't provided.<br><br>ex:<pre>default_ingress_cidr_range = ["32.0.10.0/24","32.32.10.9/32"]</pre> | `list(string)` | `[]` | no |
-| <a name="input_enable_firewall_rule_logging"></a> [enable\_firewall\_rule\_logging](#input\_enable\_firewall\_rule\_logging) | (Optional) Determines whether to enable logging for firewall rules.<br><br>ex:<pre>enable_firewall_rule_logging = true</pre> | `bool` | `true` | no |
-| <a name="input_firewall_policy_description"></a> [firewall\_policy\_description](#input\_firewall\_policy\_description) | (Optional) The description of the firewall policy.<br><br>ex:<pre>firewall_policy_description = "Anyscale VPC Firewall Policy"</pre> | `string` | `"Anyscale VPC Firewall Policy"` | no |
-| <a name="input_firewall_policy_name"></a> [firewall\_policy\_name](#input\_firewall\_policy\_name) | (Optional) The name of the firewall policy.<br><br>If left `null`, the firewall name will default to the vpc name.<br><br>ex:<pre>firewall_policy_name = "anyscale-vpc-firewall-policy"</pre> | `string` | `null` | no |
-| <a name="input_ingress_from_cidr_map"></a> [ingress\_from\_cidr\_map](#input\_ingress\_from\_cidr\_map) | (Optional) List of ingress rules to create with cidr ranges.<br>This can use rules from `predefined_firewall_rules` or custom rules.<br>ex:<pre>ingress_from_cidr_map = [<br>  {<br>    rule        = "https-443-tcp"<br>    cidr_blocks = "10.100.10.10/32"<br>  },<br>  { rule = "nfs-tcp" },<br>  {<br>    ports       = "10-20"<br>    protocol    = "tcp"<br>    description = "Service name is TEST"<br>    cidr_blocks = "10.100.10.10/32"<br>  }<br>]</pre>Default is an empty list. | `list(map(string))` | `[]` | no |
-| <a name="input_ingress_from_gcp_health_checks"></a> [ingress\_from\_gcp\_health\_checks](#input\_ingress\_from\_gcp\_health\_checks) | (Optional) List of ingress rules to create to allow GCP health check probes.<br><br>This only uses rules from `predefined_firewall_rules`.<br>More information on GCP health checks can be found here:<br>https://cloud.google.com/load-balancing/docs/health-check-concepts#ip-ranges<br><br>ex:<pre>ingress_from_gcp_health_checks = [<br>  {<br>    rule        = "health-checks"<br>    cidr_blocks = "35.191.0.0/16, 130.211.0.0/22"<br>  }<br>]</pre> | `list(map(string))` | <pre>[<br>  {<br>    "cidr_blocks": "35.191.0.0/16,130.211.0.0/22",<br>    "rule": "health-checks"<br>  }<br>]</pre> | no |
-| <a name="input_ingress_with_self_cidr_range"></a> [ingress\_with\_self\_cidr\_range](#input\_ingress\_with\_self\_cidr\_range) | (Optional) List of CIDR range to default to if a specific mapping isn't provided.<br><br>ex:<pre>ingress_with_self_cidr_range = ["10.10.0.0/16","10.20.0.0/16"]</pre> | `list(string)` | `[]` | no |
-| <a name="input_ingress_with_self_map"></a> [ingress\_with\_self\_map](#input\_ingress\_with\_self\_map) | (Optional) List of ingress rules to create where 'self' is defined.<br><br>Default rule is `all-all` as this firewall rule is used for all Anyscale resources.<br><br>ex:<pre>ingress_with_self_map = [<br>  {<br>    rule        = "https-443-tcp"<br>  },<br>  {<br>    rule        = "http-80-tcp"<br>  },<br>  {<br>    rule        = "ssh-tcp"<br>  },<br>  {<br>    rule        = "nfs-tcp"<br>  }<br>]</pre> | `list(map(string))` | <pre>[<br>  {<br>    "rule": "all-all"<br>  }<br>]</pre> | no |
-| <a name="input_module_enabled"></a> [module\_enabled](#input\_module\_enabled) | (Optional) Determines whether to create the resources inside this module.<br><br>ex:<pre>module_enabled = true</pre> | `bool` | `true` | no |
-| <a name="input_predefined_firewall_rules"></a> [predefined\_firewall\_rules](#input\_predefined\_firewall\_rules) | (Required) Map of predefined firewall rules. | `map(list(any))` | <pre>{<br>  "all-all": [<br>    "",<br>    "all",<br>    "All protocols",<br>    1000<br>  ],<br>  "health-checks": [<br>    8000,<br>    "tcp",<br>    "Health Checks",<br>    1005<br>  ],<br>  "http-80-tcp": [<br>    80,<br>    "tcp",<br>    "HTTP",<br>    1001<br>  ],<br>  "https-443-tcp": [<br>    443,<br>    "tcp",<br>    "HTTPS",<br>    1002<br>  ],<br>  "nfs-tcp": [<br>    2049,<br>    "tcp",<br>    "NFS/EFS",<br>    1004<br>  ],<br>  "ssh-tcp": [<br>    22,<br>    "tcp",<br>    "SSH",<br>    1003<br>  ]<br>}</pre> | no |
-| <a name="input_vpc_id"></a> [vpc\_id](#input\_vpc\_id) | (Required) The ID of the VPC to apply the Firewall Policy to.<br><br>ex:<pre>vpc_id = "projects/anyscale/global/networks/anyscale-network"</pre> | `string` | n/a | yes |
-| <a name="input_vpc_name"></a> [vpc\_name](#input\_vpc\_name) | (Required) The name of the VPC to apply the Firewall Policy to.<br><br>ex:<pre>vpc_name = "anyscale-vpc"</pre> | `string` | n/a | yes |
+| <a name="input_anyscale_project_id"></a> [anyscale\_project\_id](#input\_anyscale\_project\_id) | (Optional) The ID of the project to create the resource in.<br/><br/>If it is not provided, the provider project is used.<br/><br/>ex:<pre>anyscale_project_id = "project-1234567890"</pre> | `string` | `null` | no |
+| <a name="input_default_ingress_cidr_range"></a> [default\_ingress\_cidr\_range](#input\_default\_ingress\_cidr\_range) | (Optional) List of IPv4 cidr ranges to default to if a specific mapping isn't provided.<br/><br/>ex:<pre>default_ingress_cidr_range = ["32.0.10.0/24","32.32.10.9/32"]</pre> | `list(string)` | `[]` | no |
+| <a name="input_enable_firewall_rule_logging"></a> [enable\_firewall\_rule\_logging](#input\_enable\_firewall\_rule\_logging) | (Optional) Determines whether to enable logging for firewall rules.<br/><br/>ex:<pre>enable_firewall_rule_logging = true</pre> | `bool` | `true` | no |
+| <a name="input_firewall_policy_description"></a> [firewall\_policy\_description](#input\_firewall\_policy\_description) | (Optional) The description of the firewall policy.<br/><br/>ex:<pre>firewall_policy_description = "Anyscale VPC Firewall Policy"</pre> | `string` | `"Anyscale VPC Firewall Policy"` | no |
+| <a name="input_firewall_policy_name"></a> [firewall\_policy\_name](#input\_firewall\_policy\_name) | (Optional) The name of the firewall policy.<br/><br/>If left `null`, the firewall name will default to the vpc name.<br/><br/>ex:<pre>firewall_policy_name = "anyscale-vpc-firewall-policy"</pre> | `string` | `null` | no |
+| <a name="input_ingress_from_cidr_map"></a> [ingress\_from\_cidr\_map](#input\_ingress\_from\_cidr\_map) | (Optional) List of ingress rules to create with cidr ranges.<br/>This can use rules from `predefined_firewall_rules` or custom rules.<br/>ex:<pre>ingress_from_cidr_map = [<br/>  {<br/>    rule        = "https-443-tcp"<br/>    cidr_blocks = "10.100.10.10/32"<br/>  },<br/>  { rule = "nfs-tcp" },<br/>  {<br/>    ports       = "82-84"<br/>    protocol    = "tcp"<br/>    description = "Service name is TEST"<br/>    cidr_blocks = "10.100.10.12/32"<br/>  }<br/>]</pre>Default is an empty list. | `list(map(string))` | `[]` | no |
+| <a name="input_ingress_from_gcp_health_checks"></a> [ingress\_from\_gcp\_health\_checks](#input\_ingress\_from\_gcp\_health\_checks) | (Optional) List of ingress rules to create to allow GCP health check probes.<br/><br/>This only uses rules from `predefined_firewall_rules`.<br/>More information on GCP health checks can be found here:<br/>https://cloud.google.com/load-balancing/docs/health-check-concepts#ip-ranges<br/><br/>ex:<pre>ingress_from_gcp_health_checks = [<br/>  {<br/>    rule        = "health-checks"<br/>    cidr_blocks = "35.191.0.0/16, 130.211.0.0/22"<br/>  }<br/>]</pre> | `list(map(string))` | <pre>[<br/>  {<br/>    "cidr_blocks": "35.191.0.0/16,130.211.0.0/22",<br/>    "rule": "health-checks"<br/>  }<br/>]</pre> | no |
+| <a name="input_ingress_with_self_cidr_range"></a> [ingress\_with\_self\_cidr\_range](#input\_ingress\_with\_self\_cidr\_range) | (Optional) List of CIDR range to default to if a specific mapping isn't provided.<br/><br/>ex:<pre>ingress_with_self_cidr_range = ["10.10.0.0/16","10.20.0.0/16"]</pre> | `list(string)` | `[]` | no |
+| <a name="input_ingress_with_self_map"></a> [ingress\_with\_self\_map](#input\_ingress\_with\_self\_map) | (Optional) List of ingress rules to create where 'self' is defined.<br/><br/>Default rule is `all-all` as this firewall rule is used for all Anyscale resources.<br/><br/>ex:<pre>ingress_with_self_map = [<br/>  {<br/>    rule        = "https-443-tcp"<br/>  },<br/>  {<br/>    rule        = "http-80-tcp"<br/>  },<br/>  {<br/>    rule        = "ssh-tcp"<br/>  },<br/>  {<br/>    rule        = "nfs-tcp"<br/>  }<br/>]</pre> | `list(map(string))` | <pre>[<br/>  {<br/>    "rule": "all-all"<br/>  }<br/>]</pre> | no |
+| <a name="input_module_enabled"></a> [module\_enabled](#input\_module\_enabled) | (Optional) Determines whether to create the resources inside this module.<br/><br/>ex:<pre>module_enabled = true</pre> | `bool` | `true` | no |
+| <a name="input_predefined_firewall_rules"></a> [predefined\_firewall\_rules](#input\_predefined\_firewall\_rules) | (Required) Map of predefined firewall rules. | `map(list(any))` | <pre>{<br/>  "all-all": [<br/>    "",<br/>    "all",<br/>    "All protocols",<br/>    1000<br/>  ],<br/>  "health-checks": [<br/>    8000,<br/>    "tcp",<br/>    "Health Checks",<br/>    1005<br/>  ],<br/>  "http-80-tcp": [<br/>    80,<br/>    "tcp",<br/>    "HTTP",<br/>    1001<br/>  ],<br/>  "https-443-tcp": [<br/>    443,<br/>    "tcp",<br/>    "HTTPS",<br/>    1002<br/>  ],<br/>  "nfs-tcp": [<br/>    2049,<br/>    "tcp",<br/>    "NFS/EFS",<br/>    1004<br/>  ],<br/>  "ssh-tcp": [<br/>    22,<br/>    "tcp",<br/>    "SSH",<br/>    1003<br/>  ]<br/>}</pre> | no |
+| <a name="input_vpc_id"></a> [vpc\_id](#input\_vpc\_id) | (Required) The ID or SelfLink of the VPC to apply the Firewall Policy to.<br/><br/>ex:<pre>vpc_id = "projects/anyscale/global/networks/anyscale-network"</pre> | `string` | n/a | yes |
+| <a name="input_vpc_name"></a> [vpc\_name](#input\_vpc\_name) | (Required) The name of the VPC to apply the Firewall Policy to.<br/><br/>ex:<pre>vpc_name = "anyscale-vpc"</pre> | `string` | n/a | yes |
 
 ## Outputs
 
@@ -61,7 +61,7 @@ No modules.
 | <a name="output_vpc_firewall_policy_networkfirewallpolicyid"></a> [vpc\_firewall\_policy\_networkfirewallpolicyid](#output\_vpc\_firewall\_policy\_networkfirewallpolicyid) | The Google VPC firewall policy network firewall policy id. |
 | <a name="output_vpc_firewall_policy_selflink"></a> [vpc\_firewall\_policy\_selflink](#output\_vpc\_firewall\_policy\_selflink) | The Google VPC firewall policy self link. |
 | <a name="output_vpc_firewall_policy_selflink_withid"></a> [vpc\_firewall\_policy\_selflink\_withid](#output\_vpc\_firewall\_policy\_selflink\_withid) | The Google VPC firewall policy self link with id. |
-<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
+<!-- END_TF_DOCS -->
 
 <!-- References -->
 [Terraform]: https://www.terraform.io

modules/google-anyscale-vpc-firewall/examples/README.md

@@ -22,7 +22,7 @@ This builds a VPC by passing in as many variables as possible.
 ## test_no_resources
 This should NOT build any cloudstorage resources and is here for unit testing.
 
-<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
+<!-- BEGIN_TF_DOCS -->
 ## Requirements
 
 | Name | Version |
@@ -71,7 +71,7 @@ No resources.
 | <a name="output_kitchen_sink_name"></a> [kitchen\_sink\_name](#output\_kitchen\_sink\_name) | kitchen\_sink name |
 | <a name="output_kitchen_sink_selflink"></a> [kitchen\_sink\_selflink](#output\_kitchen\_sink\_selflink) | kitchen\_sink self link |
 | <a name="output_test_no_resources"></a> [test\_no\_resources](#output\_test\_no\_resources) | The outputs of the no\_resource resource - should be empty |
-<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
+<!-- END_TF_DOCS -->
 
 <!-- References -->
 [Terraform]: https://www.terraform.io

modules/google-anyscale-vpc-firewall/examples/main.tf

@@ -28,7 +28,7 @@ module "all_defaults" {
   module_enabled = true
 
   vpc_name = module.all_defaults_vpc.vpc_name
-  vpc_id   = module.all_defaults_vpc.vpc_id
+  vpc_id   = module.all_defaults_vpc.vpc_selflink
   # vpc_self_link       = module.all_defaults_vpc.vpc_selflink
 }
 
@@ -52,7 +52,7 @@ module "anyscale_firewall_public" {
   module_enabled = true
 
   vpc_name = module.anyscale_firewall_public_vpc.vpc_name
-  vpc_id   = module.anyscale_firewall_public_vpc.vpc_id
+  vpc_id   = module.anyscale_firewall_public_vpc.vpc_selflink
 
   ingress_with_self_cidr_range = [local.public_subnet_cidr]
   ingress_from_cidr_map = [
@@ -87,7 +87,7 @@ module "anyscale_firewall_private" {
   module_enabled = true
 
   vpc_name = module.anyscale_firewall_private_vpc.vpc_name
-  vpc_id   = module.anyscale_firewall_private_vpc.vpc_id
+  vpc_id   = module.anyscale_firewall_private_vpc.vpc_selflink
 
   default_ingress_cidr_range = [var.default_ingress_cidr_range]
   ingress_from_cidr_map = [
@@ -129,7 +129,7 @@ module "kitchen_sink" {
   anyscale_project_id = var.google_project_id
 
   vpc_name = module.kitchen_sink_vpc.vpc_name
-  vpc_id   = module.kitchen_sink_vpc.vpc_id
+  vpc_id   = module.kitchen_sink_vpc.vpc_selflink
 
   firewall_policy_name         = "anyscale-tf-kitchensink-policy"
   firewall_policy_description  = "This is the Anyscale Kitchen Sink Policy"
@@ -144,7 +144,23 @@ module "kitchen_sink" {
 
   default_ingress_cidr_range = compact(concat(["10.100.10.10/32", "10.100.10.11/32"], [var.default_ingress_cidr_range]))
   ingress_from_cidr_map = [
-    { rule = "nfs-tcp" }
+    {
+      rule        = "https-443-tcp"
+      cidr_blocks = "10.100.10.10/32"
+    },
+    { rule = "nfs-tcp" },
+    # {
+    #   ports       = "10,20,30"
+    #   protocol    = "tcp"
+    #   description = "Service name is TEST"
+    #   cidr_blocks = "10.100.10.11/32"
+    # },
+    {
+      ports       = "82-84"
+      protocol    = "tcp"
+      description = "Service name is TEST"
+      cidr_blocks = "10.100.10.12/32"
+    }
   ]
 
 }

modules/google-anyscale-vpc-firewall/main.tf

@@ -93,7 +93,6 @@ resource "google_compute_network_firewall_policy_rule" "ingress_allow_from_cidr_
   count   = local.ingress_from_cidr_blocks_enabled ? length(var.ingress_from_cidr_map) : 0
   project = var.anyscale_project_id
 
-  # name        = "${local.computed_anyscale_vpcname}-ingress-allow-from-cidr-blocks-${count.index}"
   description     = lookup(var.ingress_from_cidr_map[count.index], "description", "Ingress Rule")
   direction       = "INGRESS"
   action          = "allow"
@@ -128,7 +127,7 @@ resource "google_compute_network_firewall_policy_rule" "ingress_allow_from_cidr_
           "tcp"
         )
       )
-      ports = var.predefined_firewall_rules[lookup(var.ingress_from_cidr_map[count.index], "rule", "_")][0] == "" ? null : tolist([
+      ports = lookup(var.ingress_from_cidr_map[count.index], "ports", "") == "" ? null : tolist([
         lookup(
           var.ingress_from_cidr_map[count.index],
           "ports",
@@ -140,14 +139,12 @@ resource "google_compute_network_firewall_policy_rule" "ingress_allow_from_cidr_
       ])
     }
   }
-
 }
 
 resource "google_compute_network_firewall_policy_rule" "ingress_allow_from_gcp_health_checks" {
   count   = local.ingress_from_gcp_health_checks ? length(var.ingress_from_gcp_health_checks) : 0
   project = var.anyscale_project_id
 
-  # name        = "${local.computed_anyscale_vpcname}-ingress-allow-from-cidr-blocks-${count.index}"
   description     = lookup(var.ingress_from_gcp_health_checks[count.index], "description", "Ingress Rule")
   direction       = "INGRESS"
   action          = "allow"
@@ -182,9 +179,9 @@ resource "google_compute_network_firewall_policy_rule" "ingress_allow_from_gcp_h
           "tcp"
         )
       )
-      ports = var.predefined_firewall_rules[lookup(var.ingress_from_gcp_health_checks[count.index], "rule", "_")][0] == "" ? null : tolist([
+      ports = lookup(var.ingress_from_gcp_health_checks[count.index], "ports", "") == "" ? null : tolist([
         lookup(
-          var.ingress_from_cidr_map[count.index],
+          var.ingress_from_gcp_health_checks[count.index],
           "ports",
           try(
             var.predefined_firewall_rules[lookup(var.ingress_from_gcp_health_checks[count.index], "rule", "_")][0],
@@ -194,5 +191,4 @@ resource "google_compute_network_firewall_policy_rule" "ingress_allow_from_gcp_h
       ])
     }
   }
-
 }

modules/google-anyscale-vpc-firewall/variables.tf

@@ -16,7 +16,7 @@ variable "vpc_name" {
 
 variable "vpc_id" {
   description = <<-EOT
-    (Required) The ID of the VPC to apply the Firewall Policy to.
+    (Required) The ID or SelfLink of the VPC to apply the Firewall Policy to.
 
     ex:
     ```
@@ -213,10 +213,10 @@ variable "ingress_from_cidr_map" {
       },
       { rule = "nfs-tcp" },
       {
-        ports       = "10-20"
+        ports       = "82-84"
         protocol    = "tcp"
         description = "Service name is TEST"
-        cidr_blocks = "10.100.10.10/32"
+        cidr_blocks = "10.100.10.12/32"
       }
     ]
     ```