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

feat: Firewall policy rule with resource manager tag #1

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

Samir-Cit
Copy link
Owner

@Samir-Cit Samir-Cit commented Oct 2, 2023

Hello folks.

This PR is regarding the usage of resource manager tags for firewall policy rules and on VMs.

Two new resource manager tags created for the two firewall rules that were refactored, allowing IAP SSH and IAP RDP. The peering network project was used as an example, creating a new sub network on the peered network to be used by the VM.

project = module.peering_project.project_id
}

resource "google_compute_network_firewall_policy_rule" "allow_iap_ssh" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a module for this type of firewall rule?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

PR 514 will fix the issue.

Choose a reason for hiding this comment

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

@Samir-Cit since network firewall policy module is fixed is it possible to call the module instead of resource?

@Samir-Cit Samir-Cit changed the title Feat/firewall tags feat: Firewall policy rule with resource manager tag Oct 6, 2023
Copy link

@romanini-ciandt romanini-ciandt left a comment

Choose a reason for hiding this comment

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

Your PR looks really good :) congrats!

Question: Since we are adding new resources (tags, subnets, etc), don't we need to update any .go tests to reflect/expect these new resources?

// (https://github.com/terraform-google-modules/terraform-google-bootstrap/tree/master/modules/tf_cloudbuild_workspace)
// in the 4-projects shared environment of each business unit.
// the repository name is the same key used for the app_infra_pipeline_service_accounts map and the
// roles will be granted to the service account with the same key.

Choose a reason for hiding this comment

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

How about improve a little this comment block? Looks like the written is not super fluid here.
Example: instead of ...permissions to deploy the resources, a Compute Engine instance..., how about ...permissions to deploy a Compute Engine instance...

Also, maybe skipping a line between one idea and another could be easier to understand, instead of multiple non-breaking lines of text.

4-projects/modules/base_env/example_peering_project.tf Outdated Show resolved Hide resolved
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.

5 participants