Merge lp:~mwhudson/launchpad/feature-flag-xmlrpc-2 into lp:launchpad

[Michael Hudson-Doyle]

[r=lifeless][bug=853635] Add a private XML-RPC interface to query feature flags

This relands my branch https://code.launchpad.net/~mwhudson/launchpad/feature-flag-xmlrpc/+merge/75673 which broke because feature controllers are installed before authentication happens, so looking at request.principal at that time doesn't work.

The change that fixes the problem is here: http://bazaar.launchpad.net/~mwhudson/launchpad/feature-flag-xmlrpc-2/revision/14013

I haven't added an integration level test, because it would be a lot of work I think (you could register a view with a template that accesses a feature flag and then use testbrowser to load it) and I'm not sure it would have caught this problem anyway (are you really sure that the way testbrowser does authentication is that similar to how it happens in a real appserver? maybe it is, but I don't know). I can add one if the reviewer thinks it's a good idea though.

[Robert Collins (lifeless)]

Thanks.

review: Approve

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2011-09-24 03:03:08 +0000
+++ configs/development/launchpad-lazr.conf	2011-09-28 03:34:29 +0000
@@ -158,6 +158,7 @@
 geonames_identity: lpdev
 storm_cache: generational
 storm_cache_size: 100
+feature_flags_endpoint: http://xmlrpc-private.launchpad.dev:8087/featureflags/
 
 [launchpad_session]
 cookie: launchpad_dev

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-09-24 02:40:51 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-09-28 03:34:29 +0000
@@ -1211,6 +1211,12 @@
 # The number of items to display:
 homepage_recent_posts_count: 6
 
+# The URL of the XML-RPC endpoint that allows querying of feature
+# flags. This should implement IFeatureFlagApplication.
+#
+# datatype: string
+feature_flags_endpoint:
+
 [launchpad_session]
 # The database connection string.
 # datatype: pgconnection

=== modified file 'lib/canonical/launchpad/interfaces/launchpad.py'
--- lib/canonical/launchpad/interfaces/launchpad.py	2011-09-21 14:38:47 +0000
+++ lib/canonical/launchpad/interfaces/launchpad.py	2011-09-28 03:34:29 +0000
@@ -152,6 +152,8 @@
     softwarecenteragent = Attribute(
         """Software center agent XML-RPC end point.""")
 
+    featureflags = Attribute("""Feature flag information endpoint""")
+
 
 class IAuthServerApplication(ILaunchpadApplication):
     """Launchpad legacy AuthServer application root."""

=== modified file 'lib/canonical/launchpad/xmlrpc/application.py'
--- lib/canonical/launchpad/xmlrpc/application.py	2011-09-21 14:38:47 +0000
+++ lib/canonical/launchpad/xmlrpc/application.py	2011-09-28 03:34:29 +0000
@@ -36,6 +36,7 @@
     )
 from lp.registry.interfaces.mailinglist import IMailingListApplication
 from lp.registry.interfaces.person import ISoftwareCenterAgentApplication
+from lp.services.features.xmlrpc import IFeatureFlagApplication
 
 
 # NOTE: If you add a traversal here, you should update
@@ -73,6 +74,11 @@
         """See `IPrivateApplication`."""
         return getUtility(ISoftwareCenterAgentApplication)
 
+    @property
+    def featureflags(self):
+        """See `IPrivateApplication`."""
+        return getUtility(IFeatureFlagApplication)
+
 
 class ISelfTest(Interface):
     """XMLRPC external interface for testing the XMLRPC external interface."""
@@ -127,4 +133,3 @@
 
     def run_test(self):
         return "OK"
-

=== modified file 'lib/lp/services/features/__init__.py'
--- lib/lp/services/features/__init__.py	2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/__init__.py	2011-09-28 03:34:29 +0000
@@ -125,6 +125,18 @@
      if value:
         print value
 
+Checking flags without access to the database
+=============================================
+
+Feature flags can also be checked without access to the database by making use
+of the 'getFeatureFlag' XML-RPC method.
+
+    server_proxy = xmlrpclib.ServerProxy(
+        config.launchpad.feature_flags_endpoint, allow_none=True)
+    if server_proxy.getFeatureFlag(
+        'codehosting.use_forking_server', ['user:' + user_name]):
+        pass
+
 Debugging feature usage
 =======================
 

