Merge pull request #132 from robbrockbank/remove-rogue-profile-id

Remove ID field from Rules object

calico_containers/pycalico/datastore.py

@@ -879,8 +879,7 @@ def create_policy(self, tier_name, policy_name, selector,
         policy_path = POLICY_PATH % {"tier_name": tier_name,
                                      "policy_name": policy_name}
         default_allow = Rule(action="allow")
-        rules = rules or Rules(id=policy_name,
-                               inbound_rules=[default_allow],
+        rules = rules or Rules(inbound_rules=[default_allow],
                                outbound_rules=[default_allow])
         order = order or 100
 
@@ -974,8 +973,7 @@ def create_profile(self, name, rules=None, labels=None):
         # be accepted by another profile on the endpoint.
         accept_self = Rule(action="allow", src_tag=name)
         default_allow = Rule(action="allow")
-        rules = rules or Rules(id=name,
-                               inbound_rules=[accept_self],
+        rules = rules or Rules(inbound_rules=[accept_self],
                                outbound_rules=[default_allow])
         self.etcd_client.write(profile_path + "rules", rules.to_json())
 

calico_containers/pycalico/datastore_datatypes.py

@@ -35,7 +35,7 @@
 """
 
 
-class Rules(namedtuple("Rules", ["id", "inbound_rules", "outbound_rules"])):
+class Rules(namedtuple("Rules", ["inbound_rules", "outbound_rules"])):
     """
     A set of Calico rules describing inbound and outbound network traffic
     policy.
@@ -79,8 +79,7 @@ def from_json(cls, json_str):
         outbound_rules = []
         for rule in json_dict["outbound_rules"]:
             outbound_rules.append(Rule(**rule))
-        rules = cls(id=json_dict["id"],
-                    inbound_rules=inbound_rules,
+        rules = cls(inbound_rules=inbound_rules,
                     outbound_rules=outbound_rules)
         return rules
 
@@ -389,7 +388,7 @@ def __init__(self, name):
         self.tags = set()
 
         # Default to empty lists of rules.
-        self.rules = Rules(name, [], [])
+        self.rules = Rules([], [])
 
 
 class Policy(object):
@@ -400,7 +399,7 @@ def __init__(self, tier_name, policy_name):
         self.order = 0
 
         # Default to empty lists of rules.
-        self.rules = Rules(policy_name, [], [])
+        self.rules = Rules([], [])
 
         # Default to empty selector.
         self.selector = ""

calico_containers/tests/unit/test_datastore.py

@@ -106,10 +106,9 @@
 }
 
 # A complicated set of Rules JSON for testing serialization / deserialization.
-RULES_JSON = """
+RULES_JSON_FMT = """
 {
-  "id": "PROF_GROUP1",
-  "inbound_rules": [
+  %s"inbound_rules": [
     {
       "action": "allow",
       "src_tag": "PROF_GROUP1"
@@ -141,6 +140,14 @@
   ]
 }"""
 
+# A complicated set of rules adhering to the correct etcd data model
+RULES_JSON = RULES_JSON_FMT % ""
+
+# An identical set of rules which includes an additional "id" field that was
+# added by older versions of libcalico.  We test that we can still read profiles
+# of older versions.
+RULES_JSON_WITH_ID = RULES_JSON_FMT % '      "id": "PROFILE1",\n'
+
 
 class TestRule(unittest.TestCase):
 
@@ -256,12 +263,26 @@ def test_rules(self):
         # Convert a JSON blob into a Rules object.
         rules = Rules.from_json(RULES_JSON)
 
+        # Call common check code.
+        self.check_rules(rules)
+
+    def test_rules_with_id(self):
+        """
+        Create a detailed set of rules that include an id field.  This was included
+        in older versions of libcalico.  We check here that we can read it in ok.
+        """
+        # Convert a JSON blob into a Rules object.
+        rules = Rules.from_json(RULES_JSON_WITH_ID)
+
+        # Call common check code.
+        self.check_rules(rules)
+
+    def check_rules(self, rules):
         # Convert the Rules object to JSON and then back again.
         new_json = rules.to_json()
         new_rules = Rules.from_json(new_json)
 
         # Compare the two rules objects.
-        assert_equal(rules.id, new_rules.id)
         assert_equal(rules.inbound_rules,
                      new_rules.inbound_rules)
         assert_equal(rules.outbound_rules,
@@ -723,8 +744,7 @@ def test_profile_update_tags(self):
 
         profile = Profile("TEST")
         profile.tags = {"TAG4", "TAG5"}
-        profile.rules = Rules(id="TEST",
-                              inbound_rules=[
+        profile.rules = Rules(inbound_rules=[
                                   Rule(action="allow", dst_ports=[12]),
                                   Rule(action="allow", protocol="udp"),
                                   Rule(action="deny")
@@ -747,8 +767,7 @@ def test_profile_update_rules(self):
 
         profile = Profile("TEST")
         profile.tags = {"TAG4", "TAG5"}
-        profile.rules = Rules(id="TEST",
-                              inbound_rules=[
+        profile.rules = Rules(inbound_rules=[
                                   Rule(action="allow", dst_ports=[12]),
                                   Rule(action="allow", protocol="udp"),
                                   Rule(action="deny")
@@ -1165,8 +1184,7 @@ def test_create_profile(self):
         Test create_profile()
         """
         self.datastore.create_profile("TEST")
-        rules = Rules(id="TEST",
-                      inbound_rules=[Rule(action="allow",
+        rules = Rules(inbound_rules=[Rule(action="allow",
                                           src_tag="TEST")],
                       outbound_rules=[Rule(action="allow")])
         expected_calls = [call(TEST_PROFILE_PATH + "tags", '["TEST"]'),
@@ -1177,8 +1195,7 @@ def test_create_profile_with_rules(self):
         """
         Test create_profile() with rules specified
         """
-        rules = Rules(id="TEST",
-                      inbound_rules=[Rule(action="deny")],
+        rules = Rules(inbound_rules=[Rule(action="deny")],
                       outbound_rules=[Rule(action="allow")])
         self.datastore.create_profile("TEST", rules)
         expected_calls = [call(TEST_PROFILE_PATH + "tags", '["TEST"]'),