Bug fixes for Security Groups

RM10897

Updates the redis_client and redis_sg_tool based on manual testing. Also
reworks the connection semantics to defer to the sentinel class itself
for the connection pooling. Finally, removes all SSL connection
semantics, as it was determined that Sentinel connections and SSL do
not easily mix, and thus none of the existing implementation could
work as is.

bin/redis_sg_tool

@@ -220,13 +220,12 @@ class QuarkRedisTool(object):
                                                            port["mac_address"])
                 if existing_rules:
                     overwrite_count += 1
-
+                    db_len = len(rules)
-                db_len = len(rules)
+                    existing_len = len(existing_rules["rules"])
-                existing_len = len(existing_rules["rules"])
+                    print ("== Port ID:%s - MAC:%s - Device ID:%s - "
-                print ("== Port ID:%s - MAC:%s - Device ID:%s - "
+                           "Redis Rules:%d - DB Rules:%d" %
-                       "Redis Rules:%d - DB Rules:%d" %
+                           (port["id"], mac, port["device_id"], existing_len,
-                       (port["id"], mac, port["device_id"], existing_len,
+                            db_len))
-                        db_len))
 
             if not dryrun:
                 for retry in xrange(self._retries):

quark/exceptions.py

@@ -117,6 +117,11 @@ class TenantNetworkSecurityGroupsNotImplemented(exceptions.InvalidInput):
                 "tenant networks")
 
 
+class SecurityGroupsRequireDevice(exceptions.InvalidInput):
+    message = _("Security Groups may only be applied to ports connected to "
+                "devices")
+
+
 class RedisConnectionFailure(exceptions.NeutronException):
     message = _("No connection to Redis could be made.")
 

quark/plugin_modules/ports.py

@@ -261,6 +261,9 @@ def update_port(context, id, port):
         if not STRATEGY.is_parent_network(port_db["network_id"]):
             raise q_exc.TenantNetworkSecurityGroupsNotImplemented()
 
