-
Notifications
You must be signed in to change notification settings - Fork 9.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
doc: standardize use of the id attribute design decision (#40627)
* doc: standardize use of the id attribute design decision This is a public-facing copy of an internal RFC which has already been approved by maintainers of the AWS provider. The goal of publishing this is to share the research and tradeoffs behind the decision, and document best practices going forward. * docs: apply code review feedback Co-authored-by: Kit Ewbank <[email protected]> --------- Co-authored-by: Kit Ewbank <[email protected]>
- Loading branch information
Showing
2 changed files
with
165 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
164 changes: 164 additions & 0 deletions
164
docs/design-decisions/standardize-use-of-the-id-attribute.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| # Standardize Use of the `id` Attribute | ||
|
|
||
| **Summary:** Define a standard for use of the `id` attribute given improvements to provider development and testing libraries have removed its requirement. | ||
| **Created**: 2024-05-24 | ||
| **Author**: [@jar-b](https://github.com/jar-b) | ||
|
|
||
| --- | ||
|
|
||
| Historically all resources in the Terraform AWS provider have included a read-only `id` attribute, as [Terraform Plugin SDK V2](https://developer.hashicorp.com/terraform/plugin/sdkv2) and its associated acceptance testing library requires it. In most cases, this attribute corresponds to a unique identifier generated by AWS during resource creation. However, for some resources the identifier is a value provided by the user and the resulting `id` attribute inherently duplicates the value of some other required argument. | ||
|
|
||
| With the general availability of [Terraform Plugin Framework](https://developer.hashicorp.com/terraform/plugin/framework), the separation of the provider testing functionality into its own standalone library ([`terraform-plugin-testing`](https://github.com/hashicorp/terraform-plugin-testing)), and some [corresponding enhancements](https://github.com/hashicorp/terraform-plugin-testing/pull/164) made to this library, resources are no longer required to have an `id` attribute. This leaves an opportunity for provider developers to define a standard for when to use an `id` attribute. Broadly, the two options are: | ||
|
|
||
| - Continue to require an `id` attribute in all resources for consistency. | ||
| - Omit the `id` attribute when it is redundant with some other required argument(s). | ||
|
|
||
| This RFC will propose a standard for the Terraform AWS provider. A secondary goal is to gain feedback from other HashiCorp-owned provider teams to determine if a common standard can be shared across the provider ecosystem. | ||
|
|
||
| ## Background | ||
|
|
||
| Prior to general availability of the Terraform Plugin Framework, all providers were built with the Terraform Plugin SDK. As the Terraform Plugin SDK was developed with pre-v1.0 versions of Terraform, an internal core implementation detail requiring the presence of an `id` attribute was incorporated into the library design. This requirement was accordingly passed along to provider developers in the form of an implicit requirement that all resources have an `id` attribute. | ||
|
|
||
| This convention is most noticeable in the method for setting `id` attributes (`d.SetId(“value”)`, versus `d.Set(“field”, “value”)`), the special syntax for removing objects from state during a read operation when deleted outside of Terraform (`d.SetId(“”)`), and the acceptance testing helpers for `import`, which rely on a populated `id` attribute to complete the import command and verify the results. | ||
|
|
||
| The corresponding method for removing Terraform Plugin Framework-based resources from state during read ([`State.RemoveResource`](https://pkg.go.dev/github.com/hashicorp/[email protected]/tfsdk#State.RemoveResource)) includes no mention of an ID. In version [`1.5.0`](https://github.com/hashicorp/terraform-plugin-testing/releases/tag/v1.5.0) of `terraform-plugin-testing`, [additional fields were added](https://github.com/hashicorp/terraform-plugin-testing/pull/164) to the `TestStep` struct to enable testing imports for resources that do not implement an `id` attribute. With these advancements, providers using Terraform Plugin Framework, either exclusively or via a [muxed](https://developer.hashicorp.com/terraform/plugin/framework/migrating/mux) provider configuration, and `terraform-plugin-testing` in place of the legacy testing packages embedded in Terraform Plugin SDK V2 are no longer bound to the requirement of including an `id` attribute in every resource. | ||
|
|
||
| From a Terraform core perspective, releases after 0.12 no longer treat `id` as a special attribute. Terraform 0.11 and earlier implicitly required it. Due to the linkage between Terraform 0.12 and protocol version 5, and the fact that supported provider SDKs today only speak protocol version 5 (and 6), the primary Hashicorp-owned providers, like `hashicorp/aws`, inherently only support Terraform 0.12 and later. Therefore it is safe to remove the `id` attribute since there is no concern for a Terraform version that requires it. | ||
|
|
||
| ### `id` Attributes in the AWS Provider | ||
|
|
||
| In the AWS provider, the `id` attribute most commonly corresponds to an identifier generated by AWS during creation of the resource. The Create API typically returns this value as a field named `Id` or, in some cases, `Id` with a prefixed with a resource name (eg. `InstanceId`). When a resource can be referenced by a unique, remotely generated identifier, storing this value in a computed `id` is a straightforward choice and the meaning of the attribute is easily understood by the practitioner. | ||
|
|
||
| However, there are cases where AWS does not generate a unique identifier during resource creation, and instead one or more of the arguments supplied to the Create API functions as the identifier. For single value identifiers this is commonly a field like `name`. For “relationship” resources (ie. resources creating a link between two other resources, such as an [IAM policy attachment)](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment), the unique identifier may be a “multi-part key” made up of identifiers from both resources being linked. Historically, multi-part keys [have not used a consistent delimiter](https://github.com/hashicorp/terraform-provider-aws/issues/27843), further complicating the user experience for practitioners. | ||
|
|
||
| In these situations, the AWS provider copied the identifying value (or a delimited string of values for multi-part keys) to the `id` attribute. This approach leaves ambiguity for practitioners regarding which attribute should be used if the exported value is referenced somewhere else in the configuration. | ||
|
|
||
| ## Proposal | ||
|
|
||
| **Going forward, all net-new resources for which an `id` attribute would be redundant with an existing argument(s) will omit this attribute.** In all other cases the `id` attribute should continue to be used as it has been historically. When an `id` is omitted and the unique identifier is a combination of arguments, these should always be delimited with a comma (`,`), and rely on the internal [`ExpandResourceID`](https://github.com/hashicorp/terraform-provider-aws/blob/v5.51.1/internal/flex/flex.go#L326-L354) function to handle splitting values within the import method. | ||
|
|
||
| The added clarity of removing redundant attribute values will directly benefit practitioners, especially for resources with complex multi-part keys. | ||
|
|
||
| The trade-offs being made with this decision are: | ||
|
|
||
| - A departure from a historical precedent where all resources include an `id` attribute, which some practitioners may have come to expect and rely on. | ||
| - A minor UI difference in how resources without an ID are rendered during `plan` and `apply` operations. Specifically, when no `id` attribute is present the logged line for this resource will omit this extra piece of information. | ||
|
|
||
| Team discussion around this topic consistently landed on a consensus that the clarity gained by having only a single attribute containing the identifier value outweighs the departure from historical conventions. | ||
|
|
||
| It should also be noted that this standard is enabled by an existing policy [requiring all net-new resources be implemented with Terraform Plugin Framework](https://github.com/hashicorp/terraform-provider-aws/issues/32917), and that the AWS provider has already migrated to the standalone `terraform-plugin-testing` library. | ||
|
|
||
| ### Import Methods and Testing | ||
|
|
||
| For resources omitting an `id` argument, minor changes are required to customize the import method and acceptance test the import operation. For testing specifically, the import verification `TestStep` will now require the `ImportStateVerifyIdentifierAttribute` and one of `ImportStateID` or `ImportStateIdFunc` be configured. Examples for single non-id identifier and a multi-part key are included below. | ||
|
|
||
| #### Single Value | ||
|
|
||
| `ImportState` method: | ||
|
|
||
| ```go | ||
| func (r *resourceEndpointPrivateDNS) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { | ||
| resource.ImportStatePassthroughID(ctx, path.Root("vpc_endpoint_id"), req, resp) | ||
| } | ||
| ``` | ||
|
|
||
| `TestStep`: | ||
|
|
||
| ```go | ||
| { | ||
| ResourceName: resourceName, | ||
| ImportState: true, | ||
| ImportStateIdFunc: testAccVPCEndpointPrivateDNSImportStateIdFunc(resourceName), | ||
| ImportStateVerify: true, | ||
| ImportStateVerifyIdentifierAttribute: "vpc_endpoint_id", | ||
| }, | ||
| ``` | ||
|
|
||
| `ImportStateIdFunc`: | ||
|
|
||
| ```go | ||
| func testAccVPCEndpointPrivateDNSImportStateIdFunc(resourceName string) resource.ImportStateIdFunc { | ||
| return func(s *terraform.State) (string, error) { | ||
| rs, ok := s.RootModule().Resources[resourceName] | ||
| if !ok { | ||
| return "", fmt.Errorf("Not found: %s", resourceName) | ||
| } | ||
|
|
||
| return rs.Primary.Attributes["vpc_endpoint_id"], nil | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| #### Multi-part Value | ||
|
|
||
| `ImportState` Method: | ||
|
|
||
| ```go | ||
| func (r *resourceRuntimeManagementConfig) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { | ||
| parts, err := intflex.ExpandResourceId(req.ID, runtimeManagementConfigIDParts, true) | ||
| if err != nil { | ||
| resp.Diagnostics.AddError( | ||
| "Unexpected Import Identifier", | ||
| fmt.Sprintf("Expected import identifier with format: function_name,qualifier. Got: %q", req.ID), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("function_name"), parts[0])...) | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("qualifier"), parts[1])...) | ||
| } | ||
| ``` | ||
|
|
||
| `TestStep`: | ||
|
|
||
| ```go | ||
| { | ||
| ResourceName: resourceName, | ||
| ImportState: true, | ||
| ImportStateIdFunc: testAccRuntimeManagementConfigImportStateIdFunc(resourceName), | ||
| ImportStateVerify: true, | ||
| ImportStateVerifyIdentifierAttribute: "function_name", | ||
| }, | ||
| ``` | ||
|
|
||
| `ImportStateIdFunc`: | ||
|
|
||
| ```go | ||
| func testAccRuntimeManagementConfigImportStateIdFunc(resourceName string) resource.ImportStateIdFunc { | ||
| return func(s *terraform.State) (string, error) { | ||
| rs, ok := s.RootModule().Resources[resourceName] | ||
| if !ok { | ||
| return "", fmt.Errorf("Not found: %s", resourceName) | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s,%s", rs.Primary.Attributes["function_name"], rs.Primary.Attributes["qualifier"]), nil | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Abandoned Ideas | ||
|
|
||
| The alternative to omitting `id` attributes when possible is to continue requiring all resources to include an `id` attribute. While this approach provides consistency with the historical design of the provider, the drawbacks of redundant attribute values and complex (and often inconsistently delimited) multi-part identifiers remain. Consensus among the team has been that the benefits of clarity outweigh the departure from precedent in this case. | ||
|
|
||
| ## References | ||
|
|
||
| Terraform Plugin Framework | ||
|
|
||
| - [https://developer.hashicorp.com/terraform/plugin/framework/acctests\#no-id-found-in-attributes](https://developer.hashicorp.com/terraform/plugin/framework/acctests#no-id-found-in-attributes) | ||
|
|
||
| Terraform Plugin Testing | ||
|
|
||
| - [https://github.com/hashicorp/terraform-plugin-testing/issues/84](https://github.com/hashicorp/terraform-plugin-testing/issues/84) | ||
| - [https://github.com/hashicorp/terraform-plugin-testing/pull/164](https://github.com/hashicorp/terraform-plugin-testing/pull/164) | ||
|
|
||
| Terraform AWS Provider | ||
|
|
||
| - Prototype resources implemented without an `id` attribute | ||
| - `aws_vpc_endpoint_private_dns` | ||
| - The identifier is a single value, `vpc_endpoint_id` | ||
| - [PR](https://github.com/hashicorp/terraform-provider-aws/pull/37628) | ||
| - [Registry documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpc_endpoint_private_dns) | ||
| - `aws_lambda_runtime_management_config` | ||
| - The identifier is a multi-part key, made up of the required `function_name` and optional `qualifier` arguments | ||
| - [PR](https://github.com/hashicorp/terraform-provider-aws/pull/37643) | ||
| - [Registry documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_runtime_management_config) |