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

autoscaling: set conflict between availability_zones and vpc_zone_identifier #12927

Merged
merged 5 commits into from
Jul 14, 2020

Conversation

bendrucker
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment 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 pull request followers and do not help prioritize the request

This PR adds ConflictsWith to vpc_zone_identifier, ensuring that a user that has passed VPC subnet IDs does not also pass a set of AZ names. This configuration would have previously resulted in an error on apply and now will trigger and error during validation.

Relates to #9622

Release note for CHANGELOG:

Prevent `availability_zones` and `vpc_zone_identifier` from being specified together

@bendrucker bendrucker requested a review from a team April 21, 2020 19:26
@ghost ghost added needs-triage Waiting for first response or review from a maintainer. size/XS Managed by automation to categorize the size of a PR. service/autoscaling Issues and PRs that pertain to the autoscaling service. labels Apr 21, 2020
@bflad bflad added the breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. label Apr 23, 2020
@bflad bflad added this to the v3.0.0 milestone Apr 23, 2020
@bflad
Copy link
Contributor

bflad commented Apr 23, 2020

Hi @bendrucker 👋 Thank you for submitting this change. Please note I've marked this as a breaking change, since it currently causes some existing acceptance testing to fail that previously succeeded:

--- FAIL: TestAccAWSAutoScalingGroup_classicVpcZoneIdentifier (4.08s)
    testing.go:669: Step 0 error: config is invalid: "vpc_zone_identifier": conflicts with availability_zones

--- FAIL: TestAccAWSAutoScalingGroup_emptyAvailabilityZones (23.59s)
    testing.go:669: Step 0 error: errors during apply:
        
        Error: "vpc_zone_identifier": conflicts with availability_zones

The test configurations with these are using empty lists with the arguments, e.g.

resource "aws_autoscaling_group" "test" {
  min_size = 0
  max_size = 0

  availability_zones   = []
  launch_configuration = "${aws_launch_configuration.test.name}"
  vpc_zone_identifier  = ["${aws_subnet.test.id}"]
}

Which the resource code is designed to only pass elements when there is more than 0, e.g.

https://github.com/terraform-providers/terraform-provider-aws/blob/8a8845ed183d2f084f356cb8631d09870d05d340/aws/resource_aws_autoscaling_group.go#L533

The Terraform Plugin SDK ConflictsWith behavior strictly checks argument existence; not the final semantics of the given configuration values. Given that we probably should implement this at the schema level so it does return a plan-time validation error, at the cost of forcing operators to specify null for missing lists instead [], I will put this our next major release milestone for future consideration.

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Apr 23, 2020
@ghost ghost added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. and removed size/XS Managed by automation to categorize the size of a PR. labels Apr 23, 2020
@bendrucker
Copy link
Contributor Author

Thank you! Rushed through this a bit and didn't have a chance to run the acceptance tests. Totally understand that this is breaking, I have a similar change or two (adding ConflictsWith) similarly queued up for the Google provider.

  • Updated TestAccAWSAutoScalingGroup_classicVpcZoneIdentifier to not use an empty list for vpc_zone_identifiers
  • Removed TestAccAWSAutoScalingGroup_emptyAvailabilityZones, which is now appears irrelevant

I can set up and actually run the tests later today.

It also seemed like DiffSuppressFunc wasn't necessary anymore but then started to doubt myself.

@bendrucker
Copy link
Contributor Author

Verified that the remaining acceptance test passes:

make testacc TESTARGS='-run=TestAccAWSAutoScalingGroup_classicVpcZoneIdentifier'

@bflad bflad self-assigned this Jul 14, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After minor adjustments to fix another test configuration that snuck in with the conflicting arguments, LGTM, thanks @bendrucker 🚀

Output from acceptance testing:

