bug 8863: generate usage event when PF/LB/StaticNat rule is set with Revoke status, not when it's actually removed on the backend.

status 8863: resolved fixed

server/src/com/cloud/network/NetworkManager.java

@@ -172,8 +172,6 @@ public interface NetworkManager extends NetworkService {
 
     boolean zoneIsConfiguredForExternalNetworking(long zoneId);
     
-    void unassignPublicIpAddress(IPAddressVO addr);
-    
     Map<Capability, String> getServiceCapability(long zoneId, Service service);
     
     boolean applyIpAssociations(Network network, boolean continueOnError) throws ResourceUnavailableException;

server/src/com/cloud/network/NetworkManagerImpl.java

@@ -307,25 +307,6 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         txn.commit();
     }
 
-    @Override @DB
-    public void unassignPublicIpAddress(IPAddressVO addr) {
-        Transaction txn = Transaction.currentTxn();
-        Account owner = _accountMgr.getAccount(addr.getAccountId());
-        long isSourceNat = (addr.isSourceNat()) ? 1 : 0;
-        
-        txn.start();
-        
-        _ipAddressDao.unassignIpAddress(addr.getId());
-        if (owner.getAccountId() != Account.ACCOUNT_ID_SYSTEM) {
-            UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_IP_RELEASE, owner.getId(), addr.getDataCenterId(), addr.getId(), addr.getAddress().toString(), isSourceNat);
-            _usageEventDao.persist(usageEvent);
-        }
-        
-        _accountMgr.decrementResourceCount(owner.getId(), ResourceType.public_ip);
-        
-        txn.commit();
-    }
-
     @Override
     @DB
     public PublicIp assignSourceNatIpAddress(Account owner, Network network, long callerId) throws ConcurrentOperationException, InsufficientAddressCapacityException {
@@ -472,7 +453,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
                 } else if (addr.getState() == IpAddress.State.Releasing) {
                     //Cleanup all the resources for ip address if there are any, and only then unassign ip in the system
                     if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount())) {
-                        unassignPublicIpAddress(addr);
+                        _ipAddressDao.unassignIpAddress(addr.getId());
                     } else {
                         success = false;
                         s_logger.warn("Failed to release resources for ip address id=" + addr.getId());
@@ -614,11 +595,11 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         }
     }
 
