adding security_policy field to TargetInstance

[felipegc]

Fixes hashicorp/terraform-provider-google#15175

Adding security_policy field in google_compute_target_instance resource.

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 Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

compute: added `security_policy` field to `google_compute_target_instance` resource (beta)

[modular-magician]

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

I've detected that you're a community contributor. @melinath, 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.

---

[felipegc]

@melinath I have a question:

This feature is only available for whitelist project now. Is that possible to proceed with the code review, merge and release for that with some kind of "warning" or "disclaimer"?
If so is there any feature in the same situation you may know so I can try to follow?

Thanks

[melinath]

It's okay to add fields that require being on an allowlist as long as they are publicly visible in the API documentation. I'm not sure whether it's required on our end to explain the allowlist in the field's documentation, but it probably wouldn't hurt.

However, we will need to make sure that our test projects get added to the relevant allowlist (if they aren't already) so that we can run tests related to the feature. Could you provide a link to any relevant docs?

[felipegc]

It's okay to add fields that require being on an allowlist as long as they are publicly visible in the API documentation. I'm not sure whether it's required on our end to explain the allowlist in the field's documentation, but it probably wouldn't hurt.

However, we will need to make sure that our test projects get added to the relevant allowlist (if they aren't already) so that we can run tests related to the feature. Could you provide a link to any relevant docs?

Thanks for the quick reply. Here is the api documentation: https://cloud.google.com/compute/docs/reference/rest/beta/targetInstances/setSecurityPolicy

We will probably need to add the build project/region in allowlist. In order to run my tests locally my project was added including the “asia-southeast” region.

If the build projects are not added in allowlist what is the next step to do?

Thanks

[melinath]

It looks like the tests are currently failing with:

Error: Error creating TargetInstance: googleapi: Error 400: Invalid value for field 'resource.instance': 'asia-southeast1-a'. Invalid zone asia-southeast1-a for the instance. It must be same as target instance's zone, invalid

This happens while trying to create the TargetInstance, so I can't tell whether the test project is on the allowlist already or not. If not, we'll have to work with the team adding the feature to get on the allowlist (if that is possible.)

[felipegc]

It looks like the tests are currently failing with:

Error: Error creating TargetInstance: googleapi: Error 400: Invalid value for field 'resource.instance': 'asia-southeast1-a'. Invalid zone asia-southeast1-a for the instance. It must be same as target instance's zone, invalid

This happens while trying to create the TargetInstance, so I can't tell whether the test project is on the allowlist already or not. If not, we'll have to work with the team adding the feature to get on the allowlist (if that is possible.)

@melinath This probably happened because the test was not setting the zone so it likely used the default zone set in the environment which was different from the one used in the test. I changed the test to make sure to use the same zone as the instance now.

If the project needs to be allowlisted we should expect an error like: "Required 'access' permission for 'Compute API'" while executing the setSecurityPolicy method.

Could you please run again?
Thanks

[melinath]

Please make the security_policy field updatable and add an update 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, 96 insertions(+))
Terraform Beta: Diff ( 4 files changed, 473 insertions(+))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
TF OiCS: Diff ( 4 files changed, 179 insertions(+))

[melinath]

thanks for adding the update test! The one thing missing is a lifecycle block to ensure that the updates are actually happening rather than a destroy + recreate

Examples:

• TestAccAlloydbCluster_update
• TestAccDatastreamConnectionProfile_update
• TestAccComputeDisk_pdHyperDiskProvisionedIopsLifeCycle

[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, 96 insertions(+))
Terraform Beta: Diff ( 4 files changed, 480 insertions(+))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
TF OiCS: Diff ( 4 files changed, 179 insertions(+))

[modular-magician]

Tests analytics

Total tests: 2963
Passed tests 2660
Skipped tests: 302
Affected tests: 1

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

TestAccComputeTargetInstance_targetInstanceWithSecurityPolicyExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeTargetInstance_targetInstanceWithSecurityPolicyExample[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