=== modified file 'lib/lp/services/features/configure.zcml'
--- lib/lp/services/features/configure.zcml	2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/configure.zcml	2011-09-28 03:34:29 +0000
@@ -19,4 +19,11 @@
         handler="lp.services.features.webapp.end_request"
         />
 
+    <securedutility
+        class="lp.services.features.xmlrpc.FeatureFlagApplication"
+        provides="lp.services.features.xmlrpc.IFeatureFlagApplication">
+        <allow
+            interface="lp.services.features.xmlrpc.IFeatureFlagApplication"/>
+    </securedutility>
+
 </configure>

=== modified file 'lib/lp/services/features/scopes.py'
--- lib/lp/services/features/scopes.py	2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/scopes.py	2011-09-28 03:34:29 +0000
@@ -9,9 +9,14 @@
 """
 
 __all__ = [
+    'DefaultScope',
+    'default_scopes',
+    'FixedScope',
     'HANDLERS',
+    'MultiScopeHandler',
     'ScopesForScript',
     'ScopesFromRequest',
+    'TeamScope',
     'undocumented_scopes',
     ]
 
@@ -19,9 +24,7 @@
 
 import re
 
-from zope.component import getUtility
-
-from canonical.launchpad.webapp.interfaces import ILaunchBag
+from lp.registry.interfaces.person import IPerson
 from lp.services.propertycache import cachedproperty
 import canonical.config
 
@@ -60,14 +63,7 @@
         return True
 
 
-class BaseWebRequestScope(BaseScope):
-    """Base class for scopes that key off web request attributes."""
-
-    def __init__(self, request):
-        self.request = request
-
-
-class PageScope(BaseWebRequestScope):
+class PageScope(BaseScope):
     """The current page ID.
 
     Pageid scopes are written as 'pageid:' + the pageid to match.  Pageids
@@ -81,6 +77,9 @@
 
     pattern = r'pageid:'
 
+    def __init__(self, request):
+        self._request = request
+
     def lookup(self, scope_name):
         """Is the given scope match the current pageid?"""
         pageid_scope = scope_name[len('pageid:'):]
@@ -105,20 +104,32 @@
     @cachedproperty
     def _request_pageid_namespace(self):
         return tuple(self._pageid_to_namespace(
-            self.request._orig_env.get('launchpad.pageid', '')))
+            self._request._orig_env.get('launchpad.pageid', '')))
 
 
 class TeamScope(BaseScope):
-    """The current user's team memberships.
+    """A user's team memberships.
 
     Team ID scopes are written as 'team:' + the team name to match.
 
     The scope 'team:launchpad-beta-users' will match members of the team
     'launchpad-beta-users'.
+
+    The constructor takes a callable that returns the currently logged in
+    person because Scopes are set up very early in the request publication
+    process -- in particular, before authentication has happened.
     """
 
     pattern = r'team:'
 
+    def __init__(self, get_person):
+        self._get_person = get_person
+        self._person = None
+
+    @cachedproperty
+    def person(self):
+        return self._get_person()
+
     def lookup(self, scope_name):
         """Is the given scope a team membership?
 
@@ -126,11 +137,9 @@
         team based scopes in use to a small number. (Person.inTeam could be
         fixed to reduce this to one query).
         """
-        team_name = scope_name[len('team:'):]
-        person = getUtility(ILaunchBag).user
-        if person is None:
-            return False
-        return person.inTeam(team_name)
+        if self.person is not None:
+            team_name = scope_name[len('team:'):]
+            return self.person.inTeam(team_name)
 
 
 class ServerScope(BaseScope):
@@ -169,6 +178,20 @@
         return scope_name == self.script_scope
 
 
+class FixedScope(BaseScope):
+    """A scope that matches an exact value.
+
+    Functionally `ScriptScope` and `DefaultScope` are equivalent to instances
+    of this class, but their docstings are used on the +feature-info page.
+    """
+
+    def __init__(self, scope):
+        self.pattern = re.escape(scope) + '$'
+
+    def lookup(self, scope_name):
+        return True
+
+
 # These are the handlers for all of the allowable scopes, listed here so that
 # we can for example show all of them in an admin page.  Any new scope will
 # need a scope handler and that scope handler has to be added to this list.
@@ -213,21 +236,33 @@
             undocumented_scopes.add(scope_name)
 
 
+default_scopes = (DefaultScope(),)
+
+
 class ScopesFromRequest(MultiScopeHandler):
-    """Identify feature scopes based on request state."""
+    """Identify feature scopes based on request state.
+
+    Because the feature controller is constructed very very early in the
+    publication process, this needs to be very careful about looking at the
+    request -- in particular, this is called before authentication happens.
+    """
 
     def __init__(self, request):
-        super(ScopesFromRequest, self).__init__([
-            DefaultScope(),
+        def person_from_request():
+            return IPerson(request.principal, None)
+        scopes = list(default_scopes)
+        scopes.extend([
             PageScope(request),
-            TeamScope(),
-            ServerScope()])
+            ServerScope(),
+            TeamScope(person_from_request)
+            ])
+        super(ScopesFromRequest, self).__init__(scopes)
 
 
 class ScopesForScript(MultiScopeHandler):
     """Identify feature scopes for a given script."""
 
     def __init__(self, script_name):
-        super(ScopesForScript, self).__init__([
-            DefaultScope(),
-            ScriptScope(script_name)])
+        scopes = list(default_scopes)
+        scopes.append(ScriptScope(script_name))
+        super(ScopesForScript, self).__init__(scopes)

=== modified file 'lib/lp/services/features/tests/test_scopes.py'
--- lib/lp/services/features/tests/test_scopes.py	2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/tests/test_scopes.py	2011-09-28 03:34:29 +0000
@@ -9,18 +9,20 @@
 from lp.testing import TestCaseWithFactory
 from lp.services.features.scopes import (
     BaseScope,
-    BaseWebRequestScope,
     MultiScopeHandler,
     ScopesForScript,
     ScriptScope,
     )
 
 
-class FakeScope(BaseWebRequestScope):
+class FakeScope(BaseScope):
     pattern = r'fake:'
 
+    def __init__(self, name):
+        self.name = name
+
     def lookup(self, scope_name):
-        return scope_name == (self.pattern + self.request)
+        return scope_name == (self.pattern + self.name)
 
 
 class TestScopes(TestCaseWithFactory):

=== modified file 'lib/lp/services/features/tests/test_webapp.py'
--- lib/lp/services/features/tests/test_webapp.py	2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/tests/test_webapp.py	2011-09-28 03:34:29 +0000
@@ -24,8 +24,8 @@
 
     def test_pageid_scope_normal(self):
         request = LaunchpadTestRequest()
+        scopes = webapp.ScopesFromRequest(request)
         request.setInWSGIEnvironment('launchpad.pageid', 'foo:bar')
-        scopes = webapp.ScopesFromRequest(request)
         self.assertTrue(scopes.lookup('pageid:'))
         self.assertTrue(scopes.lookup('pageid:foo'))
         self.assertTrue(scopes.lookup('pageid:foo:bar'))
@@ -33,8 +33,8 @@
 
     def test_pageid_scope_collection(self):
         request = LaunchpadTestRequest()
+        scopes = webapp.ScopesFromRequest(request)
         request.setInWSGIEnvironment('launchpad.pageid', 'scoped:thing:#type')
-        scopes = webapp.ScopesFromRequest(request)
         self.assertTrue(scopes.lookup('pageid:'))
         self.assertTrue(scopes.lookup('pageid:scoped'))
         self.assertTrue(scopes.lookup('pageid:scoped:thing'))
@@ -43,8 +43,8 @@
 
     def test_pageid_scope_empty(self):
         request = LaunchpadTestRequest()
+        scopes = webapp.ScopesFromRequest(request)
         request.setInWSGIEnvironment('launchpad.pageid', '')
-        scopes = webapp.ScopesFromRequest(request)
         self.assertTrue(scopes.lookup('pageid:'))
         self.assertFalse(scopes.lookup('pageid:foo'))
         self.assertFalse(scopes.lookup('pageid:foo:bar'))
@@ -87,7 +87,7 @@
     def test_team_scope_outside_team(self):
         request = LaunchpadTestRequest()
         scopes = webapp.ScopesFromRequest(request)
-        self.factory.loginAsAnyone()
+        self.factory.loginAsAnyone(request)
         self.assertFalse(scopes.lookup('team:nonexistent'))
 
     def test_team_scope_in_team(self):
@@ -95,5 +95,5 @@
         scopes = webapp.ScopesFromRequest(request)
         member = self.factory.makePerson()
         team = self.factory.makeTeam(members=[member])
-        login_as(member)
+        login_as(member, request)
         self.assertTrue(scopes.lookup('team:%s' % team.name))

=== added file 'lib/lp/services/features/tests/test_xmlrpc.py'
--- lib/lp/services/features/tests/test_xmlrpc.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/tests/test_xmlrpc.py	2011-09-28 03:34:29 +0000
@@ -0,0 +1,99 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for FeatureFlagApplication."""
+
+__metaclass__ = type
+
+import xmlrpclib
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.config import config
+from lp.services import features
+from lp.services.features.flags import FeatureController
+from lp.services.features.rulesource import StormFeatureRuleSource
+from lp.services.features.scopes import (
+    DefaultScope,
+    FixedScope,
+    MultiScopeHandler,
+    )
+from lp.services.features.xmlrpc import FeatureFlagApplication
+from lp.testing import (
+    feature_flags,
+    set_feature_flag,
+    TestCaseWithFactory,
+    )
+from lp.testing.xmlrpc import XMLRPCTestTransport
+
+
+class TestGetFeatureFlag(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.endpoint = FeatureFlagApplication()
+
+    def installFeatureController(self, feature_controller):
+        old_features = features.get_relevant_feature_controller()
+        features.install_feature_controller(feature_controller)
+        self.addCleanup(
+            features.install_feature_controller, old_features)
+
+    def test_getFeatureFlag_returns_None_by_default(self):
+        self.assertIs(None, self.endpoint.getFeatureFlag(u'unknown'))
+
+    def test_getFeatureFlag_returns_true_for_set_flag(self):
+        flag_name = u'flag'
+        with feature_flags():
+            set_feature_flag(flag_name, u'1')
+            self.assertEqual(u'1', self.endpoint.getFeatureFlag(flag_name))
+
+    def test_getFeatureFlag_ignores_relevant_feature_controller(self):
+        # getFeatureFlag should only consider the scopes it is asked to
+        # consider, not any that happen to be active due to the XML-RPC
+        # request itself.
+        flag_name = u'flag'
+        scope_name = u'scope'
+        self.installFeatureController(
+            FeatureController(
+                MultiScopeHandler(
+                    [DefaultScope(), FixedScope(scope_name)]).lookup,
+                StormFeatureRuleSource()))
+        set_feature_flag(flag_name, u'1', scope_name)
+        self.assertEqual(None, self.endpoint.getFeatureFlag(flag_name))
+
+    def test_getFeatureFlag_considers_supplied_scope(self):
+        flag_name = u'flag'
+        scope_name = u'scope'
+        with feature_flags():
+            set_feature_flag(flag_name, u'value', scope_name)
+            self.assertEqual(
+                u'value',
+                self.endpoint.getFeatureFlag(flag_name, [scope_name]))
+
+    def test_getFeatureFlag_turns_user_into_team_scope(self):
+        flag_name = u'flag'
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[person])
+        with feature_flags():
+            set_feature_flag(flag_name, u'value', u'team:' + team.name)
+            self.assertEqual(
+                u'value',
+                self.endpoint.getFeatureFlag(
+                    flag_name, ['user:' + person.name]))
+
+    def test_xmlrpc_interface_unset(self):
+        sp = xmlrpclib.ServerProxy(
+            config.launchpad.feature_flags_endpoint,
+            transport=XMLRPCTestTransport(), allow_none=True)
+        self.assertEqual(None, sp.getFeatureFlag(u'flag'))
+
+    def test_xmlrpc_interface_set(self):
+        sp = xmlrpclib.ServerProxy(
+            config.launchpad.feature_flags_endpoint,
+            transport=XMLRPCTestTransport(), allow_none=True)
+        flag_name = u'flag'
+        with feature_flags():
+            set_feature_flag(flag_name, u'1')
+            self.assertEqual(u'1', sp.getFeatureFlag(flag_name))

