Add conditional validation, allow sending empty capacity scaler for regional backend service #3033

emilymye · 2020-01-28T23:52:48Z

Fixes hashicorp/terraform-provider-google#5449

compute: Added conditional requirement of `google_compute_**region**_backend_service` `backend.capacity_scaler` to no longer accept the API default if not INTERNAL. Non-INTERNAL backend services must now specify `capacity_scaler` explicitly and have a total capacity greater than 0. In addition, API default of 1.0 must now be explicitly set and will be treated as nil or zero if not set in config.

compute: Fixed `google_compute_**region**_backend_service` so it no longer has a permadiff if `backend.capacity_scaler` is unset in config by requiring capacity scaler.

compute: Fixed `backend.capacity_scaler` to actually set zero (0.0) value.

This PR includes a breaking change/bug fix and weird validation because RegionBackendService has a conditional default, and schema.Set cannot tell the difference between nil and zero values for scalars like float/int.

Changes:

Adds ability to send empty/zero capacity scaler --> forces capacity scaler to be required when settable (i.e. non-INTERNAL). (added validation)
Adds an encoder to throw out fields that can't be sent to the API for INTERNAL requests and validation to make sure INTERNAL backends don't have these values set explicitly

Context

RegionBackendService has typically only allowed for Internal Load Balancing (load_balancing_scheme defaults to INTERNAL), and the API rejects INTERNAL backend requests with specific fields in backend set like capacity_scaler, max_rate, etc
A new INTERNAL_MANAGED lb_scheme allows users to specify fields that were previously managed (unsettable). If not sent in the request, the API returns 1.0 for the value of capacity_scaler. NOTE: This is technically also true for BackendService, but as BackendService cannot be INTERNAL, it will always return 1.0 in the case of nil capacity_scaler so we slapped a default value on and it works fine.
Then, for INTERNAL_MANAGED region backend services: If capacity_scaler is not set in config, the API returns 1.0 --> schema.Set cannot tell capacity_scaler = 0.0 and not-set-capacity-scaler apart --> a diff is detected because of new hash-value of set object, which we can't suppress. d,

We also need to be able to send an empty value if non-INTERNAL, but we can't send the empty value if the RegionBackendService is INTERNAL (default)

FUTURE WORK

Right now, BackendService/RegionBackendService has unfixable behavior in that there is no way to send zero values for the other fields either BackendService or RegionBackendService backend. On read, schema.Set will zero any unset fields out, and they in turn will get sent to the API on next apply. We can force capacity_scaler to be set just because it's a requirement (hence default 1.0), but the other fields cannot be required because they can't all be set at once on the object (max_rate, max_rate_per_instance, etc have conflicts)

modular-magician · 2020-01-28T23:58:48Z

Hi! I'm the modular magician. Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2020-01-29T00:02:23Z

Hi! I'm the modular magician. Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2020-01-30T02:08:04Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

modular-magician · 2020-01-30T22:39:43Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

modular-magician · 2020-01-30T22:54:44Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

modular-magician · 2020-01-30T23:21:14Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

rileykarson

Won't https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-3033-old..auto-pr-3033#diff-212e80d75682146384aa4a2de927dec7L203-L214 cause a real breaking change / regression for users who haven't defined a value? As I understand their state will have 1.0 recorded but Terraform will now see 0 and cause a diff.

rileykarson · 2020-01-31T18:19:42Z

templates/terraform/constants/region_backend_service.go.erb

+// float and because INTERNAL backends can't have the value set, so there will be a permadiff for a
+// situational non-zero default returned from the API. We can't diff suppress or customdiff a
+// field inside a set object in ResourceDiff, since the value also determines the hash for that set object.
+func validateNonInternalBackendServiceBackends(backends []interface{}, d *schema.ResourceDiff) error {


One thing to watch out for here is unknown values. If I interpolate a variable into one of the fields and that variable isn't known at plan-time it'll appear to be unset/zero. This may fail an otherwise valid config if the value will have a value that works.

You can check values with d.NewValueKnown. I forget the exact interaction with sets- whether a single unknown value makes the parent count as unknown or not.

Handling unknown values properly is hard though, and it's may not be worth handling correctly. It doesn't come up much and it's painful to simulate with a small config 🤷‍♀

rileykarson · 2020-01-31T18:27:27Z

templates/terraform/constants/region_backend_service.go.erb

@@ -0,0 +1,100 @@
+<%# The license inside this block applies to this file.
+	# Copyright 2019 Google Inc.


nit: license blocks should all be 2020

rambleraptor

Ansible looks good!

emilymye · 2020-01-31T19:01:50Z

@rileykarson yep, that's the breaking change I mentioned - it also has to create a permadiff right now because if they don't have it defined in config, Terraform calculates the set value as 0.00 which causes a hashcode change. If they just are using the INTERNAL default (which i think most people are) then the API doesn't return a value anyways.

rileykarson

Can you write in the changelog (in the breaking change or as a note) that Terraform now defaults the field to 0.0 while previously it was 1.0, and that users will need to explicitly specify it to have it set to 1.0? It's implied by your current message, but making it explicit would help.

templates/terraform/constants/region_backend_service.go.erb

modular-magician · 2020-01-31T20:44:20Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 59 insertions(+), 499 deletions(-))
Terraform Beta: Diff ( 7 files changed, 69 insertions(+), 511 deletions(-))
Ansible: Diff ( 2 files changed, 33 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 1 insertion(+), 45 deletions(-))
Inspec: Diff ( 1 file changed, 8 insertions(+), 8 deletions(-))

slevenick

InSpec looks fine!

modular-magician · 2020-01-31T21:12:44Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 59 insertions(+), 503 deletions(-))
Terraform Beta: Diff ( 8 files changed, 69 insertions(+), 516 deletions(-))
Ansible: Diff ( 2 files changed, 33 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 1 insertion(+), 45 deletions(-))
Inspec: Diff ( 1 file changed, 8 insertions(+), 8 deletions(-))

…egional backend service (GoogleCloudPlatform#3033) * fix region backend service capacity scaler * add capacity scaler to fr example * remove recopied handwritten test * move constant list into expanders for tpg conversion * change licenses

googlebot added the cla: yes label Jan 28, 2020

emilymye force-pushed the backend_service branch 2 times, most recently from 56886d1 to 2932dde Compare January 30, 2020 02:02

emilymye added 2 commits January 30, 2020 14:33

fix region backend service capacity scaler

2393d68

add capacity scaler to fr example

bcbf99c

emilymye force-pushed the backend_service branch from 2932dde to eaa8cdc Compare January 30, 2020 22:33

remove recopied handwritten test

af493af

emilymye force-pushed the backend_service branch from eaa8cdc to af493af Compare January 30, 2020 22:48

emilymye changed the title ~~WIP~~ Add conditional validation, allow sending empty capacity scaler for regional backend service Jan 30, 2020

move constant list into expanders for tpg conversion

3c86e34

emilymye requested review from slevenick, chrisst, rambleraptor and rileykarson and removed request for chrisst January 30, 2020 23:35

rileykarson reviewed Jan 31, 2020

View reviewed changes

rambleraptor approved these changes Jan 31, 2020

View reviewed changes

rileykarson approved these changes Jan 31, 2020

View reviewed changes

templates/terraform/constants/region_backend_service.go.erb Show resolved Hide resolved

change licenses

ec1821d

slevenick approved these changes Jan 31, 2020

View reviewed changes

emilymye force-pushed the backend_service branch from c4f6243 to ec1821d Compare January 31, 2020 21:07

emilymye merged commit e47e9a5 into GoogleCloudPlatform:master Jan 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conditional validation, allow sending empty capacity scaler for regional backend service #3033

Add conditional validation, allow sending empty capacity scaler for regional backend service #3033

emilymye commented Jan 28, 2020 •

edited

Loading

modular-magician commented Jan 28, 2020

modular-magician commented Jan 29, 2020

modular-magician commented Jan 30, 2020

modular-magician commented Jan 30, 2020

modular-magician commented Jan 30, 2020

modular-magician commented Jan 30, 2020

rileykarson left a comment

rileykarson Jan 31, 2020

rileykarson Jan 31, 2020

rambleraptor left a comment

emilymye commented Jan 31, 2020

rileykarson left a comment

modular-magician commented Jan 31, 2020

slevenick left a comment

modular-magician commented Jan 31, 2020

		@@ -0,0 +1,100 @@
		<%# The license inside this block applies to this file.
		# Copyright 2019 Google Inc.

Add conditional validation, allow sending empty capacity scaler for regional backend service #3033

Add conditional validation, allow sending empty capacity scaler for regional backend service #3033

Conversation

emilymye commented Jan 28, 2020 • edited Loading

Changes:

Context

FUTURE WORK

modular-magician commented Jan 28, 2020

modular-magician commented Jan 29, 2020

modular-magician commented Jan 30, 2020

Diff report:

modular-magician commented Jan 30, 2020

Diff report:

modular-magician commented Jan 30, 2020

Diff report:

modular-magician commented Jan 30, 2020

Diff report:

rileykarson left a comment

Choose a reason for hiding this comment

rileykarson Jan 31, 2020

Choose a reason for hiding this comment

rileykarson Jan 31, 2020

Choose a reason for hiding this comment

rambleraptor left a comment

Choose a reason for hiding this comment

emilymye commented Jan 31, 2020

rileykarson left a comment

Choose a reason for hiding this comment

modular-magician commented Jan 31, 2020

Diff report:

slevenick left a comment

Choose a reason for hiding this comment

modular-magician commented Jan 31, 2020

Diff report:

emilymye commented Jan 28, 2020 •

edited

Loading