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

net-vpc-firewall when using yaml rule format fails to generate correct rule with source_tags #1058

Closed
hql898 opened this issue Dec 19, 2022 · 13 comments · Fixed by #1073
Closed
Assignees
Labels
bug Something isn't working on:modules

Comments

@hql898
Copy link

hql898 commented Dec 19, 2022

When configured GCP firewall rule with source_tags in Yaml format as the following, the net-vpc-firewall module always generate additional source_ranges = [0.0.0.0/0]. Same would also happened even I tried to set source_ranges = [].
Was that something I missed? How do I generate rule (in yaml) with source_tags but without source_ranges?

Thank you

============
Input yaml file with the following rule configuration:

source tag setting is not working;
always add sourceRanges 0.0.0.0/0 to the rule in addition to the specified sourceTags

deny-connector-by-src-tag:
  description: test firewall rule using src tag
  direction: INGRESS
  action: deny
  sources: [vpc-connector]
  targets: []
  use_service_accounts: false
  rules:
    - protocol: all
      ports: null
  extra_attributes:
    disabled: false
    priority: 990
    logging: 'INCLUDE_ALL_METADATA'

Describe rule output:

user@cloudshell:~$ gcloud compute firewall-rules describe deny-connector-by-src-tag --project project-1-123456
creationTimestamp: '2022-12-19T14:36:47.854-08:00'
denied:
- IPProtocol: all
description: test firewall rule using src tag
direction: INGRESS
disabled: false
id: '2129710410780989840'
kind: compute#firewall
logConfig:
  enable: true
  metadata: INCLUDE_ALL_METADATA
name: deny-connector-by-src-tag
network: https://www.googleapis.com/compute/v1/projects/project-1-123456/global/networks/myvpc
priority: 990
selfLink: https://www.googleapis.com/compute/v1/projects/project-1-123456/global/firewalls/deny-connector-by-src-tag
sourceRanges:
- 0.0.0.0/0
sourceTags:
- vpc-connector
user@cloudshell:~$

Terraform apply output:

~/Documents/cloud-foundation-fabric-master/xxxx/$terraform apply
google_compute_firewall.rules: Refreshing state... [id=projects/project-1-123456/global/firewalls/test-deny-connector-traffic-by-src-tag]
google_compute_network.vpc_network: Refreshing state... [id=projects/project-1-123456/global/networks/myvpc]
google_compute_subnetwork.mysubnet2: Refreshing state... [id=projects/project-1-123456/regions/us-east5/subnetworks/subnet5]
google_compute_subnetwork.mysubnet1: Refreshing state... [id=projects/project-1-123456/regions/us-central1/subnetworks/subnet1]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the
following symbols:
  + create

