adding security policy field to instance

[felipegc]

Fixes: hashicorp/terraform-provider-google#15719

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 `network_interface.security_policy` field to `google_compute_instance` resource (beta)

[modular-magician]

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

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

[felipegc]

@roaks3 Would you mind helping me out with something here please?

The api(https://cloud.google.com/compute/docs/reference/rest/beta/instances/setSecurityPolicy) states that "You can only set a security policy for network interfaces with an access config" and expects something like this:

{
  "networkInterfaces": [
    "nic0",
    "nic1"
   ],
   "securityPolicy": "path/to/security/policy"
}

But the "security_policy" is actually only retrieved inside of the access_configs inside the networkInterface. This field is read_only and the networkInterface might have two access_config: one for ipv4 and another one for ipv6.
From the beginning I was thinking to let the user set the "security_policy" inside the "access_config" but then I found out we also have the "ipv6_access_config". My question is which option of the following 2 do you think might be better?

1- Setting the security_policy inside the access_config and somehow make sure the user will be setting exactly the same
"security_policy" in both "access_config". For this option I will need to make sure the use set exactly the same policy inside both access config for create and update. You may look at this example:

resource "google_compute_instance" "target-vm" {
  name         = "felipegc-tf-vm"
  machine_type = "e2-medium"
  zone         = "asia-southeast1-a"

  desired_status = "RUNNING"

  boot_disk {
    initialize_params {
      image = data.google_compute_image.vmimage.self_link
    }
  }
  
  network_interface {       
    network = google_compute_network.net2.self_link
    subnetwork = google_compute_subnetwork.subnetipv6.self_link
    stack_type = "IPV4_IPV6"
    ipv6_access_config {
      external_ipv6               = google_compute_address.ipv6-address.address
      external_ipv6_prefix_length = 96
      name                        = "external-ipv6-access-config"
      network_tier                = "PREMIUM"
      security_policy = google_compute_region_security_policy.policyforinstance.self_link
    }
    access_config {
      network_tier = "PREMIUM"
      nat_ip = google_compute_address.foo.address
      security_policy = google_compute_region_security_policy.policyforinstance.self_link
    }
  }
}

2- Create the "security_policy" field inside the "network_attachment" as a "virtual_field" since it does not exist inside the api/proto and let the "security_policy" inside both access_config as read only? For this option I will check if there is one access_config and take the value from the first one I find to set the value while "flattening" the network_attachment for read operations. For update I will just need to check if there is one access_config.
I believe the implementation and maintenance for this options seems to be easier and kind follow the api behavior. You may look at the example:

resource "google_compute_instance" "target-vm" {
  name         = "felipegc-tf-vm"
  machine_type = "e2-medium"
  zone         = "asia-southeast1-a"

  desired_status = "RUNNING"

  boot_disk {
    initialize_params {
      image = data.google_compute_image.vmimage.self_link
    }
  }
  
  network_interface {       
    network = google_compute_network.net2.self_link
    subnetwork = google_compute_subnetwork.subnetipv6.self_link
    stack_type = "IPV4_IPV6"
    ipv6_access_config {
      external_ipv6               = google_compute_address.ipv6-address.address
      external_ipv6_prefix_length = 96
      name                        = "external-ipv6-access-config"
      network_tier                = "PREMIUM"
    }
    access_config {
      network_tier = "PREMIUM"
      nat_ip = google_compute_address.foo.address
    }
    security_policy = google_compute_region_security_policy.policyforinstance2.self_link
  }
}

[roaks3]

@felipegc I agree with your point about the second option being easier for the user, but I think this could get complicated if we stray too far from the API behavior. Can there be separate security policies, one for ipv6_access_config and one for access_config? Does the API prevent this, and if not, what does it return?

[felipegc]

@felipegc I agree with your point about the second option being easier for the user, but I think this could get complicated if we stray too far from the API behavior. Can there be separate security policies, one for ipv6_access_config and one for access_config? Does the API prevent this, and if not, what does it return?

The api does not prevent anything. It basically applies the security policy to the whole network interface. If the network interface has 2 access config(one for ipv4 and another one for ipv6) it will apply for both of them. The security_field is read only inside the access_config, which means if we set the security_policy, all access_configs inside the network_interface will have the same value. Actually I believe the second option follows the api more than the first one.

[roaks3]

If the network interface has 2 access config(one for ipv4 and another one for ipv6) it will apply for both of them

if we set the security_policy, all access_configs inside the network_interface will have the same value

To clarify: does the API support a different security_policy in each *access_config, or does it force all *access_config to use the same security_policy?

[felipegc]

If the network interface has 2 access config(one for ipv4 and another one for ipv6) it will apply for both of them

if we set the security_policy, all access_configs inside the network_interface will have the same value

To clarify: does the API support a different security_policy in each *access_config, or does it force all *access_config to use the same security_policy?

It force all access_config to use the same security_policy.

[roaks3]

Ok thanks! My personal read here is that your Option 2 is more intuitive, and perhaps the API should have been modeled that way, but I think Option 1 more closely matches where the API is today. Particularly, it seems that the API considers the security_policy to be a part of the *access_config, so taking it out could be confusing to users, and risk contradicting future changes on the API side.

I would recommend proceeding with Option 1 here, but I'll consult with the team in case there are existing patterns I'm unaware of. Thanks for taking the time to consider the best solution here.

[felipegc]

Ok thanks! My personal read here is that your Option 2 is more intuitive, and perhaps the API should have been modeled that way, but I think Option 1 more closely matches where the API is today. Particularly, it seems that the API considers the security_policy to be a part of the *access_config, so taking it out could be confusing to users, and risk contradicting future changes on the API side.

I would recommend proceeding with Option 1 here, but I'll consult with the team in case there are existing patterns I'm unaware of. Thanks for taking the time to consider the best solution here.

Agreed. I proceeded with solution one and all tests I created are passing. Thanks for the help

[roaks3]

Ok, so I've got an update after speaking with the team. Here's the way we'd like to move forward:

• Make the network_interface.access_config.security_policy and network_interface.ipv6_access_config.security_policy fields output-only, so that they are populated by the API, but cannot be set by the user.
• Add a network_interface.security_policy field with ignore_read, so that it is not populated by the API. This is similar to your virtual field idea, where this field allows the user to set (and unset) the policy.
• Optional: Manually update the network_interface.security_policy field when changes are made to the *access_config.security_policy fields, to detect drift.

We believe this strategy will be closest to the API, and prevent difficulties in keeping all fields in sync (for example, if lifecycle.ignore_changes is used on one field and not the other).

I did look through your current implementation, which looks good. I think you would just need to tweak it a bit to achieve the desired model.

[roaks3]

I will also be on PTO coming up, so re-rolling the reviewer

[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, 5 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1158 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 8 insertions(+), 2 deletions(-))

Missing test report

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

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

resource "google_compute_instance_from_machine_image" "primary" {
  network_interface {
    access_config {
      security_policy = # value needed
    }
    ipv6_access_config {
      security_policy = # value needed
    }
  }
}

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

resource "google_compute_instance_from_template" "primary" {
  network_interface {
    access_config {
      security_policy = # value needed
    }
    ipv6_access_config {
      security_policy = # value needed
    }
  }
}

[modular-magician]

$\textcolor{red}{\textsf{The provider crashed while running the VCR tests in REPLAYING mode}}$
$\textcolor{red}{\textsf{Please fix it to complete your PR}}$
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, 2 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1314 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 1 file changed, 19 insertions(+))

