Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

terraform_remote_state id returns state's datetime #7982

Closed
gir opened this issue Aug 4, 2016 · 10 comments
Closed

terraform_remote_state id returns state's datetime #7982

gir opened this issue Aug 4, 2016 · 10 comments

Comments

@gir
Copy link

gir commented Aug 4, 2016

I don't know if it's not documented, or I can't find it, but if I have a remote state with output id I want to reference, it returns the datetime of the last update to the terraform state file. The state doesn't even need an output id and referencing "${data.terraform_remote_state.foo.id}" will still return the datetime.

Terraform Version

0.7.0

Affected Resource

  • terraform_remote_state

Terraform Configuration Files

data "terraform_remote_state" "foo" {
  backend = "s3"

  config {
    bucket = "foo"
    region = "us-east-1"
    key    = "bar/terraform.tfstate"
  }
}

resource "aws_security_group" "foo" {
  ...
  vpc_id = "${data.terraform_remote_state.foo.id}"
  ...
}

Expected Behavior

Previously "${terraform_remote_state.foo.id}" would return the value of the id attribute.

Actual Behavior

Updating to 0.7.0 and using terraform_remote_state as a data instead of a resource when referencing id returns the datetime of the state file.

Steps to Reproduce

  1. terraform plan or terraform apply
    • Output
...
vpc_id: "vpc-00000000" => "2016-08-04 16:27:35.288448516 +0000 UTC" (forces new resource)
...
@apparentlymart
Copy link
Member

Hi @gir. Sorry for the problem here.

"id" is a special attribute in Terraform which contains the value that Terraform uses to connect a resource cached in the state with a real resource in the target system. terraform_remote_state just sets this to the timestamp as a placeholder since there is no reasonable id to use, but Terraform core still considers "id" to be special in this case, unfortunately.

The only workaround for the moment is to use an attribute name other than "id". Perhaps in future we will relax the requirement for data sources to set ids since they are not really that useful in this case... they primarily exist to allow Terraform to diff and update managed resources, and data resources just inherited this special case because they share a lot of behavior in common with managed resources.

@apparentlymart
Copy link
Member

I've added the "documentation" tag to this because it seems like we should call this out specifically in the terraform_remote_state docs as one of the reserved keys you can't use in your remote state.

@jedi4ever
Copy link

@apparentlymart I guess it make sense to check all output variables so they don't use the '.id' field

@andersonkyle
Copy link

Ran into this recently. It certainly should be documented, but also I think a warning/error should be thrown (during validate?). These silent exceptions can really throw a person off track.

The "id" attribute is a commonly exported default attribute, for example aws_vpc.

If the intention is to reserve the "id" attribute I think all of the providers should follow suit as well. For the vpc example, instead of aws_vpc.id it could be aws_vpc.vpc_id

If the providers aren't updated it requires the user to write outputs for attributes that are already exported which seems unnecessary.

@robinbowes
Copy link

I lost half a day to this problem. Grrrr.

If id is a special attribute then the user should not be allowed to write code using id as an output name.

@apparentlymart
Copy link
Member

In order to avoid these surprising collisions, in the next major release of Terraform we are planning to change the interface of this data source so that the outputs belong to a separate map value rather than being exported directly as resource attributes. (This collision also applies the argument names backend and config.)

resource "aws_security_group" "foo" {
  # ...
  vpc_id = data.terraform_remote_state.foo.outputs.id
  # ...
}

While we'd always prefer to avoid this sort of breaking change, this approach is the better long-term solution because it means that there's no risk that new features in this data source will cause collisions with existing outputs in future, and the next release is a good opportunity to do this since we'll be including a tool to automatically migrate certain changes anyway, and so that tool should be able to deal with adding the .outputs accessor to existing configurations.

With that in mind, I'm going to re-label this as a config-related issue so it'll be included in our release preparation process and we can verify that it's been fixed and the migration tool is working as expected before we close this out.

Sorry to everyone for the confusion and frustration here.

@robinbowes
Copy link

@apparentlymart I'd rather you got all the breaking changes out there sooner rather than later. The longer you leave it, the more users you have, the harder it is to change.

I've said this elsewhere several times, but it bears repeating here:

The concept of Terraform is fantastic. It's doing for infrastructure management what tools like puppet did for SCM a few years ago. Sadly, it is making all the same mistakes. Terraform feels like puppet did 8 years ago - full of promise, but with many frustrating holes in functionality, bugs, and missing features. I really hope you can manage to do what puppet did and create a first-class DSL and module infrastructure which is powerful, flexible, and expressive while remaining easy to use. I hope it doesn't take 8 years.

Thanks for updating this issue. I'm looking forward to the next release.

PS. by "major" release, do you mean semantically major, or 0.12.0?

@apparentlymart
Copy link
Member

Terraform Core does not use semantic versioning because that scheme is designed for libraries, not for applications. With that said, within the 0.X.Y series we consider incrementing "X" to be a major release (breaking changes), while "Y" is a minor release (enhancements and bug fixes with no breaking changes intended).

v1.0.0 will represent the point where we will be explicitly no further breaking changes for significant parts of Terraform, but in practice we intend v0.12 to be the last significant breaking change to the configuration language before that release, and are holding off on declaring it as such only because we want to have room to make minor changes in response to v0.12 feedback, and we'll also need to make some other non-config-related changes in a subsequent release to fix other usability problems. v0.12 is a big release specifically because we are front-loading all of the expected configuration language breaking changes into it.

Some more details on the above were included in the section at the end of the Terraform Provider Versioning announcement, which is several months old at this point but remains accurate.

@apparentlymart
Copy link
Member

Hi all! Sorry for the long silence here.

In Terraform v0.12.0-alpha1 we now have the change I mentioned above where the outputs for remote state are exported in a map attribute called outputs rather than each one being its own attribute. This then avoids collisions with the other attributes of this resource such as id, backend, etc.

This means that the configuration in the opening comment on this issue would be rewritten like this:

data "terraform_remote_state" "foo" {
  backend = "s3"

  config = {
    bucket = "foo"
    region = "us-east-1"
    key    = "bar/terraform.tfstate"
  }
}

resource "aws_security_group" "foo" {
  ...
  vpc_id = data.terraform_remote_state.foo.outputs.id
  ...
}

Since the backends themselves were not heavily tested prior to the alpha, we've since found some other issues with them including #19217 which make it hard to test out this behavior in v0.12.0-alpha1, but it's possible to see it working using backend = "local" with a local file, as I did to verify #11534, and so I'm going to close this out to reflect that this particular bit of work is complete even though there are other issues to be dealt with separately.

This change to include .outputs in references to remote state is one of the updates that the automatic configuration upgrade tool coming in the v0.12.0 final release should deal with. That tool is not yet complete as of v0.12.0-alpha1, but after final release it will not be necessary to manually update uses of the old style.

Thanks for reporting this, and thanks for your patience while we did the groundwork to get this fixed.

@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@hashicorp hashicorp locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants