#167 Implement Etcd-based backend
Closed 7 years ago by puiterwijk. Opened 7 years ago by puiterwijk.
puiterwijk/ipsilon etcd-split  into  master

file modified
+1
@@ -99,6 +99,7 @@ 

  	PYTHONPATH=./ ./tests/tests.py --path=$(TESTDIR) --test=attrs

  	PYTHONPATH=./ ./tests/tests.py --path=$(TESTDIR) --test=trans

  	PYTHONPATH=./ ./tests/tests.py --path=$(TESTDIR) --test=pgdb

+ 	PYTHONPATH=./ ./tests/tests.py --path=$(TESTDIR) --test=testetcd

  	PYTHONPATH=./ ./tests/tests.py --path=$(TESTDIR) --test=fconf

  	PYTHONPATH=./ ./tests/tests.py --path=$(TESTDIR) --test=ldap

  	PYTHONPATH=./ ./tests/tests.py --path=$(TESTDIR) --test=ldapdown

file modified
+1
@@ -38,6 +38,7 @@ 

  cfgfile = find_config(None, None)

  

  cherrypy.lib.sessions.SqlSession = ipsilon.util.sessions.SqlSession

+ cherrypy.lib.sessions.EtcdSession = ipsilon.util.sessions.EtcdSession

  cherrypy.config.update(cfgfile)

  

  # Force cherrypy logging to work. Note that this ignores the config-file

file modified
+11 -4
@@ -7,7 +7,7 @@ 

  from jinja2 import Environment, FileSystemLoader

  import ipsilon.util.sessions

  from ipsilon.util.data import AdminStore, Store, UserStore, TranStore

- from ipsilon.util.sessions import SqlSession

+ from ipsilon.util.sessions import SqlSession, EtcdSession

  from ipsilon.root import Root

  

  import logging
@@ -62,6 +62,7 @@ 

  

  def execute_upgrade(cfgfile):

      cherrypy.lib.sessions.SqlSession = ipsilon.util.sessions.SqlSession

+     cherrypy.lib.sessions.EtcdSession = ipsilon.util.sessions.EtcdSession

      cherrypy.config.update(cfgfile)

  

      # pylint: disable=protected-access
@@ -84,13 +85,19 @@ 

  

      # Handle the session store if that is Sql

      logger.info('Handling sessions datastore')

-     if cherrypy.config['tools.sessions.storage_type'] != 'sql':

-         logger.info('Not SQL-based, skipping')

-     else:

+     sesstype = cherrypy.config['tools.sessions.storage_type'].lower()

+     if sesstype == 'sql':

          dburi = cherrypy.config['tools.sessions.storage_dburi']

          SqlSession.setup(storage_dburi=dburi)

          if _upgrade_database(SqlSession._store) not in [True, None]:

              return upgrade_failed()

+     elif sesstype == 'etcd':

+         dburi = cherrypy.config['tools.sessions.storage_dburi']

This line appears to be identical to line 27 - you could pull both out to happen before the if statement to deduplicate code.

+         EtcdSession.setup(storage_dburi=dburi)

+         if _upgrade_database(EtcdSession._store) not in [True, None]:

+             return upgrade_failed()

+     else:

+         logger.info('File based, skipping')

  

      # Now handle the rest of the default datastores

      for store in [UserStore, TranStore]:

file modified
+287
@@ -10,8 +10,14 @@ 

                                 CreateIndex)

  from sqlalchemy.sql import select, and_

  import ConfigParser

+ try:

+     import etcd

+ except ImportError:

+     etcd = None

  import os

+ import json

  import uuid

+ from urlparse import urlparse

  import logging

  import time

  
@@ -319,6 +325,284 @@ 

          raise NotImplementedError

  

  

+ class EtcdStore(BaseStore):

+     """Etcd-based storage

+ 

+     Example URI: etcd://server/rootpath?port=2379&scheme=https

+     The rootpath indicates at what point in the etcd key-space we will insert

+     our keys.

+     The parts after the ? are passed as key-value to the etcd client.

+     """

+ 

+     def __init__(self, uri):

+         if etcd is None:

+             raise NotImplementedError('Etcd client not available')

+         url = urlparse(uri)

+         self.rootpath = url.path

+         config = dict([cfg.split('=', 1) for cfg in url.query.split('&')])

+ 

+         if 'port' in config:

+             config['port'] = int(config['port'])

+ 

