Add update support to serverTlsPolicy field in region_target_https_proxy

[gservat]

Adds support to update "server_tls_policy" field in "compute_region_target_https_proxy" via the PATCH endpoint, removing its ForceNew flag, as well as related tests.

Fixes: hashicorp/terraform-provider-google#18757

Release Note Template for Downstream PRs (will be copied)

compute: added update support for the 'server_tls_policy' field in 'compute_region_target_https_proxy' resource

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[github-actions]

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

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

[gservat]

@NickElliot: I added the test first (before the update to RegionTargetHttpsProxy.yaml) and confirmed it fails:

❯ make testacc TEST=./google-beta/services/compute TESTARGS='-run=TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule'
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta/services/compute -v -run=TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
=== PAUSE TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
=== CONT  TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
    vcr_utils.go:152: Step 3/4 error: Error running apply: exit status 1

        Error: Error when reading or editing RegionTargetHttpsProxy: googleapi: Error 400: The target_https_proxy resource 'projects/gservat-75cb0b26/regions/us-central1/targetHttpsProxies/https-proxy-avjnpbb2w0' is already being used by 'projects/gservat-75cb0b26/regions/us-central1/forwardingRules/https-frwd-rule-avjnpbb2w0', resourceInUseByAnotherResource

--- FAIL: TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule (159.03s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute        159.370s
FAIL
make: *** [testacc] Error 1

After making the necessary changes to RegionTargetHttpsProxy.yaml to support updating serverTlsPolicy via PATCH, I've confirmed that it works as expected:

❯ make testacc TEST=./google-beta/services/compute TESTARGS='-run=TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule'
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta/services/compute -v -run=TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
=== PAUSE TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
=== CONT  TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
--- PASS: TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule (256.12s)
PASS
ok      github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute        256.464s

[gservat]

@trodge @slevenick hi 👋 any chance I can get you to review this PR?

[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 ( 2 files changed, 75 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 2 files changed, 293 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 972
Passed tests: 896
Skipped tests: 74
Affected tests: 2

Click here to see the affected service packages

• compute

Action taken

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

• TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
• TestAccComputeRegionTargetHttpsProxy_addSslPolicy_withForwardingRule

Get to know how VCR tests work

[modular-magician]

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

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

---

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

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

[github-actions]

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

[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 ( 2 files changed, 75 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 2 files changed, 421 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 975
Passed tests: 901
Skipped tests: 74
Affected tests: 0

Click here to see the affected service packages

• compute

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

[NickElliot]

Could you add a third test step returning to the testAccComputeRegionTargetHttpsProxy_withForwardingRule_withoutServerTlsPolicy config? Thanks!

[gservat]

Could you add a third test step returning to the testAccComputeRegionTargetHttpsProxy_withForwardingRule_withoutServerTlsPolicy config? Thanks!

@NickElliot sure, added.

[gservat]

@NickElliot hrm, just re-ran tests locally and it didn't work:

    vcr_utils.go:152: Step 5/6 error: Error running apply: exit status 1

        Error: Error when reading or editing ServerTlsPolicy: googleapi: Error 400: Resource '"projects/gservat-75cb0b26/locations/us-central1/serverTlsPolicies/tls-policy-l1qnvviwm7"' is already being used by resource(s) '"//compute.googleapis.com/projects/529811265946/locations/us-central1/targetHttpsProxies/https-proxy-l1qnvviwm7"'

I guess the issue here is that the third test is deleting the server TLS policy, which is tied to the HTTPS proxy, and Terraform may be destroying the resource before updating it?

[gservat]

I ran the Terraform manually using the locally built provider and this is the plan when applying the third-test:

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place
  - destroy

Terraform will perform the following actions:

  # google_certificate_manager_trust_config.default will be destroyed
  # (because google_certificate_manager_trust_config.default is not in configuration)
  - resource "google_certificate_manager_trust_config" "default" {
      - create_time      = "2024-07-23T23:33:26.717461128Z" -> null
      - effective_labels = {} -> null
      - id               = "projects/gservat-75cb0b26/locations/us-central1/trustConfigs/trust-config-test" -> null
      - labels           = {} -> null
      - location         = "us-central1" -> null
      - name             = "trust-config-test" -> null
      - project          = "gservat-75cb0b26" -> null
      - terraform_labels = {} -> null
      - update_time      = "2024-07-23T23:33:29.201885080Z" -> null
        # (1 unchanged attribute hidden)

      - trust_stores {
          - intermediate_cas {
              - pem_certificate = (sensitive value) -> null
            }
          - trust_anchors {
              - pem_certificate = (sensitive value) -> null
            }
        }
    }

  # google_compute_region_target_https_proxy.default-https will be updated in-place
  ~ resource "google_compute_region_target_https_proxy" "default-https" {
        id                               = "projects/gservat-75cb0b26/regions/us-central1/targetHttpsProxies/https-proxy-test"
        name                             = "https-proxy-test"
      - server_tls_policy                = "//networksecurity.googleapis.com/projects/gservat-75cb0b26/locations/us-central1/serverTlsPolicies/tls-policy-test" -> null
        # (10 unchanged attributes hidden)
    }

  # google_network_security_server_tls_policy.default will be destroyed
  # (because google_network_security_server_tls_policy.default is not in configuration)
  - resource "google_network_security_server_tls_policy" "default" {
      - allow_open       = false -> null
      - create_time      = "2024-07-23T23:33:43.463918095Z" -> null
      - effective_labels = {} -> null
      - id               = "projects/gservat-75cb0b26/locations/us-central1/serverTlsPolicies/tls-policy-test" -> null
      - labels           = {} -> null
      - location         = "us-central1" -> null
      - name             = "tls-policy-test" -> null
      - project          = "gservat-75cb0b26" -> null
      - terraform_labels = {} -> null
      - update_time      = "2024-07-23T23:33:45.984326762Z" -> null
        # (1 unchanged attribute hidden)

      - mtls_policy {
          - client_validation_mode         = "REJECT_INVALID" -> null
          - client_validation_trust_config = "projects/529811265946/locations/us-central1/trustConfigs/trust-config-test" -> null
        }
    }

Plan: 0 to add, 1 to change, 2 to destroy.

The change to the google_compute_region_target_https_proxy resource is being done in-place (as expected), however the issue is that Terraform is doing the destroy first instead of doing the update first:

Plan: 0 to add, 1 to change, 2 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

google_network_security_server_tls_policy.default: Destroying... [id=projects/gservat-75cb0b26/locations/us-central1/serverTlsPolicies/tls-policy-test]
╷
│ Error: Error when reading or editing ServerTlsPolicy: googleapi: Error 400: Resource '"projects/gservat-75cb0b26/locations/us-central1/serverTlsPolicies/tls-policy-test"' is already being used by resource(s) '"//compute.googleapis.com/projects/529811265946/locations/us-central1/targetHttpsProxies/https-proxy-test"'

I think this affects just about any resource destroy operation that is linked to from another resource as Terraform does a destroy before create. There's a 2 year old issue about this: hashicorp/terraform#31309

Any ideas on how to proceed here @NickElliot?

[gservat]

@NickElliot: just a friendly nudge to see if I can get this PR re-reviewed.

[github-actions]

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

[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 ( 2 files changed, 91 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 2 files changed, 449 insertions(+), 3 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 8 insertions(+))

[modular-magician]

Tests analytics

Total tests: 977
Passed tests: 898
Skipped tests: 74
Affected tests: 5

Click here to see the affected service packages

• compute

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

• TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule
• TestAccComputeRegionTargetHttpsProxy_addSslPolicy_withForwardingRule
• TestAccComputeRegionTargetHttpsProxy_regionTargetHttpsProxyBasicExample
• TestAccComputeRegionTargetHttpsProxy_regionTargetHttpsProxyCertificateManagerCertificateExample
• TestAccComputeRegionTargetHttpsProxy_update

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeRegionTargetHttpsProxy_addServerTlsPolicy_withForwardingRule[Debug log]
TestAccComputeRegionTargetHttpsProxy_addSslPolicy_withForwardingRule[Debug log]
TestAccComputeRegionTargetHttpsProxy_regionTargetHttpsProxyBasicExample[Debug log]
TestAccComputeRegionTargetHttpsProxy_regionTargetHttpsProxyCertificateManagerCertificateExample[Debug log]
TestAccComputeRegionTargetHttpsProxy_update[Debug log]

$\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

[github-actions]

@GoogleCloudPlatform/terraform-team @NickElliot This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

[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 ( 3 files changed, 105 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 4 files changed, 475 insertions(+), 3 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 8 insertions(+))

[modular-magician]

Tests analytics

Total tests: 1016
Passed tests: 941
Skipped tests: 75
Affected tests: 0

Click here to see the affected service packages

• compute
• networksecurity

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

[NickElliot]

a couple docs changes and this should be good!

[gservat]

a couple docs changes and this should be good!

@NickElliot thanks for reviewing. Just pushed a commit with your feedback 🙏

[gservat]

@NickElliot: Hi, just a friendly nudge to see if we can get this merged. Thanks!

[github-actions]

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

[NickElliot]

LGTM