Missing test report

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

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

resource "google_compute_instance_from_machine_image" "primary" {
  network_interface {
    security_policy = # value needed
  }
}

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

resource "google_compute_instance_from_template" "primary" {
  network_interface {
    security_policy = # value needed
  }
}

[modular-magician]

Tests analytics

Total tests: 3076
Passed tests 2772
Skipped tests: 299
Affected tests: 5

Action taken

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

TestAccBigQueryDataTable_bigtable|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccHealthcareDatasetIamPolicy

Get to know how VCR tests work

[modular-magician]

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

Rerun these tests in REPLAYING mode to catch issues

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

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigQueryDataTable_bigtable[Error message] [Debug log]
TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample[Error message] [Debug log]
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample[Error message] [Debug log]
TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample[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

[felipegc]

Hi @zli82016 are these failures related to my code?

[zli82016]

They are not. Sorry, can you please resolve the conflicts? Thanks.

[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, 2 insertions(+))
Terraform Beta: Diff ( 5 files changed, 1315 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 13 insertions(+))

Missing test report

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

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

resource "google_compute_instance_from_machine_image" "primary" {
  network_interface {
    security_policy = # value needed
  }
}

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

resource "google_compute_instance_from_template" "primary" {
  network_interface {
    security_policy = # value needed
  }
}

[modular-magician]

Tests analytics

Total tests: 3078
Passed tests 2778
Skipped tests: 299
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

TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Get to know how VCR tests work

[modular-magician]

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

[zli82016]

LGTM. Thanks.