hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.85k stars 9.19k forks source link

[Bug]: Data resource causing cycle error with default_tags #28772

Open thenom opened 1 year ago

thenom commented 1 year ago

Terraform Core Version

1.3.3

AWS Provider Version

4.49.0

Affected Resource(s)

data.aws_caller_identity

Expected Behavior

No cycle error

Actual Behavior

The data resource seems to be included in the default tagging which is triggering the cycle error

Relevant Error/Panic Output Snippet

Error: Cycle: provider["registry.terraform.io/hashicorp/aws"], module.tags.data.aws_caller_identity.current, module.tags.local.using_account_id (expand), module.tags.local.output_tags (expand), module.tags.output.tags (expand)

Terraform Configuration Files

Main.tf

provider "aws" {
  region = "eu-west-1"

  default_tags {
    tags = module.tags.tags
  }
}

module "tags" {
  source = "./module"
}

module/module.tf

data "aws_caller_identity" "current" {}

locals {
  using_account_id = var.account_id_override == "" ? data.aws_caller_identity.current.id : var.account_id_override

  output_tags = {
    "account_id" = local.using_account_id
  }
}

output "tags" {
  description = "The final tags map output"
  value       = local.output_tags
}

Steps to Reproduce

Run in the folder:

terraform validate

Debug Output

No response

Panic Output

No response

Important Factoids

This is a minimised version of my larger module. This larger module use case is to supply a list of default tags based on the account it is run in but allow for a use case when terraform is run in a different account to manage another, hence the account_id_override variable.

I cant see why the data resource is even involved in the cycle check with regards to default_tags but there might be a mis-understanding on my part.

References

No response

Would you like to implement a fix?

None

github-actions[bot] commented 1 year ago

Community Note

Voting for Prioritization

Volunteering to Work on This Issue

thenom commented 1 year ago

This also seems to have the same issue when the data provider is in the parent: main.tf

provider "aws" {
  region = "eu-west-1"

  default_tags {
    tags = module.tags.tags
  }
}

data "aws_caller_identity" "current" {}

module "tags" {
  source = "./module"

  account_id_override = data.aws_caller_identity.current.id
}

module/module.tf

locals {
  using_account_id = var.account_id_override

  output_tags = {
    "account_id" = local.using_account_id
  }
}

output "tags" {
  description = "The final tags map output"
  value       = local.output_tags
}

The only way i have found to fix this is add an empty AWS provider to the module which i am guessing overrides the default_tags of the parent but now i get the deprecation warning on validate: module/module.tf

provider "aws" {}

data "aws_caller_identity" "current" {}

locals {
  using_account_id = var.account_id_override == "" ? data.aws_caller_identity.current.id : var.account_id_override

  output_tags = {
    "account_id" = local.using_account_id
  }
}

output "tags" {
  description = "The final tags map output"
  value       = local.output_tags
}
β•·
β”‚ Warning: Redundant empty provider block
β”‚
β”‚   on module/module.tf line 1:
β”‚    1: provider "aws" {}
β”‚
β”‚ Earlier versions of Terraform used empty provider blocks ("proxy provider configurations") for child modules to declare their need to be passed a provider configuration by their callers. That approach was ambiguous and is now deprecated.
β”‚
β”‚ If you control this module, you can migrate to the new declaration syntax by removing all of the empty provider "aws" blocks and then adding or updating an entry like the following to the required_providers block of module.tags:
β”‚     aws = {
β”‚       source = "hashicorp/aws"
β”‚     }
justinretzolk commented 1 year ago

Hey @thenom πŸ‘‹ Thanks for taking the time to raise this! This comes down to the implicit provider inheritance behavior of modules.

Because of the implicit provider inheritance, your module is attempting to use the provider from the provider block declared in the root module. This provider block is, however, has a configuration that is dependent upon the output from the module in order to set the default_tags. This is why you're receiving the cycle error -- the provider is dependent on the module, which is dependent on the provider.

