Repository navigation
network: use calling account instead of network owner for destroy context - #14346
Open
weizhouapache wants to merge 1 commit into
Open
weizhouapache wants to merge 1 commit into
weizhouapache wants to merge 1 commit into
Conversation
…text
Symptom
-------
Deleting a network whose owning account is disabled fails with:
com.cloud.exception.PermissionDeniedException: Account Account
[{"accountName":"...","id":...}] is disabled.
at com.cloud.acl.DomainChecker.checkAccess(DomainChecker.java:149)
at com.cloud.user.AccountManagerImpl.checkAccess(AccountManagerImpl.java:831)
at com.cloud.network.firewall.FirewallManagerImpl.revokeFirewallRule(FirewallManagerImpl.java:1169)
at com.cloud.network.firewall.FirewallManagerImpl.revokeAllFirewallRulesForNetwork(FirewallManagerImpl.java:1356)
at org.apache.cloudstack.engine.orchestration.NetworkOrchestrator.cleanupNetworkResources(NetworkOrchestrator.java:4192)
at org.apache.cloudstack.engine.orchestration.NetworkOrchestrator.destroyNetwork(NetworkOrchestrator.java:3512)
Root cause
----------
NetworkServiceImpl.deleteNetwork, VpcManagerImpl (private network cleanup),
and KubernetesClusterDestroyWorker each build the ReservationContext used
for destroyNetwork() with the network's *owning* account as the acting
account:
ReservationContext context = new ReservationContextImpl(null, null, callerUser, owner);
That "owner" account is then propagated all the way into
FirewallManagerImpl.revokeFirewallRule() as the checkAccess() caller when
cleaning up the network's firewall rules. DomainChecker.checkAccess()
unconditionally rejects any caller whose account state isn't ENABLED, so
if the network's owner account happens to be disabled (e.g. while it is
being cleaned up, or simply disabled by an admin without removing it),
network deletion is blocked entirely — even for a root admin performing
the deletion.
By contrast, AccountManagerImpl.cleanupAccount() (the cascade that runs
during full account deletion) already builds this same ReservationContext
with the actual calling account instead of the owner:
ReservationContext context = new ReservationContextImpl(null, null, getActiveUser(callerUserId), caller);
...which is why account deletion itself was never affected — only a
direct deleteNetwork/VPC-network-cleanup call against a disabled-but-not-
yet-removed account.
Fix
---
Use the actual calling account (the account performing the operation)
instead of the network's owner account when building the
ReservationContext for destroyNetwork(), in all three call sites, matching
the pattern already used by AccountManagerImpl.cleanupAccount().
Reproduced and verified
------------------------
Reproduced end-to-end against a running management server (pre-fix build)
by: creating a test account, creating an isolated network with the
Firewall service and implementing it (deploy a VM), adding firewall rules
on its source-NAT IP, disabling the account, destroying/expunging the VM,
then attempting to delete the network — which failed with the exact same
stack trace as above. Re-enabling the account allowed the same
deleteNetwork call to succeed immediately, confirming the account's
disabled state (via the owner-as-caller path) as the sole cause.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14346 +/- ##
============================================
- Coverage 18.02% 18.02% -0.01%
+ Complexity 16250 16246 -4
============================================
Files 5936 5936
Lines 535823 535823
Branches 65612 65612
============================================
- Hits 96582 96557 -25
- Misses 428242 428275 +33
+ Partials 10999 10991 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Damans227
reviewed
Oct 7, 2026
| final User callerUser = _accountMgr.getActiveUser(CallContext.current().getCallingUserId()); | ||
| final Account owner = _accountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM); | ||
| final ReservationContext context = new ReservationContextImpl(null, null, callerUser, owner); | ||
| final ReservationContext context = new ReservationContextImpl(null, null, callerUser, CallContext.current().getCallingAccount()); |
Collaborator
There was a problem hiding this comment.
any reason to change this one too? it already used the system account so it never hit the disabled account problem, and now a normal user deleting the gateway could fail the permission checks
This branch has not been deployed
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
2 participants
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.
Description
This PR fixes an issue while deleting a network whose owning account is disabled fails with:
NetworkServiceImpl.deleteNetwork, VpcManagerImpl (private network cleanup), and KubernetesClusterDestroyWorker each build the ReservationContext used for destroyNetwork() with the network's owning account as the acting account:
ReservationContext context = new ReservationContextImpl(null, null, callerUser, owner);
That "owner" account is then propagated all the way into FirewallManagerImpl.revokeFirewallRule() as the checkAccess() caller when cleaning up the network's firewall rules. DomainChecker.checkAccess() unconditionally rejects any caller whose account state isn't ENABLED, so if the network's owner account happens to be disabled (e.g. while it is being cleaned up, or simply disabled by an admin without removing it), network deletion is blocked entirely — even for a root admin performing the deletion.
By contrast, AccountManagerImpl.cleanupAccount() (the cascade that runs during full account deletion) already builds this same ReservationContext with the actual calling account instead of the owner:
...which is why account deletion itself was never affected — only a direct deleteNetwork/VPC-network-cleanup call against a disabled-but-not- yet-removed account.
Use the actual calling account (the account performing the operation) instead of the network's owner account when building the ReservationContext for destroyNetwork(), in all three call sites, matching the pattern already used by AccountManagerImpl.cleanupAccount().
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Reproduced end-to-end against a running management server (pre-fix build) by: creating a test account, creating an isolated network with the Firewall service and implementing it (deploy a VM), adding firewall rules on its source-NAT IP, disabling the account, destroying/expunging the VM, then attempting to delete the network — which failed with the exact same stack trace as above. Re-enabling the account allowed the same deleteNetwork call to succeed immediately, confirming the account's disabled state (via the owner-as-caller path) as the sole cause.
How did you try to break this feature and the system with this change?