=== added file 'lib/lp/services/features/xmlrpc.py'
--- lib/lp/services/features/xmlrpc.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/xmlrpc.py	2011-09-28 03:34:29 +0000
@@ -0,0 +1,59 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""FeatureFlagApplication allows access to information about feature flags."""
+
+__metaclass__ = type
+__all__ = [
+    'IFeatureFlagApplication',
+    'FeatureFlagApplication',
+    ]
+
+from zope.component import getUtility
+from zope.interface import implements
+
+from canonical.launchpad.webapp.interfaces import ILaunchpadApplication
+from lp.registry.interfaces.person import IPersonSet
+from lp.services.features.flags import FeatureController
+from lp.services.features.rulesource import StormFeatureRuleSource
+from lp.services.features.scopes import (
+    default_scopes,
+    FixedScope,
+    MultiScopeHandler,
+    TeamScope,
+    )
+
+
+class IFeatureFlagApplication(ILaunchpadApplication):
+    """Mailing lists application root."""
+
+    def getFeatureFlag(flag_name, username=None, scopes=()):
+        """Return the value of the given feature flag.
+
+        :param flag_name: The name of the flag to query.
+        :param username: If supplied, the name of a Person to use in
+            evaluating the 'team:' scope.
+        :param scopes: A list of scopes to consider active.  The 'default'
+            scope is always considered to be active, and does not need to be
+            included here.
+        """
+
+
+class FeatureFlagApplication:
+
+    implements(IFeatureFlagApplication)
+
+    def getFeatureFlag(self, flag_name, active_scopes=()):
+        scopes = list(default_scopes)
+        for scope_name in active_scopes:
+            if scope_name.startswith('user:'):
+                person = getUtility(IPersonSet).getByName(
+                    scope_name[len('user:'):])
+                if person is not None:
+                    scopes.append(TeamScope(lambda: person))
+            else:
+                scopes.append(FixedScope(scope_name))
+        flag_name = unicode(flag_name)
+        controller = FeatureController(
+            MultiScopeHandler(scopes).lookup, StormFeatureRuleSource())
+        return controller.getFlag(flag_name)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-09-23 13:14:36 +0000
+++ lib/lp/testing/factory.py	2011-09-28 03:34:29 +0000
@@ -505,7 +505,7 @@
     for any other required objects.
     """
 
-    def loginAsAnyone(self):
+    def loginAsAnyone(self, participation=None):
         """Log in as an arbitrary person.
 
         If you want to log in as a celebrity, including admins, see
@@ -513,7 +513,7 @@
         """
         login(ANONYMOUS)
         person = self.makePerson()
-        login_as(person)
+        login_as(person, participation)
         return person
 
     @with_celebrity_logged_in('admin')

=== modified file 'utilities/page-performance-report.ini'
--- utilities/page-performance-report.ini	2011-09-21 14:38:47 +0000
+++ utilities/page-performance-report.ini	2011-09-28 03:34:29 +0000
@@ -11,7 +11,8 @@
     [^/]+($|/
      (?!\+haproxy|\+opstats|\+access-token
       |((authserver|bugs|bazaar|codehosting|
-         codeimportscheduler|mailinglists|softwarecenteragent)/\w+$)))
+         codeimportscheduler|mailinglists|softwarecenteragent|
+         featureflags)/\w+$)))
 Other=^/
 
 Launchpad Frontpage=^https?://launchpad\.[^/]+(/index\.html)?$
@@ -54,7 +55,7 @@
 Private XML-RPC=^https://(launchpad|xmlrpc)[^/]+/
     (authserver|bugs|codehosting|
      codeimportscheduler|mailinglists|
-     softwarecenteragent)/\w+$
+     softwarecenteragent|featureflags)/\w+$
 
 [metrics]
 ppr_all=All Launchpad except operational pages