Terraform will perform the following actions:

  # module.firewall-rules.google_compute_firewall.custom-rules["deny-connector-by-src-tag"] will be created
  + resource "google_compute_firewall" "custom-rules" {
      + creation_timestamp = (known after apply)
      + description        = "test firewall rule using src tag"
      + destination_ranges = (known after apply)
      + direction          = "INGRESS"
      + disabled           = false
      + enable_logging     = (known after apply)
      + id                 = (known after apply)
      + name               = "deny-connector-by-src-tag"
      + network            = "myvpc"
      + priority           = 990
      + project            = "project-1-123456"
      + self_link          = (known after apply)
      + source_ranges      = [
          + "0.0.0.0/0",
        ]
      + source_tags        = [
          + "vpc-connector",
        ]

      + deny {
          + ports    = []
          + protocol = "all"
        }

      + log_config {
          + metadata = "INCLUDE_ALL_METADATA"
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

module.firewall-rules.google_compute_firewall.custom-rules["deny-connector-by-src-tag"]: Creating...
module.firewall-rules.google_compute_firewall.custom-rules["deny-connector-by-src-tag"]: Still creating... [10s elapsed]
module.firewall-rules.google_compute_firewall.custom-rules["deny-connector-by-src-tag"]: Creation complete after 12s [id=projects/project-1-123456/global/firewalls/deny-connector-by-src-tag]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Outputs:

network = "projects/project-1-123456/global/networks/myvpc"
[Mon Dec 19 17:36:56]
~/Documents/cloud-foundation-fabric-master/xxxx/$
@hql898
Copy link
Author

hql898 commented Dec 19, 2022

Sorry about the appearance of the text.
I just copied/pasted outputs of gcloud and terraform-apply from the terminal not realized that some leading characters came with the outputs would be interpreted as text formatting codes in github.

@juliocc
Copy link
Collaborator

juliocc commented Dec 20, 2022

How do I generate rule (in yaml) with source_tags but without source_ranges?

This is not supported. If you don't specify the range, we automatically use ["0.0.0.0/32"] as the default.

Is this a use case you need?

@hql898
Copy link
Author

hql898 commented Dec 22, 2022

How do I generate rule (in yaml) with source_tags but without source_ranges?

This is not supported. If you don't specify the range, we automatically use ["0.0.0.0/32"] as the default.

Is this a use case you need?

Although source ranges and source tags could be used together in combination. However, each could work independently without the other.

If both are used together, the effective source set would be the union of both (see reference link below).
https://cloud.google.com/vpc/docs/firewalls#:~:text=A%20valid%20source%20combination

For example, if I want to allow inbound traffic from specific servers with tag "mydb" only, I would build an inbound rule with source_tags="mydb" (without using any IP addresses);
If the module automatically adds source_ranges=0.0.0.0/0 to the rule,
this source_ranges addition would defeat the purpose of the original intend of the rule and make it wide open to every one!

Need to find way to suppress auto generation of source_ranges=0.0.0.0/0.

Alternatively, I could use the following generic TF module to generate rule with source_tags without source_ranges (attached below). However, I really prefer to keep every rules with the yaml file(s). It is a bad idea to scatter some rules with yaml format and other (exception) rules with various modules.

resource "google_compute_firewall" "rules" {
  project     = var.project_id
  name        = "deny-connector-traffic-by-src-tag"
  network     = var.network
  description = "deny inbound from VPC Connector by src tag"
  direction   = "INGRESS"
  priority    = 980
  deny {
    protocol  = "ALL"
    ports     = []
  }
  source_tags = ["my-vpc-connector-tag"]
}

@ludoo
Copy link
Collaborator

ludoo commented Dec 22, 2022

Can you please format your comments properly using Markdown syntax? It makes it a lot easier for us to understand what you are saying. I changed yours to show you how it's done.

@ludoo
Copy link
Collaborator

ludoo commented Dec 22, 2022

We discussed it with Julio, and our consensus is that

  • the API interpolates 0/0 when the range is null
  • specifying both range and tag is ANDed, so 0/0 makes no difference to the tag

I am closing this, feel free to reopen if you think our conclusions are not correct.

@hql898
Copy link
Author

hql898 commented Dec 22, 2022

Understood the point of the AND operation.
I just finished another round of independent tests by comparing behaviors of the following rules one after the other:
Test-1 with Rule-1 is to allow ICMP ping from source_tags and source_ranges = 0.0.0.0/0 (created with the net-vpc-firewall module)
Test-2 with Rule-2 is to allow ICMP ping from source_tags ONLY (without source_range = 0.0.0.0/0) (created from the Console)

In theory, both should produce same results but, surprisingly,
Rule-1 allows ping from everyone (worked incorrectly)
Rule-2 allows ping from resources with the source_tags only (worked correctly)

It is either Google's documentation is wrong or there is a bug.
I'll report this finding to Google.

Unfortunately, my plan of using the Yaml file to maintain firewall rules is no longer viable due to this bug. Unless I am willing to configure rules with source_tags by a separate module as outline earlier.
Manually remove source_ranges=0.0.0.0/0 from the Console does work but not feasible when the objective is to use TF to build everything.

Thank you very much your quick responses and enjoy the holidays.
HQL

@ludoo ludoo reopened this Dec 23, 2022
@ludoo
Copy link
Collaborator

ludoo commented Dec 23, 2022

Wow I did not expect this. We'll try a few more tests before tossing in the towel. I reopened this issue to keep track.

@ludoo
Copy link
Collaborator

ludoo commented Dec 23, 2022

Hmmm, I just did a few tests and I'm not seeing the API interpolating 0/0. Guess we might indeed have to change our default.

@ludoo
Copy link
Collaborator

ludoo commented Dec 23, 2022

The behaviour using the resource is

  • if I create a rule with source_ranges null or set to the empty list, 0/0 is not interpolated
  • if I create a rule with 0/0 and then I set it to null, 0/0 stays
  • if I create a rule with 0/0 and then I set it to to the empty list, 0/0 is removed

My proposal is to do this in the module:

  • if source_ranges (or destination_ranges for egress) is null, interpolate 0/0
  • if source_ranges (or destination_ranges for egress) is the empty list, do not interpolate

This mimicks the behaviour of the underlying resource. Would this work for your use case?

@hql898
Copy link
Author

hql898 commented Dec 23, 2022

The behaviour using the resource is

  • if I create a rule with source_ranges null or set to the empty list, 0/0 is not interpolated
  • if I create a rule with 0/0 and then I set it to null, 0/0 stays
  • if I create a rule with 0/0 and then I set it to to the empty list, 0/0 is removed

My proposal is to do this in the module:

  • if source_ranges (or destination_ranges for egress) is null, interpolate 0/0
  • if source_ranges (or destination_ranges for egress) is the empty list, do not interpolate

This mimicks the behaviour of the underlying resource. Would this work for your use case?

Thanks ludoo, I believe your proposal would work.

@ludoo
Copy link
Collaborator

ludoo commented Dec 23, 2022

Thanks a lot for being persistent, and actually I was wrong: the API page is clear on the fact that ranges and tags are ORed. Thie fix is now merged, let me know if you run into more issues.

@hql898
Copy link
Author

hql898 commented Dec 24, 2022

It is working beautifully.
Much appreciated for the quick action in a holiday season.
Have a Happy New Year.

@ludoo
Copy link
Collaborator

ludoo commented Dec 24, 2022

It is working beautifully. Much appreciated for the quick action in a holiday season. Have a Happy New Year.

Fantastic, thanks for confirming and have a nice holiday season!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working on:modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants