From b842b825ab4511dc6d56dea860ddf7347f14b4bd Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Thu, 14 Oct 2021 17:07:32 -0400 Subject: [PATCH] Make the schema cache TTL user-configurable The API schema is not checked for changes until after a TTL is expired. A one-hour TTL was hardcoded which makes development tedious because the only way to force a schema update is to remember to remove files between invocations. This adds a new environment variable, schema_ttl, to configure the TTL returned by the server to schema() calls. This can be set low to ensure a frequent refresh during development. If the client is in compat mode, that is if client is working against a server that doesn't support the schema() command, then use the client's schema_ttl instead so that the user still has control. Re-check validity before writing the cache. This saves us both a disk write and the possibility of updating the expiration with a ttl of 0. This can happen if the fingerprint is still valid (not expired, no language change) the schema check is skipped so we have no server-provided ttl. https://pagure.io/freeipa/issue/8492 Signed-off-by: Rob Crittenden Reviewed-By: Stanislav Levin Reviewed-By: Alexander Bokovoy --- client/man/default.conf.5 | 3 ++ ipaclient/remote_plugins/__init__.py | 15 +++--- ipaclient/remote_plugins/compat.py | 5 +- ipaclient/remote_plugins/schema.py | 7 ++- ipalib/constants.py | 3 ++ ipaserver/plugins/schema.py | 8 +-- ipatests/test_cmdline/test_schema.py | 80 ++++++++++++++++++++++++++++ 7 files changed, 101 insertions(+), 20 deletions(-) create mode 100644 ipatests/test_cmdline/test_schema.py diff --git a/client/man/default.conf.5 b/client/man/default.conf.5 index 1fdceb122..4231d2a53 100644 --- a/client/man/default.conf.5 +++ b/client/man/default.conf.5 @@ -172,6 +172,9 @@ Specifies the Kerberos realm. .B replication_wait_timeout The time to wait for a new entry to be replicated during replica installation. The default value is 300 seconds. .TP +.B schema_ttl +The number of seconds for the ipa tool to cache the IPA API and help schema. Reducing this value during development is helpful so that API changes are seen sooner in the tool. Setting this on a server will define the TTL for all client versions > 4.3.1. Client versions > 4.3.1 that connect to IPA servers older than 4.3.1 will use the client-side configuration value. The default is 3600 seconds. 0 disables the cache. A change in the ttl will not be immediately recognized by clients. They will use the new value once their current cache expires. +.TP .B server Specifies the IPA Server hostname. .TP diff --git a/ipaclient/remote_plugins/__init__.py b/ipaclient/remote_plugins/__init__.py index fe75b81b8..a4901335a 100644 --- a/ipaclient/remote_plugins/__init__.py +++ b/ipaclient/remote_plugins/__init__.py @@ -34,6 +34,7 @@ class ServerInfo(MutableMapping): hostname = DNSName(api.env.server).ToASCII() self._path = os.path.join(self._DIR, hostname) self._force_check = api.env.force_schema_check + self._now = time.time() self._dict = {} # copy-paste from ipalib/rpc.py @@ -87,12 +88,11 @@ class ServerInfo(MutableMapping): def __len__(self): return len(self._dict) - def update_validity(self, ttl=None): - if ttl is None: - ttl = 3600 - self['expiration'] = time.time() + ttl - self['language'] = self._language - self._write() + def update_validity(self, ttl): + if not self.is_valid(): + self['expiration'] = self._now + ttl + self['language'] = self._language + self._write() def is_valid(self): if self._force_check: @@ -105,8 +105,7 @@ class ServerInfo(MutableMapping): # if any of these is missing consider the entry expired return False - if expiration < time.time(): - # validity passed + if expiration < self._now: return False if language != self._language: diff --git a/ipaclient/remote_plugins/compat.py b/ipaclient/remote_plugins/compat.py index 2a600fccc..351669a96 100644 --- a/ipaclient/remote_plugins/compat.py +++ b/ipaclient/remote_plugins/compat.py @@ -58,7 +58,10 @@ def get_package(server_info, client): else: server_version = '2.0' server_info['version'] = server_version - server_info.update_validity() + + # in compat mode we don't get the schema TTL from the server + # so use the client context value. + server_info.update_validity(client.api.env.schema_ttl) server_version = APIVersion(server_version) diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py index 7bf714371..db39d8be6 100644 --- a/ipaclient/remote_plugins/schema.py +++ b/ipaclient/remote_plugins/schema.py @@ -375,7 +375,7 @@ class Schema: namespaces = {'classes', 'commands', 'topics'} _DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'schema', FORMAT) - def __init__(self, client, fingerprint=None): + def __init__(self, client, fingerprint=None, ttl=0): self._dict = {} self._namespaces = {} self._help = None @@ -384,7 +384,6 @@ class Schema: self._dict[ns] = {} self._namespaces[ns] = _SchemaNameSpace(self, ns) - ttl = None read_failed = False if fingerprint is not None: @@ -554,10 +553,10 @@ def get_package(server_info, client): else: schema = Schema(client, fingerprint) except SchemaUpToDate as e: - schema = Schema(client, e.fingerprint) + schema = Schema(client, e.fingerprint, e.ttl) except NotAvailable: fingerprint = None - ttl = None + ttl = 3600 # set a ttl so we don't hammer the remote server except SchemaUpToDate as e: fingerprint = e.fingerprint ttl = e.ttl diff --git a/ipalib/constants.py b/ipalib/constants.py index fb5fd95ff..9f19b0f99 100644 --- a/ipalib/constants.py +++ b/ipalib/constants.py @@ -183,6 +183,9 @@ DEFAULT_CONFIG = ( # How long to wait for a certmonger request to finish ('certmonger_wait_timeout', 300), + # Number of seconds before client should check for schema update. + ('schema_ttl', 3600), + # Web Application mount points ('mount_ipa', '/ipa/'), diff --git a/ipaserver/plugins/schema.py b/ipaserver/plugins/schema.py index d56cf28e1..4dac82c4f 100644 --- a/ipaserver/plugins/schema.py +++ b/ipaserver/plugins/schema.py @@ -20,12 +20,6 @@ from ipalib.request import context from ipalib.text import _ from ipapython.version import API_VERSION -# Schema TTL sent to clients in response to schema call. -# Number of seconds before client should check for schema update. -# This should be long enough to not slow down regular work or skripts -# but also short enough to ensure schema will be retvieved soon after -# it was updated -SCHEMA_TTL = 3600 # default: 1 hour __doc__ = _(""" API Schema @@ -855,7 +849,7 @@ class schema(Command): schema = self._generate_schema(**kwargs) self.api._schema[langs] = schema - schema['ttl'] = SCHEMA_TTL + schema['ttl'] = self.api.env.schema_ttl if schema['fingerprint'] in kwargs.get('known_fingerprints', []): raise errors.SchemaUpToDate( diff --git a/ipatests/test_cmdline/test_schema.py b/ipatests/test_cmdline/test_schema.py new file mode 100644 index 000000000..26caa8aa1 --- /dev/null +++ b/ipatests/test_cmdline/test_schema.py @@ -0,0 +1,80 @@ +# +# Copyright (C) 2021 FreeIPA Contributors see COPYING for license +# +import pytest +import time + +from ipaclient.remote_plugins import ServerInfo + + +class TestServerInfo(ServerInfo): + """Simplified ServerInfo class with hardcoded values""" + def __init__(self, fingerprint='deadbeef', hostname='ipa.example.test', + force_check=False, language='en_US', + version='2.0', expiration=None): + self._force_check = force_check + self._language = language + self._now = time.time() + self._dict = { + 'fingerprint': fingerprint, + 'expiration': expiration or time.time() + 3600, + 'language': language, + 'version': version, + } + + def _read(self): + """Running on test controller, this is a no-op""" + + def _write(self): + """Running on test controller, this is a no-op""" + + +@pytest.mark.tier0 +class TestIPAServerInfo: + """Test that ServerInfo detects changes in remote configuration""" + + def test_valid(self): + server_info = TestServerInfo() + assert server_info.is_valid() is True + + def test_force_check(self): + server_info = TestServerInfo(force_check=True) + assert server_info.is_valid() is False + + def test_language_change(self): + server_info = TestServerInfo() + assert server_info.is_valid() is True + server_info._language = 'fr_FR' + assert server_info.is_valid() is False + server_info._language = 'en_US' + + def test_expired(self): + server_info = TestServerInfo(expiration=time.time() + 2) + assert server_info.is_valid() is True + + # skip past the expiration time + server_info._now = time.time() + 5 + assert server_info.is_valid() is False + + # set a new expiration time in the future + server_info.update_validity(10) + assert server_info.is_valid() is True + + # move to the future beyond expiration + server_info._now = time.time() + 15 + assert server_info.is_valid() is False + + def test_update_validity(self): + server_info = TestServerInfo(expiration=time.time() + 1) + + # Expiration and time are one second off so the cache is ok + assert server_info.is_valid() is True + + # Simulate time passing by + server_info._now = time.time() + 2 + + # the validity should be updated because it is now expired + server_info.update_validity(3600) + + # the cache is now valid for another hour + assert server_info.is_valid() is True