Updated resource "Region Security Policy" to allow for rule creation when creating the policy resource

[RokasDevelopment]

Updated resource "google_compute_region_security_policy" to allow rule creation at the same time when creating the policy. Functionality similar to what you can do with google_compute_security_policy.

It fixes a current issue that you cannot override the default created rule when (or after) creating a regional security policy. A default allow rule gets created at the same time, which cannot be controlled/changed without jumping through hoops to manually import it into your state: hashicorp/terraform-provider-google#15687

Release Note Template for Downstream PRs (will be copied)

compute: added "rules" property to "google_compute_region_security_policy" resource

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[modular-magician]

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

Diff report

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

google provider: Diff ( 1 file changed, 391 insertions(+))
google-beta provider: Diff ( 4 files changed, 2338 insertions(+), 61 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 737 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_compute_region_security_policy (62 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_compute_region_security_policy" "primary" {
  rules {
    network_match {
      dest_ip_ranges   = # value needed
      dest_ports       = # value needed
      ip_protocols     = # value needed
      src_asns         = # value needed
      src_ip_ranges    = # value needed
      src_ports        = # value needed
      src_region_codes = # value needed
      user_defined_fields {
        name   = # value needed
        values = # value needed
      }
    }
    preconfigured_waf_config {
      exclusion {
        request_cookie {
          operator = # value needed
          value    = # value needed
        }
        request_header {
          operator = # value needed
          value    = # value needed
        }
        request_query_param {
          operator = # value needed
          value    = # value needed
        }
        request_uri {
          operator = # value needed
          value    = # value needed
        }
        target_rule_ids = # value needed
        target_rule_set = # value needed
      }
    }
    preview = # value needed
    rate_limit_options {
      ban_duration_sec = # value needed
      ban_threshold {
        count        = # value needed
        interval_sec = # value needed
      }
      conform_action = # value needed
      enforce_on_key = # value needed
      enforce_on_key_configs {
        enforce_on_key_name = # value needed
        enforce_on_key_type = # value needed
      }
      enforce_on_key_name = # value needed
      exceed_action       = # value needed
      rate_limit_threshold {
        count        = # value needed
        interval_sec = # value needed
      }
    }
  }
}

[modular-magician]

Tests analytics

Total tests: 65
Passed tests: 13
Skipped tests: 52
Affected tests: 0

Click here to see the affected service packages

• compute

#### Non-exercised tests

Tests were added that are skipped in VCR:

• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesExample
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesUpdateExample
  🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

[github-actions]

@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[RokasDevelopment]

WIP on extra tests

[melinath]

in addition to the additional tests - this is currently running into some kind of issue with float64 values in int fields while calculating the set hash for the rules:

panic: interface conversion: interface {} is float64, not int

goroutine 108163 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.SerializeValueForHash(0xc001bdcf50?, {0x418bcc0?, 0xc0020acf60?}, 0x513c4c?)
	/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/serialize.go:27 +0x297

I see there's logic in the PR related to converting floats to ints; perhaps there's a missed case?

[RokasDevelopment]

in addition to the additional tests - this is currently running into some kind of issue with float64 values in int fields while calculating the set hash for the rules:

panic: interface conversion: interface {} is float64, not int

goroutine 108163 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.SerializeValueForHash(0xc001bdcf50?, {0x418bcc0?, 0xc0020acf60?}, 0x513c4c?)
	/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/serialize.go:27 +0x297

I see there's logic in the PR related to converting floats to ints; perhaps there's a missed case?

@melinath Could you tell me - how do I replicate this error?

[modular-magician]

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

Diff report

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

google provider: Diff ( 1 file changed, 333 insertions(+))
google-beta provider: Diff ( 4 files changed, 2412 insertions(+), 77 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 597 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_compute_region_security_policy (68 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_compute_region_security_policy" "primary" {
  rules {
    preconfigured_waf_config {
      exclusion {
        request_cookie {
          value = # value needed
        }
      }
    }
    rate_limit_options {
      enforce_on_key_name = # value needed
    }
  }
}

[modular-magician]

Tests analytics

Total tests: 1033
Passed tests: 950
Skipped tests: 74
Affected tests: 9

Click here to see the affected service packages

• compute

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

• TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy
• TestAccComputeRegionSecurityPolicyRule_withPreconfiguredWafConfig
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyBasicUpdateExample
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyUserDefinedFieldsUpdate
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesBasicUpdate
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesExample
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesMultipleEnforceOnKeyConfigs
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesPreconfiguredWafConfig
• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesRateLimitOptions

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy[Debug log]
TestAccComputeRegionSecurityPolicyRule_withPreconfiguredWafConfig[Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesBasicUpdate[Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesExample[Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesMultipleEnforceOnKeyConfigs[Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesPreconfiguredWafConfig[Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesRateLimitOptions[Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

🔴 Tests failed during RECORDING mode:
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyBasicUpdateExample[Error message] [Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyUserDefinedFieldsUpdate[Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[melinath]

@melinath Could you tell me - how do I replicate this error?

Well... it looks like something you did fixed it, so there's no need. :-)

[melinath]

The two failing tests are not failing in our nightlies, so this seems likely to be related to your change. The error message for both is:

Error: Error updating RegionSecurityPolicy "projects/ci-test-project-188019/regions/us-central1/securityPolicies/tf-test490g73z6eh": googleapi: Error 400: Invalid value for field 'resource.rules[0].networkMatch': ''. Network matcher must have at least one field set., invalid

Looking through the test failures vs. our nightlies, the issue seems to be that in the nightlies, since the rules field doesn't exist at all, it isn't sent in the PATCH request. However, for your PR, since the rules field does exist (and it's default_from_api) the value returned by the API will be sent with the next API request - but only the fields supported by Terraform.

It looks like the security_policy logic for update has some pretty customized logic for determining whether to make any changes based on rule updates and calls a completely different API method. You PROBABLY don't need to replicate that; it probably pre-dates update support for the rules field via the standard PATCH method.

And it looks like the API is ignoring the updateMask parameter.

This is a very long way of saying: probably the best path forward is to also add support for the networkMatcher field (even though it isn't supported on the google_compute_security_policy resource) and potentially any other fields that are returned but not currently supported, to avoid breaking users' existing configs.

The default API response seems to be:

  "rules": [
    {
      "kind": "compute#securityPolicyRule",
      "description": "default rule",
      "priority": 2147483647,
      "networkMatch": {
        "srcIpRanges": [
"*"
        ]
      },
      "action": "allow",
      "preview": false,
      "ruleNumber": "1"
    }
  ],

https://googlecloudplatform.github.io/magic-modules/develop/test/run-tests/ has more information about how to get debug logs and run manual tests.

Additionally, please add test coverage for the fields mentioned here: #11894 (comment)

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesBasicUpdate [Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesMultipleEnforceOnKeyConfigs [Debug log]
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesRateLimitOptions [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

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

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[modular-magician]

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

Diff report

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

google provider: Diff ( 1 file changed, 391 insertions(+))
google-beta provider: Diff ( 4 files changed, 3046 insertions(+), 88 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 734 insertions(+))

[modular-magician]

Tests analytics

Total tests: 1054
Passed tests: 980
Skipped tests: 73
Affected tests: 1

Click here to see the affected service packages

• compute

Action taken

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

• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesNetworkMatch

Get to know how VCR tests work

[modular-magician]

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

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[modular-magician]

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

Diff report

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

google provider: Diff ( 1 file changed, 391 insertions(+))
google-beta provider: Diff ( 4 files changed, 3048 insertions(+), 88 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 734 insertions(+))

[modular-magician]

Tests analytics

Total tests: 1054
Passed tests: 980
Skipped tests: 73
Affected tests: 1

Click here to see the affected service packages

• compute

Action taken

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

• TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesNetworkMatch

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccComputeRegionSecurityPolicy_regionSecurityPolicyWithRulesNetworkMatch [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

🟢 All tests passed!

View the build log or the debug log for each test

[RokasDevelopment]

🥳

[melinath]

LGTM - I know this ended up getting a lot bigger than you expected, thanks for sticking with it!

[RokasDevelopment]

LGTM - I know this ended up getting a lot bigger than you expected, thanks for sticking with it!

Thank you for the support!