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

Add firewall support safer-cluster modules #570

Merged
merged 7 commits into from
Jun 23, 2020

Conversation

Dev25
Copy link
Contributor

@Dev25 Dev25 commented Jun 20, 2020

Closes #569

Signed-off-by: Dev <Dev25@users.noreply.github.com>
@Dev25 Dev25 requested review from bharathkkb, Jberlinsky and a team as code owners June 20, 2020 18:15
Signed-off-by: Dev <Dev25@users.noreply.github.com>
Signed-off-by: Dev <Dev25@users.noreply.github.com>
Comment on lines +51 to +55
{% if not private_cluster %}
depends_on = [
google_container_cluster.primary,
]
{% endif %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Private clusters have a pre defined master cidr so no need to wait for cluster creation to determine master ip range.

bharathkkb
bharathkkb previously approved these changes Jun 20, 2020
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

LGTM
If you have bandwidth a test would be great, I did a quick scan but didnt see any existing FW tests for any of the main modules.

Signed-off-by: Dev <Dev25@users.noreply.github.com>
@Dev25
Copy link
Contributor Author

Dev25 commented Jun 20, 2020

@bharathkkb Never added a new inspec test before, I've added a firewall existence check based on another tf module test

@bharathkkb
Copy link
Member

@Dev25 I think we need a new verifier system block in .kitchen.yml for using inspec.

  - name: "safer_cluster"
    driver:
      root_module_directory: test/fixtures/safer_cluster
    verifier:
      systems:
        - name: safer_cluster
          backend: local
        - name: inspec-gcp
          backend: gcp
          controls:
            - network

Signed-off-by: Dev <Dev25@users.noreply.github.com>
@Dev25
Copy link
Contributor Author

Dev25 commented Jun 20, 2020

Right thanks! Hopefully that does the trick.

@bharathkkb
Copy link
Member

bharathkkb commented Jun 20, 2020

@Dev25 I just took a look at another test we have in this module and it looks like we need another small tweak, to explicitly say only the gcloud control should use local backend. PFA new version.

  - name: "safer_cluster"
    driver:
      root_module_directory: test/fixtures/safer_cluster
    verifier:
      systems:
        - name: safer_cluster
          backend: local
          controls:
            - gcloud
        - name: inspec-gcp
          backend: gcp
          controls:
            - network

Signed-off-by: Dev <Dev25@users.noreply.github.com>
control "network" do
title "gcp network configuration"
describe google_compute_firewalls(project: project_id) do
its('firewall_names') { should include "gke-#{cluster_name[0..25]}-intra-cluster-egress" }
Copy link
Member

Choose a reason for hiding this comment

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

Not a ruby expert but looking at this [1] wont we need to discard the last char as we need length to be at most 25chars? So [0...25] or [0,25]
[1]: https://stackoverflow.com/questions/9690801/difference-between-double-dot-and-triple-dot-in-range-generation

Copy link
Member

Choose a reason for hiding this comment

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

@Dev25 Saw this error in CI
Cluster name: safer-cluster-cluster-7yid
Actual FW name when I checked: gke-safer-cluster-cluster-7yi-intra-cluster-egress
What we checked for in the test: gke-safer-cluster-cluster-7yid-intra-cluster-egress

  ×  network: gcp network configuration (2 failed)
     ×  google_compute_firewalls firewall_names should include "gke-safer-cluster-cluster-7yid-intra-cluster-egress"
     expected ["default-allow-icmp", "default-allow-internal", "default-allow-rdp", "default-allow-ssh", "gke-deplo...node-http-hc", "k8s-fw-a2ee97657700f434d9340944b00a5b61", "k8s-fw-ae0f40471c88f42199bb5196a6394f69"] to include "gke-safer-cluster-cluster-7yid-intra-cluster-egress"
     ×  google_compute_firewalls firewall_names should include "gke-safer-cluster-cluster-7yid-webhooks"
     expected ["default-allow-icmp", "default-allow-internal", "default-allow-rdp", "default-allow-ssh", "gke-deplo...node-http-hc", "k8s-fw-a2ee97657700f434d9340944b00a5b61", "k8s-fw-ae0f40471c88f42199bb5196a6394f69"] to include "gke-safer-cluster-cluster-7yid-webhooks"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to [0,25], thanks for helping debug this.

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

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

LGTM
fix for lint test timing out is in #538

@morgante morgante merged commit 7ce3c49 into terraform-google-modules:master Jun 23, 2020
@Dev25 Dev25 deleted the safer-firewall branch June 23, 2020 15:05
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
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.

configure additional firewall rules for a safer-cluster(-update-variant)
3 participants