I dont like this module #37

Open shadycuz opened 7 months ago

Is your feature request related to a problem?

I had originally wanted to start this out on a positive note. I wanted to talk about how I can see all the hard work that was put into this module. LIke the Readme having badges, the module being published to the GitLab registry and all the tests that are conducted. But I realized the title of the issue was going to betray me, so let me get straight to the point.

This module... it's really bad.

I'm not even sure it's fair to call it a terraform module. It's nothing like 99% of terraform modules I have seen. It doesn't even have a root module. Though I now realize this is by design? Just look at the title "terraform-aws-swfw-modules", it's plural. If we dig even further into the v2.0.0 release message

v2.0.0 is an unusual release in that its primary purpose is to commence new Software Firewall modules where we intend to publish both VM-Series and cloud NGFW related deployment examples and modules in one place.

So it's not a Terraform "module". It's a collection of modules from PaltoAlto. I think putting them in a single repo and publishing them as a single module and not breaking them out into individual modules was a mistake.

But these design choices aren't important if the underlying sub-modules (all 19 of them...) are useful.

To get started I went to the Centralized Design example. Where I had to copy and paste the contents of the main.tf file into my own repo. Which is 429 lines and almost a dozen sub-modules.

I noticed the example deploys a VPC but I need to use my own, so I deleted the VPC module. Then I went to the next module...

module "subnet_sets" {
  for_each = toset(flatten([for _, v in { for vk, vv in var.vpcs : vk => distinct([for sk, sv in vv.subnets : "${vk}-${sv.set}"]) } : v]))
  source   = "../../modules/subnet_set"

  name                = split("-", each.key)[1]
  vpc_id              = module.vpc[split("-", each.key)[0]].id
  has_secondary_cidrs = module.vpc[split("-", each.key)[0]].has_secondary_cidrs
  nacl_associations = {
    for i in flatten([
      for vk, vv in var.vpcs : [
        for sk, sv in vv.subnets :
        {
          az : sv.az,
          nacl_id : lookup(module.vpc[split("-", each.key)[0]].nacl_ids, sv.nacl, null)
        } if sv.nacl != null && each.key == "${vk}-${sv.set}"
    ]]) : i.az => i.nacl_id
  }
  cidrs = {
    for i in flatten([
      for vk, vv in var.vpcs : [
        for sk, sv in vv.subnets :
        {
          cidr : sk,
          subnet : sv
        } if each.key == "${vk}-${sv.set}"
    ]]) : i.cidr => i.subnet
  }
}

What in the world is this? No comments or anything in the code. So I had to go read the Readme for that module, turns out I didn't need it.

Moving on....

locals {
  vpc_routes = flatten(concat([
    for vk, vv in var.vpcs : [
      for rk, rv in vv.routes : {
        subnet_key = rv.vpc_subnet
        to_cidr    = rv.to_cidr
        next_hop_set = (
          rv.next_hop_type == "internet_gateway" ? module.vpc[rv.next_hop_key].igw_as_next_hop_set : (
            rv.next_hop_type == "nat_gateway" ? module.natgw_set[rv.next_hop_key].next_hop_set : (
              rv.next_hop_type == "transit_gateway_attachment" ? module.transit_gateway_attachment[rv.next_hop_key].next_hop_set : (
                rv.next_hop_type == "gwlbe_endpoint" ? module.gwlbe_endpoint[rv.next_hop_key].next_hop_set : null
              )
            )
          )
        )
      }
    ]
  ]))
}

What is this? How am I supposed to modify this to work in my environment?

Describe the solution you'd like

You are going to need to make some MAJOR changes if you want users to actually use this terraform code.

You will need to split these into individual modules where they make sense. For example:
- terraform-aws-vmseries-module
- terraform-aws-panorma-module
- terraform-aws-ngfw-module
  1. Stop using sub-modules for every little component. Follow the lead of some of the most popular terraform modules:
    - https://github.com/terraform-aws-modules/terraform-aws-vpc
    - https://github.com/terraform-aws-modules/terraform-aws-eks
  2. Use a root module and that module should be simple to deploy with an example in the readme.
    - https://github.com/terraform-aws-modules/terraform-aws-vpc?tab=readme-ov-file#usage
    - https://github.com/terraform-aws-modules/terraform-aws-eks/tree/master?tab=readme-ov-file#usage

I think if you make these changes then this repo would actually be useful for endusers, but in its current state... it's unusable for me.

Describe alternatives you've considered.

I thought about forking this repo to fix these issues...

I also thought about asking my boss to just use the AWS firewall.

I also think I could find another terraform module, from another firewall vendor and just use the AMI ID's for PaloAlto.

Additional context

I'm just trying to help 🙃 , I'm that friend that whispers into your ear "Your breath is terrible". Unfortunately, I don't have any gum to provide you, just directions to where you will find it.

@shadycuz in PR #49 I improved the most painful parts with subnets sets and routes. I know it's only some part of that, but we cannot change everything at once.

Nevertheless we are planning our work with complete refactor of our modules, but it will take time.

Thank you for your feedback.

PaloAltoNetworks / terraform-aws-swfw-modules