Upstream firewalls DSF update to allow unknown values

[slevenick]

Upstreams: hashicorp/terraform-provider-google#10668

It's unclear to me if this is the best that we can do, it's not quite correct, but probably a better failure mode than exists right now

If this PR is for Terraform, I acknowledge that I have:

• Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
• Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
• Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
• Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

compute: fixed a bug where `google_compute_firewall` would incorrectly find `source_ranges` to be empty during validation

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 2 files changed, 62 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 2 files changed, 62 insertions(+), 2 deletions(-))
TF Validator: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))

[jackwhelpton]

Based on the identified problem with this (source_ranges being removed), could we perhaps improve the check by using diff.HasChange?

The code below also passes the unit test I wrote, but I'm curious how it would behave if a static/known range (present in state) were then replaced with a dynamic/unknown one.

update: I've added a test for the scenario we're discussing (valid source_ranges added then removed), and the code below also fails that test, so I'm none the wiser how to address this.

		_, tagsOk := diff.GetOk("source_tags")
		_, rangesOk := diff.GetOk("source_ranges")
		_, sasOk := diff.GetOk("source_service_accounts")

		_, tagsExist := diff.GetOkExists("source_tags")
		_, rangesExist := diff.GetOkExists("source_ranges")
		_, sasExist := diff.GetOkExists("source_service_accounts")

		oldLength, newLength := diff.GetChange("source_ranges.#")
		if oldInt, ok := oldLength.(int); ok && oldInt > 0 {
			if newInt, ok := newLength.(int); !ok || newInt == 0 {
				// Ensure that ranges are not flagged as present if they are later removed.
				rangesExist = false
			}
		}

		if !tagsOk && !rangesOk && !sasOk && !tagsExist && !rangesExist && !sasExist {
			return fmt.Errorf("one of source_tags, source_ranges, or source_service_accounts must be defined")
		}

Here's the above expanded to check all fields, which also passes both of the existing tests.

		for _, fld := range []string{"source_tags", "source_ranges", "source_service_accounts"} {
			if _, ok := diff.GetOk(fld); ok {
				return nil
			}

			if _, ok := diff.GetOkExists(fld); !ok {
				continue
			}

			oldLength, newLength := diff.GetChange(fld + ".#")
			if oldInt, ok := oldLength.(int); ok && oldInt > 0 {
				if newInt, ok := newLength.(int); !ok || newInt == 0 {
					// Ensure that fields are not flagged as present if they are later removed.
					continue
				}
			}

			return nil
		}

		return fmt.Errorf("one of source_tags, source_ranges, or source_service_accounts must be defined")

[jackwhelpton]

Any progress on this? I imagine there's not much activity until after the holiday period now, but this is still a blocker for terraform-google-modules/terraform-google-kubernetes-engine#1071.

[slevenick]

/gcbrun

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 3 files changed, 93 insertions(+), 33 deletions(-))
Terraform Beta: Diff ( 3 files changed, 80 insertions(+), 20 deletions(-))
TF Validator: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))

[modular-magician]

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeFirewall_moduleOutput|TestAccContainerNodePool_withInvalidUpgradeSettings You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=235101

[megan07]

LGTM

[megan07]

nit: the spacing in this resource is tabbed rather than 2 spaces