Allow provider to define uniqueness of a resource #224

Activity

1. radeksimko

   added

   enhancementNew feature or requestNew feature or request

   on Oct 30, 2019
2. bflad commented on Nov 6, 2019

   

   bflad

   on Nov 6, 2019

   Last edited by bflad

   Contributor

   More actions

   (Updated the issue description to include a references section 👍 )

   In the Terraform AWS Provider, this functionality would become immensely more helpful if given the availability of both resource configuration value(s) (historically via ResourceData) and provider configuration values (historically via meta). As a small sample, the following resources have uniqueness constraints on both dimensions of AWS Region and a name-like identifier:

   • aws_backup_vault (unique name per AWS Region per AWS Account ID)
   • aws_dynamodb_table (unique name per AWS Region per AWS Account ID)
   • aws_sns_topic (unique name per AWS Region per AWS Account ID)

   Or 1 per provider configuration, such as:

   • aws_ebs_default_kms_key (1 per AWS Region per AWS Account ID)
   • aws_ebs_encryption_by_default (1 per AWS Region per AWS Account ID)
   • aws_iam_account_alias (1 per AWS Account ID, naming doesn't matter)
   • aws_iam_account_password_policy (1 per AWS Account ID, no naming)

   Another consideration may be resources where there can only be 1 per some resource. Sample resources:

   • aws_s3_bucket_notification (1 per S3 Bucket name)
   • aws_s3_bucket_policy (1 per S3 Bucket name) -- same applies across most, if not all aws_*_policy resources except IAM

   Or as another example, 1 per containing resource and name-like identifier, such as:

   • aws_iam_group_policy (unique name per IAM Group name)
   • aws_iam_role_policy (unique name per IAM Role name)
   • aws_iam_user_policy (unique name per IAM User name)

   ---

   Hopefully the examples above highlight that determining the uniqueness constraints might be a little more nuanced than a simple field on a single attribute. To put some of the examples into "how these values would be determined in a Create/Read/Update/Delete function manner with ResourceData and the provider interface":

   • aws_backup_vault: fmt.Sprintf("%s:%s:%s", meta.(*AWSClient).accountid, meta.(*AWSClient).region, d.Get("name").(string)
   • aws_ebs_default_kms_key: fmt.Sprintf("%s:%s", meta.(*AWSClient).accountid, meta.(*AWSClient).region
   • aws_s3_bucket_notification: d.Get("bucket").(string)
   • aws_iam_group_policy: fmt.Sprintf("%s:%s:%s", meta.(*AWSClient).accountid, d.Get("group").(string), d.Get("name").(string))

   Maybe something conceptually like the below at the resource level could do this?

   type UniquenessIdentifierFunc func(ResourceData, interface{}) string
   
   type Resource struct {
     // UniquenessIdentifier is a string identifier to determine duplicate resource configurations.
     // This is globally checked against all other Terraform resources in a single run.
     // Any values that cannot be determined at plan time, such as those not known until an upstream
     // resource has been applied, this check to be skipped. If the return value is an empty
     // string (""), this check will be skipped.
     UniquenessIdentifier UniquenessIdentifierFunc
   }
   
   // Example in a Terraform AWS Provider resource, such as aws_backup_vault
   func PerRegionalNameUniquenessIdentifier(d schema.ResourceData, meta interface{}) string {
     return fmt.Sprintf("%s:%s:%s", meta.(*AWSClient).accountid, meta.(*AWSClient).region, d.Get("name").(string))
   }
   
   UniquenessIdentifier: PerRegionalNameUniquenessIdentifier,
   
   // Another example in Terraform AWS Provider, such as aws_ebs_default_kms_key
   func PerRegionUniquenessIdentifier(d schema.ResourceData, meta interface{}) string {
     return fmt.Sprintf("%s:%s", meta.(*AWSClient).accountid, meta.(*AWSClient).region)
   }
   
   UniquenessIdentifier: PerRegionUniquenessIdentifier,

   The nice part here is that it gives resources the freedom to fully synthesize the criteria for uniqueness although it comes at the expense of being a little verbose. The logic checking these would need bypass any with computed attribute values.

   When writing up the above examples, I found it may make more sense to have the return value be a *string to allow for nil values or treat "" as a special value to skip the check. A problem that we have in the Terraform AWS Provider is that while the AWS Account ID is generally available, there are reasons to disable the provider initialization for it (skip_requesting_account_id) which leaves that crucial meta.(*AWSClient).accountid empty for us. We wouldn't want a configuration that happens to be against two AWS Accounts correctly to throw a uniqueness error (the result being ":us-east-1" for both resources in that case). Here's how the logic would more likely have to look to appropriately handle that situation:

   // Example in a Terraform AWS Provider resource, such as aws_backup_vault
   func PerRegionalNameUniquenessIdentifier(d schema.ResourceData, meta interface{}) string {
     accountID := meta.(*AWSClient).accountid
   
     if accountID == "" {
       return "" // or nil if *string
     }
   
     return fmt.Sprintf("%s:%s:%s", accountID, meta.(*AWSClient).region, d.Get("name").(string))
   }
   
   UniquenessIdentifier: PerRegionalNameUniquenessIdentifier,
   
   // Another example in Terraform AWS Provider, such as aws_ebs_default_kms_key
   func PerRegionUniquenessIdentifier(d schema.ResourceData, meta interface{}) string {
     accountID := meta.(*AWSClient).accountid
   
     if accountID == "" {
       return "" // or nil if *string
     }
   
     return fmt.Sprintf("%s:%s", accountID, meta.(*AWSClient).region)
   }
   
   UniquenessIdentifier: PerRegionUniquenessIdentifier,

   I also wonder if maybe the return value could be something like []string, where if any of the values are "" then it automatically skips the check. This would cover the AWS Account ID scenario above as well as computed values.

   type UniquenessIdentifierFunc func(ResourceData, interface{}) []string
   
   type Resource struct {
     // UniquenessIdentifier is an identifier to determine duplicate resource configurations.
     // This is globally checked against all other Terraform resources in a single run.
     // If the return value is an empty string ("") for any elements, this check will be skipped.
     // Any values that cannot be determined at plan time, such as those not known until an
     // upstream resource has been applied, will also cause this check to be skipped.
     UniquenessIdentifier UniquenessIdentifierFunc
   }
   
   // Example in a Terraform AWS Provider resource, such as aws_backup_vault
   func PerRegionalNameUniquenessIdentifier(d schema.ResourceData, meta interface{}) []string {
     return []string{
       meta.(*AWSClient).accountid,
       meta.(*AWSClient).region,
       d.Get("name").(string),
     }
   }
   
   UniquenessIdentifier: PerRegionalNameUniquenessIdentifier,
   
   // Another example in Terraform AWS Provider, such as aws_ebs_default_kms_key
   func PerRegionUniquenessIdentifier(d schema.ResourceData, meta interface{}) []string {
     return []string{
       meta.(*AWSClient).accountid,
       meta.(*AWSClient).region,
     }
   }
   
   UniquenessIdentifier: PerRegionUniquenessIdentifier,

   Thanks for the consideration! 😄
3. radeksimko commented on Nov 6, 2019

   

   radeksimko

   on Nov 6, 2019

   MemberAuthor

   More actions

   @paddycarver described to me earlier in a separate conversation that it would be helpful in GCP provider to have the ability to validate all occurrences of a given resource type. The main reason was also uniqueness - so I believe the problem definition matches one of this issue. We couldn't really come up with any other reason to run such validation other than to guarantee/validate some form of uniqueness.

   Paddy mentioned two GCP specific examples:

   • google_app_engine_application is unique per Google Project
   • a disk may only be attached to one instance at a time -> while a provider may have _attachment resource which manages the disk/instance relationship and that resource may have unique ID, not all providers have _attachment resources for various reasons

   @paddycarver feel free to add/correct anything above.
4. bflad commented on Nov 6, 2019

   

   bflad

   on Nov 6, 2019

   Contributor

   More actions

   There is a related upstream Terraform Core issue here: hashicorp/terraform#22094
5. bflad commented on Nov 6, 2019

   

   bflad

   on Nov 6, 2019

   Contributor

   More actions

   a disk may only be attached to one instance at a time -> while a provider may have _attachment resource which manages the disk/instance relationship and that resource may have unique ID, not all providers have _attachment resources for various reasons

   Oh! This reminds me of very similar problems we have in the Terraform AWS Provider where there may be two methods for managing a specific piece of infrastructure such as EC2 Security Groups, but using both methods can causes perpetual differences:

   • Inline aws_security_group resource rules via ingress and egress configuration blocks
   • Separate aws_security_group_rule resources with type argument set to ingress or egress

   In this case the "uniqueness" is even more nuanced since its involves the values of "sub" resources, but the "sub" resources have no identifiers of their own and it actually has nothing to do with overlap either (its matching EC2 Security Group IDs and the aws_security_group resource having any inline blocks of the same aws_security_group_rule resource type argument 😬).

   I'm wondering if this class of problem should be considered outside the scope of this feature request due to its inherent additional complexity and maybe treated with a different type of solution that can take specific other resource types and their attribute keys/values into consideration.
6. radeksimko

   added

   upstream-terraform

   upstream-protocolRequires change of protocol specification, i.e. can't be done under the current protocolRequires change of protocol specification, i.e. can't be done under the current protocol

   on Dec 8, 2019
7. tombuildsstuff commented on Dec 8, 2019

   

   tombuildsstuff

   on Dec 8, 2019

   Contributor

   More actions

   Presumably having this information available would make caching of duplicate Data Sources possible too? hashicorp/terraform#19942
8. radeksimko commented on Dec 9, 2019

   

   radeksimko

   on Dec 9, 2019

   Last edited by radeksimko

   MemberAuthor

   More actions

   @tombuildsstuff I would say easier.

   Due to their nature, data sources may require slightly different approach though. Unlike resources which tend to be managed (and changed) entirely via Terraform, data sources may change outside of Terraform more often and what we consider ID today is far less likely to represent unique state and data sources are expected to always have fresh state.

   For example you wouldn't want aws_ami to keep stale AMI ID, when you have a fresh one, nor you'd want to implement something like fresh = true/false that will just confuse less skilled users and make them flip it back and forth. I think I'd just expect this to work out of the box and not make unnecessary requests, but always get fresh data.

   #7 describes an example of how we may address that. Perhaps a combination of these two suggestions will work - labelling fields as "unique combination" (e.g. name+etag) and make it easier to send more "lightweight" HEAD requests to check whether anything changed, before falling back to Read() and make it easier to just attach headers such as If-None-Match to all requests.

   We'll probably need to check what major APIs support today but I'm guessing some combination of the above will satisfy the majority.
9. bflad commented on Jul 30, 2020

   

   bflad

   on Jul 30, 2020

   Contributor

   More actions

   I no longer have rights in this repository to edit the issue description references again, but please note I've consolidated the various Terraform AWS Provider use cases into hashicorp/terraform-provider-aws#14394.