Remove default from optional rule_visibility field

[smcgivern]

google_compute_security_policy -> adaptive_protection_config -> layer_7_ddos_defense_config -> rule_visibility is optional, but had a default value, and was required to be either PREMIUM or STANDARD.

This means that:

1. If you did not include this block at all, you get a persistent message about changing this from false (in state) to null (in config, due to the missing default).

2. If you included this block with enable = false, you'd get a plan attempting to set rule_visibility = "STANDARD", which would then fail:

   Invalid value for field
   'resource.adaptiveProtectionConfig.layer7DdosDefenseConfig.enable':
   'false'. Cannot set layer 7 DDoS options if it's not enabled.

This fixes the second problem, but not the first.

Part of hashicorp/terraform-provider-google#12743

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).
• 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).
• Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
• Ran relevant acceptance tests using my own Google Cloud project and credentials (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 persistent diff for `layer_7_ddos_defense_config` in security policies

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @SarahFrench, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

---

Your assigned reviewer will help review your code by:

• Ensuring it's backwards compatible, covers common error cases, etc.
• Summarizing the change into a user-facing changelog note.
• Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

---

[smcgivern]

Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).

I can't run the acceptance tests locally because my org has hit its security policy quota 🥲

Quota 'SECURITY_POLICIES' exceeded.  Limit: 10.0 globally.

I did run a manual test here, but as I said in the PR description, I've only solved the second of the two problems. If I do:

adaptive_protection_config {
  layer_7_ddos_defense_config {
    enable = false
  }
}

Then I get no diff, but ideally I wouldn't even have to write that; not having an adaptive_protection_config -> layer_7_ddos_defense_config block should be enough to say that it is disabled.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

• Field adaptive_protection_config.layer_7_ddos_defense_config.rule_visibility default value changed from STANDARD to on google_compute_security_policy - reference

