5  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.

5.1 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 contained 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.

5.2 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/<module-name>/): 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 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.

5.3 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?

# 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 GCP Secret Manager at runtime.

# 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 Secret Manager, not variables
# variable "db_password" {
#   type      = string
#   sensitive = true   # redacts from output, but the value still flows
# }                    # through Terraform state — use Secret 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 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.

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.

5.4 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 GCP Secret Manager, not in files.