-    @Override
+    @Override @DB
     public boolean releasePublicIpAddress(long addrId, long userId, Account caller) {
         
-        //mark ip address as Releasing
+        IPAddressVO ip = markIpAsUnavailable(addrId);
-        IPAddressVO ip = _ipAddressDao.markAsUnavailable(addrId);
+        
         assert (ip != null) : "Unable to mark the ip address id=" + addrId + " as unavailable.";
         if (ip == null) {
             return true;
@@ -631,9 +612,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         boolean success = true;
         
         //Cleanup all ip address resources - PF/LB/Static nat rules
-        if (cleanupIpResources(addrId, userId, caller)) {
+        if (!cleanupIpResources(addrId, userId, caller)) {
-            unassignPublicIpAddress(ip);
-        } else {
             success = false;
             s_logger.warn("Failed to release resources for ip address id=" + addrId);
         }
@@ -1278,14 +1257,6 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         return _nicDao.listByVmIdIncludingRemoved(vm.getId());
     }
 
-    private Account findAccountByIpAddress(Long ipAddressId) {
-        IPAddressVO address = _ipAddressDao.findById(ipAddressId);
-        if ((address != null) && (address.getAllocatedToAccountId() != null)) {
-            return _accountMgr.getActiveAccount(address.getAllocatedToAccountId());
-        }
-        return null;
-    }
-
     @Override
     public List<NicProfile> getNicProfiles(VirtualMachine vm) {
         List<NicVO> nics = _nicDao.listByVmId(vm.getId());
@@ -1314,49 +1285,18 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         Long ipAddressId = cmd.getIpAddressId();
 
         // Verify input parameters
-        Account accountByIp = findAccountByIpAddress(ipAddressId);
-        if (accountByIp == null) {
-            throw new InvalidParameterValueException("Unable to find account owner for ip " + ipAddressId);
-        }
-
-        Long accountId = accountByIp.getId();
-        if (!_accountMgr.isAdmin(caller.getType())) {
-            if (caller.getId() != accountId.longValue()) {
-                throw new PermissionDeniedException("account " + caller.getAccountName() + " doesn't own ip address id=" + ipAddressId);
-            }
-        } else {
-            Domain domain = _domainDao.findById(accountByIp.getDomainId());
-            _accountMgr.checkAccess(caller, domain);
-        }
-
-        try {
         IPAddressVO ipVO = _ipAddressDao.findById(ipAddressId);
         if (ipVO == null) {
-                return false;
+            throw new InvalidParameterValueException("Unable to find ip address by id " + ipAddressId);
         }
         
         if (ipVO.getAllocatedTime() == null) {
+            s_logger.debug("Ip Address id= " + ipAddressId + " is not allocated, so do nothing.");
             return true;
         }
 
-            Account account = _accountMgr.getAccount(accountId);
+        if (ipVO.getAllocatedToAccountId() != null) {
-            if (account == null) {
+            _accountMgr.checkAccess(caller, ipVO);
-                return false;
-            }
-
-            if ((ipVO.getAllocatedToAccountId() == null) || (ipVO.getAllocatedToAccountId().longValue() != accountId)) {
-                // FIXME: is the user visible in the admin account's domain????
-                if (!BaseCmd.isAdmin(account.getType())) {
-                    if (s_logger.isDebugEnabled()) {
-                        s_logger.debug("permission denied disassociating IP address id=" + ipAddressId + "; acct: " + accountId + "; ip (acct / dc / dom / alloc): " + ipVO.getAllocatedToAccountId() + " / " + ipVO.getDataCenterId() + " / "
-                                + ipVO.getAllocatedInDomainId() + " / " + ipVO.getAllocatedTime());
-                    }
-                    throw new PermissionDeniedException("User/account does not own supplied address");
-                }
-            }
-
-            if (ipVO.getAllocatedTime() == null) {
-                return true;
         }
 
         if (ipVO.isSourceNat()) {
@@ -1369,20 +1309,11 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         }
 
         // Check for account wide pool. It will have an entry for account_vlan_map.
-            if (_accountVlanMapDao.findAccountVlanMap(accountId, ipVO.getVlanId()) != null) {
+        if (_accountVlanMapDao.findAccountVlanMap(ipVO.getAccountId(), ipVO.getVlanId()) != null) {
-                throw new PermissionDeniedException("Ip address id=" + ipAddressId + " belongs to Account wide IP pool and cannot be disassociated");
+            throw new InvalidParameterValueException("Ip address id=" + ipAddressId + " belongs to Account wide IP pool and cannot be disassociated");
         }
 
         return releasePublicIpAddress(ipAddressId, userId, caller);
-
-        } catch (PermissionDeniedException pde) {
-            throw pde;
-        } catch (IllegalArgumentException iae) {
-            throw iae;
-        } catch (Throwable t) {
-            s_logger.error("Disassociate IP address threw an exception.", t);
-            throw new IllegalArgumentException("Disassociate IP address threw an exception");
-        }
     }
 
     @Override
@@ -2041,7 +1972,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         //release all ip addresses
         List<IPAddressVO> ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId, null);
         for (IPAddressVO ipToRelease : ipsToRelease) {
-            IPAddressVO ip = _ipAddressDao.markAsUnavailable(ipToRelease.getId());
+            IPAddressVO ip = markIpAsUnavailable(ipToRelease.getId());
             assert (ip != null) : "Unable to mark the ip address id=" + ipToRelease.getId() + " as unavailable.";
         }
         
@@ -2660,4 +2591,30 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         }
         return accountNetworks;
     }
+    
+    @DB
+    private IPAddressVO markIpAsUnavailable(long addrId) {
+        Transaction txn = Transaction.currentTxn();
+ 
+        IPAddressVO ip = _ipAddressDao.findById(addrId);
+        
+        if (ip.getState() != State.Releasing) {
+            txn.start();
+            
+            ip = _ipAddressDao.markAsUnavailable(addrId);
+            
+            _accountMgr.decrementResourceCount(_ipAddressDao.findById(addrId).getAccountId(), ResourceType.public_ip);
+            
+            long isSourceNat = (ip.isSourceNat()) ? 1 : 0;
+            
+            if (ip.getAccountId() != Account.ACCOUNT_ID_SYSTEM) {
+                UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_IP_RELEASE, ip.getAccountId(), ip.getDataCenterId(), addrId, ip.getAddress().addr(), isSourceNat);
+                _usageEventDao.persist(usageEvent);
+            }
+            
+            txn.commit();
+        }
+        
+        return ip;
+    }
 }

server/src/com/cloud/network/guru/DirectNetworkGuru.java

