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

Major refactor for modules and examples #53

Open
4 of 13 tasks
migara opened this issue Jun 27, 2024 · 0 comments
Open
4 of 13 tasks

Major refactor for modules and examples #53

migara opened this issue Jun 27, 2024 · 0 comments
Assignees
Labels
enhancement New feature or request v3
Milestone

Comments

@migara
Copy link
Member

migara commented Jun 27, 2024

This issue tracks all the major refactoring of the examples and modules to make it more readable and usable.

Background: #37

An issue tracker for modules (and examples) refactor.

Introduction

The key points to address when refactoring the modules:

  • Ensure all the cloud resources get created in AWS doesn't have any hardcoded suffixes/prefixes. The name values should be configurable via variables.
  • define types for all variables, even complex ones, use optional keyword
  • bump min supported TF version to 1.5
  • migrate to new documentation format:
    • README in document layout
    • introduction, purpose, usage should be moved to .header.md
    • every variable should have a description, where the 1st sentence should contain a summary
    • when documenting complex types, follow the format described below
    • keep the variables in variables.tf in order that makes sense (see below)
    • keep the description of variables that repeat between example the same, like name or tags (see below for a copy&paste pattern to follow)
  • get rid of lookup/try statements where possible -> rely on default values (for complex variables as well)
  • get rid of boolean variables that do not add any value, like enable_zones - use smart defaults instead
  • simplify or rebuild all locals in main.tf in examples e.g. constructing bootstrap options with GWLB endpoints:
    locals {
    subinterface_gwlb_endpoint_eastwest = { for i, j in var.vmseries : i => join(",", compact(concat(flatten([
    for sk, sv in j.subinterfaces.eastwest : [for k, v in module.gwlbe_endpoint[sv.gwlb_endpoint].endpoints : format("aws-gwlb-associate-vpce:%s@%s", v.id, sv.subinterface)]
    ])))) }
    subinterface_gwlb_endpoint_outbound = { for i, j in var.vmseries : i => join(",", compact(concat(flatten([
    for sk, sv in j.subinterfaces.outbound : [for k, v in module.gwlbe_endpoint[sv.gwlb_endpoint].endpoints : format("aws-gwlb-associate-vpce:%s@%s", v.id, sv.subinterface)]
    ])))) }
    subinterface_gwlb_endpoint_inbound = { for i, j in var.vmseries : i => join(",", compact(concat(flatten([
    for sk, sv in j.subinterfaces.inbound : [for k, v in module.gwlbe_endpoint[sv.gwlb_endpoint].endpoints : format("aws-gwlb-associate-vpce:%s@%s", v.id, sv.subinterface)]
    ])))) }
    plugin_op_commands_with_endpoints_mapping = { for i, j in var.vmseries : i => format("%s,%s,%s,%s", j.bootstrap_options["plugin-op-commands"],
    local.subinterface_gwlb_endpoint_eastwest[i], local.subinterface_gwlb_endpoint_outbound[i], local.subinterface_gwlb_endpoint_inbound[i]) }
    bootstrap_options_with_endpoints_mapping = { for i, j in var.vmseries : i => [
    for k, v in j.bootstrap_options : k != "plugin-op-commands" ? "${k}=${v}" : "${k}=${local.plugin_op_commands_with_endpoints_mapping[i]}"
    ] }
    }
  • adjust all examples to have the same main.tf for groups of deployments e.g. VM-Series , autoscaling, Cloud NGFW, Panorama or others if required
  • adjust all modules & examples to be IPv6 ready
  • be bold, if you see some old code, or you think something can be done better, DO IT (this will be a breaking change anyway)
  • when you work on a module, do fix all examples using that module. Make sure the code is the same across all examples
  • inside module, add a comment with a link to Terraform Registry before a resource block, like this:
    # https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/...
    resource "aws_vpc" "example" {
    name                = "example-vpc"
    ...
  • use a # EXAMPLE \n comment format to indicate a start of a section
  • Remove sets from each modules (ex. natgw, transit_gateway_attachment, etc.)
  • Adjust examples to use map of objects instead of single object
  • Added comments under each locals

Issues per module

(many issues have already been merged to main refactor branch in the old archived repo)

Issues per example

  • TBD

Module Refactor Considerations

  • Reconsider the implementation of set concept in the current modules.
  • Avoid rigid variable construction concepts (for example, implicit specification of variables which means the first time users need to have some domain specific knowledge in order to use the example. For more context fix(examples): Refactor examples for reference architectures #49)
  • Collapse multiple modules into one where it makes sense
  • Keep the module separated as is if the complexity going to increase (example: NLB and ALB modules)
  • Restructure the naming of the outputs (align input and output names where applicable and also keep consistency in the noun usage across modules). This will improve the readability of the code for users

Variables ordering

Some basic principles:

  • ensure the variable descriptions are consistent across all the examples (and modules if applicable)
  • unless a module is constructed in a different way, variables.tf should be sorted based on logical hierarchical order:
    • name
    • region
  • keep vars related to the same type of resource next to each other, i.e. create_subnets should be next to subnets
  • order the variables, either:
    • based on decision steps when designing infrastructure, i.e. create_subnets 1st, then subnets definition (you have to decide if you create subnets, or do you source them, then you provide the specs, which will differ based on your decision)
    • based on dependencies between resources, i.e. you define SGs and RTs 1st, then Subnets, as in subnets variable we point to already defined SGs and RTs
    • if no other criteria matches, based on importance or usage frequency
  • group the variables into more complex objects where it makes sense and improves module's readability, e.g. variable interfaces grouping all network interface related settings

Keep in mind that his order will be retained in a README.md.

Description format

Follow the example below:

description = <<-EOF
A short, one sentence description of the variable.

Some more detailed description, can be multiple lines.

List of either required or important properties:

- `name`                   - (`string`, required) name of the Network Security Group.
- `some_optional_value`    - (`string`, optional, defaults to `null`) some description.
- `some_complex_property`  - (`map`, optional) A list of objects representing something.
  - `name`                    - (`string`, required) name of the something.
  - `some_number`             - (`number`, optional, defaults to `5`) numeric value.
  - `some_value_1`            - (`string`, required, mutually exclusive with `some_value_2`) some description.
  - `some_value_2`            - (`string`, required, mutually exclusive with `some_value_1`) some description.
  - `some_optional_value`     - (`bool`, optional, defaults to `false`) some description.

List of other, optional properties:

- `less_important_1`    - (`string`, optional, defaults to `null`) some description.
- `less_important_2`    - (`string`, optional, defaults Azure defaults) some description.
- `less_important_3`    - (`string`, optional, defaults to `""`) some description.
- `less_important_4`    - (`list(string)`, optional, defaults to `[]`) some description.

Example:
```hcl
{
  "object_1" = {
    name = "name of object 1"
    .....
  }
}
```
EOF

Common variables

Replace the following variables with these definitions:

variable "name" {
  description = "The name of the Virtual Private Network."
  type        = string
}

variable "tags" {
  description = "The map of tags to assign to all created resources."
  default     = {}
  type        = map(string)
}

Bootstrap options for examples [Done]

The new examples will support the default bootstrap method (user-data) and may be ability to use other bootstrap methods as options (by commenting out certain variables)

@migara migara changed the title Example Refactor Major refactor for modules and examples Jun 27, 2024
@migara migara added the enhancement New feature or request label Jun 27, 2024
@migara migara added this to the v3 milestone Jun 27, 2024
@migara migara added the v3 label Jun 27, 2024
migara pushed a commit that referenced this issue Aug 29, 2024
* remove subnet_set module
* extend vpc module to create subnets and route tables
* simplify subnets creation (not using complex locals from subnet_set module)
* adjust the variables in variables.tf and outputs inoutputs.tf in order that makes sense
* refactor variables e.g. create new variable internet_gateway, which defines object with attributes for IGW creation/usage
* adjust variables description to standard from Major refactor for modules and examples #53
* adjust all examples to new approach with vpc module
* adjust CloudNGFW examples to changes delivered for other examples in fix(examples): Refactor examples for reference architectures #49
* change approach for CloudWatch logs group in CloudNGFW (as the same name PaloAltoCloudNGFW has to be used for both examples, groups was created on account used in workflows and in TFVARS we are not creating new log group, we are reusing existing one)
* added support for all possible attributes for every resource used in module
* README changed to new standard with new file .header.md

Co-authored-by: michalbil <[email protected]>
@sebastianczech sebastianczech removed their assignment Aug 29, 2024
@lstadnik lstadnik mentioned this issue Oct 4, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v3
Projects
None yet
Development

No branches or pull requests

6 participants