Support for hierarchical firewall policies #343
Conversation
juliocc
requested review from
bharathkkb,
rjerrems and
a team
as code owners
| target_resources = each.value.target_resources | ||
| target_service_accounts = each.value.target_service_accounts | ||
| enable_logging = each.value.logging | ||
| # preview = each.value.preview |
| enable_logging = each.value.logging | ||
| # preview = each.value.preview | ||
| match { | ||
| # description = each.value.description |
There was a problem hiding this comment.
This looks like it's being used in envs/shared
|
|
||
| // Allow SSH via IAP when using the allow-iap-ssh tag for Linux workloads. | ||
| resource "google_compute_firewall" "allow_iap_ssh" { | ||
| count = var.optional_fw_rules_enabled ? 1 : 0 |
There was a problem hiding this comment.
In shared can we continue to support flags like optional_fw_rules_enabled? Perhaps a merge on rules map for allow-iap-ssh-rdp etc?
There was a problem hiding this comment.
We can do that but it means all environments get the same policy (i.e. can't have an environment without the optional rules and another environment with them)
What do you think?
There was a problem hiding this comment.
I see, in that case perhaps these should be regular FW rules which we can control per env and can override inherited HFW rules. @rjerrems WDYT?
There was a problem hiding this comment.
I think that makes sense to me
| target_resources = null | ||
| logging = var.firewall_policies_enable_logging | ||
| } | ||
| allow-windows-activation = { |
There was a problem hiding this comment.
without the target tags, how would users make use of these rules? Is the default that this will apply to all instances if no target resource or service accounts are supplied?
There was a problem hiding this comment.
I believe it's inherited and will be applied to all unless overridden with a regular FW rule. Some patterns here https://cloud.google.com/vpc/docs/firewall-policies-examples#example_4_configure_organization-wide_and_folder-specific_rules
There was a problem hiding this comment.
These rules (allow-iap-ssh-rdp, allow-windows-activation, allow-google-hbs-and-hcs) would always be applied. The only one that can be overridden is delegate-rfc1918-ingress.
Check out section 7.6.1 of the draft document.
| data.google_active_folder.non-production.name, | ||
| ] | ||
| rules = { | ||
| delegate-rfc1918-ingress = { |
There was a problem hiding this comment.
What are the purpose of these two delegation rules? Is it to allow all internal traffic in both directions prior to VPC firewall rules being evaluated?
There was a problem hiding this comment.
These two rules delegate RFC1918 traffic to lower levels (i.e. we're not making any decision in regards to that traffic here).
Again, check the document for the rationale.
There was a problem hiding this comment.
Ok thanks - just read through it. The doc in 7.6.1 also mentions blocking egress to the internet but I can't see a rule for that here?
| */ | ||
|
|
||
| module "hierarchical_firewall_policy" { | ||
| source = "../../modules/hierarchical_firewall_policy/" |
There was a problem hiding this comment.
It may be worthwhile considering adding this as a submodule of https://github.com/terraform-google-modules/terraform-google-network
@bharathkkb what do you think?
There was a problem hiding this comment.
yes I think thats a good idea, this will be a general ask outside of foundations too.
There was a problem hiding this comment.
I can take care of that.
| logging = false | ||
| } | ||
| allow-iap-ssh-rdp = { | ||
| description = "Always allow SSH and RDP from IAP" |
There was a problem hiding this comment.
In the previous configuration, these rules were optional and disabled by default. https://github.com/terraform-google-modules/terraform-example-foundation/blob/develop/3-networks/envs/development/variables.tf#L107
Are we changing that behaviour here?
There was a problem hiding this comment.
Indeed. Before, SSH/RDP from IAP was optional, whereas now is always enabled.
There was a problem hiding this comment.
Ok cool - just clarifying that is intended with the team as in the previous release we explicitly did the opposite.
|
@rjerrems agreed that a module would be better. I fact, some of the code actually comes from the Hierarchical Firewall module of Cloud Foundation Fabric I think it should be fairly easy to port some of the code back to CFT. Let me know if we want to do that now or if we want to merge this as is. |
I think we can probably merge once we agree on implementation, then have a follow up task to implement as a submodule in the CFT network module & replace the reference here. @bharathkkb WDYT? |
|
ACK. I'll try to get it done today |
|
Rebased and all checks pass. Let me know if you want me to merge. |
|
great thanks @juliocc |
No description provided.