Improve cycle debugging experience for users

[lijok]

Terraform Version

Terraform v0.13.2
+ provider registry.terraform.io/hashicorp/archive v1.3.0
+ provider registry.terraform.io/hashicorp/aws v3.5.0
+ provider registry.terraform.io/hashicorp/null v2.1.2
+ provider registry.terraform.io/hashicorp/random v2.3.0
+ provider registry.terraform.io/terraform-providers/grafana v1.5.0

Terraform Configuration Files

# main.tf
terraform {
  backend local {
    path = "terraform.tfstate"
  }
}

provider aws {
  alias = "us_east_1"

  region                  = "us-east-1"
  profile                 = "development"
}

terraform {
  required_version = "= 0.13.2"

  required_providers {
    aws = {
      version = "= 3.5.0"
      source  = "hashicorp/aws"
    }
  }
}

variable deploy_cloudfront_us_east_1 {default=0}
variable deploy_s3_bucket_us_east_1 {default=0}

module s3_bucket_us_east_1 {
  source = "./module_a"
  providers = {
    aws = aws.us_east_1
  }
  for_each = var.deploy_s3_bucket_us_east_1 == 1 ? {this = "this"} : {}

  name = "my-test-s3-bucket"
  cloudfront_oai = [module.cloudfront_us_east_1[each.key].oai_id]
}

module cloudfront_us_east_1 {
  source = "./module_b"
  providers = {
    aws = aws.us_east_1
  }
  for_each = var.deploy_cloudfront_us_east_1 == 1 ? {this = "this"} : {}

  bucket_name = module.s3_bucket_us_east_1[each.value].bucket_name
}

# module_a/main.tf
variable cloudfront_oai {type=list(string)}
variable name {}
output bucket_name {value=aws_s3_bucket.this.bucket}

data aws_iam_policy_document this {

  statement {
    actions = [
      "s3:GetObject"
    ]
    effect = "Allow"
    resources = [
      "arn:aws:s3:::${var.name}/*",
    ]
    principals {
      type = "AWS"
      identifiers = [
        for oai in var.cloudfront_oai :
        "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity ${oai}"
      ]
    }
  }
}

resource aws_s3_bucket this {
  bucket              = var.name
  acl                 = "private"
  policy              = data.aws_iam_policy_document.this.json
  force_destroy       = false
}

# module_b/main.tf
variable bucket_name {}

resource aws_cloudfront_origin_access_identity this {
  comment = "identity.whatever.com"
}

resource aws_cloudfront_distribution this {
  comment             = "Managed by Terraform"
  enabled             = true
  is_ipv6_enabled     = true
  price_class         = PriceClass_100
  wait_for_deployment = true

  origin {
    domain_name = "${var.bucket_name}.s3.amazonaws.com"
    origin_id   = var.bucket_name

    s3_origin_config {
      origin_access_identity = aws_cloudfront_origin_access_identity.this.id
    }
  }

  default_cache_behavior {
    allowed_methods = [
      "GET",
      "HEAD",
      "OPTIONS"
    ]
    cached_methods = [
      "GET",
      "HEAD"
    ]
    compress               = false
    smooth_streaming       = false
    target_origin_id       = var.bucket_name
    viewer_protocol_policy = "redirect-to-https"

    forwarded_values {
      query_string = false

      cookies {
        forward = "none"
      }
    }
  }

  viewer_certificate {
    cloudfront_default_certificate = true
  }

  restrictions {
    geo_restriction {
      restriction_type = "none"
    }
  }
}

Debug Output

Crash Output

Expected Behavior

Resources deployed

Actual Behavior

Error: Cycle: module.cloudfront_us_east_1.aws_cloudfront_distribution.this, module.s3_bucket_us_east_1 (close), module.s3_bucket_us_east_1.aws_s3_bucket.this, module.s3_bucket_us_east_1.output.bucket_name (expand), module.cloudfront_us_east_1.var.bucket_name (expand), module.cloudfront_us_east_1 (close), module.s3_bucket_us_east_1.var.cloudfront_oai (expand), module.s3_bucket_us_east_1.data.aws_iam_policy_document.this

Steps to Reproduce

1. terraform init
2. terraform apply

Additional Context

If you replace each.value in the module call variables with a static "this", this problem dissapears

References

[jbardin]

Hi @lijok

Thanks for submitting the example here. While the cycle is initially expected in this configuration, since the s3_bucket_us_east_1 and s3_bucket_us_east_1 module calls reference each other, the inconsistent behavior definitely make this confusing.

When modules are not expanded in any way, they essentially don't exist within the operation of terraform core, only acting as namespaces within the configuration. Once expansion needs to happen, every instance within those modules needs to be expander later based on the parent module's expansion, so modules themselves behave much more like discrete objects, and referencing rules similar to resources apply more strictly.

The tooling to detect and troubleshoot these cycles in complex configurations is definitely something that can be improved in the cli.

[lijok]

@jbardin So under what circumstances do modules get expanded, and when do they not?

[jbardin]

@lijok, modules are expanded when you use count or for_each. This means that rather than the implied single instance of the module, everything within the module and submodules depends on the modules being "expanded" to determine the final number of instances and their configuration values.

[apparentlymart]

Another way to think about this, in case it's helpful, is to think of the for_each or count expression as being a separate dependency node in its own right, with its own dependencies. Anything defined in the module, including the module's output values. implicitly depends on the for_each or count expression because Terraform must determine how many to evaluate before it can evaluate any of them.

Conversely, the reason that it works when you don't have for_each or count is that then there's no repetition expression for everything in the module to depend on. As previously noted, modules don't normally exist by themselves in the dependency graph -- the dependencies just propagate through individual input variables and output values -- so in normal circumstances it's possible to create dependency chains that thread in opposite directions through a pair of modules.

[marco-m-pix4d]

I stumbled upon a cycle with a "(expand)". I found @jbardin explanation above useful:

modules are expanded when you use count or for_each [..]

A possible improvement to the error message (although by all means not enough) is to augment "(expand)" with the cause.

Simplified example: Instead of the current

Error: Cycle: module.cloudfront_us_east_1.aws_cloudfront_distribution.this, 
module.s3_bucket_us_east_1.output.bucket_name (expand),
module.cloudfront_us_east_1.var.bucket_name (expand), ...

something like

Error: Cycle: module.cloudfront_us_east_1.aws_cloudfront_distribution.this, 
module.s3_bucket_us_east_1.output.bucket_name (expand, cause: "count"),
module.cloudfront_us_east_1.var.bucket_name (expand, cause: "for_each"), ...

[techdragon]

The cause as @marco-m-pix4d highlights, would be a very valuable addition towards making debugging cycles easier.