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

Ability to Get/Set Only the State Resource Identifier (Without id Attribute Handling) #541

Closed
bflad opened this issue Aug 20, 2020 · 6 comments
Labels
enhancement New feature or request subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.

Comments

@bflad
Copy link
Contributor

bflad commented Aug 20, 2020

SDK version

v2.0.1

Use-cases

For some data sources and resources, there is no concept of an identifier. For example, the Terraform AWS Provider has quite a few "plural" data sources that return multiple objects of resource type. An id attribute is extraneous.

Due to certain changes in Terraform CLI 0.13.0 (potentially some behaviors rolling back in 0.13.1) and confirmed for future versions of Terraform CLI, unstable data source attributes are expected to show in the Terraform plan output. Providers now must be careful to ensure all attributes are stable, including the id attribute, even if it is meaningless.

Attempted Solutions

Trying to use the following inconsistently implemented or "this seems much harder than it should be" resource identifiers:

  • Static strings
  • resource.UniqueID()
  • time.Now()
  • Concatenating a bunch of values, manually
  • Hashing the a bunch of values, manually

Proposal

Personally I lean towards 1 here, as it is the least risky overall, but present other random thoughts on the subject as well for discussion points. Proposal 3 seems kind of interesting but it does further couple providers to "special" Terraform Plugin SDK logic in this project, meaning changing the SDK or muxing it with other SDKs could be more cumbersome.

One caveat to any of these changes is that historically every Terraform Resource (using this Terraform Plugin SDK anyways in all its historical forms) has always had an id attribute. This "absolute" will no longer be the case.

Proposal 1

Currently the (*helper/schema.ResourceData).SetId() receiver method combines two pieces of functionality:

  • Setting the underlying resource identifier
  • Setting the resource id attribute

While the related (*helper/schema.ResourceData).Id() receiver method performs the reverse of that functionality:

  • Getting the underlying resource identifier
  • If not found, getting the resource id attribute

It might make sense to add separate receiver methods that only work with the underlying resource identifier and do not work with the resource id attribute at all. For example:

  • func (d *ResourceData) GetResourceID() string
  • func (d *ResourceData) SetResourceID(v string)
  • (Super optional, but while we are here likely more clear than the d.SetId("") required today) func (d *ResourceData) RemoveFromState()

To further this concept and align with Terraform core's concepts of the resource identifier being wholly separate from the id attribute, it could be considered to deprecate the existing SetId() and Id() receiver methods in preference of the new ones above. Existing d.SetId() behavior could be preserved with:

"id": {
  Type: schema.TypeString,
  Computed: true,
},

// ...

d.SetResourceID(/* ... */)
d.Set("id", /* ... */)

Proposal 2

Another consideration would be approaching this issue differently, where the resource identifier is automatically calculated from the schema and state values. In this proposal, instead of calling SetId() manually, the resource schema could declare which attributes participate in the value generation, e.g.

&Resource{
  Identifier: []string{"attr1", "attr2"},
  Schema: map[string]*Schema{
    "attr1": { /* ... */ },
    "attr2": { /* ... */ },
  },
}

And the SDK automatically does things to manage the identifier outside the resource logic, except for triggering resource removal. This has a ton more work associated with it and plenty of caveats to think through, e.g.

  • It likely would not be safe for use with reading the information back out in a meaningful way.
  • What if some or all attribute values are empty?
  • Does anything happen when attributes need to be changed or removed?

Proposal 3

Another variation of proposal 2 is changing the state resource identifier to a be completely opaque value to the resource, such as a one-time UUID after successful creation/import, or a hash of the ResourceData/InstanceState. This removes the provider and resource logic from managing this identifier at all, except for triggering resource removal.

References

@bflad bflad added the enhancement New feature or request label Aug 20, 2020
@bflad
Copy link
Contributor Author

bflad commented Aug 20, 2020

To be extra confusing, this logic called by ReadDataSource does imply it would allow data source Read function logic to remove d.SetId() calls:

if state != nil && state.ID == "" {
// Data sources can set an ID if they want, but they aren't
// required to; we'll provide a placeholder if they don't,
// to preserve the invariant that all resources have non-empty
// ids.
state.ID = "-"
}

But in Terraform CLI 0.13.0 with the following applied configuration:

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

  required_version = "0.13.0"
}

provider "aws" {
  region = "us-east-2"
}

data "aws_availability_zones" "test" {}

output "az" {
  value = data.aws_availability_zones.test.names
}

