From 12d9204f794da974689bb13597c12719ddfd7541 Mon Sep 17 00:00:00 2001
From: Cameron Porter <porterca@us.ibm.com>
Date: Wed, 28 Jun 2017 14:20:37 -0500
Subject: [PATCH] Check for billing before attempting to cancel a firewall.

This prevents an unexpected error from occurring when billing does not exist for a firewall but cancellation is attempted.
---
 SoftLayer/managers/firewall.py   | 22 ++++++++++++++-------
 tests/managers/firewall_tests.py | 34 ++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/SoftLayer/managers/firewall.py b/SoftLayer/managers/firewall.py
index 210d481c6..8afe57f1b 100644
--- a/SoftLayer/managers/firewall.py
+++ b/SoftLayer/managers/firewall.py
@@ -5,6 +5,7 @@
 
     :license: MIT, see LICENSE for more details.
 """
+from SoftLayer import exceptions
 from SoftLayer import utils
 
 RULE_MASK = ('mask[orderValue,action,destinationIpAddress,'
@@ -87,9 +88,8 @@ def cancel_firewall(self, firewall_id, dedicated=False):
         """
 
         fwl_billing = self._get_fwl_billing_item(firewall_id, dedicated)
-        billing_id = fwl_billing['billingItem']['id']
-        billing_item = self.client['Billing_Item']
-        return billing_item.cancelService(id=billing_id)
+        billing_item_service = self.client['Billing_Item']
+        return billing_item_service.cancelService(id=fwl_billing['id'])
 
     def add_standard_firewall(self, server_id, is_virt=True):
         """Creates a firewall for the specified virtual/hardware server.
@@ -150,12 +150,20 @@ def _get_fwl_billing_item(self, firewall_id, dedicated=False):
         :returns: A dictionary of the firewall billing item.
         """
 
-        mask = ('mask[id,billingItem[id]]')
+        mask = 'mask[id,billingItem[id]]'
         if dedicated:
-            fwl_svc = self.client['Network_Vlan_Firewall']
+            firewall_service = self.client['Network_Vlan_Firewall']
         else:
-            fwl_svc = self.client['Network_Component_Firewall']
-        return fwl_svc.getObject(id=firewall_id, mask=mask)
+            firewall_service = self.client['Network_Component_Firewall']
+        firewall = firewall_service.getObject(id=firewall_id, mask=mask)
+        if firewall is None:
+            raise exceptions.SoftLayerError(
+                "Unable to find firewall %d" % firewall_id)
+        if firewall.get('billingItem') is None:
+            raise exceptions.SoftLayerError(
+                "Unable to find billing item for firewall %d" % firewall_id)
+
+        return firewall['billingItem']
 
     def _get_fwl_port_speed(self, server_id, is_virt=True):
         """Determines the appropriate speed for a firewall.
diff --git a/tests/managers/firewall_tests.py b/tests/managers/firewall_tests.py
index 730fdf29b..c114a63d4 100644
--- a/tests/managers/firewall_tests.py
+++ b/tests/managers/firewall_tests.py
@@ -6,6 +6,7 @@
 """
 
 import SoftLayer
+from SoftLayer import exceptions
 from SoftLayer import fixtures
 from SoftLayer import testing
 
@@ -137,6 +138,23 @@ def test_cancel_firewall(self):
         self.assert_called_with('SoftLayer_Billing_Item', 'cancelService',
                                 identifier=21370814)
 
+    def test_cancel_firewall_no_firewall(self):
+        mock = self.set_mock('SoftLayer_Network_Component_Firewall', 'getObject')
+        mock.return_value = None
+
+        self.assertRaises(exceptions.SoftLayerError,
+                          self.firewall.cancel_firewall, 6327, dedicated=False)
+
+    def test_cancel_firewall_no_billing(self):
+        mock = self.set_mock('SoftLayer_Network_Component_Firewall', 'getObject')
+        mock.return_value = {
+            'id': 6327,
+            'billingItem': None
+        }
+
+        self.assertRaises(exceptions.SoftLayerError,
+                          self.firewall.cancel_firewall, 6327, dedicated=False)
+
     def test_cancel_dedicated_firewall(self):
         # test dedicated firewalls
         result = self.firewall.cancel_firewall(6327, dedicated=True)
@@ -149,6 +167,22 @@ def test_cancel_dedicated_firewall(self):
         self.assert_called_with('SoftLayer_Billing_Item', 'cancelService',
                                 identifier=21370815)
 
+    def test_cancel_dedicated_firewall_no_firewall(self):
+        mock = self.set_mock('SoftLayer_Network_Vlan_Firewall', 'getObject')
+        mock.return_value = None
+
+        self.assertRaises(exceptions.SoftLayerError,
+                          self.firewall.cancel_firewall, 6327, dedicated=True)
+
+    def test_cancel_dedicated_firewall_no_billing(self):
+        mock = self.set_mock('SoftLayer_Network_Vlan_Firewall', 'getObject')
+        mock.return_value = {
+            'id': 6327,
+            'billingItem': None
+        }
+        self.assertRaises(exceptions.SoftLayerError,
+                          self.firewall.cancel_firewall, 6327, dedicated=True)
+
     def test_add_standard_firewall_virtual_server(self):
         # test standard firewalls for virtual servers
         self.firewall.add_standard_firewall(6327, is_virt=True)