diff --git a/docs/changes.md b/docs/changes.md new file mode 100644 index 0000000..5dd9673 --- /dev/null +++ b/docs/changes.md @@ -0,0 +1,27 @@ +# Changes & Updates to the Standards + +Our development standards and best practices are always evolving. This repository is the source of truth for the +standards and best practices we use at Turo. This document is a changelog of the changes and updates to the standards as +well as the toll that we use the make these changes. + +If you have a suggestion for a change or update to the standards, please open an issue in this repository. + +Changes are managed through pull requests and merged using +the [Rebase & Merge](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges#rebase-and-merge-your-pull-request-commits) +workflow. + +## Guidelines for changes + +At Turo we believe in convention & prior art over configuration and bespoke creations. To that end, we try to base our +standards and practices off of industry standards and best practices. When making changes to the standards or +substantial existing prior art. + +We use this document to allow us: + +- To clarify how an existing standard might be applied +- To extend an existing pattern where we see a need -- for example: allowing for splitting `variables.tf` into + multiple `variables..tf` files. +- To define best practices where we have found no prior examples or subtantial prior art + +In general, if you are proposing a new change we hope that the change is based on prior art and that you can provide +examples of how the change is being used in the industry. diff --git a/docs/index.md b/docs/index.md index dae6fa2..968ad5c 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1 +1,20 @@ # Turo Terraform Development Standards + +## Baseline standards + +We use the following standards as a baseline for all Terraform code at Turo: + +- [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/) +- [Rebase & Merge](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges#rebase-and-merge-your-pull-request-commits) + workflow +- [pre-commit](https://pre-commit.com/) for linting +- [mkdocs](https://www.mkdocs.org/) for documentation -- For full documentation + visit [mkdocs.org](https://www.mkdocs.org). +- Formatting -- all files are formatted using appropriate language specific tools during the pre-commit phase to ensure + baseline style is consistent. +- Terraform Documentation -- all Terraform code is documented using the [terraform_docs](https://terraform-docs.io/) +- Standards are enforced with code whereever possible. For example, we use [pre-commit](https://pre-commit.com/) to + enforce + formatting and linting standards. +- Scripts to rule them all -- we use [scripts to rule them all](https://github.com/github/scripts-to-rule-them-all) to + provide consistent usage and functionality across all repositories. diff --git a/docs/modules/input-variables.md b/docs/modules/input-variables.md new file mode 100644 index 0000000..efeeaf4 --- /dev/null +++ b/docs/modules/input-variables.md @@ -0,0 +1,10 @@ +# Module: Input Variables + +All module input variables **MUST** be in files named either `variables.tf` or `variables..tf`. + +Developers **MAY** group variables into files by function. For example, if a module has variables that are used for logging, +the developer **MAY** choose to group those variables in a file named `variables.logging.tf`. + +When module is to be sourced from outside the current GitHub repo, its variables **MUST** have a description. + +When variables are expected to be used as inputs to other modules, developers **SHOULD** attempt to structure them in a diff --git a/docs/modules/outputs.md b/docs/modules/outputs.md new file mode 100644 index 0000000..ce97e34 --- /dev/null +++ b/docs/modules/outputs.md @@ -0,0 +1,12 @@ +# Module: Outputs + +All module outputs **MUST** be in files named either `outputs.tf` or `outputs..tf`. + +Developer **MAY** choose to group outputs by function. For example, if a module has outputs that are used for logging, +the developer **MAY** choose to group those outputs in a file named `outputs.logging.tf`. + +When module is to be sourced from outside the current GitHub repo, its outputs **MUST** have a description. + +When outputs are expected to be used as inputs to other modules, developers **SHOULD** attempt to structure them in a +way that makes it easy to understand what the output is and how it should be used and can easily be used as an input to +another module. diff --git a/docs/sources.md b/docs/sources.md new file mode 100644 index 0000000..99ffe60 --- /dev/null +++ b/docs/sources.md @@ -0,0 +1,4 @@ +# Sources of Inspiration + +- https://developer.hashicorp.com/terraform/language +- https://www.terraform-best-practices.com/ diff --git a/docs/style/conditional-resource-creation.md b/docs/style/conditional-resource-creation.md new file mode 100644 index 0000000..d3ed82a --- /dev/null +++ b/docs/style/conditional-resource-creation.md @@ -0,0 +1,96 @@ +# Conditional Resource Creation + +When you have to create a resource conditionally based on a boolean or some other binary value, opt for the count +control block for most cases. The most cases here mean, a simple conditional creation of an isolated resource which may +have just one or two references as inputs for other resources for example + +```hcl +# example for using count block for conditionally creating a resource +data "aws_iam_policy_document" "this" { + count = local.iam_role_enabled ? 1 : 0 + + statement { + configurations = "..." + } +} + +resource "aws_iam_policy" "this" { + name = var.name + path = "/" + description = "IAM policy for..." + policy = data.aws_iam_policy_document.this[0].json + tags = var.tags +} + +``` + +Avoid count block if you have multiple resources created using the same binary condition and in such cases favor +for_each block (or even better consider modularizing the portion of resources that are conditionally created using the +same binary value) + +```hcl +# example for_each use case where multiple resources are dependant on one single +# binary condition and chain referenced using the same key consistently across. If +# we had used count block, the references then would have to have used indexes which +# are hard to understand. + +locals { + iam_resources = toset(var.iam_role_enabled ? ["resource_name"] : []) +} + +data "aws_kms_key" "this" { + for_each = iam_resources + + key_id = var.kms_key_id +} + +data "aws_iam_policy_document" "this" { + for_each iam_resources + + statement { + sid = "statement identifier..." + effect = "Allow" + actions = ["kms:Decrypt"] + resources = [data.aws_kms_key.this[each.key].arn] + } +} + +resource "aws_iam_policy" "this" { + for_each = iam_resources + + name = var.name + path = "/" + description = "IAM policy for..." + policy = data.aws_iam_policy_document.this[ + each.key + ].json + + tags = var.tags +} + +``` + +One more thing to consider is before going with one of the above solution is to check whether we can group the +conditionally created resources together as an internal module. Are there enough resources that are related to each +other and created using the same binary condition/check? If we go with the option to create an internal module, the +whole of it is instantiated behind the one binary condition and each individual resource in it don’t have to be guarded +with the checks. If the module is enabled, the whole group of resources under it are created. If the module is not +enabled none are created. In this scenario, go with count block for enabling or disabling the module. for_each is not +preferred for the module since we are dealing with one single place to enable/disable the creation and count is easier +to comprehend. + +```hcl +# assuming all of the iam related resources from the example above is changed into +# an internal module named "iam_role" (with a directory named "iam_role"), here is +# an example of enabling the module. The resources that make up this module don't +# specify the conditional check individually. + +module "iam_role" { + source = "./iam-role" + + count = local.iam_role_enabled ? 1 : 0 + + configuration = "..." + tags = local.tags +} +``` diff --git a/docs/style/control-blocks.md b/docs/style/control-blocks.md new file mode 100644 index 0000000..000d335 --- /dev/null +++ b/docs/style/control-blocks.md @@ -0,0 +1,13 @@ +# Control blocks: `for_each` and `count` + +Control blocks **MUST** have an empty line after them to make it easy to know that they are a control block rather than +an attribute. + +```hcl +resource "aws_sns_topic_policy" "allow" { + for_each = {for k, v in var.sns_notifications : k => v if var.create_sns_policy} + + arn = each.value.topic_arn + policy = data.aws_iam_policy_document.sns[each.key].json +} +``` diff --git a/docs/style/locals.md b/docs/style/locals.md new file mode 100644 index 0000000..de155f1 --- /dev/null +++ b/docs/style/locals.md @@ -0,0 +1,27 @@ +## Locals & Data + +## Hardcoded values + +Please store hardcoded values as local variables with appropriate names + +### Do this + +```hcl +locals { + segment_aws_identifier = "arn:aws:iam::107630771604:user/s3-copy" # Segment AWS account +} + +principals { + type = "AWS" + identifiers = [local.segment_aws_identifier] +} +``` + +### Not this + +```hcl +principals { + type = "AWS" + identifiers = ["arn:aws:iam::107630771604:user/s3-copy"] +} +``` diff --git a/docs/style/maps.md b/docs/style/maps.md new file mode 100644 index 0000000..509b1fe --- /dev/null +++ b/docs/style/maps.md @@ -0,0 +1,19 @@ +# Maps + +We should prefer using `=` over `:` in maps + +## Good + +```hcl +common_tags = { + Environment = "production" +} +``` + +## Bad + +```hcl +common_tags = { + Environment : "production" +} +``` diff --git a/docs/style/module-sourcing.md b/docs/style/module-sourcing.md new file mode 100644 index 0000000..ab3e3e4 --- /dev/null +++ b/docs/style/module-sourcing.md @@ -0,0 +1,16 @@ +# Module source + +Module source **MUST** have an empty line after them to make it easy to know that they are not attributes. + +If the module has a `for_each` or `count` the source **MUST** be the first attribute and there should be a newline +between the source and the `for_each`/`count` and that control statement **MUST** have a newline after it. + +```hcl +module "wrapper" { + source = "../" + + for_each = var.items + + create_bucket = try(each.value.create_bucket, var.defaults.create_bucket, true) +} +``` diff --git a/docs/style/naming.md b/docs/style/naming.md new file mode 100644 index 0000000..e69de29 diff --git a/docs/test.md b/docs/test.md deleted file mode 100644 index da26d16..0000000 --- a/docs/test.md +++ /dev/null @@ -1,3 +0,0 @@ -# Test 2 - -This is a test! diff --git a/docs/versioning.md b/docs/versioning.md new file mode 100644 index 0000000..4aa8371 --- /dev/null +++ b/docs/versioning.md @@ -0,0 +1,69 @@ +# Versioning + +We have the following guidance for versioning within our repositories. + +## Lock files + +We check in lock files for all our projects. This is to ensure that we are all using the same versions of +dependencies.This is especially important for our libraries, as we want to ensure that the version of a library that we +are using is the same version that our consumers are using. + +We lock two platform: + +1. `linux_amd64` for our CI & Terraform Cloud agent platform +2. `darwin_amd64` for our local development platform -- note that `arm` support is not consistent enough for us to use + that for local development. That may change in the future. + +Locking can be performed with the following command: + +```hcl +terraform -chdir = "$TERRAFORM_DIR" providers lock -platform = darwin_amd64 -platform = linux_amd64 +``` + +Where `$TERRAFORM_DIR` is the directory containing the Terraform code. + +## Provider Version Constraints + +Here are the conventions we use when specifying the version of a given Terraform provider. The thought is that by having +only minimum versions specified in our Terraform modules, we can update the provider version in a Terraform workspace +without having to release new versions of our Terraform modules. + +In general, when using version constraint pinning syntax, pin to the major version. e.g. `version = "~> 3.5"`. + +`version = ">= 5.0.0, <6.0.0"` + +See here for details for how Terraform interprets version constraints. + +## Terraform Workspaces + +Use major version pinning here. Use the ~> syntax to allow the rightmost version number to vary as new provider releases +become available. e.g. `version = "~> 3.5"` which indicates any version is acceptable from the `3.x` series, as long as the +version is `3.5` or higher. + +## Terraform Modules + +Use the `>=` syntax to allow the version specified to be a minimum. e.g. `version = ">= 3.5.0"` which indicates version +`3.5.0` or later, including `3.6`, `4.0`, etc. + +This is true unless we know that the module will not be compatible with a future version of the provider. + +## Tests within Terraform Modules + +Use major version pinning here. Use the `~>` syntax to allow the rightmost version number to vary as new provider releases +become available. e.g. `version = "~> 3.5"` which indicates any version is acceptable from the `3.x` series, as long as the +version is `3.5` or higher. + +## CI Validation + +We expect that repos will perform the following linting steps during the pre-commit and CI process. + +```hcl + - repo: https://github.com/antonbabenko/pre-commit-terraform + rev: v1.77.1 + hooks: + - id: terraform_providers_lock + stages: [commit] + args: + - --args=-platform=darwin_amd64 + - --args=-platform=linux_amd64 +```