And removing the d.SetId() call from the aws_availability_zones data source (init'ing the local built provider), Terraform removes the data source outputs:

terraform apply
data.aws_availability_zones.test: Refreshing state... [id=2020-08-20 01:05:46.951632 +0000 UTC]

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:

Terraform will perform the following actions:

Plan: 0 to add, 0 to change, 0 to destroy.

Changes to Outputs:
  - az = [
      - "us-east-2a",
      - "us-east-2b",
      - "us-east-2c",
    ] -> null

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes


Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

So no magic bullet for data sources, at least there.

bflad added a commit to bflad/terraform-plugin-sdk that referenced this issue Aug 20, 2020
… receiver methods and only passthrough id attribute state if present

Reference: hashicorp#541

This allows resources to decouple from the implicit `id` attribute by only working with the underlying resource identifier (`InstanceState.ID`). Any `id` attribute handling is omitted from these new receiver methods. For reference, the existing `Id()` receiver method always read the `id` attribute as a fallback if the resource identifier was missing and `SetId()` always set the `id` attribute to match the resource identifier.

Also part of this support, the `id` attribute is no longer always "promoted" using the resource identifier if the attribute is missing when retrieving the state, however it is always passed through if present in the previous or new state. Existing logic using `SetId()` and the existing import functionality are unaffected as they continue to set the `id` attribute.
bflad added a commit to bflad/terraform-plugin-sdk that referenced this issue Aug 20, 2020
… receiver methods and only passthrough id attribute state if present

Reference: hashicorp#541

This allows resources to decouple from the implicit `id` attribute by only working with the underlying resource identifier (`InstanceState.ID`). Any `id` attribute handling is omitted from these new receiver methods. For reference, the existing `Id()` receiver method always read the `id` attribute as a fallback if the resource identifier was missing and `SetId()` always set the `id` attribute to match the resource identifier.

Also part of this support, the `id` attribute is no longer always "promoted" using the resource identifier if the attribute is missing when retrieving the state, however it is always passed through if present in the previous or new state. Existing logic using `SetId()` and the existing import functionality are unaffected as they continue to set the `id` attribute.
@bflad
Copy link
Contributor Author

bflad commented Aug 20, 2020

Took a shot with proposal 1 here: #542

@paddycarver
Copy link
Contributor

I'm a little confused here, and I think the shims may be muddying the issue.

After 0.12, there's no such thing as a Resource ID, as far as I know. It's not part of the protocol at all, that I can find? Everything I'm being told, which I'm inclined to believe, indicates that IDs are a legacy bit of the SDK that doesn't actually matter to Terraform anymore. If it weren't for the backwards compatibility concerns, I think we could just.... remove them.

Personally, I'm inclined to leave them alone at the moment, or if the need is great enough (to resolve #586 for example) then to add a flag to schema.Resource that just disables the shim behaviors around IDs entirely, turning them off as an opt-in. It would just depend on how much work that was and how fragile the code paths it would change are.

I think I can pull together a terraform-plugin-go example of a data source without any ID fairly easily to prove the concept before we invest in trying to untangle the SDK...

@paddycarver paddycarver added the subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it. label Jan 6, 2021
@apparentlymart
Copy link
Contributor

Hi! 👋 I happened to coincidentally end up here as a result of following some other links today and saw the comment above, so wanted to offer some confirmation:

Terraform Core mostly doesn't treat id as anything special... it's just another attribute defined in the schema as far as the protocol is concerned. The wire protocol signals the absense of an object by using the msgpack nil value, which Terraform itself considers to be the same as null in the Terraform language.

The SDK still has these special cases because we could see no other way in the pre-existing ResourceData API to distinguish between an object existing or not, and so IIRC (which I may not, since it's been multiple years) the compatibility shims in the SDK detect the situation where the id isn't set and transform that into returning nil instead.

I think to remove it would therefore require introducing some other way to signal the presence or absence of an object, independently of setting its id.


The "mostly" in my initial statement above deserves some elaboration: although the wire protocol and Terraform Core don't treat id as special, there is some logic in the UI layer of Terraform CLI which uses some special attribute names as part of a heuristic for inferring a reasonable identifier to show for a resource instance in contexts where it would be too verbose to print out the entire object, such as in the "Still applying..." messages during terraform apply. You can see the full gory details of these heuristics in the function format.ObjectValueIDOrName.

We have in the past occasionally discussed extending the schema model in the protocol to allow providers to optionally override that heuristic for resource types that have schemas that it doesn't work with, but with the SDK so far continuing to require id anyway that hasn't risen to the top of anyone's priority list. As long as every resource ends up having id defined anyway, ObjectValidIDOrName always ends up selecting id as the identifying attribute and the rest of that logic is irrelevant.

@bflad
Copy link
Contributor Author

bflad commented Oct 14, 2021

Going to close this out -- I don't think we'll want to do this in this version of the framework and its not worth keeping it as an open issue.

@bflad bflad closed this as completed Oct 14, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.
Projects
None yet
3 participants