If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An override-breaking-change label can be added to allow merging.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 1 deletion(-))
Terraform Beta: Diff ( 1 file changed, 1 deletion(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2758
Passed tests 2466
Skipped tests: 283
Affected tests: 9

Action taken

Found 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccComputeFirewallPolicyRule_multipleRules|TestAccAlloydbBackup_missingLocation|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbCluster_missingLocation|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample

Get to know how VCR tests work

[smcgivern]

Changing a default value will absolutely cause a breakage. The mechanism of break for both scenarios is current terraform deployments now gain a diff with sequential applies where the diff is the new or changed default value

This is accurate, although I always find this case tricky: I contend that the current default is wrong and already causing problems, but I understand if this means it needs to wait for a new major version. The only case (AFAICT) that will break in this way is:

adaptive_protection_config {
  layer_7_ddos_defense_config {
    enable = true
  }
}

Which will need to become:

adaptive_protection_config {
  layer_7_ddos_defense_config {
    enable          = true
    rule_visibility = "STANDARD"
  }
}

Is there a way to say that the default only applies when the other field is present? It looks like I could do that in expandLayer7DdosDefenseConfig, but I think that only affects what is applied, not what shows up in the plan?

[modular-magician]

Tests passed during RECORDING mode:
TestAccAlloydbBackup_missingLocation[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccAlloydbCluster_missingLocation[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccDataSourceAlloydbLocations_basic[Debug log]

Tests failed during RECORDING mode:
TestAccBillingSubaccount_basic[Error message] [Debug log]
TestAccBillingSubaccount_renameOnDestroy[Error message] [Debug log]
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

[SarahFrench]

Hi 👋 - Thanks for your PR to address this issue! I'm currently thinking about whether there are different ways to approach this so it is less of a breaking change for users who have enable = true and no rule_visibility value in their config but already have rule_visibility = "STANDARD" in their state file. I'll get back to you on that after I've had a think.

Some feedback I can give now though is that a good addition to this PR would be a test that shows a google_compute_security_policy resource can be made without error when it contains this block:

  adaptive_protection_config {
    layer_7_ddos_defense_config {
      enable = false
    }
  }

I saw that you're not able to run acceptance tests yourself, but could you please update the acceptance tests for this resource to include creation of a policy where the config contains that block above? Although you can't run the tests you can push test code to this PR to run them. I've supplied some info below to help with this as not being able to run the tests makes writing them tougher.

---

Acceptance test context & info

Currently there's an acceptance test called TestAccComputeSecurityPolicy_withAdaptiveProtection that covers use of this resource but is missing the scenario that triggers this bug. That test starts by creating the adaptive protection config with enable = true + rule_visibility = "STANDARD" and then performs an update that changes it to enable = false + rule_visibility = "STANDARD" in a subsequent update. The test doesn't fail because it seems the API doesn't have the same validation when creating a policy versus when updating it.

I'll leave it up to you to decide whether to adapt the existing TestAccComputeSecurityPolicy_withAdaptiveProtection function to have an extra step or to create a new standalone test, but you will need to create a new function to supply the config that triggers this problem addressed by this PR (achieved by making a variation of this function).

[smcgivern]

Thanks @SarahFrench, that's very helpful. I have never looked at the internals of a Terraform provider before, but here's what I think I want to express:

1. If there is no layer_7_ddos_defense_config block provided (adaptive_protection_config may be present or not), the value for enable should be false, not null. Or, alternatively, there should be no diff for false -> null, but I'm not sure if that's feasible.
2. If there is a layer_7_ddos_defense_config block provided, and it's disabled, we should not try to set rule_visibility.
3. If there is a layer_7_ddos_defense_config block provided, and it's enabled, rule_visibility should default to STANDARD and should not be allowed to be empty.

I'd love a better way to do this than I have right now.

---

I will update the tests next week, thanks for the pointers. (I'll also see if we can get our quota increased.)

The test doesn't fail because it seems the API doesn't have the same validation when creating a policy versus when updating it.

This is interesting, because we are updating an existing policy in our case 🤔 Maybe it's allowed for a state change, like you suggest. I might have a play around with the API once I get a quota increase too.

[SarahFrench]

This is interesting, because we are updating an existing policy in our case 🤔 Maybe it's allowed for a state change

In case I'm causing confusion, the test creates a google_compute_security_policy resource with:

  adaptive_protection_config {
    layer_7_ddos_defense_config {
      enable = true
      rule_visibility = "STANDARD"
    }
  }

and then updates it to:

  adaptive_protection_config {
    layer_7_ddos_defense_config {
      enable = false
      rule_visibility = "STANDARD"
    }
  }

...and this doesn't trigger the Invalid value for field 'resource.adaptiveProtectionConfig.layer7DdosDefenseConfig.enable': 'false'. Cannot set layer 7 DDoS options if it's not enabled., invalid API validation error that happens when both fields are sent during a create action.

[smcgivern]

Right. In our case in production, I tried setting (on an existing policy that can't be represented in the current version of the provider, but is equivalent to enable = false):

adaptive_protection_config {
  layer_7_ddos_defense_config {
    enable          = false
    rule_visibility = "STANDARD"
  }
}

And got that error. I wonder if it's because in the test here, the config already has rule_visibility set, so the provider isn't actually trying to modify that value? But the only way to get to that state from where we are would be to enable it, then disable it.

[SarahFrench]

How weird! For what it's worth, the API calls in that test are:

Creating the resource

POST /compute/v1/projects/[PROJECT]/global/securityPolicies?alt=json&prettyPrint=false HTTP/1.1

{
 "adaptiveProtectionConfig": {
  "layer7DdosDefenseConfig": {
   "enable": true,
   "ruleVisibility": "STANDARD"
  }
 },
 "description": "updated description",
 "name": "tf-test-x7jr2hvufk"
}

Updating the resource

PATCH /compute/v1/projects/[PROJECT]/global/securityPolicies/tf-test-x7jr2hvufk?alt=json&prettyPrint=false HTTP/1.1

{
 "adaptiveProtectionConfig": {
  "layer7DdosDefenseConfig": {
   "enable": false,
   "ruleVisibility": "STANDARD"
  }
 },
 "fingerprint": "VFRmhQznX8E="
}

That update works without validation errors 🤷 . I think this confusion shows that beefing up the acceptance tests definitely has value.

It's the end of my Friday here in my timezone, but when you push those test changes next week we can pick up again 👋

[smcgivern]

I added a test that fails without this change. I haven't done any further exploration here yet so feel free to skip that push as it's not very interesting.

[smcgivern]

Or, alternatively, there should be no diff for false -> null, but I'm not sure if that's feasible.

It looks like DiffSuppressFunc could provide this? There are a few already in /~https://github.com/GoogleCloudPlatform/magic-modules/blob/main/tpgtools/ignored_handwritten/common_diff_suppress.go, although none handle false being considered equivalent to null.

I think if I provided that then the main problem goes away without needing to make a breaking change.

(Also, @SarahFrench I hope you weren't hit in the recent layoffs.)

[SarahFrench]

I hope you weren't hit in the recent layoffs

Thanks for your concern 💟 - I'm still here

If there is no layer_7_ddos_defense_config block provided (adaptive_protection_config may be present or not), the value for enable should be false, not null. Or, alternatively, there should be no diff for false -> null, but I'm not sure if that's feasible.

I think this would be worth addressing in a separate PR, because if we bundle non-breaking changes up with a breaking change in one PR then their release may be unnecessarily delayed. This PR could be included as a reference to refer to though, so our discussions aren't wasted.

It looks like DiffSuppressFunc could provide this?

Yeah that's the correct approach!

---

I'll have to return to this PR next week, thanks for your patience!

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

• Field adaptive_protection_config.layer_7_ddos_defense_config.rule_visibility default value changed from STANDARD to on google_compute_security_policy - reference

If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An override-breaking-change label can be added to allow merging.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 37 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 2 files changed, 37 insertions(+), 1 deletion(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2783
Passed tests 2484
Skipped tests: 292
Affected tests: 7

Action taken

Found 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccComputeSecurityPolicy_withoutAdaptiveProtection|TestAccComputeFirewallPolicyRule_multipleRules|TestAccBigQueryDataTable_bigtable|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic

Get to know how VCR tests work

[modular-magician]

Tests passed during RECORDING mode:
TestAccComputeSecurityPolicy_withoutAdaptiveProtection[Debug log]
TestAccBigQueryDataTable_bigtable[Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccDataSourceAlloydbLocations_basic[Debug log]

Tests failed during RECORDING mode:
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

[smcgivern]

@SarahFrench I'm glad to hear that. There is no need to review this as it is, let me rethink a bit now I've seen more of the options for the schema here. In #8060 (comment) I said I wanted:

1. If there is no layer_7_ddos_defense_config block provided (adaptive_protection_config may be present or not), the value for enable should be false, not null. Or, alternatively, there should be no diff for false -> null, but I'm not sure if that's feasible.
2. If there is a layer_7_ddos_defense_config block provided, and it's disabled, we should not try to set rule_visibility.
3. If there is a layer_7_ddos_defense_config block provided, and it's enabled, rule_visibility should default to STANDARD and should not be allowed to be empty.

I don't think we need the breaking change in this PR at all.

We can do 1 with the DiffSuppressFunc, as you confirmed above.

If we do that, then I personally don't care about 2 and 3. But I think a better breaking change would be to set RequiredWith on rule_visibility, possibly with a DefaultFunc as well, to express those two cases, and it wouldn't actually break in any currently-valid config.

I'll create a new PR for item 1 once I've confirmed it fixes my immediate problem (persistent diff when nothing changed).

[smcgivern]

@SarahFrench unfortunately the DiffSuppressFunc approach won't work 😞

When we run this, we effectively remove the entire block (which is why the diffs show the whole block changing), and DiffSuppressFunc runs on those blocks, not on the individual fields. I tested this by adding this func (with a different prefix) everywhere:

func(k, old, new string, d *schema.ResourceData) bool {
	fmt.Printf("adaptive_protection_config: {%#v} {%#v}\n", old, new)
	return old == new
}

When running a test to simulate this scenario, I got the below, with no calls for the enable field:

layer_7_ddos_defense_config: {"1"} {"0"}
adaptive_protection_config: {"1"} {"0"}
adaptive_protection_config: {"1"} {"0"}
layer_7_ddos_defense_config: {"1"} {"0"}
adaptive_protection_config: {"1"} {"0"}
adaptive_protection_config: {"1"} {"0"}

So I have two problems:

1. It doesn't look like DiffSuppressFunc gets called on the individual field I want.
2. The function doesn't get enough information to suppress this diff (see also DiffSuppressFunc doesn't work for list-type attributes hashicorp/terraform-plugin-sdk#477).

I can't see a way to fix the first case from the description, sadly.

This PR still fixes the second case, so if I manually add the block with enable = false I can at least remove the persistent diff. I'm happy to wait for a major release, as I don't see any other option here.

Does that work for you? If so, I'll update the PR to fix the conflicts.

[smcgivern]

@SarahFrench ping 🙂 I got a note on another PR about the 5.0 release. What do you think about including this too?

[github-actions]

@GoogleCloudPlatform/terraform-team This PR has been waiting for review for 3 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

[SarahFrench]

Hi @smcgivern sorry for the long pause in this PR. After discussing the issue in our team we think we could change this field to no longer have a default but also have the field be Optional: true and Computed: true in its definition. This would stop the change being a breaking change and we can merge outside a major release cycle.

Making the field both optional and computed (O+C) We would then make the provider either use the value from a user's config to set the field, or let users not set that field in their config and Terraform will save the value returned from the API into state. Enabling that second behaviour via Computed: true will make this a non breaking change.

Would you be available to make these changes to the resource? If not I'm happy to open a PR myself to get your desired fix merged faster.

[SarahFrench]

Superseded by #10823