Allow for more robust config checking with keystone-manage

Keystone was logging a warning saying "no configuration file was
found" when one was clearly passed to keystone-manage. This was
due to the fact that keystone-manage was really just checking for
default configuration file values. This means that users passing
a specific configuration file location to keystone-manage would
see a warning that wasn't necessarily true since keystone-manage
passes sys.argv to oslo.config, which resolves the argument
properly.

This change enhances the check to make sure keystone-manage isn't
dealing with a configuration file passed in from an end-user before
emitting the warning.

This change also cleans up some of the interactions between
keystone.cmd.manage and keystone.cmd.cli to make the variables
more clear about their purpose.

Change-Id: I6d58d9c499c447ef1cfd6edd4aeb1176130fb6ad
Closes-Bug: 1782704
This commit is contained in:
Lance Bragstad 2018-08-06 23:57:00 +00:00
parent 09975532c0
commit 254100128d
4 changed files with 53 additions and 14 deletions

View File

@ -1173,19 +1173,44 @@ command_opt = cfg.SubCommandOpt('command',
handler=add_command_parsers)
def main(argv=None, config_files=None):
def main(argv=None, developer_config_file=None):
"""Main entry point into the keystone-manage CLI utility.
:param argv: Arguments supplied via the command line using the ``sys``
standard library.
:type argv: list
:param developer_config_file: The location of a configuration file normally
found in development environments.
:type developer_config_file: string
"""
CONF.register_cli_opt(command_opt)
keystone.conf.configure()
sql.initialize()
keystone.conf.set_default_for_default_log_levels()
user_supplied_config_file = False
if argv:
for argument in argv:
if argument == '--config-file':
user_supplied_config_file = True
if developer_config_file:
developer_config_file = [developer_config_file]
# NOTE(lbragstad): At this point in processing, the first element of argv
# is the binary location of keystone-manage, which oslo.config doesn't need
# and is keystone specific. Only pass a list of arguments so that
# oslo.config can determine configuration file locations based on user
# provided arguments, if present.
CONF(args=argv[1:],
project='keystone',
version=pbr.version.VersionInfo('keystone').version_string(),
usage='%(prog)s [' + '|'.join([cmd.name for cmd in CMDS]) + ']',
default_config_files=config_files)
if not CONF.default_config_files:
default_config_files=developer_config_file)
if not CONF.default_config_files and not user_supplied_config_file:
LOG.warning('Config file not found, using default configs.')
keystone.conf.setup_logging()
CONF.command.cmd_class.main()

View File

@ -35,11 +35,7 @@ if os.path.exists(os.path.join(possible_topdir,
# entry point.
def main():
dev_conf = os.path.join(possible_topdir,
'etc',
'keystone.conf')
config_files = None
if os.path.exists(dev_conf):
config_files = [dev_conf]
cli.main(argv=sys.argv, config_files=config_files)
developer_config = os.path.join(possible_topdir, 'etc', 'keystone.conf')
if not os.path.exists(developer_config):
developer_config = None
cli.main(argv=sys.argv, developer_config_file=developer_config)

View File

@ -48,20 +48,23 @@ from keystone.tests import unit
from keystone.tests.unit import default_fixtures
from keystone.tests.unit.ksfixtures import database
from keystone.tests.unit.ksfixtures import ldapdb
from keystone.tests.unit.ksfixtures import temporaryfile
CONF = keystone.conf.CONF
PROVIDERS = provider_api.ProviderAPIs
class CliNoConfigTestCase(unit.BaseTestCase):
class CliLoggingTestCase(unit.BaseTestCase):
def setUp(self):
self.config_fixture = self.useFixture(oslo_config.fixture.Config(CONF))
self.config_fixture.register_cli_opt(cli.command_opt)
self.useFixture(fixtures.MockPatch(
'oslo_config.cfg.find_config_files', return_value=[]))
super(CliNoConfigTestCase, self).setUp()
fd = self.useFixture(temporaryfile.SecureTempFile())
self.fake_config_file = fd.file_name
super(CliLoggingTestCase, self).setUp()
# NOTE(crinkle): the command call doesn't have to actually work,
# that's what the other unit tests are for. So just mock it out.
@ -73,11 +76,19 @@ class CliNoConfigTestCase(unit.BaseTestCase):
self.logging = self.useFixture(fixtures.FakeLogger(level=log.WARN))
def test_cli(self):
def test_absent_config_logs_warning(self):
expected_msg = 'Config file not found, using default configs.'
cli.main(argv=['keystone-manage', 'db_sync'])
self.assertThat(self.logging.output, matchers.Contains(expected_msg))
def test_present_config_does_not_log_warning(self):
fake_argv = [
'keystone-manage', '--config-file', self.fake_config_file, 'doctor'
]
cli.main(argv=fake_argv)
expected_msg = 'Config file not found, using default configs.'
self.assertNotIn(expected_msg, self.logging.output)
class CliBootStrapTestCase(unit.SQLDriverOverrides, unit.TestCase):

View File

@ -0,0 +1,7 @@
---
fixes:
- |
[`bug 1782704 <https://bugs.launchpad.net/keystone/+bug/1782704>`_]
Checking for non-existant configuration files is more robust to ensure
proper logging to users when passing configuration information to
``keystone-manage``.