+    if new_security_groups and not port_db["device_id"]:
+        raise q_exc.SecurityGroupsRequireDevice()
+
     group_ids, security_group_mods = _make_security_group_list(
         context, new_security_groups)
     quota.QUOTAS.limit_check(context, context.tenant_id,

quark/security_groups/redis_client.py

@@ -14,6 +14,7 @@
 #
 
 import json
+import string
 import uuid
 
 import netaddr
@@ -30,6 +31,8 @@ CONF = cfg.CONF
 LOG = logging.getLogger(__name__)
 SECURITY_GROUP_VERSION_UUID_KEY = "id"
 SECURITY_GROUP_RULE_KEY = "rules"
+MAC_TRANS_TABLE = string.maketrans(string.ascii_uppercase,
+                                   string.ascii_lowercase)
 
 quark_opts = [
     cfg.StrOpt('redis_security_groups_host',
@@ -51,22 +54,9 @@ quark_opts = [
     cfg.StrOpt("redis_sentinel_master",
                default='',
                help=_("The name label of the master redis sentinel")),
-    cfg.BoolOpt("redis_use_ssl",
+    cfg.StrOpt("redis_password",
-                default=False,
-                help=_("Configures whether or not to use SSL")),
-    cfg.StrOpt("redis_ssl_certfile",
                default='',
-               help=_("Path to the SSL cert")),
+               help=_("The password for authenticating with redis.")),
-    cfg.StrOpt("redis_ssl_keyfile",
-               default='',
-               help=_("Path to the SSL keyfile")),
-    cfg.StrOpt("redis_ssl_ca_certs",
-               default='',
-               help=_("Path to the SSL CA certs")),
-    cfg.StrOpt("redis_ssl_cert_reqs",
-               default='none',
-               help=_("Certificate requirements. Values are 'none', "
-                      "'optional', and 'required'")),
     cfg.StrOpt("redis_db",
                default="0",
                help=("The database number to use")),
@@ -89,78 +79,58 @@ class Client(object):
     connection_pool = None
 
     def __init__(self, use_master=False):
-        self._ensure_connection_pool_exists()
-        self._sentinel_list = None
         self._use_master = use_master
 
         try:
             if CONF.QUARK.redis_use_sentinels:
-                self._client = self._client_from_sentinel(self._use_master)
+                self._compile_sentinel_list()
-            else:
+            self._ensure_connection_pool_exists(use_master)
-                self._client = self._client_from_config()
+            self._client = self._client()
-
         except redis.ConnectionError as e:
             LOG.exception(e)
             raise q_exc.RedisConnectionFailure()
 
-    def _ensure_connection_pool_exists(self):
+    def _ensure_connection_pool_exists(self, use_master):
         if not Client.connection_pool:
             LOG.info("Creating redis connection pool for the first time...")
             host = CONF.QUARK.redis_security_groups_host
             port = CONF.QUARK.redis_security_groups_port
             LOG.info("Using redis host %s:%s" % (host, port))
 
-            connect_class = redis.Connection
+            connect_kw = {}
-            connect_kw = {"host": host, "port": port}
+            if CONF.QUARK.redis_password:
+                connect_kw["password"] = CONF.QUARK.redis_password
 
-            if CONF.QUARK.redis_use_ssl:
+            connect_args = []
-                LOG.info("Communicating with redis over SSL")
-                connect_class = redis.SSLConnection
-                if CONF.QUARK.redis_ssl_certfile:
-                    cert_req = CONF.QUARK.redis_ssl_cert_reqs
-                    connect_kw["ssl_certfile"] = CONF.QUARK.redis_ssl_certfile
-                    connect_kw["ssl_cert_reqs"] = cert_req
-                    connect_kw["ssl_ca_certs"] = CONF.QUARK.redis_ssl_ca_certs
-                    connect_kw["ssl_keyfile"] = CONF.QUARK.redis_ssl_keyfile
 
             klass = redis.ConnectionPool
             if CONF.QUARK.redis_use_sentinels:
+                connect_args.append(CONF.QUARK.redis_sentinel_master)
                 klass = redis.sentinel.SentinelConnectionPool
+                connect_args.append(
+                    redis.sentinel.Sentinel(self._sentinel_list))
+                connect_kw["check_connection"] = True
+                connect_kw["use_master"] = use_master
+            else:
+                connect_kw["host"] = host
+                connect_kw["port"] = port
 
-            Client.connection_pool = klass(connection_class=connect_class,
+            Client.connection_pool = klass(*connect_args,
                                            **connect_kw)
 
-    def _get_sentinel_list(self):
+    def _compile_sentinel_list(self):
+        self._sentinel_list = [tuple(host.split(':'))
+                               for host in CONF.QUARK.redis_sentinel_hosts]
         if not self._sentinel_list:
-            self._sentinel_list = [tuple(host.split(':'))
+            raise TypeError("sentinel_list is not a properly formatted"
-                                   for host in CONF.QUARK.redis_sentinel_hosts]
+                            "list of 'host:port' pairs")
-            if not self._sentinel_list:
-                raise TypeError("sentinel_list is not a properly formatted"
-                                "list of 'host:port' pairs")
 
-        return self._sentinel_list
+    def _client(self):
-
-    def _client_from_config(self):
         kwargs = {"connection_pool": Client.connection_pool,
                   "db": CONF.QUARK.redis_db,
                   "socket_timeout": CONF.QUARK.redis_socket_timeout}
         return redis.StrictRedis(**kwargs)
 
-    def _client_from_sentinel(self, is_master=True):
-        master = is_master and "master" or "slave"
-        LOG.info("Initializing redis connection to %s node, master label %s" %
-                 (master, CONF.QUARK.redis_sentinel_master))
-
-        sentinel = redis.sentinel.Sentinel(self._get_sentinel_list())
-        func = sentinel.slave_for
-        if is_master:
-            func = sentinel.master_for
-
-        return func(CONF.QUARK.redis_sentinel_master,
-                    db=CONF.QUARK.redis_db,
-                    socket_timeout=CONF.QUARK.redis_socket_timeout,
-                    connection_pool=Client.connection_pool)
-
     def serialize_rules(self, rules):
         """Creates a payload for the redis server."""
         # TODO(mdietz): If/when we support other rule types, this comment
@@ -203,6 +173,9 @@ class Client(object):
         REDIS KEY - port_device_id.port_mac_address
         REDIS VALUE - A JSON dump of the following:
 
+        port_mac_address must be lower-cased and stripped of non-alphanumeric
+        characters
+
         {"id": "<arbitrary uuid>",
          "rules": [
            {"ethertype": <hexademical integer>,
@@ -236,11 +209,17 @@ class Client(object):
         return rules
 
     def rule_key(self, device_id, mac_address):
-        return "{0}.{1}".format(device_id, str(netaddr.EUI(mac_address)))
+        mac = str(netaddr.EUI(mac_address))
+
+        # Lower cases and strips hyphens from the mac
+        mac = mac.translate(MAC_TRANS_TABLE, ":-")
+        return "{0}.{1}".format(device_id, mac)
 
     def get_rules_for_port(self, device_id, mac_address):
-        return json.loads(self._client.get(
+        rules = self._client.get(
-            self.rule_key(device_id, mac_address)))
+            self.rule_key(device_id, mac_address))
+        if rules:
+            return json.loads(rules)
 
     def apply_rules(self, device_id, mac_address, rules):
         """Writes a series of security group rules to a redis server."""
@@ -263,7 +242,10 @@ class Client(object):
         return self._client.echo(echo_str)
 
     def vif_keys(self):
-        return self._client.keys("*.??-??-??-??-??-??")
+        keys = self._client.keys("*.????????????")
+        if isinstance(keys, str):
+            keys = [keys]
+        return [k for k in keys if k]
 
     def delete_vif_rules(self, key):
         self._client.delete(key)

quark/tests/plugin_modules/test_ports.py

@@ -891,7 +891,7 @@ class TestQuarkUpdatePortSecurityGroups(test_quark_plugin.TestQuarkPlugin):
 
     def test_update_port_security_groups_on_tenant_net_raises(self):
         with self._stubs(
-            port=dict(id=1)
+            port=dict(id=1, device_id="device")
         ) as (port_find, port_update, alloc_ip, dealloc_ip, sg_find,
               driver_port_update):
             new_port = dict(port=dict(name="ourport",
@@ -902,7 +902,7 @@ class TestQuarkUpdatePortSecurityGroups(test_quark_plugin.TestQuarkPlugin):
 
     def test_update_port_security_groups(self):
         with self._stubs(
-            port=dict(id=1), parent_net=True
+            port=dict(id=1, device_id="device"), parent_net=True
         ) as (port_find, port_update, alloc_ip, dealloc_ip, sg_find,
               driver_port_update):
             new_port = dict(port=dict(name="ourport",
@@ -929,6 +929,16 @@ class TestQuarkUpdatePortSecurityGroups(test_quark_plugin.TestQuarkPlugin):
                 mac_address=port_dict["mac_address"],
                 device_id=port_dict["device_id"])
 
+    def test_update_port_security_groups_no_device_id_raises(self):
+        with self._stubs(
+            port=dict(id=1), parent_net=True
+        ) as (port_find, port_update, alloc_ip, dealloc_ip, sg_find,
+              driver_port_update):
+            new_port = dict(port=dict(name="ourport",
+                                      security_groups=[1]))
+            with self.assertRaises(q_exc.SecurityGroupsRequireDevice):
+                self.plugin.update_port(self.context, 1, new_port)
+
 
 class TestQuarkUpdatePortSetsIps(test_quark_plugin.TestQuarkPlugin):
     @contextlib.contextmanager

quark/tests/security_groups/test_redis_client.py

@@ -47,10 +47,9 @@ class TestRedisSerialization(test_base.TestBase):
         mac_address = netaddr.EUI("AA:BB:CC:DD:EE:FF")
 
         redis_key = client.rule_key(device_id, mac_address.value)
-        expected = "%s.%s" % (device_id, str(mac_address))
+        expected = "%s.%s" % (device_id, "aabbccddeeff")
         self.assertEqual(expected, redis_key)
-        conn_pool.assert_called_with(connection_class=redis.Connection,
+        conn_pool.assert_called_with(host=host, port=port)
-                                     host=host, port=port)
 
     @mock.patch("uuid.uuid4")
     @mock.patch("redis.ConnectionPool")
@@ -196,25 +195,23 @@ class TestRedisSentinelConnection(test_base.TestBase):
         CONF.set_override("redis_sentinel_hosts", '', "QUARK")
         CONF.set_override("redis_sentinel_master", '', "QUARK")
 
+    @mock.patch("redis.sentinel.Sentinel")
     @mock.patch("redis.sentinel.SentinelConnectionPool")
     @mock.patch("redis.sentinel.Sentinel.master_for")
     @mock.patch("quark.security_groups.redis_client.redis.StrictRedis")
     def test_sentinel_connection(self, strict_redis, master_for,
-                                 sentinel_pool):
+                                 sentinel_pool, sentinel_mock):
         host = "127.0.0.1"
         port = 6379
         sentinels = ["%s:%s" % (host, port)]
         master_label = "master"
+        sentinel_mock.return_value = sentinels
 
         with self._stubs(True, sentinels, master_label):
             redis_client.Client(use_master=True)
-            master_for.assert_called_with(
+            sentinel_pool.assert_called_with(master_label, sentinels,
-                master_label,
+                                             check_connection=True,
-                db=CONF.QUARK.redis_db,
+                                             use_master=True)
-                socket_timeout=CONF.QUARK.redis_socket_timeout,
-                connection_pool=redis_client.Client.connection_pool)
-            sentinel_pool.assert_called_with(connection_class=redis.Connection,
-                                             host=host, port=port)
 
     @mock.patch("redis.sentinel.SentinelConnectionPool")
     @mock.patch("redis.sentinel.Sentinel.master_for")
@@ -229,50 +226,6 @@ class TestRedisSentinelConnection(test_base.TestBase):
                 redis_client.Client(use_master=True)
 
 
-class TestRedisSSHConnection(test_base.TestBase):
-    def setUp(self):
-        super(TestRedisSSHConnection, self).setUp()
-        # Forces the connection pool to be recreated on every test
-        redis_client.Client.connection_pool = None
-
-    @contextlib.contextmanager
-    def _stubs(self, use_sentinels, sentinels, master_label):
-        CONF.set_override("redis_use_ssl", True, "QUARK")
-        CONF.set_override("redis_ssl_certfile", 'my.cert', "QUARK")
-        CONF.set_override("redis_ssl_ca_certs", 'server.cert', "QUARK")
-        CONF.set_override("redis_ssl_keyfile", 'keyfile', "QUARK")
-        CONF.set_override("redis_ssl_cert_reqs", 'required', "QUARK")
-        yield
-        CONF.set_override("redis_ssl_cert_reqs", 'none', "QUARK")
-        CONF.set_override("redis_ssl_keyfile", '', "QUARK")
-        CONF.set_override("redis_ssl_ca_certs", '', "QUARK")
-        CONF.set_override("redis_ssl_certfile", '', "QUARK")
-        CONF.set_override("redis_use_ssl", False, "QUARK")
-
-    @mock.patch("redis.ConnectionPool")
-    @mock.patch("redis.sentinel.Sentinel.master_for")
-    @mock.patch("quark.security_groups.redis_client.redis.StrictRedis")
-    def test_ssl_connection(self, strict_redis, master_for, conn_pool):
-        host = "127.0.0.1"
-        port = 6379
-        sentinels = ["%s:%s" % (host, port)]
-        master_label = "master"
-
-        with self._stubs(True, sentinels, master_label):
-            redis_client.Client(use_master=True)
-            ssl_conn = redis.connection.SSLConnection
-            conn_pool.assert_called_with(ssl_certfile="my.cert",
-                                         ssl_keyfile="keyfile",
-                                         ssl_ca_certs="server.cert",
-                                         ssl_cert_reqs="required",
-                                         connection_class=ssl_conn,
-                                         host=host, port=port)
-            strict_redis.assert_called_with(
-                socket_timeout=CONF.QUARK.redis_socket_timeout,
-                db=CONF.QUARK.redis_db,
-                connection_pool=redis_client.Client.connection_pool)
-
-
 class TestRedisForAgent(test_base.TestBase):
     def setUp(self):
         super(TestRedisForAgent, self).setUp()