Similarly, when you attempt to call data.aws_caller_identity in the root module, pass it to the tags module, and then use the output from the tags module to configure the AWS provider instance that's used by data.aws_caller_identity, you have a cycle.

One way to get around this would be to have a second AWS provider which is not dependent on the tags module, which is passed to the tags module (explicitly passing the secondary provider. In that case, the root module would look something like this:

# main.tf

terraform {
  required_providers {
    aws = {
      source = "hashicorp/aws"
    }
  }
}

provider "aws" {
  region = "eu-west-1"

  default_tags {
    tags = module.tags.tags
  }
}

provider "aws" {
  region = "eu-west-1"
  alias  = "bootstrap"
}

module "tags" {
  source = "./module"
  providers = {
    aws = aws.bootstrap
  }
}

Adding the following to the module's configuration is good practice, and will prevent any extra warnings in the logs, though it's not strictly required.

# module/module.tf

terraform {
  required_providers {
    aws = {
      source = "hashicorp/aws"
    }
  }
}

# ...other configuration

This would enable the workflow that you described in this issue, though it would notably instantiate another instance of the AWS provider, so will take up a bit more memory, if that matters in your use-case.

thenom commented 1 year ago

Hi @justinretzolk,

Thanks for the quick reply.

What I am struggling with is why the data resource is affected by the default_tags at all to even cause the cycle error. There is no resource to tag in the first place, surely it being a data resource block and not a standard resource block makes the default_tags unaffective.

With regards to the additional provider in the parent, that should work yes but my concern with this is that as default_tags is widely used (and in turn this module for it), then adopting that method would class removing the deprecation a breaking major change. Every user would need to do this due to the new version breaking their terraform unless they applied the same method.

Thanks

thenom commented 1 year ago

Is there anymore view on this regards why the data resource is getting 'tagged' and in turn causing this cycle error?

justinretzolk commented 1 year ago

Hey @thenom πŸ‘‹ Thanks for following up with the additional question! That definitely helps me get an idea of where you're hung up.

This comes down to how Terraform works at its core. When Terraform first starts up, before it can interact with a given platform (in this case AWS), the associated (Terraform) provider must be configured. This tells Terraform how to interact with the resources that the (Terraform) provider offers, and so must be done before the provider is used in any way (regardless of exactly what you're doing with the provider).

In your case, configuration of the Terraform provider is implicitly dependent upon the tags module (by way of of the default_tags block having tags = module.tags.tags). The tags module uses the AWS Provider (to access the data.aws_caller_identity resource), and so is dependent on the AWS Provider. This is where the cycle occurs.

I hope that makes a bit more sense. If not, I'd be happy to continue to discuss!

thenom commented 1 year ago

Hi @justinretzolk, thanks for the reply.

I understand how the inheritance works i just dont get why a data resource should be involved in it at all. If i rephrase it slightly it might explain my issue a bit more.

If the data.aws_caller_identity is inheriting tags for whatever reason, what exactly is it doing with them other than throwing them away once its done? If this is the case, surely it would be more beneficial to have it not inherit and\or not be included in the tags cycle check. This would remove this issue ever occurring unnecessarily as technically there is actually no cycle problem here due to that data resource not having any dependancies on those tags.

justinretzolk commented 1 year ago

Hey @thenom πŸ‘‹ Sorry it's taken so long to get back to you on this. In this case, it's more helpful to look at the cycle from a different direction -- rather than thinking of it as the data source needing to receive the tags, it's the opposite; the provider needing the tags in order to be instantiated, which can't happen without first reading the data source (which would require that the provider be instantiated).

When Terraform is using a provider, that provider must first be configured before it may be used. In the configuration you've provided, this includes configuring the default_tags argument. In your configuration, in order for the default_tags argument to be populated, module.tags.tags must be known, so the provider is dependent on module.tags. That module, however, is attempting to use that same provider (that has not yet been configured due to the dependency on the module) in order to read data.aws_caller_identity.current. This leads to the provider configuration being dependent on the module (to configure the default_tags), which is in turn dependent on the provider (in order to read the data source), which is where the cycle is introduced.