@@ -208,7 +208,7 @@ public class DirectNetworkGuru extends AdapterBase implements NetworkGuru {
     public void deallocate(Network network, NicProfile nic, VirtualMachineProfile<? extends VirtualMachine> vm) {
         IPAddressVO ip = _ipAddressDao.findByAccountAndIp(vm.getVirtualMachine().getAccountId(), nic.getIp4Address());
         if (ip != null) {
-            _networkMgr.unassignPublicIpAddress(ip);
+            _ipAddressDao.unassignIpAddress(ip.getId());
         }
         nic.deallocate();
     }

server/src/com/cloud/network/guru/PublicNetworkGuru.java

@@ -156,7 +156,7 @@ public class PublicNetworkGuru extends AdapterBase implements NetworkGuru {
     public void deallocate(Network network, NicProfile nic, VirtualMachineProfile<? extends VirtualMachine> vm) {
         IPAddressVO ip = _ipAddressDao.findByAccountAndIp(vm.getVirtualMachine().getAccountId(), nic.getIp4Address());
         if (ip != null) {
-            _networkMgr.unassignPublicIpAddress(ip);
+            _ipAddressDao.unassignIpAddress(ip.getId());
         }
         nic.deallocate();
     }

server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java

@@ -285,11 +285,23 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager,
     }
     
     
+    @DB 
     private boolean deleteLoadBalancerRule(long loadBalancerId, boolean apply, Account caller, long callerUserId) {
         LoadBalancerVO lb = _lbDao.findById(loadBalancerId);
+        Transaction txn = Transaction.currentTxn();
+        boolean generateUsageEvent = false;
         
+        txn.start();
+        if (lb.getState() == FirewallRule.State.Staged) {
+            if (s_logger.isDebugEnabled()) {
+                s_logger.debug("Found a rule that is still in stage state so just removing it: " + lb);
+            }
+            generateUsageEvent = true;
+        } else if (lb.getState() == FirewallRule.State.Add || lb.getState() == FirewallRule.State.Active) {
             lb.setState(FirewallRule.State.Revoke);
             _lbDao.persist(lb);
+            generateUsageEvent = true;
+        }
         
         List<LoadBalancerVMMapVO> maps = _lb2VmMapDao.listByLoadBalancerId(loadBalancerId);
         if (maps != null) {
@@ -300,6 +312,14 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager,
             }  
         }
         
+        if (generateUsageEvent) {
+          //Generate usage event right after all rules were marked for revoke
+            UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_LOAD_BALANCER_DELETE, lb.getAccountId(), 0 , lb.getId(), null);
+            _usageEventDao.persist(usageEvent);
+        }
+        
+        txn.commit();
+        
         if (apply) {
             try {
                 if (!applyLoadBalancerConfig(loadBalancerId)) {
@@ -313,8 +333,6 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager,
         }
         
         _rulesDao.remove(lb.getId()); 
-        UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_LOAD_BALANCER_DELETE, lb.getAccountId(), 0 , lb.getId(), null);
-        _usageEventDao.persist(usageEvent);
         s_logger.debug("Load balancer with id " + lb.getId() + " is removed successfully");
         return true;
     }

server/src/com/cloud/network/rules/RulesManagerImpl.java

@@ -440,15 +440,24 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager {
         }
        
         Transaction txn = Transaction.currentTxn();
+        boolean generateUsageEvent = false;
+        
         txn.start();
         if (rule.getState() == State.Staged) {
             if (s_logger.isDebugEnabled()) {
                 s_logger.debug("Found a rule that is still in stage state so just removing it: " + rule);
             }
             _firewallDao.remove(rule.getId());
+            generateUsageEvent = true;
         } else if (rule.getState() == State.Add || rule.getState() == State.Active) {
             rule.setState(State.Revoke);
             _firewallDao.update(rule.getId(), rule);
+            generateUsageEvent = true;
+        }
+        
+        if (generateUsageEvent) {
+            UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_RULE_DELETE, rule.getAccountId(), 0, rule.getId(), null);
+            _usageEventDao.persist(usageEvent);
         }
         
         // Save and create the event
@@ -459,7 +468,6 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager {
             description.append("->[").append(pfRule.getDestinationIpAddress()).append(":").append(pfRule.getDestinationPortStart()).append("-").append(pfRule.getDestinationPortEnd()).append("]");
         }
         description.append(" ").append(rule.getProtocol());
-
         txn.commit();
     }
     
@@ -490,10 +498,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager {
         } else {
             success = true;
         }
-        if(success){
+        
-            UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_RULE_DELETE, rule.getAccountId(), 0, ruleId, null);
-            _usageEventDao.persist(usageEvent);
-        }
         return success;
     }
     
@@ -525,10 +530,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager {
         } else {
             success = true;
         }
-        if(success){
+        
-            UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_RULE_DELETE, rule.getAccountId(), 0, ruleId, null);
-            _usageEventDao.persist(usageEvent);
-        }
         return success;
     }