Skip to content

Add REMOVE_PEERING deletion policy to google_service_networking_connection - #18476

Open
ab0utbla-k wants to merge 1 commit into
GoogleCloudPlatform:mainfrom
ab0utbla-k:snc-remove-peering-deletion-policy
Open

Add REMOVE_PEERING deletion policy to google_service_networking_connection#18476
ab0utbla-k wants to merge 1 commit into
GoogleCloudPlatform:mainfrom
ab0utbla-k:snc-remove-peering-deletion-policy

Conversation

@ab0utbla-k

Copy link
Copy Markdown

Creating google_service_networking_connection also creates a VPC network peering on the consumer network. That peering is not a separate Terraform resource, so no configuration refers to it and nothing removes it on its own.

This shows up on destroy. A connection cannot be deleted while the service producer still holds resources behind it, and producers can hold them for a while after the service instance itself is deleted. Cloud SQL retains them so that a deleted instance can still be restored. The delete then fails with:

Failed to delete connection; Producer services (e.g. CloudSQL, Cloud Memstore, etc.)
are still using this connection.

The peering stays behind, and google_compute_network cannot be deleted while a peering references it. The result is that a configuration Terraform created in full cannot be destroyed in full, and what blocks it is an object the configuration never mentions. deletion_policy = "ABANDON" drops the connection from state but leaves the peering, so the network still cannot be deleted. Today the only way forward is to remove the peering by hand and run the destroy again.

This adds a REMOVE_PEERING value to deletion_policy. Delete behaves exactly as it does now. If the API refuses because producer resources still hold the connection, the peering is removed from the network so the network can be deleted.

Scope

This is opt in. The default stays DELETE, and PREVENT, ABANDON and DELETE are unchanged. The fallback runs only when both conditions hold: the policy is REMOVE_PEERING, and the failure is the producer-in-use error. The policy check comes first, so nothing changes for anyone who has not opted in.

Removing the peering is only correct once the service instances are gone, because while an instance is running the peering carries its traffic. That is why this is an explicit opt in rather than a default or an automatic retry. It is documented on the field and in a new section on the resource page.

Removing the peering is also not the same as deleting the connection, so the connection may still exist on the service producer side. The docs say so rather than implying the connection is gone.

Matching the error

The operation error keeps its google.rpc.Status, so the status code is read from the typed error rather than parsed out of a formatted string:

opErr, ok := errors.AsType[*tpgresource.CommonOpError](err)

FAILED_PRECONDITION covers more than this one case, so the API message is checked as well. If either check stops matching, the fallback does not run and the resource behaves as it does today. A match that breaks therefore fails safe rather than removing a peering it should not.

A question for reviewers: if this error carries an ErrorInfo in Status.Details with a stable reason, matching on that would let the message check go away entirely. I could not confirm the payload from outside, and I would rather match on a documented value if one exists.

errors.AsType would be the first use in this repository. Happy to switch it to errors.As if you prefer to keep that consistent for now.

Tests

TestUnitServiceNetworkingConnection_isProducerServicesInUseError covers the predicate, including the case where the error has been wrapped by the operation waiter, which is how it reaches the delete function.

TestAccServiceNetworkingConnection_removePeering covers a connection that no producer holds, so the delete succeeds normally and the fallback is not reached.

The fallback actually firing is not covered. Triggering it needs a service producer that still retains resources, which I could not arrange from a test. If there is a way to set that up that I have missed, I am glad to add it.

Verified by generating both providers, then go build ./..., go vet and the unit test on each.

Related

Discussed in #16275 and #18834. This does not implement what #18834 asks for. That issue proposes replacing deleteConnection with removePeering outright, which would cut the peering out from under connections that are still in use. This keeps the current delete and falls back only after the API has already refused, which addresses the same problem without changing what happens for everyone else.

servicenetworking: added `REMOVE_PEERING` value to `deletion_policy` on `google_service_networking_connection`, which removes the VPC peering when the connection cannot be deleted because service producer resources still use it

…ction

Creating the connection also creates a VPC network peering that is not a
separate Terraform resource. When a service producer still holds the
connection, deleting it fails and the peering stays behind, which blocks
deletion of the network it is attached to.

REMOVE_PEERING keeps the current delete behaviour and, when the API refuses
because producer resources are still in use, removes the peering so the
network can be deleted.
@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Jul 30, 2026
@github-actions
github-actions Bot requested a review from BBBmau July 30, 2026 22:07
@github-actions

Copy link
Copy Markdown

Googlers: For automatic test runs see go/terraform-auto-test-runs.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-approval Pull requests that need reviewer's approval to run presubmit tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants