Code review markups

calico_cni/constants.py

@@ -60,8 +60,8 @@
 ERR_CODE_GENERIC = 100   # Use this for all errors.
 
 # Policy modes.
-POLICY_MODE_ANNOTATIONS = "k8s-annotations"
-POLICY_MODE_DENY_INBOUND = "default-deny-inbound"
+POLICY_MODE_KUBERNETES_ANNOTATIONS = "k8s-annotations"
+POLICY_MODE_KUBERNETES = "k8s"
 
 # Logging Configuration
 LOG_DIR = "/var/log/calico/cni"

calico_cni/policy_drivers.py

@@ -127,7 +127,7 @@ def generate_tags(self):
         return []
 
 
-class KubernetesDefaultPolicyDriver(DefaultPolicyDriver):
+class KubernetesNoPolicyDriver(DefaultPolicyDriver):
     """
     Implements default network policy for a Kubernetes container manager.
 
@@ -307,23 +307,35 @@ def _escape_chars(self, unescaped_string):
         return re.sub('[^a-zA-Z0-9\.\_\-]', swap_char, unescaped_string)
 
 
-class DefaultDenyInboundDriver(KubernetesAnnotationDriver):
+class KubernetesPolicyDriver(KubernetesAnnotationDriver):
     """
-    This driver sets the labels and correct profile on the endpoint.
+    This driver sets the labels and correct profiles on the endpoint.
+
+    The labels are fetched from the k8s API, and a special additional label is
+    added.
+
+    The profile ID is constructed from the namespace.
     """
     def apply_profile(self, endpoint):
+        # Set profile
         profile_id = "k8s_ns.%s" % self.namespace
         _log.debug("Constructed profile ID - %s" % profile_id)
+        endpoint.profile_ids = [profile_id]
+
+        # Fetch and set the labels
         pod = self._get_api_pod()
         labels = pod["metadata"].get("labels", {})
         labels["calico/k8s_ns"] = self.namespace
         _log.debug("Got labels - %s" % labels)
         endpoint.labels = labels
-        endpoint.profile_ids = [profile_id]
+
+        # Finally, update the endpoint.
         self._client.update_endpoint(endpoint)
 
     def remove_profile(self):
-        _log.debug("Nothing to do")
+        # This policy driver didn't create any profiles so there are none to
+        # delete.
+        _log.debug("No profile to remove for pod %s", self.pod_name)
 
 
 class ApplyProfileError(Exception):
@@ -344,22 +356,30 @@ def get_policy_driver(k8s_pod_name, k8s_namespace, net_config):
     policy_config = net_config.get(POLICY_KEY, {})
     network_name = net_config["name"]
     policy_type = policy_config.get("type")
+    supported_policy_types = [None,
+                              POLICY_MODE_KUBERNETES,
+                              POLICY_MODE_KUBERNETES_ANNOTATIONS]
+    if policy_type not in supported_policy_types:
+        print_cni_error(ERR_CODE_GENERIC,
+                        "policy type set to unsupported value (%s). "
+                        "Supported values = %s" %
+                        (policy_type, supported_policy_types))
+        sys.exit(ERR_CODE_GENERIC)
 
     # Determine which policy driver to use.
-
     if k8s_pod_name:
         # Running under Kubernetes - decide which Kubernetes driver to use.
-        if policy_type == POLICY_MODE_ANNOTATIONS or \
-           policy_type == POLICY_MODE_DENY_INBOUND:
+        if policy_type == POLICY_MODE_KUBERNETES_ANNOTATIONS or \
+           policy_type == POLICY_MODE_KUBERNETES:
             assert k8s_namespace, "Missing Kubernetes namespace"
             k8s_auth_token = policy_config.get(AUTH_TOKEN_KEY)
             k8s_api_root = policy_config.get(API_ROOT_KEY,
                                              "https://10.100.0.1:443/api/v1/")
 
-            if policy_type == POLICY_MODE_DENY_INBOUND:
-                _log.debug("Using Kubernetes Default-Deny Policy Driver")
-                driver_cls = DefaultDenyInboundDriver
-            elif policy_type == POLICY_MODE_ANNOTATIONS:
+            if policy_type == POLICY_MODE_KUBERNETES:
+                _log.debug("Using Kubernetes Policy Driver")
+                driver_cls = KubernetesPolicyDriver
+            elif policy_type == POLICY_MODE_KUBERNETES_ANNOTATIONS:
                 _log.debug("Using Kubernetes Annotation Policy Driver")
                 driver_cls = KubernetesAnnotationDriver
 
@@ -369,7 +389,7 @@ def get_policy_driver(k8s_pod_name, k8s_namespace, net_config):
                            k8s_api_root]
         else:
             _log.debug("Using Default Kubernetes Policy Driver")
-            driver_cls = KubernetesDefaultPolicyDriver
+            driver_cls = KubernetesNoPolicyDriver
             driver_args = [network_name]
     else:
         _log.debug("Using default policy driver")

tests/fv/test_calico_cni.py

@@ -28,7 +28,7 @@
 from calico_cni.constants import *
 from calico_cni.container_engines import DockerEngine
 from calico_cni.policy_drivers import (DefaultPolicyDriver,
-        KubernetesDefaultPolicyDriver, KubernetesAnnotationDriver)
+                                       KubernetesNoPolicyDriver, KubernetesAnnotationDriver)
 
 
 class CniPluginFvTest(unittest.TestCase):
@@ -192,7 +192,7 @@ def test_add_mainline_kubernetes_docker(self):
         p.execute()
 
         # Assert the correct policy driver was chosen.
-        assert_true(isinstance(p.policy_driver, KubernetesDefaultPolicyDriver)) 
+        assert_true(isinstance(p.policy_driver, KubernetesNoPolicyDriver))
 
         # Assert an endpoint was created.
         self.client.create_endpoint.assert_called_once_with(ANY, 

tests/unit/test_policy_drivers.py

@@ -21,12 +21,12 @@
 from pycalico.datastore_datatypes import Endpoint, Rule, Rules
 
 from calico_cni.constants import ERR_CODE_GENERIC
-from calico_cni.policy_drivers import (ApplyProfileError, 
-                        get_policy_driver,
-                        DefaultPolicyDriver,
-                        KubernetesDefaultPolicyDriver,
-                        KubernetesAnnotationDriver,
-                        DefaultDenyInboundDriver)
+from calico_cni.policy_drivers import (ApplyProfileError,
+                                       get_policy_driver,
+                                       DefaultPolicyDriver,
+                                       KubernetesNoPolicyDriver,
+                                       KubernetesAnnotationDriver,
+                                       KubernetesPolicyDriver)
 
 
 class DefaultPolicyDriverTest(unittest.TestCase):
@@ -103,7 +103,7 @@ class KubernetesDefaultPolicyDriverTest(unittest.TestCase):
     """
     def setUp(self):
         self.network_name = "test_net_name"
-        self.driver = KubernetesDefaultPolicyDriver(self.network_name)
+        self.driver = KubernetesNoPolicyDriver(self.network_name)
         assert_equal(self.driver.profile_name, self.network_name)
 
         # Mock the DatastoreClient
@@ -343,33 +343,41 @@ def test_get_metadata_missing(self):
         # Should be None
         assert_equal(annotations, None)
 
-# class DefaultDenyInboundPolicyDriverTest(unittest.TestCase):
-#     """
-#     Test class for DefaultDenyInboundDriver class.
-#     """
-#     def setUp(self):
-#         self.network_name = "deny-inbound"
-#         self.driver = DefaultDenyInboundDriver(self.network_name)
-#
-#         # Mock the DatastoreClient
-#         self.client = MagicMock(spec=DatastoreClient)
-#         self.driver._client = self.client
-#
-#     def test_generate_rules(self):
-#         # Generate rules
-#         rules = self.driver.generate_rules()
-#
-#         # Assert correct.
-#         expected = Rules(id=self.network_name,
-#                          inbound_rules=[Rule(action="deny")],
-#                          outbound_rules=[Rule(action="allow")])
-#         assert_equal(rules, expected)
-#
-#     def test_remove_profile(self):
-#         """
-#         Should do nothing.
-#         """
-#         self.driver.remove_profile()
+class KubernetesPolicyDriverTest(unittest.TestCase):
+    """
+    Test class for DefaultDenyInboundDriver class.
+    """
+    def setUp(self):
+        self.network_name = "net-name"
+        self.namespace = "default"
+        self.driver = KubernetesPolicyDriver(self.network_name, self.namespace,
+                                             None, None)
+
+        # Mock the DatastoreClient
+        self.client = MagicMock(spec=DatastoreClient)
+        self.driver._client = self.client
+
+    def test_apply_profile(self):
+        endpoint = MagicMock(spec=Endpoint)
+        endpoint.endpoint_id = "12345"
+
+        # Mock out the k8s API call.
+        self.driver._get_api_pod = Mock(spec=self.driver._get_api_pod)
+        self.driver._get_api_pod.return_value = \
+            {"metadata": {"labels": {"label1": "labelval"}}}
+
+        # Call
+        self.driver.apply_profile(endpoint)
+
+        # Assert
+        self.client.update_endpoint.assert_called_once_with(endpoint)
+
+
+    def test_remove_profile(self):
+        """
+        Should do nothing.
+        """
+        self.driver.remove_profile()
 
 
 class GetPolicyDriverTest(unittest.TestCase): 
@@ -379,7 +387,7 @@ def test_get_policy_driver_default_k8s(self):
         k8s_namespace = "namespace"
         config = {"name": "testnetwork"}
         driver = get_policy_driver(k8s_pod_name, k8s_namespace, config)
-        assert_true(isinstance(driver, KubernetesDefaultPolicyDriver))
+        assert_true(isinstance(driver, KubernetesNoPolicyDriver))
 
     def test_get_policy_driver_k8s_annotations(self):
         k8s_pod_name = "podname"
@@ -389,13 +397,20 @@ def test_get_policy_driver_k8s_annotations(self):
         driver = get_policy_driver(k8s_pod_name, k8s_namespace, config)
         assert_true(isinstance(driver, KubernetesAnnotationDriver))
 
-    def test_get_policy_driver_deny_inbound(self):
+    def test_get_policy_driver_k8s(self):
         k8s_pod_name = "podname"
         k8s_namespace = "namespace"
         config = {"name": "testnetwork"}
-        config["policy"] = {"type": "default-deny-inbound"}
+        config["policy"] = {"type": "k8s"}
         driver = get_policy_driver(k8s_pod_name, k8s_namespace, config)
-        assert_true(isinstance(driver, DefaultDenyInboundDriver))
+        assert_true(isinstance(driver, KubernetesPolicyDriver))
+
+    def test_get_unknown_policy_driver(self):
+        config = {"name": "n", "policy": {"type": "madeup"}}
+        with assert_raises(SystemExit) as err:
+            get_policy_driver(None, None, config)
+        e = err.exception
+        assert_equal(e.code, ERR_CODE_GENERIC)
 
     @patch("calico_cni.policy_drivers.DefaultPolicyDriver", autospec=True)
     def test_get_policy_driver_value_error(self, m_driver):