Add REMOVE_PEERING deletion policy to google_service_networking_connection - #18476
Open
ab0utbla-k wants to merge 1 commit into
Open
Add REMOVE_PEERING deletion policy to google_service_networking_connection#18476ab0utbla-k wants to merge 1 commit into
ab0utbla-k wants to merge 1 commit into
Conversation
…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.
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Creating
google_service_networking_connectionalso 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:
The peering stays behind, and
google_compute_networkcannot 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_PEERINGvalue todeletion_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, andPREVENT,ABANDONandDELETEare unchanged. The fallback runs only when both conditions hold: the policy isREMOVE_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:FAILED_PRECONDITIONcovers 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
ErrorInfoinStatus.Detailswith a stablereason, 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.AsTypewould be the first use in this repository. Happy to switch it toerrors.Asif you prefer to keep that consistent for now.Tests
TestUnitServiceNetworkingConnection_isProducerServicesInUseErrorcovers the predicate, including the case where the error has been wrapped by the operation waiter, which is how it reaches the delete function.TestAccServiceNetworkingConnection_removePeeringcovers 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 vetand the unit test on each.Related
Discussed in #16275 and #18834. This does not implement what #18834 asks for. That issue proposes replacing
deleteConnectionwithremovePeeringoutright, 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.