auth_service: Fetch revisions of all configs (for UI) at once.
Also, do it in parallel. This will simplify the switch to client-side Single
Page Application by removing the need for per-tab callback (info for all tabs
will be fetched at once).
R=nodir@chromium.org
BUG=714821
Review-Url: https://codereview.chromium.org/2840053003
diff --git a/appengine/auth_service/config.py b/appengine/auth_service/config.py
index d387f20..f07b352 100644
--- a/appengine/auth_service/config.py
+++ b/appengine/auth_service/config.py
@@ -34,7 +34,6 @@
from components.auth import ipaddr
from components.auth import model
from components.config import validation
-from components.config import validation_context
from proto import config_pb2
import importer
@@ -68,10 +67,17 @@
return None
-def get_config_revision(path):
+def get_revisions():
+ """Returns a mapping {config file name => Revision instance or None}."""
+ futures = {p: _get_config_revision_async(p) for p in _CONFIG_SCHEMAS}
+ return {p: f.get_result() for p, f in futures.iteritems()}
+
+
+def _get_config_revision_async(path):
"""Returns tuple with info about last imported config revision."""
+ assert path in _CONFIG_SCHEMAS, path
schema = _CONFIG_SCHEMAS.get(path)
- return schema['revision_getter']() if schema else None
+ return schema['revision_getter']()
@utils.cache_with_expiration(expiration_sec=60)
@@ -115,7 +121,7 @@
dirty_in_authdb = {}
for path, (new_rev, conf) in sorted(configs.iteritems()):
assert path in _CONFIG_SCHEMAS, path
- cur_rev = get_config_revision(path)
+ cur_rev = _get_config_revision_async(path).get_result()
if cur_rev != new_rev or force:
if _CONFIG_SCHEMAS[path]['use_authdb_transaction']:
dirty_in_authdb[path] = (new_rev, conf)
@@ -193,10 +199,11 @@
url = ndb.StringProperty(indexed=False)
-def _get_service_config_rev(cfg_name):
+@ndb.tasklet
+def _get_service_config_rev_async(cfg_name):
"""Returns last processed Revision of given config."""
- e = _AuthServiceConfig.get_by_id(cfg_name)
- return Revision(e.revision, e.url) if e else None
+ e = yield _AuthServiceConfig.get_by_id_async(cfg_name)
+ raise ndb.Return(Revision(e.revision, e.url) if e else None)
def _get_service_config(cfg_name):
@@ -222,13 +229,14 @@
### Group importer config implementation details.
-def _get_imports_config_revision():
+@ndb.tasklet
+def _get_imports_config_revision_async():
"""Returns Revision of last processed imports.cfg config."""
- e = importer.config_key().get()
+ e = yield importer.config_key().get_async()
if not e or not isinstance(e.config_revision, dict):
- return None
+ raise ndb.Return(None)
desc = e.config_revision
- return Revision(desc.get('rev'), desc.get('url'))
+ raise ndb.Return(Revision(desc.get('rev'), desc.get('url')))
def _update_imports_config(rev, conf):
@@ -255,15 +263,16 @@
return ndb.Key(_ImportedConfigRevisions, 'self', parent=model.root_key())
-def _get_authdb_config_rev(path):
+@ndb.tasklet
+def _get_authdb_config_rev_async(path):
"""Returns Revision of last processed config given its name."""
- mapping = _imported_config_revisions_key().get()
+ mapping = yield _imported_config_revisions_key().get_async()
if not mapping or not isinstance(mapping.revisions, dict):
- return None
+ raise ndb.Return(None)
desc = mapping.revisions.get(path)
if not isinstance(desc, dict):
- return None
- return Revision(desc.get('rev'), desc.get('url'))
+ raise ndb.Return(None)
+ raise ndb.Return(Revision(desc.get('rev'), desc.get('url')))
@datastore_utils.transactional
@@ -470,7 +479,7 @@
# Config file name -> {
# 'proto_class': protobuf class of the config or None to keep it as text,
-# 'revision_getter': lambda: <latest imported Revision>,
+# 'revision_getter': lambda: ndb.Future with <latest imported Revision>
# 'validator': lambda config: <raises ValueError on invalid format>
# 'updater': lambda rev, config: True if applied, False if not.
# 'use_authdb_transaction': True to call 'updater' in AuthDB transaction.
@@ -479,26 +488,26 @@
_CONFIG_SCHEMAS = {
'imports.cfg': {
'proto_class': None, # importer configs are stored as text
- 'revision_getter': _get_imports_config_revision,
+ 'revision_getter': _get_imports_config_revision_async,
'updater': _update_imports_config,
'use_authdb_transaction': False,
},
'ip_whitelist.cfg': {
'proto_class': config_pb2.IPWhitelistConfig,
- 'revision_getter': lambda: _get_authdb_config_rev('ip_whitelist.cfg'),
+ 'revision_getter': lambda: _get_authdb_config_rev_async('ip_whitelist.cfg'),
'updater': _update_ip_whitelist_config,
'use_authdb_transaction': True,
},
'oauth.cfg': {
'proto_class': config_pb2.OAuthConfig,
- 'revision_getter': lambda: _get_authdb_config_rev('oauth.cfg'),
+ 'revision_getter': lambda: _get_authdb_config_rev_async('oauth.cfg'),
'updater': _update_oauth_config,
'use_authdb_transaction': True,
},
'settings.cfg': {
'proto_class': None, # settings are stored as text in datastore
'default': '', # it's fine if config file is not there
- 'revision_getter': lambda: _get_service_config_rev('settings.cfg'),
+ 'revision_getter': lambda: _get_service_config_rev_async('settings.cfg'),
'updater': lambda rev, c: _update_service_config('settings.cfg', rev, c),
'use_authdb_transaction': False,
},
diff --git a/appengine/auth_service/config_test.py b/appengine/auth_service/config_test.py
index 0a4eefb..368fdb1 100755
--- a/appengine/auth_service/config_test.py
+++ b/appengine/auth_service/config_test.py
@@ -16,7 +16,6 @@
from components import config as config_component
from components import utils
from components.auth import model
-from components.config import validation_context
from test_support import test_case
from proto import config_pb2
@@ -28,25 +27,30 @@
super(ConfigTest, self).setUp()
def test_refetch_config(self):
- # Mock of "current known revision" state.
- revs = {
+ initial_revs = {
'a.cfg': config.Revision('old_a_rev', 'urla'),
'b.cfg': config.Revision('old_b_rev', 'urlb'),
'c.cfg': config.Revision('old_c_rev', 'urlc'),
}
+ revs = initial_revs.copy()
bumps = []
+
def bump_rev(pkg, rev, conf):
revs[pkg] = rev
bumps.append((pkg, rev, conf, ndb.in_transaction()))
return True
+ @ndb.tasklet
+ def get_rev_async(pkg):
+ raise ndb.Return(revs[pkg])
+
self.mock(config, 'is_remote_configured', lambda: True)
self.mock(config, '_CONFIG_SCHEMAS', {
# Will be updated outside of auth db transaction.
'a.cfg': {
'proto_class': None,
- 'revision_getter': lambda: revs['a.cfg'],
+ 'revision_getter': lambda: get_rev_async('a.cfg'),
'validator': lambda body: self.assertEqual(body, 'new a body'),
'updater': lambda rev, conf: bump_rev('a.cfg', rev, conf),
'use_authdb_transaction': False,
@@ -54,7 +58,7 @@
# Will not be changed.
'b.cfg': {
'proto_class': None,
- 'revision_getter': lambda: revs['b.cfg'],
+ 'revision_getter': lambda: get_rev_async('b.cfg'),
'validator': lambda _body: True,
'updater': lambda rev, conf: bump_rev('b.cfg', rev, conf),
'use_authdb_transaction': False,
@@ -62,17 +66,23 @@
# Will be updated inside auth db transaction.
'c.cfg': {
'proto_class': None,
- 'revision_getter': lambda: revs['c.cfg'],
+ 'revision_getter': lambda: get_rev_async('c.cfg'),
'validator': lambda body: self.assertEqual(body, 'new c body'),
'updater': lambda rev, conf: bump_rev('c.cfg', rev, conf),
'use_authdb_transaction': True,
},
})
- self.mock(config, '_fetch_configs', lambda _: {
+
+ # _fetch_configs is called by config.refetch_config().
+ configs_to_fetch = {
'a.cfg': (config.Revision('new_a_rev', 'urla'), 'new a body'),
'b.cfg': (config.Revision('old_b_rev', 'urlb'), 'old b body'),
'c.cfg': (config.Revision('new_c_rev', 'urlc'), 'new c body'),
- })
+ }
+ self.mock(config, '_fetch_configs', lambda _: configs_to_fetch)
+
+ # Old revisions initially.
+ self.assertEqual(initial_revs, config.get_revisions())
# Initial update.
config.refetch_config()
@@ -82,6 +92,11 @@
], bumps)
del bumps[:]
+ # Updated revisions now.
+ self.assertEqual(
+ {k: v[0] for k, v in configs_to_fetch.iteritems()},
+ config.get_revisions())
+
# Refetch, nothing new.
config.refetch_config()
self.assertFalse(bumps)
@@ -90,7 +105,8 @@
new_rev = config.Revision('rev', 'url')
body = 'tarball{url:"a" systems:"b"}'
self.assertTrue(config._update_imports_config(new_rev, body))
- self.assertEqual(new_rev, config._get_imports_config_revision())
+ self.assertEqual(
+ new_rev, config._get_imports_config_revision_async().get_result())
def test_validate_ip_whitelist_config_ok(self):
conf = config_pb2.IPWhitelistConfig(
@@ -598,16 +614,19 @@
def test_update_service_config(self):
# Missing.
self.assertIsNone(config._get_service_config('abc.cfg'))
- self.assertIsNone(config._get_service_config_rev('abc.cfg'))
+ self.assertIsNone(
+ config._get_service_config_rev_async('abc.cfg').get_result())
# Updated.
rev = config.Revision('rev', 'url')
self.assertTrue(config._update_service_config('abc.cfg', rev, 'body'))
self.assertEqual('body', config._get_service_config('abc.cfg'))
- self.assertEqual(rev, config._get_service_config_rev('abc.cfg'))
+ self.assertEqual(
+ rev, config._get_service_config_rev_async('abc.cfg').get_result())
# Same body, returns False, though updates rev.
rev2 = config.Revision('rev2', 'url')
self.assertFalse(config._update_service_config('abc.cfg', rev2, 'body'))
- self.assertEqual(rev2, config._get_service_config_rev('abc.cfg'))
+ self.assertEqual(
+ rev2, config._get_service_config_rev_async('abc.cfg').get_result())
def test_settings_updates(self):
# Fetch only settings.cfg in this test case.
diff --git a/appengine/auth_service/handlers_frontend.py b/appengine/auth_service/handlers_frontend.py
index ef87bca..f6f3fd1 100644
--- a/appengine/auth_service/handlers_frontend.py
+++ b/appengine/auth_service/handlers_frontend.py
@@ -70,23 +70,22 @@
template_file = 'auth_service/services.html'
-def get_additional_ui_environment(handler):
+def get_additional_ui_data():
"""Gets injected into Jinja and Javascript environment."""
- # See config._CONFIG_SCHEMAS for where these paths are defined.
- if isinstance(handler, ConfigHandler):
- path = 'imports.cfg'
- elif isinstance(handler, ui.IPWhitelistsHandler):
- path = 'ip_whitelist.cfg'
- elif isinstance(handler, ui.OAuthConfigHandler):
- path = 'oauth.cfg'
- else:
- return {'auth_service_config_locked': config.is_remote_configured()}
- rev = config.get_config_revision(path)
+ if not config.is_remote_configured():
+ return {'auth_service_config_locked': False}
+ config_revisions = {}
+ for path, rev in config.get_revisions().iteritems():
+ config_revisions[path] = {
+ 'rev': rev.revision if rev else 'none',
+ 'url': rev.url if rev else 'about:blank',
+ }
return {
- 'auth_service_config_locked': config.is_remote_configured(),
- 'auth_service_config_remote_url': config.get_remote_url(),
- 'auth_service_config_rev': rev.revision if rev else 'none',
- 'auth_service_config_url': rev.url if rev else 'about:blank',
+ 'auth_service_config_locked': True,
+ 'auth_service_configs': {
+ 'remote_url': config.get_remote_url(),
+ 'revisions': config_revisions,
+ },
}
@@ -440,7 +439,7 @@
ConfigHandler,
ui.ApiDocHandler,
],
- env_callback=get_additional_ui_environment)
+ ui_data_callback=get_additional_ui_data)
template.bootstrap({'auth_service': TEMPLATES_DIR})
# Add a fake admin for local dev server.
diff --git a/appengine/auth_service/templates/config.html b/appengine/auth_service/templates/config.html
index 9d4a101..1b7def5 100644
--- a/appengine/auth_service/templates/config.html
+++ b/appengine/auth_service/templates/config.html
@@ -21,7 +21,7 @@
</div>
<div class="form-group">
{% if auth_service_config_locked %}
- {{ render_config_locked() }}
+ {{ render_config_locked("imports.cfg") }}
{% else %}
{% if is_admin %}
<button type="submit" class="btn btn-primary">Save</button>
diff --git a/appengine/components/components/auth/ui/templates/base.html b/appengine/components/components/auth/ui/templates/base.html
index b5407fa..f77a5ea 100644
--- a/appengine/components/components/auth/ui/templates/base.html
+++ b/appengine/components/components/auth/ui/templates/base.html
@@ -46,13 +46,14 @@
{% endraw %}
{# Used on auth_service if config_service is managing configs #}
-{% macro render_config_locked() -%}
+{% macro render_config_locked(config_path) -%}
+{% set rev = auth_service_configs['revisions'][config_path] %}
<hr>
<p>
The configuration is distributed by
-<a target="_blank" href="{{auth_service_config_remote_url}}">luci-config</a>.
-Using revision <a target="_blank" href="{{auth_service_config_url}}"
-style="font-family:monospace;">{{auth_service_config_rev}}</a>.
+<a target="_blank" href="{{auth_service_configs['remote_url']}}">luci-config</a>.
+Using revision <a target="_blank" href="{{rev['url']}}"
+style="font-family:monospace;">{{rev['rev']}}</a>.
</p>
{%- endmacro %}
diff --git a/appengine/components/components/auth/ui/templates/ip_whitelists.html b/appengine/components/components/auth/ui/templates/ip_whitelists.html
index ab9ad63..9ad5f70 100644
--- a/appengine/components/components/auth/ui/templates/ip_whitelists.html
+++ b/appengine/components/components/auth/ui/templates/ip_whitelists.html
@@ -47,7 +47,7 @@
</div>
{% if auth_service_config_locked %}
<div class="form-group">
- {{ render_config_locked() }}
+ {{ render_config_locked("ip_whitelist.cfg") }}
</div>
{% endif %}
</form>
diff --git a/appengine/components/components/auth/ui/templates/oauth_config.html b/appengine/components/components/auth/ui/templates/oauth_config.html
index 260ffb1..536b1b0 100644
--- a/appengine/components/components/auth/ui/templates/oauth_config.html
+++ b/appengine/components/components/auth/ui/templates/oauth_config.html
@@ -90,7 +90,7 @@
<div class="form-group">
{% if auth_service_config_locked %}
<div class="col-sm-12">
- {{ render_config_locked() }}
+ {{ render_config_locked("oauth.cfg") }}
</div>
{% else %}
{% if is_admin %}
diff --git a/appengine/components/components/auth/ui/ui.py b/appengine/components/components/auth/ui/ui.py
index c0b3956..39a2b08 100644
--- a/appengine/components/components/auth/ui/ui.py
+++ b/appengine/components/components/auth/ui/ui.py
@@ -32,26 +32,26 @@
# Global static configuration set in 'configure_ui'.
_ui_app_name = 'Unknown'
-_ui_env_callback = None
+_ui_data_callback = None
_ui_navbar_tabs = ()
-def configure_ui(app_name, ui_tabs=None, env_callback=None):
+def configure_ui(app_name, ui_tabs=None, ui_data_callback=None):
"""Modifies global configuration of Auth UI.
Args:
app_name: name of the service (visible in page headers, titles, etc.)
ui_tabs: list of UINavbarTabHandler subclasses that define tabs to show, or
None to show the standard set of tabs.
- env_callback: callable that returns a dict with additional environment for
- templates. Will be called each time a template is rendered with single
- argument: UIHandler instance. Used by auth_service.
+ ui_data_callback: an argumentless callable that returns a dict with
+ additional data to return to authenticated users. It can be used by
+ server and client side code to render templates. Used by auth_service.
"""
global _ui_app_name
- global _ui_env_callback
+ global _ui_data_callback
global _ui_navbar_tabs
_ui_app_name = app_name
- _ui_env_callback = env_callback
+ _ui_data_callback = ui_data_callback
if ui_tabs is not None:
assert all(issubclass(cls, UINavbarTabHandler) for cls in ui_tabs)
_ui_navbar_tabs = tuple(ui_tabs)
@@ -339,8 +339,8 @@
'using_gae_auth': self.auth_method == handler.gae_cookie_authentication,
'xsrf_token': self.generate_xsrf_token(),
}
- if _ui_env_callback:
- common.update(_ui_env_callback(self))
+ if _ui_data_callback:
+ common.update(_ui_data_callback())
# Name of Javascript module with page code.
js_module_name = None