+         self.debug('Etcd host: %s, rootpath: %s, config: %s' %

+                    (url.netloc, url.path, config))

+ 

+         self.client = etcd.Client(host=url.netloc, **config)

+ 

+         # We ignore the value, but this is a connection test

+         self.client.leader  # pylint: disable=pointless-statement

+ 

+         self.is_readonly = False

+ 

+     def add_constraint(self, table):

+         raise NotImplementedError()

+ 

+     def add_index(self, index):

+         raise NotImplementedError()

+ 

+     def close(self):

+         # No-op

+         return

+ 

+ 

+ class EtcdQuery(Log):

+     """

+     Class to store stuff in Etcd key-value stores.

+ 

+     A row is stored in the etcd store under

+     /<rootpath>/<table>/<pk_1>/<pk_2>/.../<pk_n>

+     Where rootpath is configurable, <table> is the name of the name of the

+     table, and pk_1, pk_2, ..., pk_n are the first, second and nth components

+     of the primary key of that table.

+ 

+     This means that tables using etcd require a primary key.

+ 

+     The value stored at those keys is a json document with all of the keys and

+     values for that object, including the primary keys.

+ 

+     Cleanup of objects in etcd we leave to Etcd: when the object gets created,

+     we store the TTL in the key.

+     """

+ 

+     def __init__(self, store, table, table_def, trans=True):

+         """Query class initialization.

+ 

+         store is a handle to a connected EtcdStore object.

+         table is the name of the "table" (key space) we are querying.

+         table_def is the table definition, look at OPTIONS_TABLE and

+             UNIQUE_DATA_TABLE for examples.

+         trans is accepted for compatibility with other Query types, but

+             ignored.

+         """

+         if etcd is None:

+             raise NotImplementedError('Etcd client not available')

+         # We don't have indexes in a EtcdQuery, so drop that info

+         if isinstance(table_def, dict) and 'primary_key' in table_def:

+             columns = table_def['columns']

+             if isinstance(columns[0], tuple):

+                 columns = [column[0] for column in columns]

+             self._primary_key = tuple(table_def['primary_key'])

+         else:

+             # This is a custom plugin that uses tables that are incompatible

+             # with etcd.

+             raise ValueError('Etcd requires primary key')

+         self._table = table

+         self._table_def = table_def

+         self._store = store

+         self._section = table

+         self._columns = columns

+         self._con = store

+ 

+     @property

+     def _table_dir(self):

+         """This returns the full path to the table key."""

+         return '%s/%s' % (self._store.rootpath, self._table)

+ 

+     def _get_most_specific_dir(self, kvfilter, del_kv=True, update=False):

+         """Get the most specific dir in which we can find stuff.

+ 

+         Return a tuple with path and then the number of path levels not used.

+ 

+         kvfilter is a dict with the keys we want to filter for.

+         del_kv is a boolean that indicates whether or not to remove used

+             filters from the kvfilter dict.

+         update is a boolean that indicates whether this is for an insert/update

+             operation. Those require a fully specified object path.

+         """

+         path = self._table_dir

+ 

+         if kvfilter is None:

+             kvfilter = {}

+ 

+         pkeys_used = 0

+         # We try to use as much of the primary key as we are able to to

+         # generate the most specific path possible.

+         for pkey in self._primary_key:

+             if pkey in kvfilter:

+                 pkeys_used += 1

+                 path = os.path.join(path, kvfilter[pkey].replace(' ', '_'))

+                 if del_kv:

+                     del kvfilter[pkey]

+             else:

+                 # Seems this next primary key value was not part of the filter

+                 break

+ 

+         levels_unused = len(self._primary_key) - pkeys_used

+ 

+         if levels_unused != 0 and update:

+             raise Exception('Fully qualified object required for updates')

+ 

+         return path, levels_unused

+ 

+     def rollback(self):

+         """Rollback is ignored because etcd doesn't have transactions."""

+         return

+ 

+     def commit(self):

+         """Commit is ignored because etcd doesn't have transactions."""

+         return

+ 

+     def create(self):

+         """Create a directory to store the current table in."""

+         try:

+             self._store.client.write(self._table_dir, None, dir=True)

+         except etcd.EtcdNotFile:

+             # This means that this key already contained a directory. In which

+             # case, we are done.

+             pass

+ 

+     def drop(self):

+         """Drop the current table and everything under it."""

+         self._store.client.delete(self._table_dir, recursive=True, dir=True)

+ 

+     def _select_objects(self, kvfilter):

+         """

+         Select all the objects that satisfy the kvfilter parts that are in the

+         primary key for this table.

+         """

+         path, levels_unused = self._get_most_specific_dir(kvfilter)

+         try:

+             res = self._store.client.read(path, recursive=levels_unused != 0)

+         except etcd.EtcdKeyNotFound:

+             return None

+ 

+         if levels_unused == 0:

+             # This was a fully qualified object, let's use the object

+             if res.dir:

+                 return []

+             else:

+                 return [res]

+         else:

+             # This was not fully qualified. Given we used recursive=True, we

+             # know that "children" is the final objects.

+             return [cld for cld in res.children if not cld.dir]

+ 

+     def _select_filter(self, kvfilter, res):

+         """

+         Filters all objects from res that do not satisfy the non-primary

+         kvfilter entries.

+         """

+         for obj in res:

+             result = json.loads(obj.value)

+ 

+             pick_object = True

+             for key in kvfilter:

+                 if key not in result:

+                     pick_object = False

+                     break

+                 if result[key] != kvfilter[key]:

+                     pick_object = False

+                     break

+             if pick_object:

+                 yield result

+ 

+     def select(self, kvfilter=None, columns=None):

+         """Select specific objects from the store.

+ 

+         kvfilter is a dict indicating which keys should be matched for.

+         columns is a list of columns to return, and their order.

+         Returns a list of column value lists.

+         """

+         if columns is None:

+             columns = self._columns

+ 

+         res = self._select_objects(kvfilter)

+         if res is None:

+             return []

+         results = self._select_filter(kvfilter, res)

+ 

+         rows = []

+         for obj in results:

+             row = []

+             for column in columns:

+                 row.append(obj[column])

+             rows.append(tuple(row))

+ 

+         return rows

+ 

+     def insert(self, value_row, ttl=None):

+         """Insert a new object into the store.

+ 

+         value_row is a list of column values.

+         ttl is the time for which the object is supposed to be kept.

+         """

+         value_row = list(value_row)

+ 

+         values = {}

+         for column in self._columns:

+             values[column] = value_row.pop(0)

+ 

+         path, _ = self._get_most_specific_dir(values, False, update=True)

+         self._store.client.write(path, json.dumps(values), ttl=ttl)

+ 

+     def update(self, values, kvfilter):

+         """Updates an item in the store.

+ 

+         Requires a single object, thus the kvfilter must be specific to match

+         a single object.

+ 

+         kvfilter is the dict of key-values that find a specific object.

+         values is the dict with key-values that we want to update to.

+         """

+         path, _ = self._get_most_specific_dir(kvfilter, update=True)

+         for key in values:

+             if key in self._primary_key:

+                 raise ValueError('Unable to update primary key values')

+ 

+         current = json.loads(self._store.client.read(path).value)

+         for key in values:

+             current[key] = values[key]

+         self._store.client.write(path, json.dumps(current))

+ 

+     def delete(self, kvfilter):

+         """Deletes an item from the store.

+ 

+         Requires a single object, thus the kvfilter must be specific to match

+         a single object.

+ 

+         kvfilter is the dict of key-values that find a specific object.

+         """

+         path, levels_unused = self._get_most_specific_dir(kvfilter)

+         if levels_unused == 0 or len(kvfilter) == 0:

+             try:

+                 current = json.loads(self._store.client.read(path).value)

+             except etcd.EtcdKeyNotFound:

+                 return

+             for key in kvfilter:

+                 if current[key] != kvfilter[key]:

+                     # We had 0 levels unused, meaning we are at a qualified

+                     # object, and it doesn't match the kvfilter. We are done.

+                     return

+             try:

+                 self._store.client.delete(path, recursive=True, dir=True)

+             except etcd.EtcdKeyNotFound:

+                 pass

+         else:

+             # This was not a fully specified object, we need to get all fully

+             # qualified objects

+             raise NotImplementedError()

+ 

+ 

  class Store(Log):

      # Static, Store-level variables

      _is_upgrade = False
@@ -345,6 +629,9 @@ 

              _, filename = name.split('://')

              self._db = FileStore(filename)

              self._query = FileQuery

+         elif name.startswith('etcd://'):

+             self._db = EtcdStore(name)

+             self._query = EtcdQuery

          else:

              self._db = SqlStore.get_connection(name)

              self._query = SqlQuery

