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

Remove Zero Values From Attribute Validations #13943

Closed
bflad opened this issue Jun 25, 2020 · 5 comments · Fixed by #22954
Closed

Remove Zero Values From Attribute Validations #13943

bflad opened this issue Jun 25, 2020 · 5 comments · Fixed by #22954
Assignees
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. proposal Proposes new design or functionality. provider Pertains to the provider itself, rather than any interaction with AWS. service/ec2 Issues and PRs that pertain to the ec2 service. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Jun 25, 2020

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Prior to Terraform 0.12, the Terraform configuration language did not have the concept of null, where the attribute and its value could be seen as completely unconfigured to the provider. As a workaround in these older versions of Terraform, the provider occasionally accepted the zero-values for attribute types (e.g. "" for schema.TypeString, 0 for schema.TypeInt) from the configuration in the associated schema validation (ValidateFunc) and ignored passing these values through to the underlying API calls (e.g. using d.GetOk() or d.Get() and conditionalizing based on zero value). For example:

# Example Terraform 0.11 and earlier workaround for lack of null support
variable "destination" {
  default = "10.0.0.0/8"
}

variable "destination_type" {
  default = "ipv4"
}

resource "aws_route" "example" {
  destination_cidr_block      = "${var.destination_type == "ipv4" ? var.destination : ""}"
  destination_ipv6_cidr_block = "${var.destination_type == "ipv6" ? var.destination : ""}"
  route_table_id              = "${aws_route_table.example.id}"
  vpc_peering_connection_id   = "${aws_vpc_peering_connection.example.id}"
}

In terms of the schema implementation, this required the validation to accept either the zero value or an actual valid value. In this example:

"destination_cidr_block": {
	Type:     schema.TypeString,
	Optional: true,
	ForceNew: true,
	ValidateFunc: validation.Any(
		validation.StringIsEmpty,
		validateIpv4CIDRNetworkAddress,
	),
},

"destination_ipv6_cidr_block": {
	Type:     schema.TypeString,
	Optional: true,
	ForceNew: true,
	ValidateFunc: validation.Any(
		validation.StringIsEmpty,
		validateIpv6CIDRNetworkAddress,
	),
	DiffSuppressFunc: suppressEqualCIDRBlockDiffs,
},

Other than the slight extra implementation required, this does have an additional downsides:

  • The validation cannot make decisions based purely on an attribute being configured or not. For example, the above cannot implement the proper ConflictsWith validation since it represents a breaking change (nor ExactlyOneOf or RequiredWhen if it was appropriate).
  • Other future schema validation enhancements will also likely be based on values being unconfigured when necessary
  • Future schema types that support nullable values in addition to zero values will show differences between an unconfigured value and the zero value for practitioners.

Given that Terraform AWS Provider 3.0.0 and later will only support Terraform 0.12, we should consider fixing and enhancing these validations in a major version release to no longer support the workarounds required in Terraform 0.11 and earlier.

In the above case as the final breaking change:

"destination_cidr_block": {
	Type:     schema.TypeString,
	Optional: true,
	ForceNew: true,
	ValidateFunc: validateIpv4CIDRNetworkAddress,
	ConflictsWith: []string{"destination_cidr_block"},
},

"destination_ipv6_cidr_block": {
	Type:     schema.TypeString,
	Optional: true,
	ForceNew: true,
	ValidateFunc: validateIpv6CIDRNetworkAddress,
	ConflictsWith: []string{"destination_cidr_block"},
	DiffSuppressFunc: suppressEqualCIDRBlockDiffs,
},

Affected Resource(s)

TBD

References

List to be fully filled in the future:

@bflad bflad added proposal Proposes new design or functionality. technical-debt Addresses areas of the codebase that need refactoring or redesign. provider Pertains to the provider itself, rather than any interaction with AWS. labels Jun 25, 2020
@bflad bflad added this to the v4.0.0 milestone Jun 25, 2020
@ghost ghost added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jun 25, 2020
@ewbankkit
Copy link
Contributor

Another downside of the additional validation for an empty string is that the error message for a failed validation can be confusing, for example:

Error: aws_route_table.test: "10.4.0.1/16" is not a valid IPv4 CIDR block; did you mean "10.4.0.0/16"?



Error: aws_route_table.test: expected "route.0.cidr_block" to be an empty string: got 10.4.0.1/16

bflad added a commit that referenced this issue Jun 26, 2020
…ddress validation

Reference: #13943

Output from acceptance testing:

```
--- PASS: TestAccAWSEFSMountTarget_disappears (148.46s)
--- PASS: TestAccAWSEFSMountTarget_IpAddress_EmptyString (149.17s)
--- PASS: TestAccAWSEFSMountTarget_IpAddress (149.93s)
--- PASS: TestAccAWSEFSMountTarget_basic (262.20s)
```
bflad added a commit that referenced this issue Jun 26, 2020
…ddress validation (#13958)

Reference: #13943

Output from acceptance testing:

```
--- PASS: TestAccAWSEFSMountTarget_disappears (148.46s)
--- PASS: TestAccAWSEFSMountTarget_IpAddress_EmptyString (149.17s)
--- PASS: TestAccAWSEFSMountTarget_IpAddress (149.93s)
--- PASS: TestAccAWSEFSMountTarget_basic (262.20s)
```
@bflad
Copy link
Contributor Author

bflad commented Jan 21, 2021

One major function this affects is validateArn, which is a valid change but it has a wide blast radius. It would fix confusing bugs though, especially with data sources that accept the arn as an argument such as: #10524

@ewbankkit
Copy link
Contributor

One (minor) benefit of removing the zero value ("") for the aws_route destination_cidr_block and destination_ipv6_cidr_block attributes is that we can add an ExactlyOneOf check to ensure that one and only one route destination attribute is specified.

@github-actions
Copy link

This functionality has been released in v4.0.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@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 May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. proposal Proposes new design or functionality. provider Pertains to the provider itself, rather than any interaction with AWS. service/ec2 Issues and PRs that pertain to the ec2 service. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants