diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000..06acf66 --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,9 @@ +# CODEOWNERS +# +# Each line is a file pattern followed by one or more owners. +# Owners are notified when a pull request modifies files matching the pattern. +# +# Update this file to reflect the current maintaining team or individuals. +# See: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners + +* @datasciencecampus/cloud-enablement-admins diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..a0672dc --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,96 @@ +# Copilot Instructions — Cloud Engineering Standards + +## About this repository + +This is a public guidance and standards repository for cloud engineering at the +Strategic Innovation and Analysis team at ONS. It is published as a Quarto +book to GitHub Pages and is intended for data scientists and engineers who are +beginning their journey into cloud infrastructure. + +Content is written in UK English and should be accurate, clear, and accessible +to someone who is not yet a cloud specialist. + +## Audience + +The primary audience is **data scientists and engineers new to cloud +infrastructure**. Do not assume familiarity with Terraform, cloud networking, or +infrastructure as code concepts. Explain terms when first used, or direct the +reader to the glossary. + +## Tone and style + +- Follow [GDS content design principles](https://www.gov.uk/guidance/content-design/writing-for-gov-uk): + plain English, active voice, short sentences, no jargon without explanation. +- Use UK English spelling (e.g. "authorise" not "authorize", "organisation" not + "organization"). +- Write in the second person (for example, "you should review the plan" or + "check that logging is enabled") rather than the + passive ("it should be checked"). +- Avoid marketing language and unnecessary superlatives. +- Use numbered lists for sequential steps; bullet lists for unordered items. + +## Content format + +- All content pages are Quarto `.qmd` files. +- Use Quarto callout blocks for notes, warnings, tips, and important caveats: + - `:::{.callout-note}` — general supplementary information + - `:::{.callout-tip}` — helpful hints or shortcuts + - `:::{.callout-warning}` — actions that could cause problems if missed + - `:::{.callout-important}` — critical requirements or constraints +- GCP-specific examples should appear in a callout block labelled clearly as + GCP-specific, so the surrounding content remains cloud-agnostic. +- Cross-reference other sections using Quarto cross-reference syntax + (`@sec-label`) rather than bare URLs where possible. +- YAML frontmatter must include at minimum: `title` and `order`. + +## Repository and infrastructure context + +- Terraform examples should align with the structure used in + [datasciencecampus/terraform-template](https://github.com/datasciencecampus/terraform-template): + environments split into `01_sandbox`, `02_dev_nonprod`, `03_stg_prod`, + `04_prd_prod`; reusable code in `modules/`. +- The four environments are: **sandbox**, **non-production**, **pre-production**, + **production**. Use these terms consistently. +- HMG security classifications in scope are `OFFICIAL` and `OFFICIAL-SENSITIVE`. + Do not provide guidance on `SECRET` or above — note that a separate + architecture and assurance conversation is needed. + +## Commit messages + +Use [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/): + +- `docs:` — content additions or corrections +- `feat:` — new chapter, section, or significant new guidance +- `fix:` — factual corrections or broken references +- `chore:` — maintenance (config, CI, dependency updates) +- `refactor:` — restructuring without changing meaning + +The Release Please GitHub Actions workflow uses commit messages to generate the +CHANGELOG and version bumps. +`feat:` produces a minor bump; `fix:` produces a patch; `feat!:` produces a major. + +## Security and sensitivity checks + +**Flag immediately and do not proceed if you detect any of the following:** + +- Credentials, API keys, tokens, passwords, or secrets of any kind. +- Personal data (names, emails, NI numbers, addresses, or any data that could + identify an individual). +- Internal system names, hostnames, IP addresses, or project IDs that are not + already publicly documented. +- Content marked or described as `OFFICIAL-SENSITIVE`, `SECRET`, or above. +- Commercial or contractual information that is not intended for public release. +- Any content that appears to have been copied from a non-public internal source. + +This repository is **public**. Anything committed here is immediately visible to +the world. When in doubt, leave it out and ask. + +## What Copilot should not do + +- Do not add undocumented features, refactor existing content, or expand scope + beyond what is asked. +- Do not add fictional tool outputs, version numbers, or command responses. + All examples must reflect real, documented behaviour. +- Do not reference internal ONS systems, team names, or processes that are not + already public. +- Do not provide guidance on SECRET or above security classifications. diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..0720437 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,21 @@ +version: 2 + +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" + day: "tuesday" + time: "07:00" + timezone: "Europe/London" + open-pull-requests-limit: 10 + rebase-strategy: "auto" + commit-message: + prefix: "deps" + include: "scope" + labels: + - "github-actions" + groups: + github-actions: + patterns: + - "*" diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..287330f --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,34 @@ + + +## Summary + + + +## Context + + + +## Changes + + + +- + +## Verification + + + +## Risks and notes + + + +--- + +## Sensitivity declaration + +> This repository is public. Tick all boxes before requesting review. + +- [ ] No credentials, API keys, tokens, or passwords +- [ ] No personal data (names, emails, NI numbers, or other identifying information) +- [ ] No internal hostnames, IP addresses, or project IDs not already publicly documented +- [ ] No content marked `OFFICIAL-SENSITIVE` or above diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml new file mode 100644 index 0000000..6d4db77 --- /dev/null +++ b/.github/workflows/publish.yml @@ -0,0 +1,45 @@ +on: + workflow_dispatch: + push: + branches: [main] + +name: Quarto Publish + +permissions: + contents: read + pages: write + id-token: write + +jobs: + build: + runs-on: ubuntu-latest + + steps: + - name: Check out repository + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Set up Quarto + uses: quarto-dev/quarto-actions/setup@8a96df13519ee81fd526f2dfca5962811136661b # v2.2.0 + + - name: Configure GitHub Pages + uses: actions/configure-pages@45bfe0192ca1faeb007ade9deae92b16b8254a0d # v6.0.0 + + - name: Render site + run: quarto render + + - name: Upload GitHub Pages artifact + uses: actions/upload-pages-artifact@fc324d3547104276b827a68afc52ff2a11cc49c9 # v5.0.0 + with: + path: _site + + deploy: + needs: build + runs-on: ubuntu-latest + environment: + name: github-pages + url: ${{ steps.deployment.outputs.page_url }} + + steps: + - name: Deploy to GitHub Pages + id: deployment + uses: actions/deploy-pages@cd2ce8fcbc39b97be8ca5fce6e763baed58fa128 # v5.0.0 diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml new file mode 100644 index 0000000..b9b1933 --- /dev/null +++ b/.github/workflows/release-please.yml @@ -0,0 +1,17 @@ +on: + push: + branches: [main] + +name: Release Please + +permissions: + contents: write + pull-requests: write + +jobs: + release-please: + runs-on: ubuntu-latest + steps: + - uses: googleapis/release-please-action@45996ed1f6d02564a971a2fa1b5860e934307cf7 # v5.0.0 + with: + token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.gitignore b/.gitignore index 82ffd73..207ad4b 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,5 @@ Thumbs.db # Editor .vscode/ *.swp + +**/*.quarto_ipynb diff --git a/.release-please-manifest.json b/.release-please-manifest.json new file mode 100644 index 0000000..0ee8c01 --- /dev/null +++ b/.release-please-manifest.json @@ -0,0 +1,3 @@ +{ + ".": "0.3.0" +} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f79207..7cda4c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,43 @@ # Changelog +## [0.3.0](https://github.com/datasciencecampus/cloud-engineering-standards/compare/v0.2.1...v0.3.0) (2026-06-10) + + +### Features + +* add custom CSS for dark mode code readability ([040e368](https://github.com/datasciencecampus/cloud-engineering-standards/commit/040e368e1f4a0f519d5200707159c30a421d7bf1)) + + +### Bug Fixes + +* clarify wording in tone and style section for improved clarity ([df7c7c7](https://github.com/datasciencecampus/cloud-engineering-standards/commit/df7c7c75755bf1658a030e14cf9b8ae8206a1ffd)) +* clarify wording on Release Please workflow for better understanding ([cbc126d](https://github.com/datasciencecampus/cloud-engineering-standards/commit/cbc126da7e4e2a077d77f94ffb669ae5571c4bdd)) +* clarify wording on secrets management in Terraform review documentation to be cloud agnostic ([49ffebe](https://github.com/datasciencecampus/cloud-engineering-standards/commit/49ffebe73eb8baf0bd24ce9a96dbf6407a105063)) +* correct wording in side effects comparison for clarity ([597dfe1](https://github.com/datasciencecampus/cloud-engineering-standards/commit/597dfe15e9f5da3981d15e0f22b27e31dec2338a)) + +## [0.2.1](https://github.com/datasciencecampus/cloud-engineering-standards/compare/v0.2.0...v0.2.1) (2026-06-08) + + +### Bug Fixes + +* Improve clarity and formatting in Terraform review guide sections ([663e79f](https://github.com/datasciencecampus/cloud-engineering-standards/commit/663e79fe1e59a5a4746694f5d831ca40c6a62c35)) +* remove number-sections configuration to guide metadata ([a54777e](https://github.com/datasciencecampus/cloud-engineering-standards/commit/a54777eae1fb8888c4b54c05a1bbd4da9d3c45f2)) +* Update deploy-pages action to version 5.0.0 in GitHub Actions workflow ([e076100](https://github.com/datasciencecampus/cloud-engineering-standards/commit/e076100551023d29cdfeea2c94a038865fc30b14)) + +## [0.2.0](https://github.com/datasciencecampus/cloud-engineering-standards/compare/v0.1.0...v0.2.0) (2026-06-08) + + +### Features + +* Add initial documentation for environments, glossary, and patterns sections ([e35b1fb](https://github.com/datasciencecampus/cloud-engineering-standards/commit/e35b1fb09f2932fef190da8dd8a894739771ffba)) + + +### Bug Fixes + +* Update documentation links and improve clarity in Terraform review guide ([9b43a87](https://github.com/datasciencecampus/cloud-engineering-standards/commit/9b43a874bff0a8a674eba8f9d998ef15186d2407)) + +## Changelog + All notable changes to this project will be documented in this file. This changelog is managed by [Release Please](https://github.com/googleapis/release-please) and follows [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..51f8703 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,188 @@ +# Contributing to Cloud Engineering Standards + +Thank you for taking the time to contribute. This document explains how to raise +issues and submit changes to this repository. + +This repository is public. Everything you contribute will be immediately visible +to anyone on the internet. Read the [sensitivity checks](#sensitivity-checks) +section before you start. + +## Contents + +- [Who can contribute](#who-can-contribute) +- [Sensitivity checks](#sensitivity-checks) +- [Raising an issue](#raising-an-issue) +- [Submitting a change](#submitting-a-change) +- [Commit messages](#commit-messages) +- [Content style](#content-style) +- [Branch protection and review](#branch-protection-and-review) +- [Security and Dependabot alerts](#security-and-dependabot-alerts) + +--- + +## Who can contribute + +Contributions from within the ONS team are welcome via pull +request. If you are from outside the organisation and have spotted an error or +have a suggestion, please [open an issue](https://github.com/datasciencecampus/cloud-engineering-standards/issues) +rather than submitting a pull request directly. + +Unsolicited pull requests from outside the organisation will not be merged. This +is in line with the ONS GitHub Usage Policy. + +--- + +## Sensitivity checks + +This repository is **public**. Before raising a PR or pushing any commit, check +that your changes do not include: + +- Credentials, API keys, tokens, passwords, or secrets of any kind +- Personal data (names, email addresses, NI numbers, or any other data that + could identify an individual) +- Internal system names, hostnames, IP addresses, or project IDs that are not + already publicly documented +- Content marked or described as `OFFICIAL-SENSITIVE`, `SECRET`, or above +- Commercial or contractual information not intended for public release +- Content copied from a non-public internal source + +If you are unsure whether something is suitable for inclusion, leave it out and +raise an issue to discuss it first. + +Secret scanning and push protection are enabled on this repository. Commits +containing detected secrets will be blocked. + +--- + +## Raising an issue + +Use [GitHub Issues](https://github.com/datasciencecampus/cloud-engineering-standards/issues) +to report errors, suggest new content, or ask questions. When raising an issue: + +- Use a clear, descriptive title +- Describe what you found or what you are suggesting +- For errors, include the page title or URL and the text that needs correcting +- For suggestions, explain what you would like added and why it would be useful + for the target audience (data scientists and engineers new to cloud infrastructure) + +--- + +## Submitting a change + +1. Fork or branch from `main`. Use a descriptive branch name: + - `feat/add-glossary-term-idempotency` + - `fix/correct-tflint-install-command` + - `docs/expand-pr-checklist` + +2. Make your changes. All content pages are Quarto `.qmd` files. See + [Content style](#content-style) below. + +3. Check that the book renders without errors: + + ```bash + quarto render + ``` + +4. Commit your changes following the [commit message convention](#commit-messages). + +5. Push your branch and open a pull request against `main`. Complete all items + in the pull request template before requesting a review. + +6. At least one maintainer approval is required before a PR can be merged. + Branch protection rules are enforced on `main`. + +Signed commits are recommended. See [GitHub's documentation on signing commits](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits). + +--- + +## Commit messages + +This repository uses [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/). +Commit messages drive the automated changelog and version bumps managed by +Release Please: + +| Prefix | Use for | Version bump | +|---|---|---| +| `feat:` | New chapter, section, or significant new guidance | Minor | +| `fix:` | Factual corrections or broken references | Patch | +| `docs:` | Small content additions or corrections | Patch | +| `chore:` | Maintenance: config, CI, dependency updates | None | +| `refactor:` | Restructuring without changing meaning | None | +| `feat!:` | Breaking restructure (e.g. renaming a section others link to) | Major | + +Examples: + +``` +feat: add reading-tf-plan chapter to terraform review guide +fix: correct checkov installation command for Python 3.12 +docs: clarify blast radius definition in glossary +chore: pin actions/checkout to SHA +``` + +--- + +## Content style + +- Write in UK English (e.g. "authorise", "organisation", "behaviour"). +- Use plain language and active voice. Follow the + [GDS content design guidance](https://www.gov.uk/guidance/content-design/writing-for-gov-uk). +- Write in the second person: "you should check" not "it should be checked". +- Use numbered lists for sequential steps; bullet lists for unordered items. +- Explain technical terms when first used, or link to the glossary. +- Use Quarto callout blocks for notes, warnings, tips, and important caveats: + + ```markdown + :::{.callout-note} + General supplementary information. + ::: + + :::{.callout-tip} + A helpful hint or shortcut. + ::: + + :::{.callout-warning} + An action that could cause problems if missed. + ::: + + :::{.callout-important} + A critical requirement or constraint. + ::: + ``` + +- GCP-specific examples must appear inside a clearly labelled callout so the + surrounding content stays cloud-agnostic. +- Cross-reference other sections using Quarto syntax (`@sec-label`) rather than + bare URLs where possible. +- Every `.qmd` file must include at minimum a `title` and `order` in its YAML + frontmatter. + +--- + +## Branch protection and review + +The `main` branch has the following protections enabled: + +- Pull requests are required before merging +- At least one approving review is required +- Status checks (Quarto publish workflow) must pass before merge +- Branch deletion is prevented + +--- + +## Security and Dependabot alerts + +Dependabot is enabled on this repository. Alert resolution targets are: + +| Severity | Target resolution | +|---|---| +| Critical | 5 working days | +| High | 15 working days | +| Medium | 60 working days | +| Low | 90 working days | + +Repository maintainers are responsible for monitoring and resolving security +alerts. Unresolved alerts may be escalated as part of audit activity. + +To report a security vulnerability (other than a Dependabot alert), follow the +[GitHub security advisory process](https://docs.github.com/en/code-security/security-advisories/guidance-on-reporting-and-writing/privately-reporting-a-security-vulnerability) +or contact the maintaining team directly. diff --git a/README.md b/README.md index c288445..bd4b178 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![Quarto Publish](https://github.com/datasciencecampus/cloud-engineering-standards/actions/workflows/publish.yml/badge.svg)](https://github.com/datasciencecampus/cloud-engineering-standards/actions/workflows/publish.yml) -Guidance and standards for cloud engineering at the Data Science Campus, Office for National Statistics. +Guidance and standards for cloud engineering at the Strategic Innovation and Analysis team at ONS. Published at: **https://datasciencecampus.github.io/cloud-engineering-standards/** @@ -18,4 +18,4 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for guidance on raising issues and submit ## Licence -© Crown Copyright (Office for National Statistics). Released under the [MIT Licence](LICENSE). +© Crown Copyright. Released under the [MIT Licence](LICENSE). diff --git a/_quarto.yml b/_quarto.yml index d294e8e..6c21367 100644 --- a/_quarto.yml +++ b/_quarto.yml @@ -4,7 +4,7 @@ project: book: title: "Cloud Engineering Standards" - author: "Data Science Campus, Office for National Statistics" + author: "Strategic Innovation and Analysis, ONS" date: last-modified date-format: "D MMMM YYYY" repo-url: https://github.com/datasciencecampus/cloud-engineering-standards @@ -12,7 +12,7 @@ book: search: true page-footer: left: | - © Crown Copyright (Office for National Statistics). Released under the [MIT Licence](https://github.com/datasciencecampus/cloud-engineering-standards/blob/main/LICENSE). + © Crown Copyright. Released under the [MIT Licence](https://github.com/datasciencecampus/cloud-engineering-standards/blob/main/LICENSE). right: | Built with [Quarto](https://quarto.org/) @@ -49,6 +49,7 @@ format: theme: light: flatly dark: darkly + css: styles.css toc: true toc-depth: 3 number-sections: true diff --git a/environments/index.qmd b/environments/index.qmd new file mode 100644 index 0000000..25eedae --- /dev/null +++ b/environments/index.qmd @@ -0,0 +1,14 @@ +--- +title: "Environments" +order: 1 +--- + +This part of the guide covers the four environments used in this team's cloud +infrastructure: sandbox, non-production, pre-production, and production. + +::: {.callout-note} +This section is in development. In the meantime, see +[Environment-specific review](../terraform-review/environments.qmd) in the +Terraform review guide for how environment differences affect what you should +check in a pull request. +::: diff --git a/glossary.qmd b/glossary.qmd new file mode 100644 index 0000000..93dd40f --- /dev/null +++ b/glossary.qmd @@ -0,0 +1,130 @@ +--- +title: "Glossary" +order: 1 +--- + +::: {#sec-glossary} +::: + +Key terms used throughout this guide. + +## Apply + +The Terraform operation that executes the changes described in a plan, creating, +updating, or destroying real cloud resources. Running `terraform apply` makes +changes to your infrastructure immediately. + +## Backend + +The location where Terraform stores its state file. In this team's +infrastructure, the backend is a remote Cloud Storage bucket, so that state is +shared across the team and not stored on a local machine. + +## Blast radius + +The scope of potential damage if a change goes wrong. A resource that affects +only one service account has a small blast radius; a change to a shared network +or organisation-level IAM policy has a large one. Keeping blast radius small is +a core infrastructure review principle. + +## CI/CD + +Continuous integration and continuous deployment. In this context, the automated +pipelines (GitHub Actions) that run linting, validation, and security checks on +every pull request, and deploy changes on merge. + +## Destroy + +The Terraform operation that removes a resource from both the cloud environment +and the state file. Destructive changes appear as `-` in a plan output. + +## Drift + +The difference between what Terraform's state file believes exists in your cloud +environment and what actually exists. Drift occurs when changes are made outside +Terraform — for example, manually through the cloud console. Running +`terraform plan` will show drift as unexpected changes. + +## Idempotency + +The property of an operation that produces the same result regardless of how +many times it is run. Terraform apply should be idempotent: running it twice +with no code changes should produce no changes on the second run. + +## Infrastructure as Code (IaC) + +The practice of managing and provisioning cloud infrastructure through +machine-readable configuration files rather than manual processes or interactive +tools. Terraform is the IaC tool used in this team. + +## Least privilege + +The principle that a user, service account, or system component should be +granted only the permissions it needs to perform its function — nothing more. +Applying least privilege limits the damage that can be caused by a mistake or +a compromised credential. + +## Module + +A reusable, self-contained unit of Terraform configuration. Modules are stored +in `modules/` and called from root modules in each environment directory. Using +modules avoids repeating the same configuration across environments. + +## OFFICIAL + +The baseline security classification for most UK government work. Data that is +`OFFICIAL` requires standard controls but is not considered sensitive enough to +warrant enhanced protective measures. See the +[Government Security Classifications Policy](https://www.gov.uk/government/publications/government-security-classifications) +for the authoritative definition. + +## OFFICIAL-SENSITIVE + +A handling caveat within the `OFFICIAL` classification, used for information +that warrants additional access controls. Infrastructure handling +`OFFICIAL-SENSITIVE` data should be documented with the service's security and +information management leads before it is built. + +## Plan + +The Terraform operation that computes the difference between your configuration +and the current state, and shows what changes would be made by `terraform apply`. +A plan does not make any changes. See +[Reading a Terraform plan](terraform-review/reading-tf-plan.qmd) for how to +read one. + +## Provider + +A plugin that allows Terraform to interact with a specific cloud platform or +service. This team uses the `hashicorp/google` provider for Google Cloud +Platform resources. Provider versions should be pinned to avoid unexpected +changes from provider updates. + +## Root module + +The top-level Terraform configuration for a specific environment — the +`main.tf`, `variables.tf`, `outputs.tf`, `versions.tf`, and `backend.tf` files +in directories like `01_sandbox/`. Root modules call reusable modules and define +what is actually deployed in each environment. + +## State file + +The file (`terraform.tfstate`) in which Terraform records what resources it has +created and their current configuration. The state file is the source of truth +for what Terraform believes exists in your cloud environment. It is stored +remotely in a Cloud Storage bucket and must not be edited manually. + +## Trust boundary + +The perimeter within which resources share the same access controls, identity +model, and security policies. In this team's infrastructure, each Google Cloud +project is a trust boundary for one environment. Keeping environments in +separate projects means that a compromise in sandbox cannot directly affect +production. + +## Workspace + +A Terraform feature that allows multiple state files to be managed from the same +configuration. This team does not use workspaces to separate environments — +each environment has its own root module directory instead, which provides +stronger separation. diff --git a/index.qmd b/index.qmd new file mode 100644 index 0000000..f615110 --- /dev/null +++ b/index.qmd @@ -0,0 +1,81 @@ +--- +title: "About this guide" +order: 1 +number-sections: false +--- + +This guide sets out cloud engineering standards for the Strategic Innovation +and Analysis team at ONS. It is intended for **data scientists and +engineers who are beginning to work with cloud infrastructure** — not for +experienced platform engineers, though they may find it a useful reference. + +If you have been asked to review a pull request containing Terraform code for +the first time, or you are trying to understand what good infrastructure as code +looks like, this is the right place to start. + +## What this guide covers + +- How to review Terraform infrastructure as code, including the principles, + tools, checklists, and worked examples you need to do it well +- How to read a Terraform plan and understand what changes it will make +- The environment model used in this team — sandbox, + non-production, pre-production, and production — and what each one means + for how you write and review infrastructure + +Later sections, which are in development, will cover: + +- Choosing between common infrastructure patterns such as batch processing, + streaming, and event-driven architectures + +## What this guide does not cover + +This guide gives practical, working guidance. It is not: + +- A substitute for formal security accreditation or information assurance + decisions. If your service is processing sensitive data, you need to involve + your security and information management leads. +- A complete Terraform tutorial. If you are new to Terraform itself, work + through the [official Terraform tutorials](https://developer.hashicorp.com/terraform/tutorials) + first, then return here. +- Authoritative guidance on HMG security classifications. For that, refer to + the [Government Security Classifications Policy](https://www.gov.uk/government/publications/government-security-classifications). + +## How to use this guide + +If you are reviewing Terraform code for the first time, start with +[Review principles](terraform-review/principles.qmd) and +[Skills, expectations, and escalation](terraform-review/skills-expectations.qmd). +These will tell you what you are +looking for and how much confidence to have in your own judgement. + +If you have been handed a specific pull request to review, the +[pull request review checklist](terraform-review/pr-checklist.qmd) gives you a +structured list of things to check. + +If you want to understand what a particular tool does (such as `checkov` or +`tflint`), go to [Review tools](terraform-review/tools.qmd). + +## Infrastructure as code in this team + +This team uses Terraform to manage cloud infrastructure. Terraform +code in this organisation follows the structure defined in +[datasciencecampus/terraform-template](https://github.com/datasciencecampus/terraform-template), +with environments organised into separate directories: + +| Directory | Environment | +|---|---| +| `01_sandbox/` | Sandbox | +| `02_dev_nonprod/` | Non-production | +| `03_stg_prod/` | Pre-production | +| `04_prd_prod/` | Production | + +Reusable code lives in `modules/`. This separation is important: each directory +represents a different trust boundary, with progressively tighter controls as +you move from sandbox towards production. + +You will encounter this structure throughout the Terraform review guidance. + +## Glossary + +Technical terms are explained when first used. You can also find a full +[glossary](glossary.qmd) at the back of this guide. diff --git a/patterns/index.qmd b/patterns/index.qmd new file mode 100644 index 0000000..7993d3f --- /dev/null +++ b/patterns/index.qmd @@ -0,0 +1,12 @@ +--- +title: "Patterns" +order: 1 +--- + +This part of the guide covers common infrastructure patterns — batch processing, +streaming, and event-driven architectures — and guidance on choosing between +them based on your workload's needs. + +::: {.callout-note} +This section is in development. +::: diff --git a/release-please-config.json b/release-please-config.json new file mode 100644 index 0000000..1dee2e9 --- /dev/null +++ b/release-please-config.json @@ -0,0 +1,7 @@ +{ + "$schema": "https://raw.githubusercontent.com/googleapis/release-please/main/schemas/config.json", + "release-type": "simple", + "packages": { + ".": {} + } +} diff --git a/styles.css b/styles.css new file mode 100644 index 0000000..51d279f --- /dev/null +++ b/styles.css @@ -0,0 +1,6 @@ +/* Keep inline code readable in dark mode. */ +body.quarto-dark :not(pre) > code { + color: #f8f9fa; + background-color: #2b3035; + border: 1px solid #495057; +} diff --git a/terraform-review/documentation.qmd b/terraform-review/documentation.qmd new file mode 100644 index 0000000..e05185a --- /dev/null +++ b/terraform-review/documentation.qmd @@ -0,0 +1,149 @@ +--- +title: "Documentation expectations" +order: 6 +--- + +::: {#sec-documentation} +::: + +Good documentation is part of good infrastructure. Terraform code that is not +documented is harder to review, harder to maintain, and harder to hand over. +This chapter explains what documentation to expect — and how to assess it as +a reviewer. + +## Why documentation matters more in IaC + +In application code, a well-named function often tells you what it does. In +Terraform, a well-named resource tells you what it creates, but not necessarily +why it is configured the way it is, why it exists in this environment, or what +would break if it were removed. + +Documentation fills this gap. A reviewer cannot properly assess a Terraform +change without understanding its context. + +## Variable and output descriptions + +Every variable and output should have a `description` field. This is the +minimum standard. + +```hcl +# Good +variable "region" { + description = "The GCP region in which to create resources." + type = string + default = "europe-west2" +} + +# Poor — no description, no type +variable "region" { + default = "europe-west2" +} +``` + +```hcl +# Good +output "bucket_name" { + description = "The name of the Cloud Storage bucket used for pipeline outputs." + value = google_storage_bucket.pipeline_outputs.name +} +``` + +**As a reviewer:** If a variable or output has no description, request one. The +description should explain what the value is used for, not just repeat the +variable name. + +## Module README files + +Every module in `modules/` should have a `README.md` that explains: + +- What the module creates +- What inputs it requires (name, type, description, whether required or optional) +- What outputs it produces +- An example of how to call the module + +[terraform-docs](https://terraform-docs.io/) can generate this automatically +from the variable and output descriptions in the code: + +```bash +terraform-docs markdown table . > README.md +``` + +This is another reason why variable and output descriptions matter — they feed +directly into generated documentation. + +**As a reviewer:** If a PR adds or significantly modifies a module, check that +the README exists and is up to date. A PR that changes a module's interface (adds +or removes variables, changes output names) must update the README. + +## Root module documentation + +Root modules (the environment directories like `01_sandbox/`) do not always need +a full README, but they should have enough context for someone unfamiliar with +the project to understand what is being deployed. This can be: + +- A `README.md` describing the purpose of the environment and what it contains +- Comments at the top of `main.tf` explaining any non-obvious architectural + choices + +## Architecture decision records + +For significant infrastructure decisions — choosing a particular network +topology, deciding to use Cloud SQL rather than Firestore, choosing a specific +service account structure — an architecture decision record (ADR) provides a +durable record of what was decided and why. + +ADRs are stored in the `docs/` directory, following the convention established +in [datasciencecampus/terraform-template](https://github.com/datasciencecampus/terraform-template). + +An ADR does not need to be lengthy. A short document covering: + +- **Context:** What problem were you solving? +- **Decision:** What did you decide to do? +- **Consequences:** What are the trade-offs or implications? + +is enough to be useful. + +**As a reviewer:** If a PR makes a significant architectural decision without an +ADR, you can suggest one — particularly for changes that are hard to reverse or +that constrain future options. Not every PR needs an ADR, but reviewers are well +placed to spot when one would be valuable. + +## Inline comments + +Inline comments in Terraform code are appropriate for: + +- Explaining why a resource is configured in a non-obvious way +- Noting a known limitation or a follow-up action +- Explaining why a `lifecycle` rule or `checkov` suppression is in place + +```hcl +resource "google_storage_bucket" "pipeline_outputs" { + name = "${var.project_id}-pipeline-outputs" + location = var.region + + # Uniform bucket-level access is required by our data handling policy. + # Fine-grained ACLs are disabled to prevent accidental per-object permissions. + uniform_bucket_level_access = true + + # Prevent accidental deletion of the bucket in non-sandbox environments. + # To delete this bucket, you must first set this to false and apply. + lifecycle { + prevent_destroy = true + } +} +``` + +**As a reviewer:** Comments that explain *why* (not just *what*) are valuable. +Comments that merely repeat what the code already says are noise. If something +in the code is unclear and there is no comment explaining it, ask for one. + +## What to check in a review + +| Item | Minimum standard | Better standard | +|---|---|---| +| Variables | `description` set | `description`, `type`, sensible `default` or no default, and `sensitive = true` where needed | +| Outputs | `description` set | `description` and `sensitive = true` where needed | +| Module README | Exists | Generated by `terraform-docs`, includes example usage | +| Root module | `main.tf` has a brief header comment | Full `README.md` | +| Inline comments | Present for non-obvious choices | Explains *why*, not just *what* | +| ADRs | Not required for every PR | Present for significant or hard-to-reverse decisions | diff --git a/terraform-review/environments.qmd b/terraform-review/environments.qmd new file mode 100644 index 0000000..1dc0dbb --- /dev/null +++ b/terraform-review/environments.qmd @@ -0,0 +1,162 @@ +--- +title: "Environment-specific review" +order: 7 +--- + +::: {#sec-environments} +::: + +Not all Terraform changes carry the same risk. A change to a sandbox environment +that uses only synthetic data is very different from a change to the production +environment that processes live operational data. This chapter explains how to +calibrate your review based on the target environment. + +## The four environments + +This organisation uses four environments, each with its own Terraform root in +the `terraform/` directory: + +| Directory | Environment | Purpose | +|---|---|---| +| `01_sandbox/` | Sandbox | Early exploration, proof of concept, low-risk experimentation | +| `02_dev_nonprod/` | Non-production | Day-to-day development, integration, and feature testing | +| `03_stg_prod/` | Pre-production | Final validation and operational rehearsal before go-live | +| `04_prd_prod/` | Production | Live service delivery and operational processing | + +Each environment is a separate Google Cloud project. This separation is the +trust boundary — identities, state, quotas, policies, and audit trails are +isolated by environment. A mistake in sandbox cannot directly affect production. + +## What changes between environments + +As infrastructure moves from sandbox towards production, several things should +change: + +- **Data:** Sandbox should use only synthetic, anonymised, or openly published + data. Production may process live operational data, depending on the service's + approval. +- **Access controls:** Production access should be restricted to the smallest + practical group. Non-production environments can be more permissive. +- **Monitoring and logging:** Production and pre-production should have audit + logging, alerting, and monitoring configured. These are optional in sandbox. +- **Deletion protection:** Stateful resources such as databases and storage + buckets should have `prevent_destroy = true` in pre-production and production. +- **Encryption:** Explicit at-rest and in-transit encryption configuration should + be present in pre-production and production. +- **Resilience:** Production resources may need high-availability configuration + (multi-region storage, read replicas) that is unnecessary in lower environments. + +## Review expectations by environment + +### Sandbox + +Sandbox is the most forgiving environment. The intent is to allow experimentation +without the overhead of production-grade controls. + +**What to check:** + +- The code does not reference production project IDs or real data sources +- There are no hardcoded secrets +- The change is clearly scoped to `01_sandbox/` and does not affect shared + modules unexpectedly +- Even in sandbox, IAM should be reasonably scoped — wide-open permissions are + a bad habit to normalise + +**What is acceptable that would not be in production:** + +- Missing deletion protection on resources +- Less strict firewall rules +- Reduced or absent logging configuration +- Use of less expensive, lower-resilience resource tiers + +### Non-production + +Non-production is used for day-to-day development. It should have baseline +controls in place, because integration testing with broken security configurations +provides false assurance. + +**What to check (in addition to sandbox checks):** + +- IAM bindings follow least privilege +- Logging is enabled on resources that will carry logging in production +- Variable defaults are not set to production values +- The code is structured so it can be promoted to pre-production without + significant rework + +**What to watch for:** + +- `OFFICIAL-SENSITIVE` data in non-production should be by exception only and + agreed with the information owner +- Service accounts should not have cross-environment permissions + +### Pre-production + +Pre-production should mirror production controls as closely as practical. Its +purpose is operational rehearsal — if it does not match production, the rehearsal +is not meaningful. + +**What to check (in addition to non-production checks):** + +- Deletion protection is enabled on stateful resources +- Monitoring and alerting are configured +- The resource configuration matches production (same instance types, same + network topology, same encryption settings) +- Access controls are as restrictive as production + +**What to watch for:** + +- Pre-production may use production-like data in some cases. If so, the same + data handling controls as production should apply +- Changes that diverge from the production configuration without explanation + +### Production + +Production changes require the highest level of scrutiny. Mistakes here affect +live services and potentially real data. + +**What to check (in addition to pre-production checks):** + +- Is there a `terraform plan` output attached? You should not approve a + production change without having seen what it will do. +- Are destructive changes (`-/+` in the plan) clearly expected and explained? + If not, block the PR and ask. +- Is deletion protection enabled on all stateful resources? +- Has the change been tested in pre-production first? +- Are there any IAM changes? If so, have they been reviewed with particular care? + +**Escalate to a platform engineer if:** + +- You are not confident you understand all implications of the change +- The plan shows unexpected replacements +- The change touches network perimeter rules, organisation-level policies, or + audit logging configuration + +## Security classifications + +UK government uses `OFFICIAL`, `OFFICIAL-SENSITIVE`, `SECRET`, and `TOP SECRET` +as its security classification scheme. This team's work is almost entirely within +`OFFICIAL`. For the purposes of this infrastructure: + +- Most routine service data is handled at `OFFICIAL` +- `OFFICIAL-SENSITIVE` data requires additional handling controls and should be + documented with the service's security and information management leads before + infrastructure is built to process it + +:::{.callout-important} +If a PR introduces infrastructure intended to process `OFFICIAL-SENSITIVE` data, +and this has not been agreed with the information owner and documented, block the +PR and raise the issue before approving. +::: + +## Checking the right environment is targeted + +A common mistake is submitting a change against the wrong environment directory. +Always check: + +- Does the PR description say which environment is being changed? +- Do the changed files match? A PR described as "adds storage bucket to sandbox" + should only touch `01_sandbox/`. +- Are any shared modules changed? If so, understand which environments will be + affected. +- Does the `backend.tf` in the changed directory point to the correct state + backend for that environment? diff --git a/terraform-review/index.qmd b/terraform-review/index.qmd new file mode 100644 index 0000000..aa9df60 --- /dev/null +++ b/terraform-review/index.qmd @@ -0,0 +1,47 @@ +--- +title: "Reviewing Terraform" +order: 1 +--- + +This part of the guide covers everything you need to review Terraform +infrastructure as code (IaC) effectively. It is written for people who are +comfortable reading code but may not yet have experience with infrastructure +review specifically. + +## Why infrastructure review is different + +When you review Python code, you are mostly reasoning about logic: what the +code does when it runs, whether it handles edge cases, whether it is readable +and maintainable. If the code is wrong, you can usually fix it quickly and +redeploy. + +Reviewing Terraform is different in two important ways. + +**Infrastructure changes have side effects that persist.** Terraform manages +real resources — virtual machines, storage buckets, network rules, IAM bindings. +A change that looks minor in code can delete a database, open a firewall port, +or grant someone access they should not have. Those effects do not disappear +when you close the pull request. + +**Terraform tracks state.** Terraform keeps a record (the *state file*) of what +it believes exists in your cloud environment. If the code and the state diverge, +or if someone makes changes outside Terraform, the next `apply` may produce +unexpected results. A reviewer needs to think not just about what the code says, +but about what Terraform will actually do when it runs against the current state. + +These differences do not make infrastructure review harder than application code +review — but they do mean you need to think about different things. + +## What this part covers + +The chapters in this part walk you through: + +- **[Review principles](principles.qmd)** — The core principles that should guide every IaC review, regardless of what the code does. +- **[Skills, expectations, and escalation](skills-expectations.qmd)** — What reviewers are expected to know, how to calibrate your confidence, and when to escalate. +- **[What to review](what-to-review.qmd)** — A detailed guide to what to look for in Terraform code, with a comparison to Python code review. +- **[Review tools](tools.qmd)** — The automated tools available to support your review: `terraform fmt`, `terraform validate`, `checkov`, and `tflint`. +- **[Documentation expectations](documentation.qmd)** — What documentation to expect and how to assess it. +- **[Environment-specific review](environments.qmd)** — How review expectations change depending on which environment the code targets. +- **[Pull request review checklist](pr-checklist.qmd)** — A structured checklist you can work through when reviewing any Terraform pull request. +- **[Writing review comments](review-comments.qmd)** — How to write clear, useful review comments, with worked examples. +- **[Reading a Terraform plan](reading-tf-plan.qmd)** — How to read and interpret a `terraform plan` output. diff --git a/terraform-review/pr-checklist.qmd b/terraform-review/pr-checklist.qmd new file mode 100644 index 0000000..5aa7b3e --- /dev/null +++ b/terraform-review/pr-checklist.qmd @@ -0,0 +1,116 @@ +--- +title: "Pull request review checklist" +order: 8 +--- + +::: {#sec-pr-checklist} +::: + +Use this checklist when reviewing a Terraform pull request. You do not need to +work through it in order — use it to make sure you have not missed anything. + +Not every item applies to every PR. Use your judgement about what is relevant +to the change in front of you. + +## Before you start + +- [ ] The PR description clearly explains what changed and why +- [ ] The target environment directory is correct for the stated change +- [ ] A `terraform plan` output is attached or linked (required for + non-production, pre-production, and production; recommended for sandbox) +- [ ] CI checks have passed: `terraform fmt`, `terraform validate`, `tflint`, + and `checkov` + +## Code structure and quality + +- [ ] Provider and Terraform version constraints are declared in `versions.tf` + and pinned to specific versions or tight ranges +- [ ] All variables have a `description` and a declared `type` +- [ ] Sensitive variables are marked `sensitive = true` +- [ ] All outputs have a `description` +- [ ] Sensitive outputs are marked `sensitive = true` +- [ ] Resource names are meaningful and consistent +- [ ] No hardcoded project IDs, region names, or resource identifiers that + should be variables +- [ ] Any committed `.tfvars` files contain only non-sensitive configuration + (project IDs, regions, feature flags) — no passwords, tokens, or API keys + +## Security + +- [ ] No hardcoded secrets, passwords, tokens, or API keys anywhere in the code + or variable files +- [ ] IAM bindings use the most specific role available — no `roles/owner` or + `roles/editor` without strong justification +- [ ] `google_project_iam_binding` vs `google_project_iam_member` is used + correctly (binding is authoritative; member adds without removing) +- [ ] Storage resources have public access explicitly configured (usually + disabled) +- [ ] Encryption at rest is explicitly configured on data resources in + non-sandbox environments +- [ ] Audit logging is enabled where expected for the target environment +- [ ] All `checkov` suppressions have an inline comment explaining the rationale + +## Lifecycle and state + +- [ ] `prevent_destroy = true` is set on stateful resources (databases, storage + buckets) in pre-production and production +- [ ] Any `ignore_changes` rules have a comment explaining why the attribute is + being ignored +- [ ] `count` and `for_each` usage is correct — changing between them on existing + resources causes destructive replacement +- [ ] The plan does not show unexpected resource replacements (`-/+`) + +## Modules + +- [ ] Changes to shared modules in `modules/` are understood to affect all + environments that call them +- [ ] Module README is present and updated if the module interface has changed +- [ ] Module inputs are validated where appropriate (using `validation` blocks) + +## Documentation + +- [ ] Non-obvious configuration choices have inline comments explaining why +- [ ] Module README is up to date (generated by `terraform-docs` or equivalent) +- [ ] An ADR has been raised if the PR makes a significant or hard-to-reverse + architectural decision + +## Environment-specific checks + +### Sandbox (`01_sandbox/`) +- [ ] No references to production project IDs or live data sources +- [ ] `terraform plan` output is attached or linked (recommended) + +### Non-production (`02_dev_nonprod/`) +- [ ] `terraform plan` output is attached or linked +- [ ] Logging is configured on resources that will carry logging in production +- [ ] IAM follows least privilege + +### Pre-production (`03_stg_prod/`) +- [ ] `terraform plan` output is attached or linked +- [ ] Deletion protection is enabled on stateful resources +- [ ] Resource configuration matches production (instance types, network topology, + encryption) +- [ ] Monitoring and alerting are configured + +### Production (`04_prd_prod/`) +- [ ] `terraform plan` output is attached or linked +- [ ] All destructive changes are expected and explained +- [ ] Change has been tested in pre-production first +- [ ] IAM changes have been reviewed with particular care +- [ ] Deletion protection is enabled on all stateful resources +- [ ] If you are not confident in any aspect of this change, you have escalated + to a platform engineer before approving + +## Approval + +A PR is ready to merge when: + +1. All required CI checks pass +2. At least one reviewer has approved +3. All unresolved review comments have been addressed +4. No items in this checklist are outstanding without a clear justification + +:::{.callout-warning} +Do not approve a production PR if you have outstanding doubts about what the +plan will do. Approve once you are confident, or escalate if you are not. +::: diff --git a/terraform-review/principles.qmd b/terraform-review/principles.qmd new file mode 100644 index 0000000..7953090 --- /dev/null +++ b/terraform-review/principles.qmd @@ -0,0 +1,148 @@ +--- +title: "Review principles" +order: 2 +--- + +::: {#sec-principles} +::: + +Good Terraform review is guided by a small set of principles. These apply +regardless of what the code is doing — whether it creates a storage bucket or +defines a complex network topology. Keep them in mind as you read through a +pull request. + +## Correctness + +The code should do what the pull request says it does. This sounds obvious, but +it is easy to miss a mismatch between the description ("adds read-only IAM +binding for service account X") and the actual resource definition (which grants +a broader role, or targets the wrong project). + +Check: + +- Does the resource configuration match the intent described in the PR? +- Are resource names, labels, and references consistent throughout? +- Are there any typos in resource names or identifiers that Terraform would + accept but would create the wrong resource? + +## Least privilege + +Infrastructure should grant only the permissions needed to do the job — nothing +more. This applies to IAM roles, network firewall rules, service account scopes, +and any other access control mechanism. + +Check: + +- Does each IAM binding use the most specific role available rather than a + broad primitive role such as `roles/editor` or `roles/owner`? +- Are firewall rules restricted to the ports and source ranges actually needed? +- Does the service account used by a workload have only the permissions that + workload requires? + +:::{.callout-warning} +Primitive IAM roles (`roles/owner`, `roles/editor`, `roles/viewer`) grant +access across an entire project. They should almost never appear in production +Terraform code. If you see one, ask why a more specific role cannot be used. +::: + +:::{.callout-note} +**GCP-specific:** In Google Cloud, prefer predefined roles (e.g. +`roles/storage.objectViewer`) or custom roles over primitive roles. Where a +resource supports it, consider using resource-level IAM bindings rather than +project-level bindings. +::: + +## Idempotency + +Running `terraform apply` twice in a row with no code changes should produce +no changes on the second run. Code that is not idempotent — where the same +apply produces different results each time — is fragile and hard to debug. + +In practice, idempotency issues often arise from: + +- Resources that generate random values outside Terraform's knowledge +- External data sources that return different values on each run +- `local-exec` or `remote-exec` provisioners that run shell commands with + side effects + +Check: + +- Does the code use `random_*` resources correctly, with a `lifecycle` block to + prevent unnecessary recreation? +- Are external data sources used appropriately, and are their results stable? +- Are provisioners (if present at all) genuinely necessary and side-effect safe? + +## Explicitness + +Terraform allows many defaults. Relying on defaults makes code harder to review +and harder to reason about when defaults change in a future provider version. + +Check: + +- Are important settings declared explicitly rather than relying on provider or + resource defaults? +- Are `required_version` and `required_providers` version constraints set? +- Are provider versions pinned to a specific version or a tight range, rather + than accepting any version? + +:::{.callout-tip} +A `terraform` block with `required_version` and `required_providers` at the top +of each root module makes the code's dependencies clear and prevents unexpected +behaviour when provider versions change. +::: + +## Minimal blast radius + +A change should affect only what it needs to affect. Infrastructure changes that +touch many resources at once are harder to review, harder to test, and more +likely to cause an outage if something goes wrong. + +Check: + +- Is the scope of this change appropriate? Could it be broken into smaller, + safer PRs? +- Does the change target only the intended environment directories + (e.g. `01_sandbox/`) or does it affect shared modules used by all environments? +- Are there any resources marked for replacement (`-/+` in the plan) that are + not expected to be recreated? + +## Auditability + +Infrastructure changes should be traceable. Anyone looking at the state, the +plan, or the deployed resources six months from now should be able to understand +what exists, why it exists, and who is responsible for it. + +Check: + +- Are resources tagged or labelled consistently? At minimum, resources should + carry labels that identify the project, environment, and owning team. +- Is there a clear description or comment for any non-obvious configuration + choice? +- Does the module or directory have up-to-date documentation? + +:::{.callout-note} +**GCP-specific:** Google Cloud supports resource labels at the project and +resource level. A labelling convention (e.g. `env`, `team`, `service`) makes +cost attribution, access reviews, and incident response significantly easier. +::: + +## No secrets in code + +Secrets — passwords, API keys, tokens, private keys — must never appear in +Terraform code or state files committed to version control. + +Check: + +- Are sensitive values passed in via variables marked `sensitive = true`, rather + than hardcoded? +- Are secrets retrieved from a secrets manager rather than stored in `.tfvars` + files that could be committed? +- Does the `.gitignore` exclude `*.tfvars`, `terraform.tfstate`, and + `terraform.tfstate.backup`? + +:::{.callout-important} +If you find a hardcoded secret in a pull request, block the PR and treat it as a +potential secret leak. Even if the PR is not merged, the secret may already be +visible in the branch history. Follow your organisation's incident response +process. +::: diff --git a/terraform-review/reading-tf-plan.qmd b/terraform-review/reading-tf-plan.qmd new file mode 100644 index 0000000..b5c1a0a --- /dev/null +++ b/terraform-review/reading-tf-plan.qmd @@ -0,0 +1,217 @@ +--- +title: "Reading a Terraform plan" +order: 10 +--- + +::: {#sec-reading-tf-plan} +::: + +Before changes are applied to a cloud environment, Terraform produces a *plan* +— a description of exactly what it intends to do. Reading this plan is one of +the most important review skills you can develop. It tells you what will actually +happen, not just what the code says. + +This chapter explains how to read and interpret a `terraform plan` output. + +## What a plan is + +When you run `terraform plan`, Terraform: + +1. Reads your configuration files +2. Queries the current state (what it believes exists in your cloud environment) +3. Optionally refreshes state by querying the cloud provider APIs +4. Computes the difference between desired state (your code) and current state +5. Outputs the changes it would make if you ran `terraform apply` + +The plan does not make any changes. It only tells you what *would* happen. + +## How to read the symbols + +Terraform uses symbols to indicate the type of change for each resource: + +| Symbol | Meaning | +|---|---| +| `+` | Create — a new resource will be added | +| `-` | Destroy — an existing resource will be removed | +| `~` | Update in place — an existing resource will be modified without recreation | +| `-/+` | Replace — the resource will be destroyed and recreated | +| `<=` | Read — a data source will be read from the provider | + +The most important to watch for are **`-`** (destroy) and **`-/+`** (replace), +because these can cause service disruption or data loss. + +## A simple example + +``` +Terraform will perform the following actions: + + # google_storage_bucket.pipeline_outputs will be created + + resource "google_storage_bucket" "pipeline_outputs" { + + id = (known after apply) + + location = "EUROPE-WEST2" + + name = "my-project-pipeline-outputs" + + project = "my-project-12345" + + uniform_bucket_level_access = true + + + lifecycle_rule { + + action { + + type = "Delete" + } + + condition { + + age = 90 + } + } + } + +Plan: 1 to add, 0 to change, 0 to destroy. +``` + +This plan creates one resource and destroys nothing. The `+` on every line shows +that all of these are new attributes on a new resource. `(known after apply)` +means the value will be assigned by the cloud provider when the resource is +created — this is normal for things like generated IDs and self-links. + +## An update in place + +``` + # google_storage_bucket.pipeline_outputs will be updated in-place + ~ resource "google_storage_bucket" "pipeline_outputs" { + id = "my-project-pipeline-outputs" + ~ labels = { + + "env" = "sandbox" + # (1 unchanged element hidden) + } + name = "my-project-pipeline-outputs" + # (6 unchanged attributes hidden) + } + +Plan: 0 to add, 1 to change, 0 to destroy. +``` + +The `~` on the resource and the `+` on the label show that a new label is being +added to an existing bucket. Unchanged attributes are hidden by default for +readability. The bucket itself will not be deleted or recreated. + +## A replacement — the most important case + +``` + # google_sql_database_instance.main must be replaced +-/+ resource "google_sql_database_instance" "main" { + ~ connection_name = "my-project:europe-west2:main" -> (known after apply) + - database_version = "POSTGRES_15" -> null # forces replacement + + database_version = "POSTGRES_16" + name = "main" + # (10 unchanged attributes hidden) + } + +Plan: 1 to add, 0 to change, 1 to destroy. +``` + +The `-/+` symbol means the database instance will be **destroyed and recreated**. +This is because `database_version` is an immutable attribute — you cannot change +it on a running Cloud SQL instance; Terraform must create a new one. + +The comment `# forces replacement` tells you which attribute triggered the +replacement. + +:::{.callout-warning} +A replacement of a database, storage bucket, or any other stateful resource can +cause **data loss**. Data stored in the resource may not survive the +destroy-and-recreate cycle unless you have taken explicit steps to preserve it +(such as a database backup or a bucket migration). + +Before approving a plan that shows a `-/+` on a stateful resource, make sure +you understand why the replacement is happening and what will happen to the data. +::: + +## The summary line + +The last line of a plan always shows a summary: + +``` +Plan: 2 to add, 1 to change, 0 to destroy. +``` + +Read this first. If it says anything other than `0 to destroy` and you did not +expect any destruction, that is your first signal to read the plan carefully. + +## Dangerous patterns to look for + +### Unexpected resource replacement + +``` +-/+ resource "google_compute_instance" "worker" { +``` + +Ask: why is this being replaced? Is it expected? What happens to the data or +workload on this instance during the replacement? + +### Destruction of a resource with no replacement + +``` + # google_storage_bucket.archive will be destroyed + - resource "google_storage_bucket" "archive" { +``` + +Ask: is this bucket expected to be removed? If it contains data, where does that +data go? Is `prevent_destroy = true` missing from this resource? + +### A large number of resources affected + +``` +Plan: 47 to add, 12 to change, 8 to destroy. +``` + +A plan with many changes is harder to review. Ask the author whether this is +expected, and consider whether the change can be broken into smaller, safer +steps. + +### IAM changes + +``` + # google_project_iam_binding.data_access will be updated in-place + ~ resource "google_project_iam_binding" "data_access" { + ~ members = [ + - "serviceAccount:old-service@my-project.iam.gserviceaccount.com", + + "serviceAccount:new-service@my-project.iam.gserviceaccount.com", + ] + role = "roles/bigquery.dataViewer" + } +``` + +An IAM change that removes a member (`-`) means someone or something will lose +access. Make sure this is intentional. + +### `google_project_iam_binding` replacing all members + +``` + ~ resource "google_project_iam_binding" "editors" { + ~ members = [ + - "serviceAccount:service-a@my-project.iam.gserviceaccount.com", + - "serviceAccount:service-b@my-project.iam.gserviceaccount.com", + + "serviceAccount:service-c@my-project.iam.gserviceaccount.com", + ] +``` + +This is an authoritative binding. It will remove `service-a` and `service-b` +from the role entirely. If those service accounts are still running workloads, +those workloads will break. Check whether this is intentional. + +## Attaching the plan to a PR + +For any non-sandbox environment, the plan output should be attached to or linked +from the pull request. The author should run: + +```bash +terraform plan -out=tfplan +terraform show -no-color tfplan > plan.txt +``` + +and attach `plan.txt`, or paste the relevant sections into the PR description. + +:::{.callout-tip} +Some CI pipelines post the plan output as a PR comment automatically using tools +like [Atlantis](https://www.runatlantis.io/) or +[Terraform Cloud](https://app.terraform.io/). If your team uses one of these, +the plan will appear in the PR without the author needing to do anything extra. +::: diff --git a/terraform-review/review-comments.qmd b/terraform-review/review-comments.qmd new file mode 100644 index 0000000..494ed6f --- /dev/null +++ b/terraform-review/review-comments.qmd @@ -0,0 +1,147 @@ +--- +title: "Writing review comments" +order: 9 +--- + +::: {#sec-review-comments} +::: + +A good review comment is specific, constructive, and clear about what is being +asked. This chapter explains how to write effective review comments for +Terraform pull requests, and shows examples of both blocking and non-blocking +feedback. + +## Blocking versus non-blocking + +Not all issues are equally important. Be clear about the severity of your +feedback so the author knows what must be fixed before the PR can merge, and +what is a suggestion they can take or leave. + +A common convention is to prefix comments with a label: + +- **Blocking** — this must be fixed before the PR is approved +- **Suggestion** — this would improve the code but is not required for approval +- **Question** — you want to understand something before deciding whether it is + a problem +- **Nit** — a minor style or preference point; the author can decide + +You do not need to use these exact labels — any clear signal works. What matters +is that the author is not left guessing whether a comment requires action. + +## What makes a good comment + +A useful review comment: + +1. **Identifies the specific problem**, not just that something is wrong +2. **Explains why it is a problem** — the reasoning, not just the judgement +3. **Suggests what to do instead** where possible +4. **Distinguishes required changes from suggestions** + +A comment that just says "this is wrong" is not helpful. The author cannot act +on it, and it does not help them learn. + +## Example comments + +### Hardcoded project ID — blocking + +> **Blocking:** The project ID `my-project-12345` is hardcoded here. If this +> module is called from a different environment, it will point to the wrong +> project. Please pass the project ID in via a variable, or use +> `var.project_id` if it is already declared. + +### Overly permissive IAM role — blocking + +> **Blocking:** `roles/editor` grants write access to almost every resource in +> the project. This is much broader than the service account needs. Can you +> check the documentation for the specific service being used and find a +> narrower predefined role? For example, if this service account only needs to +> read from Cloud Storage, `roles/storage.objectViewer` would be appropriate. + +### Missing `prevent_destroy` on a database — blocking (for production) + +> **Blocking:** This Cloud SQL instance does not have `prevent_destroy = true` +> in its `lifecycle` block. In a production environment, an accidental +> `terraform destroy` or a resource replacement could cause data loss. Please +> add: +> +> ```hcl +> lifecycle { +> prevent_destroy = true +> } +> ``` + +### `google_project_iam_binding` used where `member` was likely intended — blocking + +> **Blocking:** `google_project_iam_binding` is authoritative — it will remove +> any other members from `roles/cloudsql.client` that are not listed in this +> resource. If there are other service accounts or users that currently have +> this role, they will lose access on the next apply. Was this intentional? +> If you only want to add this one member without affecting others, use +> `google_project_iam_member` instead. + +### Missing variable description — suggestion + +> **Suggestion:** The variable `enable_versioning` has no description. Can you +> add one? It makes the code easier to understand and feeds into generated +> module documentation: +> +> ```hcl +> variable "enable_versioning" { +> description = "Whether to enable object versioning on the storage bucket." +> type = bool +> default = false +> } +> ``` + +### Suppressed `checkov` finding without explanation — blocking + +> **Blocking:** `CKV_GCP_62` (access logging) has been suppressed on this +> bucket with no explanation. For a bucket in a non-production environment, this +> may be acceptable, but please add a comment explaining why: +> +> ```hcl +> #checkov:skip=CKV_GCP_62:Access logging not required for ephemeral test fixture bucket +> ``` +> +> If access logging should be enabled here, please remove the suppression and +> configure logging. + +### Question about a lifecycle rule + +> **Question:** I see `ignore_changes = [labels]` on this resource. Is this +> because labels are being managed outside Terraform? If so, it would be helpful +> to add a comment explaining that, so the next person who looks at this does +> not assume it was forgotten. + +### Non-obvious configuration — nit + +> **Nit:** The `uniform_bucket_level_access = true` setting is correct here, but +> it might be worth a brief comment to explain that this is set by our data +> handling policy, not just as a default. Helps future reviewers understand why +> it is explicitly set. + +## Tone + +Review comments should be collegial and focused on the code, not the person. + +- "This variable is undescribed" is better than "you forgot to describe this + variable" +- "This role is too broad" is better than "why did you use such a wide role?" +- Offer context and suggestions, not just problems + +Infrastructure review often involves calling out potential security or data +integrity issues. That is a healthy part of the process — not a criticism of +the author's ability. + +## When you are not sure + +If you see something you do not fully understand, ask rather than guessing. +A question is a legitimate and valuable review comment: + +> **Question:** I am not familiar with this resource type. The +> `google_compute_backend_service` with `load_balancing_scheme = "INTERNAL"` +> — is this used for internal load balancing only, or can it receive external +> traffic? Want to make sure I understand the network exposure here. + +Asking a question is not a sign of inexperience. It often surfaces important +context that should be in the PR description or code comments anyway. diff --git a/terraform-review/skills-expectations.qmd b/terraform-review/skills-expectations.qmd new file mode 100644 index 0000000..6ebdb20 --- /dev/null +++ b/terraform-review/skills-expectations.qmd @@ -0,0 +1,128 @@ +--- +title: "Skills, expectations, and escalation" +order: 3 +--- + +::: {#sec-skills-expectations} +::: + +Reviewing infrastructure code is a skill that develops with practice. This +chapter explains what is expected of reviewers at different levels of experience, +how to calibrate your confidence, and when to ask for help. + +## What reviewers are expected to do + +A reviewer's job is not to be a human linter. Automated tools handle formatting +and many common errors. Your job is to apply judgement where tools cannot: + +- Is this the right approach for this problem? +- Does this change do what the author says it does? +- Could this change cause unintended harm, now or later? +- Is the code clear enough that someone else could maintain it? + +You do not need to know every Terraform resource type or every cloud provider +feature. You do need to read carefully, ask questions when something is unclear, +and be willing to say "I am not sure — let's check." + +## Expectations by experience level + +### Reviewing as someone new to infrastructure + +If you are new to Terraform or cloud infrastructure, you can still add value as +a reviewer. Focus on: + +- **Clarity and intent:** Is it clear what the code is doing? Would you be able + to explain it to someone else? If not, that is a signal to ask for better + comments or documentation. +- **The sensitivity checklist:** Are there any hardcoded values that look like + secrets? Does the PR description match what the code actually does? +- **Automated tool output:** Has the author run the required tools (`terraform + validate`, `checkov`, `tflint`)? Are there any unresolved findings? + +:::{.callout-note} +It is fine to approve a PR while noting that you have reviewed it from a content +clarity perspective and suggest another reviewer checks the technical +implementation. Saying what your review covers is more honest and useful than +silence. +::: + +### Reviewing as someone with some infrastructure experience + +If you have written Terraform before but are not a platform specialist, you +should additionally check: + +- **Resource configuration:** Are the settings appropriate? Are required fields + set? Are optional fields that have security implications (logging, encryption, + public access) addressed? +- **IAM and access:** Does the code follow least-privilege principles? Are roles + specific and scoped correctly? +- **Environment targeting:** Does the change go to the right environment + directory? Does it affect only what it should? +- **The plan:** If a plan output is attached, does it show the expected changes? + Are there any unexpected replacements? + +### Reviewing as an experienced platform engineer + +If you have deep infrastructure experience, you should additionally check: + +- **State implications:** Could this change cause Terraform to lose track of + existing resources? Are there resources being renamed or moved that require + `terraform state mv`? +- **Module design:** If the change touches a shared module in `modules/`, what + is the impact on all environments that use it? +- **Provider and version constraints:** Are the pinned versions appropriate? + Are there known issues with the specified versions? +- **Security posture:** Does the infrastructure posture align with the + environment's classification requirements? + +## Calibrating your confidence + +You do not need to be certain about everything you review. A useful rule of +thumb: + +- **Confident:** You understand what the code does and why. You can approve or + request changes with clear reasoning. +- **Uncertain:** Something looks odd or unfamiliar. Ask a question in the review + rather than guessing. "Can you explain why X is configured this way?" is a + legitimate and valuable review comment. +- **Out of your depth:** The change touches something you do not understand well + enough to assess. This is a signal to escalate. + +Approving a PR you do not understand is more dangerous in infrastructure than in +application code, because the consequences can be immediate and hard to reverse. +When in doubt, ask. + +## When to escalate + +Escalate to a more experienced platform engineer when: + +- The change affects production or pre-production environments and you are not + confident you understand all the implications +- The change modifies shared modules used by multiple environments +- The change touches IAM, network perimeter rules, or security controls +- The plan shows resource replacements (`-/+`) that are not clearly explained + and expected +- The automated tools report findings that the author has dismissed without a + clear explanation +- You have identified a potential secret or sensitive value in the code + +Escalation is not a failure. It is the correct response to uncertainty in a +context where mistakes have real consequences. + +## The author's responsibilities + +Reviewers can only do their job well if the author has done theirs. As an +author, you should: + +- Write a clear PR description that explains what changed and why +- Attach or link to the `terraform plan` output for any non-trivial change +- Run and pass all required automated tools before requesting review +- Respond to review comments promptly and constructively +- Break large changes into smaller PRs where possible — the smaller the change, + the better the review + +:::{.callout-tip} +A PR that changes one thing is much easier to review than a PR that changes ten +things. If you find yourself writing a long list of changes, consider whether +they can be submitted as separate PRs. +::: diff --git a/terraform-review/tools.qmd b/terraform-review/tools.qmd new file mode 100644 index 0000000..9c22538 --- /dev/null +++ b/terraform-review/tools.qmd @@ -0,0 +1,216 @@ +--- +title: "Review tools" +order: 5 +--- + +::: {#sec-tools} +::: + +Several tools can automate checks that would otherwise need to be done manually. +Running these before requesting a review — and checking their output as part of +your review — saves time and catches a consistent set of common problems. + +This chapter covers the four tools used in this organisation: + +- `terraform fmt` — code formatting +- `terraform validate` — basic syntax and configuration validation +- `tflint` — linting for best practices and provider-specific rules +- `checkov` — security and compliance scanning + +These tools complement each other. None of them replaces human review. They +catch different things, and all four should pass before a PR is merged. + +## `terraform fmt` + +`terraform fmt` reformats Terraform code to the canonical style defined by +HashiCorp. It handles indentation, spacing, and alignment of `=` signs. + +**Why it matters for review:** Consistent formatting makes diffs easier to read. +A PR that mixes formatting changes with functional changes makes it harder to +spot the actual logic change. + +**Run it:** + +```bash +# Format all files in the current directory and subdirectories +terraform fmt -recursive + +# Check without writing (useful in CI) +terraform fmt -recursive -check +``` + +**As a reviewer:** If a PR introduces files with inconsistent formatting, ask +the author to run `terraform fmt` first. Most CI pipelines run this as a check. + +:::{.callout-tip} +If you are using VS Code, the +[HashiCorp Terraform extension](https://marketplace.visualstudio.com/items?itemName=HashiCorp.terraform) +can run `terraform fmt` automatically on save. +::: + +## `terraform validate` + +`terraform validate` checks that the configuration is syntactically correct and +internally consistent. It checks that all required arguments are present, that +references between resources are valid, and that variable types are correct. + +**What it does not check:** It does not make API calls to your cloud provider. +It does not know whether a referenced project, service account, or resource +actually exists. It does not check whether your IAM bindings are appropriate. + +**Run it:** + +```bash +# Initialise first (needed to download providers) +terraform init + +# Then validate +terraform validate +``` + +**As a reviewer:** `terraform validate` should always pass. A PR that does not +pass validation has a fundamental configuration error. Check that the CI +pipeline includes a validate step and that it passed. + +:::{.callout-note} +`terraform validate` requires the providers to be initialised (`terraform init`) +but does not need real credentials or network access to a cloud environment. It +is safe to run locally. +::: + +## `tflint` + +[TFLint](https://github.com/terraform-linters/tflint) is a linter that catches +issues that `terraform validate` misses. It includes rules for: + +- Invalid resource configurations that Terraform accepts but cloud providers + reject +- Deprecated arguments that still parse but will be removed in future provider + versions +- Missing required tags or labels +- Naming convention violations + +TFLint works through a plugin model. To enable provider-specific rules, install +the relevant plugin. For GCP, use the +[`tflint-ruleset-google`](https://github.com/terraform-linters/tflint-ruleset-google) +plugin. + +**Run it:** + +```bash +# Initialise plugins (first time or after config changes) +tflint --init + +# Run against the current directory +tflint + +# Run recursively across all modules +tflint --recursive +``` + +**Configuration:** TFLint is configured via a `.tflint.hcl` file. The +`terraform-template` repository includes a shared configuration in `configs/`. +Use it as your starting point. + +**As a reviewer:** Check that TFLint has been run and that any findings are +either fixed or have a clear justification for being ignored. A `.tflint.hcl` +that disables most rules is a red flag — ask why the rules are disabled. + +:::{.callout-note} +**GCP-specific:** The `tflint-ruleset-google` plugin can catch GCP-specific +issues such as invalid machine types, deprecated API versions, and missing +required labels. Enable it in your `.tflint.hcl`: + +```hcl +plugin "google" { + enabled = true + version = "0.30.0" + source = "github.com/terraform-linters/tflint-ruleset-google" +} +``` +::: + +## `checkov` + +[Checkov](https://www.checkov.io/) is a static analysis tool for infrastructure +as code. It scans Terraform (and other IaC formats) against a library of +security and compliance checks. These checks cover things like: + +- Storage buckets with public access enabled +- Databases without encryption at rest +- Firewall rules that allow unrestricted access +- Missing logging or audit configuration +- IAM roles with excessive permissions + +**Install it:** + +```bash +pip install checkov +``` + +**Run it:** + +```bash +# Scan a directory +checkov -d terraform/01_sandbox + +# Output results in a more readable format +checkov -d terraform/01_sandbox --output cli + +# Run only specific check IDs +checkov -d . --check CKV_GCP_28,CKV_GCP_29 +``` + +**Understanding the output:** Checkov reports findings as `PASSED`, `FAILED`, +or `SKIPPED`. A failed check means the code does not meet a defined security +standard. Review failed checks carefully before deciding whether to fix them +or accept the risk. + +**Suppressing findings:** Some findings may be intentionally accepted — for +example, a sandbox storage bucket that is publicly readable for a legitimate +reason. Use inline suppressions with a comment explaining the rationale: + +```hcl +resource "google_storage_bucket" "public_dataset" { + name = "my-public-dataset" + + #checkov:skip=CKV_GCP_62:Bucket is intentionally public for published open data + #checkov:skip=CKV_GCP_78:Access logging not required for public read-only bucket +} +``` + +**As a reviewer:** Every suppressed finding should have a comment. If a finding +is suppressed without explanation, ask why. If a finding is suppressed with an +explanation that does not make sense for the environment (for example, disabling +an encryption check in production), block the PR. + +:::{.callout-note} +**GCP-specific:** Checkov includes a large set of GCP-specific checks (prefixed +`CKV_GCP_`). The most important ones to watch for in review include: + +- `CKV_GCP_28` — ensure that Cloud Storage buckets are not anonymously or + publicly accessible +- `CKV_GCP_62` — ensure that Cloud Storage buckets have access logging enabled +- `CKV_GCP_78` — ensure that Cloud SQL instances have deletion protection enabled +- `CKV_GCP_111` — ensure that Cloud Storage buckets have uniform bucket-level + access enabled +::: + +## Summary + +| Tool | What it checks | Must pass? | +|---|---|---| +| `terraform fmt` | Code formatting | Yes — no exceptions | +| `terraform validate` | Syntax and internal consistency | Yes — no exceptions | +| `tflint` | Best practices, deprecated usage, provider rules | Yes — findings must be fixed or suppressed with a comment explaining why | +| `checkov` | Security and compliance posture | Yes — findings must be fixed or suppressed with a comment explaining why | + +All four tools must have a clean result before a PR is approved. "Clean" means +either all checks pass, or any that do not pass are explicitly suppressed with +an inline comment explaining the reason. A finding that is silently ignored — +no suppression, no explanation — is not acceptable. + +The PR author is responsible for running all tools and resolving or suppressing +findings before requesting review. Most tools also run in CI. As a reviewer, +check the CI status and read every suppression before approving — an unexplained +suppression should be treated the same as a failing check. diff --git a/terraform-review/what-to-review.qmd b/terraform-review/what-to-review.qmd new file mode 100644 index 0000000..5bdbba3 --- /dev/null +++ b/terraform-review/what-to-review.qmd @@ -0,0 +1,198 @@ +--- +title: "What to review" +order: 4 +--- + +::: {#sec-what-to-review} +::: + +This chapter walks through the specific things to look for when reviewing +Terraform code. It also explains how infrastructure review differs from +reviewing Python or other application code — which matters if your background +is primarily in data science or software development. + +## How IaC review differs from Python code review + +If your mental model of code review comes from reviewing Python, R, or similar +languages, it is worth understanding the key differences before you start. + +| | Python / application code | Terraform / infrastructure code | +|---|---|---| +| **Effect of a bug** | Wrong output, crash, or bad data | Deleted resource, open firewall, wrong IAM binding | +| **Reversibility** | Usually easy to redeploy a fix | Some changes are destructive and hard to undo | +| **State** | Stateless between runs (mostly) | Terraform tracks state; divergence causes problems | +| **Testing** | Unit tests, integration tests | `terraform validate`, plan inspection, limited test frameworks | +| **Secrets** | Should not appear in code | Must not appear in code *or* state files | +| **Side effects** | Usually constrained to the application | Changes affect shared cloud environments immediately on apply | +| **Dependencies** | Import errors are caught at startup | Provider version mismatches may only surface at apply time | +| **Refactoring** | Rename a variable, update references | Rename a resource, Terraform may destroy and recreate it | + +The most important difference is **consequences**. A Python bug in development +causes a failed test or an error in a notebook. A Terraform mistake in production +can cause an outage, a data loss event, or a security incident. This is why +review — especially for changes targeting non-development environments — requires +a higher level of care. + +## The structure of a Terraform change + +When you open a Terraform pull request, you will typically see changes to some +combination of: + +- **Root module files** (in an environment directory like `01_sandbox/`): + `main.tf`, `variables.tf`, `outputs.tf`, `versions.tf`, `backend.tf` +- **Module files** (in `modules//`): the reusable building blocks + called by root modules +- **Variable definitions** (`.tfvars` files — configuration values for a + specific environment, such as project IDs, regions, and feature flags. + These should be committed to version control. Secrets must not appear in + them; see [Review principles](principles.qmd) for the no-secrets-in-code + principle) + +Understanding which of these has changed tells you a lot about the potential +impact. A change to a shared module in `modules/` affects every environment that +calls it. A change inside `01_sandbox/` only affects sandbox. + +## What to check + +### The `versions.tf` or `terraform {}` block + +Every root module should declare its required Terraform version and provider +versions. Check: + +- Is `required_version` set to a specific version or a tight constraint (e.g. + `>= 1.5.0, < 2.0.0`)? An unpinned version (e.g. `>= 1.0`) is a risk. +- Are provider versions pinned to specific versions or tight ranges in + `required_providers`? Provider updates can contain breaking changes. +- Does the declared version match what is actually in use in the team's + toolchain? + +```hcl +# Good: pinned to a tight range +terraform { + required_version = ">= 1.9.0, < 2.0.0" + + required_providers { + google = { + source = "hashicorp/google" + version = "~> 6.0" + } + } +} +``` + +### Variables and inputs + +Check: + +- Does each variable have a `description`? A variable without a description is + not self-documenting and makes review harder. +- Are variable types declared? Untyped variables accept any value, which can + cause confusing errors at apply time. +- Are default values appropriate? A default of `""` or `null` for a variable + that is always required just adds confusion. +- Are any variables marked `sensitive = true`? This is correct for + configuration values you want redacted from plan output (such as internal + endpoint URLs or restricted identifiers), but it is not a substitute for + using a secret manager. Passwords, tokens, and API keys should not be + Terraform variables at all — they should be retrieved from a secrets + manager at runtime. + +```hcl +# Good: typed, described, and redacted from plan output +variable "reporting_endpoint" { + description = "Internal URL for the reporting service. Redacted from plan output." + type = string + sensitive = true +} + +# Avoid: secrets should come from a secrets manager, not variables +# variable "db_password" { +# type = string +# sensitive = true # redacts from output, but the value still flows +# } # through Terraform state — use a secrets manager instead +``` + +### Resources + +For each resource being created, modified, or destroyed: + +- Does the resource name clearly describe what it is? `google_storage_bucket.main` + tells you nothing; `google_storage_bucket.pipeline_outputs` is better. +- Are required arguments present? Check the provider documentation if you are + unsure. +- Are security-relevant optional arguments set explicitly? + - Encryption: is at-rest encryption configured? + - Logging: is audit or access logging enabled? + - Public access: are public access settings explicitly set to their expected + value (usually disabled)? + - Deletion protection: for stateful resources such as databases, is deletion + protection enabled in production? + +### IAM bindings + +IAM changes deserve particular scrutiny because they affect who can do what in +your cloud environment. Check: + +- Is the role the most specific one that satisfies the need? (See + [Review principles](principles.qmd) for the least privilege principle.) +- Does the member (service account, user, or group) actually need this binding? +- Is the binding at the appropriate resource level — project, folder, or + specific resource? Prefer specific resource bindings over project-level ones + where possible. +- Are `google_project_iam_binding` and `google_project_iam_member` used + correctly? Using `binding` when you mean `member` will remove all other + members from that role. + +:::{.callout-important} +`google_project_iam_binding` is **authoritative** for its role — it will remove +any other members not listed in the resource. `google_project_iam_member` adds a +single member without affecting others. Using the wrong one is a common and +potentially serious mistake. +::: + +### Outputs + +Check: + +- Are outputs that expose sensitive values (connection strings, generated + passwords) marked `sensitive = true`? +- Are outputs clearly described? +- Are there outputs that expose more information than necessary to callers of + the module? + +### Lifecycle rules + +`lifecycle` blocks let you override Terraform's default behaviour. They are +occasionally necessary but can also mask problems. Check: + +- `prevent_destroy = true` — is this set on stateful resources (databases, + storage) in non-sandbox environments? Its absence in production is a risk. +- `ignore_changes` — is it clear why certain attributes are ignored? If a + resource attribute is being ignored, there should be a comment explaining why. +- `create_before_destroy` — does this make sense for this resource? Some + resources cannot have two instances with the same name at the same time. + +### Local values and data sources + +- Are `data` sources used to look up existing resources rather than hardcoding + IDs or names? This is generally better practice. +- Are `local` values used to avoid repeating the same expression multiple times? + Duplication in Terraform code is a maintenance risk. + +## Common mistakes to look for + +- **Hardcoded project IDs or resource IDs** — these break when the code is used + in a different environment and make it harder to promote changes through the + environment pipeline. +- **Hardcoded regions** — use variables so the code is portable. +- **Missing or empty descriptions** on variables and outputs. +- **`count` or `for_each` used incorrectly** — changing from `count` to + `for_each` or vice versa on an existing resource will cause Terraform to + destroy and recreate all instances. +- **Sensitive values in `locals` or `outputs` without `sensitive = true`** — + these will appear in plan and apply output in plain text. +- **Secrets in `.tfvars` files** — `.tfvars` files record each environment's + configuration and should be committed to version control. Secrets — + passwords, tokens, API keys — must never appear in them. If you spot + anything that looks like a credential, block the PR. Secrets belong in a + secrets manager, not in files. diff --git a/version.txt b/version.txt index 6e8bf73..0d91a54 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -0.1.0 +0.3.0