Support IPV6_ONLY configurations for compute API

[karolgorc]

related to b/360733056

This provides support to set external and internal IPV6 addresses only for VM's and templates

I'm not sure if changing ipv6_access_config to Computed: true is a breaking change but it's needed because when providing external ipv6 subnetwork the field will be filled from API. CI tests should spot this if it is breaking but would love some feedback here

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

compute: `stack_type` can now be set to `IPV6_ONLY` on `google_compute_subnetwork`, `google_compute_instance`, `google_compute_instance_template` and `google_compute_region_instance_template`.

[github-actions]

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

@slevenick, 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 ( 8 files changed, 12 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 8 files changed, 12 insertions(+), 9 deletions(-))

[modular-magician]

Tests analytics

Total tests: 1060
Passed tests: 986
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

• TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Get to know how VCR tests work

[slevenick]

Why are we adding Computed on this field?

[karolgorc]

As stated in PR's description, if the subnetwork given to the network_interface is a subnetwork of external IPv6 addresses ipv6_access_config will get filled from the API without user's interaction

resource "google_compute_subnetwork" "test" {
  name = "testing"
  network = google_compute_network.test.self_link
  stack_type = "IPV6_ONLY"
  ipv6_access_type = "EXTERNAL"
}

resource "google_compute_instance" "test" {
  name = "test"
  machine_type = "n2-standard-2"

  boot_disk {
    initialize_params {
      image = "debian-cloud/debian-11"
    }
  }

  network_interface {
    subnetwork = google_compute_subnetwork.test.self_link
    stack_type = "IPV6_ONLY"
  }
}

[modular-magician]

🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [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

[karolgorc]

What does this fail on?

TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Can't test this locally

Error: Error creating RegionSecurityPolicy: googleapi: Error 400: Invalid value for field 'resource.type': 'CLOUD_ARMOR_NETWORK'. Network Security Policies are not supported as part of the current Cloud Armor service tier., invalid

[slevenick]

What does this fail on?

TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Can't test this locally

Error: Error creating RegionSecurityPolicy: googleapi: Error 400: Invalid value for field 'resource.type': 'CLOUD_ARMOR_NETWORK'. Network Security Policies are not supported as part of the current Cloud Armor service tier., invalid

resource_compute_instance_test.go:3893: Step 5/5, expected an error with pattern, no match on: Error running apply: exit status 1

    Error: Error deleting old access_config: googleapi: Error 400: Invalid value for field 'accessConfig': 'external-nat'. Cannot delete an access config with a security policy set. Please remove the security policy first., invalid

[github-actions]

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

[karolgorc]

I'm guessing that it's this subtest specifically testAccComputeInstance_nic_securityPolicyCreateWithAccessConfigUpdateAccessConfig

There is a weird handling of errors here on terraform's side because the error can be thrown both by the Security policy and by the Access Config (having different error messages). So if any changes occur in the API this test is very fragile.

Already fixed that one time #11350

Does this fail in other PR's? Or is this caused by my change? If it's only here i'm guessing that the only think causing this could be the Computed: true so i'll try to do a ComputedIf function if that's the case

[slevenick]

/gcbrun I'll rerun it

[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 ( 8 files changed, 12 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 8 files changed, 12 insertions(+), 9 deletions(-))

[modular-magician]

Tests analytics

Total tests: 1064
Passed tests: 990
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

• TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Get to know how VCR tests work

[modular-magician]

🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [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

[zli82016]

@karolgorc , the test TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy also failed during the nightly runs since this PR was merged on Nov 18. Do you have the plan to fix it soon? Or we can consider reverting this PR.

[karolgorc]

Not sure why this was merged because i can't test this without Cloud Armor on my GCP environment to tell whether this was local or caused by something else =P.

Does this fail in other PR's? Or is this caused by my change?

I have a fix. I'll create a new PR with it but i'm also looking out for any customer issues related to this as of now

[zli82016]

@karolgorc , there are not customer issues related to this now, as this change has not been released to the customers yet.

Thanks for working on the fix. The new PR needs to be merged by end of Tuesday next week, when the next release will be cut. Otherwise, this PR has to be reverted.

[zli82016]

@karolgorc , can you help me understand why adding Computed: true to ipv6_access_config causes the test failing?

In the test TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy/access_config_update_access_config, stack_type is "IPV4_IPV6", not IPV6_ONLY

Thanks.

[karolgorc]

can you help me understand why adding Computed: true to ipv6_access_config causes the test failing?

Not sure about the API inner workings here but we have a weird race condition here where the smallest change cloud alter the error message. It's either thrown by the access config or by the security policy. At least that's what it was when last fixing it.

Not sure why it behaves this way on "IPV6_ONLY"

We cannot really make a condition in terraform here to use ComputedIf because this would require queries for the type of subnetwork that's provided to the subnetwork resource itself.

Basically for now i'd say that this feature generates a diff that can only be handled by using "Computed: True" and breaks this test. Unless we want to do some weird hacks with CustomizeDiff but i'd say that would be troublesome to maintain down the line.

As i said i don't have an environment with Cloud Armor to closely look into this.