--- PASS: TestAccAWSAutoScalingGroup_ALB_TargetGroups (149.76s)
--- PASS: TestAccAWSAutoScalingGroup_ALB_TargetGroups_ELBCapacity (305.03s)
--- PASS: TestAccAWSAutoScalingGroup_autoGeneratedName (48.42s)
--- PASS: TestAccAWSAutoScalingGroup_basic (263.71s)
--- PASS: TestAccAWSAutoScalingGroup_classicVpcZoneIdentifier (110.01s)
--- PASS: TestAccAWSAutoScalingGroup_enablingMetrics (201.08s)
--- PASS: TestAccAWSAutoScalingGroup_initialLifecycleHook (343.64s)
--- PASS: TestAccAWSAutoScalingGroup_launchTemplate (40.12s)
--- PASS: TestAccAWSAutoScalingGroup_LaunchTemplate_IAMInstanceProfile (61.25s)
--- PASS: TestAccAWSAutoScalingGroup_launchTemplate_update (139.49s)
--- PASS: TestAccAWSAutoScalingGroup_launchTempPartitionNum (45.25s)
--- PASS: TestAccAWSAutoScalingGroup_LoadBalancers (703.48s)
--- PASS: TestAccAWSAutoScalingGroup_MaxInstanceLifetime (79.42s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy (266.34s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_OnDemandAllocationStrategy (106.47s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_OnDemandBaseCapacity (79.45s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_OnDemandPercentageAboveBaseCapacity (48.15s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_SpotAllocationStrategy (96.45s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_SpotInstancePools (72.86s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_SpotMaxPrice (77.33s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_UpdateToZeroOnDemandBaseCapacity (45.55s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_LaunchTemplateSpecification_LaunchTemplateName (52.49s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_LaunchTemplateSpecification_Version (73.51s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_Override_InstanceType (76.41s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_Override_WeightedCapacity (161.24s)
--- PASS: TestAccAWSAutoScalingGroup_namePrefix (55.45s)
--- PASS: TestAccAWSAutoScalingGroup_serviceLinkedRoleARN (48.37s)
--- PASS: TestAccAWSAutoScalingGroup_suspendingProcesses (206.53s)
--- PASS: TestAccAWSAutoScalingGroup_tags (261.72s)
--- PASS: TestAccAWSAutoScalingGroup_TargetGroupArns (185.94s)
--- PASS: TestAccAWSAutoScalingGroup_terminationPolicies (133.73s)
--- PASS: TestAccAWSAutoScalingGroup_VpcUpdates (76.21s)
--- PASS: TestAccAWSAutoScalingGroup_WithLoadBalancer (339.36s)
--- PASS: TestAccAWSAutoScalingGroup_WithLoadBalancer_ToTargetGroup (332.66s)
--- PASS: TestAccAWSAutoScalingGroup_withMetrics (129.58s)
--- PASS: TestAccAWSAutoScalingGroup_withPlacementGroup (168.23s)

bflad added a commit that referenced this pull request Jul 14, 2020
…one_identifier ConflictsWith

Reference: #12927

Output from acceptance testing:

```
--- PASS: TestAccAWSAutoScalingGroup_ALB_TargetGroups (149.76s)
--- PASS: TestAccAWSAutoScalingGroup_ALB_TargetGroups_ELBCapacity (305.03s)
--- PASS: TestAccAWSAutoScalingGroup_autoGeneratedName (48.42s)
--- PASS: TestAccAWSAutoScalingGroup_basic (263.71s)
--- PASS: TestAccAWSAutoScalingGroup_classicVpcZoneIdentifier (110.01s)
--- PASS: TestAccAWSAutoScalingGroup_enablingMetrics (201.08s)
--- PASS: TestAccAWSAutoScalingGroup_initialLifecycleHook (343.64s)
--- PASS: TestAccAWSAutoScalingGroup_launchTemplate (40.12s)
--- PASS: TestAccAWSAutoScalingGroup_LaunchTemplate_IAMInstanceProfile (61.25s)
--- PASS: TestAccAWSAutoScalingGroup_launchTemplate_update (139.49s)
--- PASS: TestAccAWSAutoScalingGroup_launchTempPartitionNum (45.25s)
--- PASS: TestAccAWSAutoScalingGroup_LoadBalancers (703.48s)
--- PASS: TestAccAWSAutoScalingGroup_MaxInstanceLifetime (79.42s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy (266.34s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_OnDemandAllocationStrategy (106.47s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_OnDemandBaseCapacity (79.45s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_OnDemandPercentageAboveBaseCapacity (48.15s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_SpotAllocationStrategy (96.45s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_SpotInstancePools (72.86s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_SpotMaxPrice (77.33s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_InstancesDistribution_UpdateToZeroOnDemandBaseCapacity (45.55s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_LaunchTemplateSpecification_LaunchTemplateName (52.49s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_LaunchTemplateSpecification_Version (73.51s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_Override_InstanceType (76.41s)
--- PASS: TestAccAWSAutoScalingGroup_MixedInstancesPolicy_LaunchTemplate_Override_WeightedCapacity (161.24s)
--- PASS: TestAccAWSAutoScalingGroup_namePrefix (55.45s)
--- PASS: TestAccAWSAutoScalingGroup_serviceLinkedRoleARN (48.37s)
--- PASS: TestAccAWSAutoScalingGroup_suspendingProcesses (206.53s)
--- PASS: TestAccAWSAutoScalingGroup_tags (261.72s)
--- PASS: TestAccAWSAutoScalingGroup_TargetGroupArns (185.94s)
--- PASS: TestAccAWSAutoScalingGroup_terminationPolicies (133.73s)
--- PASS: TestAccAWSAutoScalingGroup_VpcUpdates (76.21s)
--- PASS: TestAccAWSAutoScalingGroup_WithLoadBalancer (339.36s)
--- PASS: TestAccAWSAutoScalingGroup_WithLoadBalancer_ToTargetGroup (332.66s)
--- PASS: TestAccAWSAutoScalingGroup_withMetrics (129.58s)
--- PASS: TestAccAWSAutoScalingGroup_withPlacementGroup (168.23s)
```
@bflad bflad merged commit effd988 into hashicorp:master Jul 14, 2020
bflad added a commit that referenced this pull request Jul 14, 2020
@bendrucker
Copy link
Contributor Author

Thank you!

@bendrucker bendrucker deleted the asg-az-vpc-zone-id-conflict branch July 14, 2020 15:14
@ghost
Copy link

ghost commented Jul 31, 2020

This has been released in version 3.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 for triage. Thanks!

@ghost
Copy link

ghost commented Aug 13, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Aug 13, 2020
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. documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/autoscaling Issues and PRs that pertain to the autoscaling service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants