Skip to content

Fix cloudstack_ipaddress delete silently succeeding on source-NAT IPs - #331

Open
sudo87 wants to merge 2 commits into
mainfrom
fix-ipaddress-sourcenat-delete
Open

Fix cloudstack_ipaddress delete silently succeeding on source-NAT IPs#331
sudo87 wants to merge 2 commits into
mainfrom
fix-ipaddress-sourcenat-delete

Conversation

@sudo87

@sudo87 sudo87 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

resourceCloudStackIPAddressDelete skipped the disassociate call entirely when is_source_nat was true and returned nil, so Terraform reported the resource destroyed while CloudStack left the IP allocated. Always call DisassociateIpAddress and propagate whatever CloudStack returns instead of swallowing it, matching every other error path in this file (the pre-existing "already gone" special case, which maps the "entity does not exist" error to nil for idempotent deletes, is intentionally kept).

Verified against a live 4.23.0.0 lab: against an implemented network with a running virtual router, CloudStack now correctly rejects the destroy (error 530, IP still in use for source NAT) instead of Terraform falsely reporting success; against a network that was never implemented, the disassociate genuinely succeeds and is now reported accurately.

resourceCloudStackIPAddressDelete skipped the disassociate call entirely
when is_source_nat was true and returned nil, so Terraform reported the
resource destroyed while CloudStack left the IP allocated. Always call
DisassociateIpAddress and propagate whatever CloudStack returns instead
of swallowing it, matching every other error path in this file.

Verified against a live 4.23.0.0 lab: against an implemented network
with a running virtual router, CloudStack now correctly rejects the
destroy (error 530, IP still in use for source NAT) instead of Terraform
falsely reporting success; against a network that was never implemented,
the disassociate genuinely succeeds and is now reported accurately.
@sudo87
sudo87 requested review from sureshanaparti and a lite review from Copilot August 28, 2026 09:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes Terraform incorrectly reporting CloudStack IP address resources as deleted when the IP is a source-NAT IP by always issuing the CloudStack disassociate call and returning CloudStack’s response.

Changes:

  • Removed the is_source_nat conditional that skipped disassociation during delete.
  • Always calls DisassociateIpAddress and returns the resulting error (except for the existing “already gone” special-case).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cloudstack/resource_cloudstack_ipaddress.go
Comment on lines +286 to 291
// This is a very poor way to be told the ID does no longer exist :(
if strings.Contains(err.Error(), fmt.Sprintf(
"Invalid parameter id value=%s due to incorrect long value format, "+
"or entity does not exist", d.Id())) {
return nil
}

@sudo87 sudo87 Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, that special-case (mapping CloudStack's "entity does not exist" error to nil) is intentional, for idempotent deletes when the IP is already gone. Updated the PR description to call that out explicitly rather than implying every error is now propagated.

@sureshanaparti sureshanaparti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

CloudStack rejects DisassociateIpAddress for a source NAT IP while its
owning network/VPC still exists, so the now-unconditional disassociate
call added in the previous commit fails terraform destroy on any
config that manages a source NAT IP as its own cloudstack_ipaddress
resource (e.g. imported directly). Deleting the network/VPC releases
the IP as a side effect, so treat that specific CloudStack error as
success here instead of failing the destroy.

@kiranchavala kiranchavala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM tested manually

resource "cloudstack_network" "test_net" {
  name             = "sourcenat-delete-test"
  cidr             = "10.50.0.0/24"
  network_offering = var.network_offering
  zone             = var.zone
}

# Deploying an instance forces CloudStack to *implement* the network,
# which is what actually creates the virtual router + source NAT IP.
resource "cloudstack_instance" "trigger_vr" {
  name             = "sourcenat-delete-trigger"
  service_offering = var.service_offering
  template         = var.template
  network_id       = cloudstack_network.test_net.id
  zone             = var.zone
  expunge          = true 

}

terraform apply


Terraform used the selected providers to generate the following execution plan. Resource actions are indicated
with the following symbols:
  + create

Terraform will perform the following actions:

  # cloudstack_instance.trigger_vr will be created
  + resource "cloudstack_instance" "trigger_vr" {
      + delete_protection      = (known after apply)
      + disk_offering          = (known after apply)
      + display_name           = (known after apply)
      + expunge                = true
      + group                  = (known after apply)
      + id                     = (known after apply)
      + ip_address             = (known after apply)
      + name                   = "sourcenat-delete-trigger"
      + network_id             = (known after apply)
      + override_disk_offering = (known after apply)
      + project                = (known after apply)
      + root_disk_size         = (known after apply)
      + service_offering       = "df040e78-a92f-4502-bd09-b8eba6c5e3d0"
      + start_vm               = true
      + tags                   = (known after apply)
      + template               = "298bed72-7127-4e3d-8e33-d38f2af651a9"
      + uefi                   = false
      + zone                   = "27befe1a-4e0b-4aad-b0ee-c57b6a118212"
    }

  # cloudstack_network.test_net will be created
  + resource "cloudstack_network" "test_net" {
      + acl_id                    = "none"
      + bypass_vlan_overlap_check = false
      + cidr                      = "10.50.0.0/24"
      + display_text              = (known after apply)
      + endip                     = (known after apply)
      + endipv6                   = (known after apply)
      + gateway                   = (known after apply)
      + id                        = (known after apply)
      + ip6gateway                = (known after apply)
      + name                      = "sourcenat-delete-test"
      + network_domain            = (known after apply)
      + network_offering          = "ee001adf-b20d-4958-9b87-f0a93457b694"
      + project                   = (known after apply)
      + source_nat_ip_address     = (known after apply)
      + source_nat_ip_id          = (known after apply)
      + startip                   = (known after apply)
      + startipv6                 = (known after apply)
      + tags                      = (known after apply)
      + zone                      = "27befe1a-4e0b-4aad-b0ee-c57b6a118212"
    }

Plan: 2 to add, 0 to change, 0 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

cloudstack_network.test_net: Creating...
cloudstack_network.test_net: Creation complete after 1s [id=9663cf7c-2a5b-4131-ac6c-915973089b84]
cloudstack_instance.trigger_vr: Creating...
cloudstack_instance.trigger_vr: Still creating... [00m10s elapsed]
cloudstack_instance.trigger_vr: Still creating... [00m20s elapsed]
cloudstack_instance.trigger_vr: Still creating... [00m30s elapsed]
cloudstack_instance.trigger_vr: Still creating... [00m40s elapsed]
cloudstack_instance.trigger_vr: Still creating... [00m50s elapsed]
cloudstack_instance.trigger_vr: Still creating... [01m00s elapsed]
cloudstack_instance.trigger_vr: Still creating... [01m10s elapsed]
cloudstack_instance.trigger_vr: Still creating... [01m20s elapsed]
cloudstack_instance.trigger_vr: Still creating... [01m30s elapsed]
cloudstack_instance.trigger_vr: Creation complete after 1m35s [id=ef8953f4-a8c7-4881-a667-5ae6db4c39ca]

Add to your config:

resource "cloudstack_ipaddress" "source_nat" {
network_id = cloudstack_network.test_net.id
}

Then:

terraform import cloudstack_ipaddress.source_nat
terraform plan # should show no drift once imported

terraform import cloudstack_ipaddress.source_nat d3915dac-0211-4e75-bfe1-15262aa99e05
cloudstack_ipaddress.source_nat: Importing from ID "d3915dac-0211-4e75-bfe1-15262aa99e05"...
cloudstack_ipaddress.source_nat: Import prepared!
  Prepared cloudstack_ipaddress for import
cloudstack_ipaddress.source_nat: Refreshing state... [id=d3915dac-0211-4e75-bfe1-15262aa99e05]

Import successful!

The resources that were imported are shown above. These resources are now in
your Terraform state and will henceforth be managed by Terraform.


╰─ terraform plan
cloudstack_network.test_net: Refreshing state... [id=9663cf7c-2a5b-4131-ac6c-915973089b84]
cloudstack_ipaddress.source_nat: Refreshing state... [id=d3915dac-0211-4e75-bfe1-15262aa99e05]
cloudstack_instance.trigger_vr: Refreshing state... [id=ef8953f4-a8c7-4881-a667-5ae6db4c39ca]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no
changes are needed.

Scenario A — network still exists (the bug PR 331 fixes):

terraform destroy -target=cloudstack_ipaddress.source_nat

Before the fix Terraform reports success immediately and removes it from state, but the IP is still allocated in CloudStack — cmk list publicipaddresses id= still shows it.

After the fix CloudStack still rejects it with the "used for source nat purposes and can not be disassociated" error,

2026-09-03 05:57:34,916 ERROR [c.c.a.ApiAsyncJobDispatcher] (API-Job-Executor-24:[ctx-37a60340, job-258]) (logid:1eeaa6d7) Unexpected exception while executing org.apache.cloudstack.api.command.user.address.DisassociateIPAddrCmd java.lang.IllegalArgumentException: IP address is used for source nat purposes and can not be disassociated.
	at com.cloud.network.NetworkServiceImpl.releaseIpAddressInternal(NetworkServiceImpl.java:1302)
	at com.cloud.network.NetworkServiceImpl.releaseIpAddress(NetworkServiceImpl.java:1268)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

Scenario B — regression check on non-source-NAT IPs:

Acquire a second, plain IP on the same network (cloudstack_ipaddress.extra with just network_id set, no import needed) and terraform destroy -target=cloudstack_ipaddress.extra — this should disassociate normally and succeed, confirming the fix didn't affect the ordinary delete path.

resource "cloudstack_ipaddress" "extra_ip" {
  network_id = cloudstack_network.test_net.id
}

terraform apply

cloudstack_network.test_net: Refreshing state... [id=9663cf7c-2a5b-4131-ac6c-915973089b84]
cloudstack_ipaddress.source_nat: Refreshing state... [id=d3915dac-0211-4e75-bfe1-15262aa99e05]
cloudstack_instance.trigger_vr: Refreshing state... [id=ef8953f4-a8c7-4881-a667-5ae6db4c39ca]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated
with the following symbols:
  + create

Terraform will perform the following actions:

  # cloudstack_ipaddress.extra_ip will be created
  + resource "cloudstack_ipaddress" "extra_ip" {
      + id            = (known after apply)
      + ip_address    = (known after apply)
      + is_portable   = false
      + is_source_nat = (known after apply)
      + network_id    = "9663cf7c-2a5b-4131-ac6c-915973089b84"
      + project       = (known after apply)
      + tags          = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 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

cloudstack_ipaddress.extra_ip: Creating...
cloudstack_ipaddress.extra_ip: Creation complete after 1s [id=da3243d7-b9e6-4fce-b18e-80426f3e6ce6]
terraform destroy -target=cloudstack_ipaddress.extra_ip
cloudstack_network.test_net: Refreshing state... [id=9663cf7c-2a5b-4131-ac6c-915973089b84]
cloudstack_ipaddress.extra_ip: Refreshing state... [id=da3243d7-b9e6-4fce-b18e-80426f3e6ce6]

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

Terraform will perform the following actions:

  # cloudstack_ipaddress.extra_ip will be destroyed
  - resource "cloudstack_ipaddress" "extra_ip" {
      - id            = "da3243d7-b9e6-4fce-b18e-80426f3e6ce6" -> null
      - ip_address    = "10.0.56.65" -> null
      - is_portable   = false -> null
      - is_source_nat = false -> null
      - network_id    = "9663cf7c-2a5b-4131-ac6c-915973089b84" -> null
      - tags          = {} -> null
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 0 to change, 1 to destroy.
╷
│ Warning: Resource targeting is in effect
│ 
│ You are creating a plan with the -target option, which means that the result of this plan may not represent
│ all of the changes requested by the current configuration.
│ 
│ The -target option is not for routine use, and is provided only for exceptional situations such as recovering
│ from errors or mistakes, or when Terraform specifically suggests to use it as part of an error message.
╵

Do you really want to destroy all resources?
  Terraform will destroy all your managed infrastructure, as shown above.
  There is no undo. Only 'yes' will be accepted to confirm.

  Enter a value: yes

cloudstack_ipaddress.extra_ip: Destroying... [id=da3243d7-b9e6-4fce-b18e-80426f3e6ce6]
cloudstack_ipaddress.extra_ip: Destruction complete after 1s
╷
│ Warning: Applied changes may be incomplete
│ 
│ The plan was created with the -target option in effect, so some changes requested in the configuration may
│ have been ignored and the output values may not be fully updated. Run the following command to verify that no
│ other changes are pending:
│     terraform plan
│ 
│ Note that the -target option is not suitable for routine use, and is provided only for exceptional situations
│ such as recovering from errors or mistakes, or when Terraform specifically suggests to use it as part of an
│ error message.
╵

Destroy complete! Resources: 1 destroyed

Scenario C — network destroyed too (confirms the IP is genuinely released as a side effect):

terraform destroy

This tears down the instance, then the network — deleting the network releases the source NAT IP. Confirm with cmk list publicipaddresses id= that it now returns nothing.

terraform destroy                                      
cloudstack_network.test_net: Refreshing state... [id=9663cf7c-2a5b-4131-ac6c-915973089b84]
cloudstack_ipaddress.source_nat: Refreshing state... [id=d3915dac-0211-4e75-bfe1-15262aa99e05]
cloudstack_instance.trigger_vr: Refreshing state... [id=ef8953f4-a8c7-4881-a667-5ae6db4c39ca]

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

Terraform will perform the following actions:

  # cloudstack_instance.trigger_vr will be destroyed
  - resource "cloudstack_instance" "trigger_vr" {
      - delete_protection = false -> null
      - display_name      = "sourcenat-delete-trigger" -> null
      - expunge           = true -> null
      - id                = "ef8953f4-a8c7-4881-a667-5ae6db4c39ca" -> null
      - ip_address        = "10.50.0.186" -> null
      - name              = "sourcenat-delete-trigger" -> null
      - network_id        = "9663cf7c-2a5b-4131-ac6c-915973089b84" -> null
      - root_disk_size    = 3 -> null
      - service_offering  = "df040e78-a92f-4502-bd09-b8eba6c5e3d0" -> null
      - start_vm          = true -> null
      - tags              = {} -> null
      - template          = "298bed72-7127-4e3d-8e33-d38f2af651a9" -> null
      - uefi              = false -> null
      - zone              = "27befe1a-4e0b-4aad-b0ee-c57b6a118212" -> null
        # (3 unchanged attributes hidden)
    }

  # cloudstack_ipaddress.source_nat will be destroyed
  - resource "cloudstack_ipaddress" "source_nat" {
      - id            = "d3915dac-0211-4e75-bfe1-15262aa99e05" -> null
      - ip_address    = "10.0.56.63" -> null
      - is_portable   = false -> null
      - is_source_nat = true -> null
      - network_id    = "9663cf7c-2a5b-4131-ac6c-915973089b84" -> null
      - tags          = {} -> null
        # (1 unchanged attribute hidden)
    }

  # cloudstack_network.test_net will be destroyed
  - resource "cloudstack_network" "test_net" {
      - acl_id                    = "none" -> null
      - bypass_vlan_overlap_check = false -> null
      - cidr                      = "10.50.0.0/24" -> null
      - display_text              = "sourcenat-delete-test" -> null
      - gateway                   = "10.50.0.1" -> null
      - id                        = "9663cf7c-2a5b-4131-ac6c-915973089b84" -> null
      - name                      = "sourcenat-delete-test" -> null
      - network_domain            = "cs2cloud.internal" -> null
      - network_offering          = "ee001adf-b20d-4958-9b87-f0a93457b694" -> null
      - tags                      = {} -> null
      - zone                      = "27befe1a-4e0b-4aad-b0ee-c57b6a118212" -> null
        # (4 unchanged attributes hidden)
    }

Plan: 0 to add, 0 to change, 3 to destroy.

Do you really want to destroy all resources?
  Terraform will destroy all your managed infrastructure, as shown above.
  There is no undo. Only 'yes' will be accepted to confirm.

  Enter a value: yes

cloudstack_ipaddress.source_nat: Destroying... [id=d3915dac-0211-4e75-bfe1-15262aa99e05]
cloudstack_instance.trigger_vr: Destroying... [id=ef8953f4-a8c7-4881-a667-5ae6db4c39ca]
cloudstack_ipaddress.source_nat: Destruction complete after 1s
cloudstack_instance.trigger_vr: Still destroying... [id=ef8953f4-a8c7-4881-a667-5ae6db4c39ca, 00m10s elapsed]
cloudstack_instance.trigger_vr: Destruction complete after 17s
cloudstack_network.test_net: Destroying... [id=9663cf7c-2a5b-4131-ac6c-915973089b84]
cloudstack_network.test_net: Destruction complete after 7s

Destroy complete! Resources: 3 destroyed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants