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

Update subnet_flow_logs and subnet_private_access to strings from boolean #13

Merged
merged 1 commit into from
Sep 26, 2018

Conversation

andreyk-code
Copy link
Contributor

No description provided.

@@ -98,4 +124,30 @@ module "test-vpc-module-02" {

test-network-02-subnet-02 = []
}

fw_rules_ingress_with_tags = [
Copy link
Contributor

@morgante morgante Sep 25, 2018

Choose a reason for hiding this comment

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

@andreyk-code Is it possible to have a single fw_rules variable? This seems suboptimal.

Copy link
Contributor Author

@andreyk-code andreyk-code Sep 25, 2018

Choose a reason for hiding this comment

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

I don't think so. Trying to control the firewall type within the actual resource is difficult with current interpolation syntax. Since the firewall rule resource uses different inputs depending on if it's an ingress/egress and if it uses as network tags or service_accounts. It can't have both source ranges/destination ranges or accounts/tags, and sending a null resource isn't possible. This removes ability for such logic:

  fw_rules = [
    {
      rule_name       = "allow-icmp"
      type = "service_account"
      direction = "egress"
      ...
    }
]

Doing it through count is challenging since we can't nullify the resource with "0" based off previous logic. Something like this would error since we can't track count outside of the resource:

  count = "${lookup(var.fw_rules_ingress_with_tags[count.index], "type", "") == "tags,ingress" ? "${var.fw_rules_ingress_with_tags}" : 0 }"

Maybe it's not worth trying to add firewall rules until v0.12 is released? It's fairly convoluted and almost simpler to just create independent firewall resources in the current version. In 0.12, it will become way cleaner since we can null out inputs:

  fw_rules_allow = [
    {
      rule_name     = "example-rule-01"
      direction     = "INGRESS"
      target_tags   = "example test"
      source_ranges = "0.0.0.0/0"

      icmp = true
      tcp  = true

      tcp_ports = "22 80"
    },
  ]
resource "google_compute_firewall" "allow-rule" {
  count = "${length(var.fw_rules_allow)}"
  ...
  direction               = "${lookup(var.fw_rules_allow[count.index], "direction")}"
  source_ranges           = "${lookup(var.fw_rules_allow[count.index], "direction") == "INGRESS" ? split(" ", lookup(var.fw_rules_allow[count.index], "source_ranges", " ")) : null}"
  destination_ranges      = "${lookup(var.fw_rules_allow[count.index], "direction") == "EGRESS" ? split(" ", lookup(var.fw_rules_allow[count.index], "destination_ranges", " ")) : null}"

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's not worth trying to add firewall rules until v0.12 is released?

Yeah, I think that might end up being easiest.

Could you separate the firewall support into its own PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, pulled out firewall support

@andreyk-code andreyk-code changed the title Add firewall support and update subnet_flow_logs, subnet_private_access to strings from boolean Update subnet_flow_logs and subnet_private_access to strings from boolean Sep 25, 2018
@andreyk-code
Copy link
Contributor Author

Updated PR title from "Add firewall support and update subnet_flow_logs, subnet_private_access to strings from boolean" to "Update subnet_flow_logs and subnet_private_access to strings from boolean"

@morgante morgante merged commit ae39351 into terraform-google-modules:master Sep 26, 2018
@Jeanno Jeanno self-requested a review October 11, 2019 15:12
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.

2 participants