file modified
+109 -2
@@ -1,4 +1,4 @@ 

- # Copyright (C) 2014 Ipsilon project Contributors, for license see COPYING

+ # Copyright (C) 2014,2016 Ipsilon project Contributors, for license see COPYING

  

  import base64

  from cherrypy.lib.sessions import Session
@@ -6,11 +6,16 @@ 

  import threading

  import datetime

  try:

+     import etcd

+ except ImportError:

+     etcd = None

+ import json

+ import time

+ try:

      import cPickle as pickle

  except ImportError:

      import pickle

  

- 

  SESSION_TABLE = {'columns': ['id', 'data', 'expiration_time'],

                   'primary_key': ('id', ),

                   'indexes': [('expiration_time',)]
@@ -108,3 +113,105 @@ 

          """Release the lock on the currently-loaded session data."""

          self.locks[self.id].release()

          self.locked = False

+ 

+ 

+ class EtcdSessionStore(Store):

+     def _initialize_schema(self):

+         return

+ 

+     def _upgrade_schema(self, old_version):

+         raise NotImplementedError()

+ 

+     def _cleanup(self):

+         return

+ 

+ 

+ class EtcdSession(Session):

+     """Cherrypy-compatible session store backed by Etcd.

+ 

+     All implemented functions are part of the standard cherrypy session manager

+     API.

+     """

+ 

+     dburi = None

+     _client = None

+     _store = None

+     _proto = 2

+ 

+     @classmethod

+     def setup(cls, **kwargs):

+         """Initialization for EtcdSession.

+ 

+         Called by cherrypy with all session options.

+         """

+         if etcd is None:

+             raise NotImplementedError('Etcd client not available')

+         for k, v in kwargs.items():

+             if k == 'storage_dburi':

+                 cls.dburi = v

+ 

+         cls._store = EtcdSessionStore(database_url=cls.dburi)

+         # pylint: disable=protected-access

+         cls._rootpath = cls._store._db.rootpath

+         # pylint: disable=protected-access

+         cls._client = cls._store._db.client

+ 

+     @property

+     def _session_path(self):

+         """Returns a path in etcd where we store sessions."""

+         return '%s/sessions/%s' % (self._rootpath, self.id)

+ 

+     @property

+     def _lock(self):

+         """Returns an etcd.Lock to lock the session across instances."""

+         lock = etcd.Lock(self._client,

+                          'session/%s' % self.id)

+         # We need to do this manually because otherwise python-etcd invents

+         # a new uuid for each lock instantiation, while we want to make the

+         # lock specific for the path.

+         # pylint: disable=protected-access

+         lock._uuid = 'wellknown'

+         return lock

+ 

+     def _exists(self):

+         """Returns a boolean whether the current session exists in the store.

+         """

+         try:

+             self._client.read(self._session_path)

+             return True

+         except etcd.EtcdKeyNotFound:

+             return False

+ 

+     def _load(self):

+         """Tries to load the current session from the store."""

+         try:

+             data = self._client.read(self._session_path)

+             # pylint: disable=no-member

+             value, exp_time = json.loads(data.value)

+             exp_dt = datetime.datetime.utcfromtimestamp(exp_time)

+             return value, exp_dt

+         except etcd.EtcdKeyNotFound:

+             return None

+ 

+     def _save(self, expiration_time):

+         """Saves the current session to the store."""

+         expiration_time = int(time.mktime(expiration_time.timetuple()))

+         ttl = expiration_time - int(time.time())

+         self._client.write(self._session_path,

+                            json.dumps((self._data, expiration_time)),

+                            ttl=ttl)

+ 

+     def _delete(self):

+         """Deletes and invalidates the current session."""

+         try:

+             self._client.delete(self._session_path)

+         except etcd.EtcdKeyNotFound:

+             pass

+ 

+     def acquire_lock(self):

+         self._lock.acquire(blocking=True)

+         self.locked = True

+ 

+     def release_lock(self):

+         self._lock.release()

+         self.locked = False

@@ -0,0 +1,1 @@ 

+ RUN yum install -y etcd python-etcd

file modified
+17 -1
@@ -73,13 +73,14 @@ 

  

  class IpsilonTestBase(object):

  

-     def __init__(self, name, execname):

+     def __init__(self, name, execname, allow_wrappers=True):

          self.name = name

          self.execname = execname

          self.rootdir = os.getcwd()

          self.testdir = None

          self.testuser = pwd.getpwuid(os.getuid())[0]

          self.processes = []

+         self.allow_wrappers = allow_wrappers

  

      def platform_supported(self):

          """This return whether the current platform supports this test.
@@ -255,6 +256,21 @@ 

          with open(filename, 'a') as f:

              f.write(auth)

  

+     def start_etcd_server(self, datadir, addr, clientport, srvport, env):

+         env['ETCD_NAME'] = 'ipsilon'

+         env['ETCD_DATA_DIR'] = datadir

+         env['ETCD_LISTEN_CLIENT_URLS'] = 'http://%s:%s' % (addr, clientport)

+         env['ETCD_LISTEN_PEER_URLS'] = 'http://%s:%s' % (addr, srvport)

+         env['ETCD_FORCE_NEW_CLUSTER'] = 'true'

+         env['ETCD_INITIAL_CLUSTER'] = 'ipsilon=http://%s:%s' % (addr, srvport)

+         env['ETCD_ADVERTISE_CLIENT_URLS'] = 'http://%s:%s' % (addr, clientport)

+         env['ETCD_INITIAL_ADVERTISE_PEER_URLS'] = 'http://%s:%s' % (addr,

+                                                                     srvport)

+         p = subprocess.Popen(['/usr/bin/etcd'],

+                              env=env, preexec_fn=os.setsid)

+         self.processes.append(p)

+         return p

+ 

      def start_http_server(self, conf, env):

          env['MALLOC_CHECK_'] = '3'

          env['MALLOC_PERTURB_'] = str(random.randint(0, 32767) % 255 + 1)

file added
+261
@@ -0,0 +1,261 @@ 

+ #!/usr/bin/python

+ #

+ # Copyright (C) 2017 Ipsilon project Contributors, for license see COPYING

+ 

+ from helpers.common import IpsilonTestBase  # pylint: disable=relative-import

+ from helpers.http import HttpSessions  # pylint: disable=relative-import

+ import os

+ import pwd

+ import sys

+ import subprocess

+ from string import Template

+ 

+ idp_g = {'TEMPLATES': '${TESTDIR}/templates/install',

+          'CONFDIR': '${TESTDIR}/etc',

+          'DATADIR': '${TESTDIR}/lib',

+          'CACHEDIR': '${TESTDIR}/cache',

+          'HTTPDCONFD': '${TESTDIR}/${NAME}/conf.d',

+          'STATICDIR': '${ROOTDIR}',

+          'BINDIR': '${ROOTDIR}/ipsilon',

+          'WSGI_SOCKET_PREFIX': '${TESTDIR}/${NAME}/logs/wsgi'}

+ 

+ 

+ idp_a = {'hostname': '${ADDRESS}:${PORT}',

+          'admin_user': '${TEST_USER}',

+          'system_user': '${TEST_USER}',

+          'instance': '${NAME}',

+          'testauth': 'yes',

+          'pam': 'no',

+          'gssapi': 'no',

+          'ipa': 'no',

+          'server_debugging': 'True',

+          'openidc': 'yes',

+          'database_url': 'etcd://127.0.0.10/ipsilon/%(dbname)s?port=42379',

+          'session_type': 'etcd',

+          'session_dburi': 'etcd://127.0.0.10/ipsilon/sessions?port=42379'}

+ 

+ 

+ sp_g = {'HTTPDCONFD': '${TESTDIR}/${NAME}/conf.d',

+         'SAML2_TEMPLATE': '${TESTDIR}/templates/install/saml2/sp.conf',

+         'CONFFILE': '${TESTDIR}/${NAME}/conf.d/ipsilon-%s.conf',

+         'HTTPDIR': '${TESTDIR}/${NAME}/%s'}

+ 

+ 

+ sp_a = {'hostname': '${ADDRESS}',

+         'saml_idp_metadata': 'https://127.0.0.10:45080/idp1/saml2/metadata',

+         'saml_auth': '/sp',

+         'httpd_user': '${TEST_USER}'}

+ 

+ sp2_g = {'HTTPDCONFD': '${TESTDIR}/${NAME}/conf.d',

+          'SAML2_TEMPLATE': '${TESTDIR}/templates/install/saml2/sp.conf',

+          'CONFFILE': '${TESTDIR}/${NAME}/conf.d/ipsilon-%s.conf',

+          'HTTPDIR': '${TESTDIR}/${NAME}/%s'}

+ 

+ sp2_a = {'hostname': '${ADDRESS}',

+          'saml_idp_url': 'https://127.0.0.10:45080/idp1',

+          'admin_user': '${TEST_USER}',

+          'admin_password': '${TESTDIR}/pw.txt',

+          'saml_sp_name': 'sp2-test.example.com',

+          'saml_auth': '/sp',

+          'httpd_user': '${TEST_USER}'}

+ 

+ keyless_metadata = """<?xml version='1.0' encoding='UTF-8'?>

+ <md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata"

+     xmlns:ds="http://www.w3.org/2000/09/xmldsig#" cacheDuration="P7D"

+     entityID="http://keyless-sp">

+   <md:SPSSODescriptor

+         protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">

+     <md:AssertionConsumerService

+         Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"

+         Location="http://keyless-sp/postResponse" index="0"/>

+     <md:NameIDFormat>

+ urn:oasis:names:tc:SAML:2.0:nameid-format:transient</md:NameIDFormat>

+   </md:SPSSODescriptor>

+ </md:EntityDescriptor>

+ """

+ 

+ 

+ def fixup_sp_httpd(httpdir):

+     location = """

+ 

+ Alias /sp ${HTTPDIR}/sp

+ 

+ <Directory ${HTTPDIR}/sp>

+     <IfModule mod_authz_core.c>

+         Require all granted

+     </IfModule>

+     <IfModule !mod_authz_core.c>

+         Order Allow,Deny

+         Allow from All

+     </IfModule>

+ </Directory>

+ """

+     index = """WORKS!"""

+ 

+     t = Template(location)

+     text = t.substitute({'HTTPDIR': httpdir})

+     with open(httpdir + '/conf.d/ipsilon-saml.conf', 'a') as f:

+         f.write(text)

+ 

+     os.mkdir(httpdir + '/sp')

+     with open(httpdir + '/sp/index.html', 'w') as f:

+         f.write(index)

+ 

+ 

+ class IpsilonTest(IpsilonTestBase):

+ 

+     def __init__(self):

+         # Etcd is written in golang, which links everything statically. As such

+         # the wrappers are not going to work.

+         super(IpsilonTest, self).__init__('testetcd', __file__,

+                                           allow_wrappers=False)

+ 

+     def platform_supported(self):

+         try:

+             p = subprocess.Popen(['/usr/bin/etcd', '--version'],

+                                  stdout=subprocess.PIPE)

+             stdout, _ = p.communicate()

+         except OSError:

+             print "No etcd installed"

+             return False

+         if not p.wait() == 0:

+             print 'No etcd installed'

+             return False

+         # Example line: etcd Version: 3.0.13

+         if int(stdout.split('\n')[0].split(': ')[1][0]) < 3:

+             print 'Etcd version < 3.0'

+             return False

+         try:

+             import etcd  # pylint: disable=unused-variable,import-error

+         except ImportError:

+             print 'No python-etcd available'

+             return False

+         return True

+ 

+     def setup_servers(self, env=None):

+ 

+         print "Starting IDP's etcd server"

+         datadir = os.path.join(self.testdir, 'etcd')

+         os.mkdir(datadir)

+         addr = '127.0.0.10'

+         clientport = '42379'

+         srvport = '42380'

+         self.start_etcd_server(datadir, addr, clientport, srvport, env)

+ 

+         print "Installing IDP server"

+         name = 'idp1'

+         addr = '127.0.0.10'

+         port = '45080'

+         idp = self.generate_profile(idp_g, idp_a, name, addr, port)

+         conf = self.setup_idp_server(idp, name, addr, port, env)

+ 

+         print "Starting IDP's httpd server"

+         self.start_http_server(conf, env)

+ 

+         print "Installing first SP server"

+         name = 'sp1'

+         addr = '127.0.0.11'

+         port = '45081'

+         sp = self.generate_profile(sp_g, sp_a, name, addr, port)

+         conf = self.setup_sp_server(sp, name, addr, port, env)

+         fixup_sp_httpd(os.path.dirname(conf))

+ 

+         print "Starting first SP's httpd server"

+         self.start_http_server(conf, env)

+ 

+         print "Installing second SP server"

+         name = 'sp2-test.example.com'

+         addr = '127.0.0.11'

+         port = '45082'

+         sp = self.generate_profile(sp2_g, sp2_a, name, addr, port)

+         with open(os.path.dirname(sp) + '/pw.txt', 'a') as f:

+             f.write('ipsilon')

+         conf = self.setup_sp_server(sp, name, addr, port, env)

+         os.remove(os.path.dirname(sp) + '/pw.txt')

+         fixup_sp_httpd(os.path.dirname(conf))

+ 

+         print "Starting second SP's httpd server"

+         self.start_http_server(conf, env)

+ 

+ 

+ if __name__ == '__main__':

+ 

+     idpname = 'idp1'

+     sp1name = 'sp1'

+     sp2name = 'sp2-test.example.com'

+     user = pwd.getpwuid(os.getuid())[0]

+ 

+     sess = HttpSessions()

+     sess.add_server(idpname, 'https://127.0.0.10:45080', user, 'ipsilon')

+     sess.add_server(sp1name, 'https://127.0.0.11:45081')

+     sess.add_server(sp2name, 'https://127.0.0.11:45082')

+ 

+     print "etcd: Authenticate to IDP ...",

+     try:

+         sess.auth_to_idp(idpname)

+     except Exception, e:  # pylint: disable=broad-except

+         print >> sys.stderr, " ERROR: %s" % repr(e)

+         sys.exit(1)

+     print " SUCCESS"

+ 

+     print "etcd: Add first SP Metadata to IDP ...",

+     try:

+         sess.add_sp_metadata(idpname, sp1name)

+     except Exception, e:  # pylint: disable=broad-except

+         print >> sys.stderr, " ERROR: %s" % repr(e)

+         sys.exit(1)

+     print " SUCCESS"

+ 

+     print "etcd: Access first SP Protected Area ...",

+     try:

+         page = sess.fetch_page(idpname, 'https://127.0.0.11:45081/sp/')

+         page.expected_value('text()', 'WORKS!')

+     except ValueError, e:

+         print >> sys.stderr, " ERROR: %s" % repr(e)

+         sys.exit(1)

+     print " SUCCESS"

+ 

+     print "etcd: Access second SP Protected Area ...",

+     try:

+         page = sess.fetch_page(idpname, 'https://127.0.0.11:45082/sp/')

+         page.expected_value('text()', 'WORKS!')

+     except ValueError, e:

+         print >> sys.stderr, " ERROR: %s" % repr(e)

+         sys.exit(1)

+     print " SUCCESS"

+ 

+     print "etcd: Update second SP ...",

+     try:

+         # This is a test to see whether we can update SAML SPs where the name

+         # is an FQDN (includes hyphens and dots). See bug #196

+         sess.set_attributes_and_mapping(idpname, [],

+                                         ['namefull', 'givenname', 'surname'],

+                                         spname=sp2name)

+     except Exception, e:  # pylint: disable=broad-except

+         print >> sys.stderr, " ERROR: %s" % repr(e)

+         sys.exit(1)

+     else:

+         print " SUCCESS"

+ 

+     print "etcd: Try authentication failure ...",

+     newsess = HttpSessions()

+     newsess.add_server(idpname, 'https://127.0.0.10:45080', user, 'wrong')

+     try:

+         newsess.auth_to_idp(idpname)

+         print >> sys.stderr, " ERROR: Authentication should have failed"

+         sys.exit(1)

+     except Exception, e:  # pylint: disable=broad-except

+         print " SUCCESS"

+ 

+     print "etcd: Add keyless SP Metadata to IDP ...",

+     try:

+         sess.add_metadata(idpname, 'keyless', keyless_metadata)

+         page = sess.fetch_page(idpname, 'https://127.0.0.10:45080/idp1/admin/'

+                                         'providers/saml2/admin')

+         page.expected_value('//div[@id="row_provider_http://keyless-sp"]/'

+                             '@title',

+                             'WARNING: SP does not have signing keys!')

+     except Exception, e:  # pylint: disable=broad-except

+         print >> sys.stderr, " ERROR: %s" % repr(e)

+         sys.exit(1)

+     print " SUCCESS"

file modified
+3 -3
@@ -40,8 +40,8 @@ 

      return vars(parser.parse_args())

  

  

- def try_wrappers(base, wrappers):

-     if wrappers == 'no':

+ def try_wrappers(base, wrappers, allow_wrappers):

+     if wrappers == 'no' or not allow_wrappers:

          return {}

  

      pkgcfg = subprocess.Popen(['pkg-config', '--exists', 'socket_wrapper'])
@@ -96,7 +96,7 @@ 

  

      test.setup_base(args['path'], test)

  

-     env = try_wrappers(test.testdir, args['wrappers'])

+     env = try_wrappers(test.testdir, args['wrappers'], test.allow_wrappers)

      env['PYTHONPATH'] = test.rootdir

      env['TESTDIR'] = test.testdir

  

no initial comment

Should this be "File based…"?

You can use urlparse to get these components split out nicely:

https://docs.python.org/2/library/urlparse.html

If you want to ignore the value, all you need to do is not assign it to anything.

I recommend using ticket trackers for TODOs instead of putting them in the code.

Why assign columns if you are going to raise an Exception?

I recommend adding a comment here explaining why/when table_def would not be a dictionary.

I recommend adding a docblock here to document what each of these is for, and what type it is. It's not clear below what table_def['columns'] is expected to be, for example, so it's hard for me to follow this code.

I recommend a docblock here too.

I recommend documenting the parameters, types of parameters, and return value and return type here.

Modifying an iterable as you iterate can be risky. I recommend against doing it this way. A better way would be to keep track of which keys you wanted to delete in a list and doing the deletes after the iteration.

I recommend adding docblocks to both of the above, explaining what they are for and why we don't want them to do anything here (or at least the latter).

Why do we want to pass on this Exception? I'd recommend documenting that here for future readers.

IMO this function is a bit long and hard to follow as a result. I'd recommend making some subfunctions to break the work up into smaller logical blocks. This makes the code easier to follow, but it also makes testing easier since you can test the units by themselves instead of having to test them together.

You could use urlparse here too, if you want.

I recommend adding docblocks on all of the above.

I recommend docblocks on every unit in this class, including the class itself.

If you use unittest with individual tests, you don't have to sys.exit() whenever you encounter an error. I think this is valuable because if there is more than one test failure you can know which tests fail in a single run, rather than having it exit upon hitting the first failure. By the way, unittest test runners often have a mode that can exit upon hitting the first failure too, so that way you can still have this behavior when wanted (best of both worlds!) For example, nosetests -x will exit upon the first failure, but plain nosetests will run them all and then tell you which ones failed.

Nothing in this seems like a no-go, but I do have some recommendations for you. In particular, I'd like to call attention to that loop that is deleting items as it iterates. That could lead to a bug. I also highly recommend adding docblocks on everything and explaining the answers to my questions in code comments so they stay answered for future readers ☺

That's why I'm not doing that: I'm iterating the self._primary_key list, and I'm only doing an "if pkey in kvfilter" on every iteration.

This object is only supposed to be instantiated by the Store classes. For the definitions of table_def's, they are in data.py at the top: UNIQUE_DATA_TABLE and OPTIONS_TABLE.

4 new commits added

  • Add tests for Etcd data and session stores
  • Implement Etcd-based session store
  • Implement Etcd-based data store
  • Allow tests to indicate they don't play well with wrappers
7 years ago

This line appears to be identical to line 27 - you could pull both out to happen before the if statement to deduplicate code.

It might be good to rename config to uri or something else that's more descriptive.

You could turn this into a docblock for this method, and expand on it a bit.

You could raise ValueError here as a more specific Exception, since it was the values of the arguments that caused the Exception.

It would be good to document all of these arguments and their types in a docblock, and mention the raising of the exception.

I recommend adding test coverage here so you can ensure this works as you expect.

Why do you want to break when you hit a key that isn't in the kvfilter? I recommend adding a comment explaining that here.

I think you can drop the call to list() here, as the list comprehension is already a list.

I recommend adding a docblock, and documenting the arguments and their types.

I recommend a docblock here.

This seems like a common pattern. Should the Exception be added to _get_most_specific_driver() so this isn't repeated? If not, consider making a helper wrapper method that does this if/raise pattern for you so you don't have to repeat yourself.

Additionally, consider using a ValueError here instead of the generic Exception.

This could be a ValueError as well.

Adding a comment here explaining why we want to return here would be handy.

LGTM, everything I wrote is just suggestions. Sorry about the e-mailed one you probably got about deleting a key while iterating - I had misread the code. I deleted that comment here so it won't appear.

I am fully aware of that, but that's a whole project in itself.
We are currently using a mostly manual test framework because of test inter-dependencies and the fact that our test case is not your average one.
For one, we don't run the "system under test" inside our own process: we start an apache webserver to actually run it.

This is used quite heavily by the test suite in general.

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

Pull-Request has been closed by puiterwijk

7 years ago