[#15779] Add google_network_security_security_profile resource

[LucaPrete]

Adds the new google_network_security_security_profile resource.

fixes hashicorp/terraform-provider-google/issues/15779

Release Note Template for Downstream PRs (will be copied)

`google_network_security_security_profile`

[LucaPrete]

Waiting to merge this PR and rebasing so that the copyright tests can pass.

[ScottSuarez]

/gcbrun

[modular-magician]

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@roaks3, 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.

Terraform GA: Diff ( 2 files changed, 221 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1235 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 213 insertions(+))

Missing test report

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

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

resource "google_network_security_security_profile" "primary" {
  threat_prevention_profile {
    threat_overrides {
      action    = # value needed
      threat_id = # value needed
    }
  }
  type = # value needed
}

[modular-magician]

Tests analytics

Total tests: 29
Passed tests 26
Skipped tests: 0
Affected tests: 3

Click here to see the affected service packages

• networksecurity

Action taken

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

TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileBasicExample|TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileSeverityOverrideExample|TestAccNetworkSecuritySecurityProfiles_update

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileBasicExample[Debug log]
TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileSeverityOverrideExample[Debug log]
TestAccNetworkSecuritySecurityProfiles_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
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.

Terraform GA: Diff ( 2 files changed, 226 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1247 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 213 insertions(+))

[modular-magician]

Tests analytics

Total tests: 29
Passed tests 28
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages

• networksecurity

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

TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileOverridesExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileOverridesExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[LucaPrete]

/gcbrun

[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.

Terraform GA: Diff ( 2 files changed, 226 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1247 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 213 insertions(+))

[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.

Terraform GA: Diff ( 2 files changed, 226 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1247 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 213 insertions(+))

[modular-magician]

Tests analytics

Total tests: 29
Passed tests 28
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages

• networksecurity

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

TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileOverridesExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileOverridesExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
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.

Terraform GA: Diff ( 2 files changed, 226 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1247 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 213 insertions(+))

[modular-magician]

Tests analytics

Total tests: 29
Passed tests 29
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• networksecurity

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

[roaks3]

Overall LGTM, just a few minor comments regarding the fields

[roaks3]

I'm unfamiliar with this paradigm, so just checking, do you know if it is needed here?

[LucaPrete]

Centralizing the discussion here, given I was using this in multiple resources "empirically" and I got the same question - cc @melinath

FYI when I say empirically, I mean I copied this from the networksecurity/AddressGroup resource, part of the networkservice as well, being a very similar resource, after seeing my resource not working. I then decided to understand better what I was doing. Here is the result of my investigation:

Without doing any modification, you would see all tests failing with the following error

Error: ../../../go/pkg/mod/github.com/modular-magician/terraform-provider-google-beta@v1.20.1-0.20240112173651-8e753b30b478/google-beta/services/networksecurity/resource_network_security_security_profile.go:425:4: not enough arguments in call to NetworkSecurityOperationWaitTime
    have (*"github.com/hashicorp/terraform-provider-google-beta/google-beta/transport".Config, map[string]interface{}, string, string, time.Duration)
    want (*"github.com/hashicorp/terraform-provider-google-beta/google-beta/transport".Config, map[string]interface{}, string, string, string, time.Duration)

TL;DR the operationWaitTime call seems different from other networksecurity calls and takes another param in input: the project. It doesn't need specific values. It can just be an empty string.

Indeed, I checked the go file that was produced and this is what the NetworkSecurityOperationWaitTime function before the modifications looks like:

err = NetworkSecurityOperationWaitTime(
        config, res, "Updating SecurityProfile", userAgent,
        d.Timeout(schema.TimeoutUpdate))

include_project: true in the SecurityProfile.yaml file creates a project variable in the resulting built file and passes it into the NetworkSecurityOperationWaitTime function.

Indeed, after adding it and going again to the resulting go file:

• at the top of the create function, there's a declaration var project string

• the NetworkSecurityOperationWaitTime function becomes:

err = NetworkSecurityOperationWaitTime(
        config, res, project, "Creating SecurityProfile", userAgent,
        d.Timeout(schema.TimeoutCreate))

I honestly don't understand the meaning of the custom_code block. They simply initialize that variable to an empty string, which should happen by default in go as soon as we declare it as a string (effect of include_project: true). Indeed, removing the custom code directive and the related pre* files has no negative effects on the tests.

If that's the case, I'll send another PR to remove them as well for the AuthorizationPolicy resource.

Please, let me know if this helps to clarify.

[melinath]

If removing the custom code doesn't have any negative impacts, then it would be great to remove. Additionally, include_project does seem to actually be used in our templates but I'm not familiar with the

include_project does seem to actually have an impact, but it is a very unusual flag - I only see 8 uses across the entire provider at the moment. According to

magic-modules/mmv1/api/async.rb

Line 69 in 2031a55

# If true, include project as an argument to OperationWaitTime.

, it's intended for resources that calculate the project from a selflink field (which doesn't seem to apply here). I'd say that it would be good to remove the usage here (and from the PR I'm reviewing) if possible.

[melinath]

Taking a closer look at this, I think that include_project: true actually probably is a good thing to use here.

Operations are a shared class within a given service, which can cause some weird edge cases because we're not currently consistent about their configuration being shared - some templates reference the product.yaml async block, and some reference resource async blocks. In particular, resource files (which call the operation) check whether to include a project based on the resource's config - but the operation go file for a service uses the async block from the first resource it finds in the service with autogen_async: true set.

So, because the first matching resource in the networksecurity product happens to generate an operation that requires the project argument, all resources in the product need to be sending that argument.

The project is ultimately just passed through to transport_tpg.SendRequest, which uses it to set the X-Goog-User-Project header (for project billing overrides).

I don't think it makes sense to remove support for this from resources that previously supported it, so using include_project: true seems like a reasonable step.

Technically there's probably some additional work here to add support for extracting the project from the parent field (if that would ever be a project.) But I don't think that's necessarily a blocker (especially if the parent would only ever be an organization or folder) - I'll check on this though.

[LucaPrete]

Thanks @melinath ! This definitely helped me to understand one more piece of mmv1 :)

[roaks3]

Just circling back on this, it looks like we've removed the custom code blocks, and now just use include_project: true, which I believe is the best option moving forward. I would advocate for unblocking this PR since it is matching an existing pattern, and if we would like to make improvements to this usage pattern, do so afterward across all impacted resources.

I'll wait to hear back from @melinath before merging anything.

[roaks3]

Caught up offline: sounds like we agree this is not a blocker, so we can move forward with merging.

[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.

Terraform GA: Diff ( 2 files changed, 226 insertions(+))
Terraform Beta: Diff ( 6 files changed, 1383 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 213 insertions(+))

[modular-magician]

Tests analytics

Total tests: 29
Passed tests 29
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• networksecurity

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

[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.

Terraform GA: Diff ( 2 files changed, 227 insertions(+))
Terraform Beta: Diff ( 6 files changed, 1386 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 213 insertions(+))

[modular-magician]

Tests analytics

Total tests: 29
Passed tests 29
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• networksecurity

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log