Prevent duplicate config file entries

It is currently possible to list default zuul config file paths in the
extra-config-paths config directive. Doing so will duplicate the configs
in Zuul which can cause problems. Prevent this entirely via
configuration validation.

Note: There has been a bit of refactoring to ensure that the voluptuous
schema is validated when reading the config. This ensures that an
invalid config doesn't produce hard to understand error messages because
loadTPCs() has attempted to process configuration that isn't valid.
Instead we can catch schema errors early and report them with human
friendly messages.

Change-Id: I07e9d4d3614cbc6cdee06b2866f7ae41d7779135
This commit is contained in:
Clark Boylan 2021-11-11 14:30:54 -08:00
parent 567a664ab9
commit 5b1ba567c8
6 changed files with 94 additions and 9 deletions

View File

@ -0,0 +1,10 @@
- tenant:
name: tenant-one
source:
gerrit:
config-projects:
- common-config
untrusted-projects:
- org/project1
- org/project2:
extra-config-paths: 1

View File

@ -0,0 +1,11 @@
- tenant:
name: tenant-one
source:
gerrit:
config-projects:
- common-config
untrusted-projects:
- org/project1
- org/project2:
extra-config-paths:
- .zuul.yaml

View File

@ -15,6 +15,7 @@ import fixtures
import logging
import textwrap
import testtools
import voluptuous as vs
from collections import defaultdict
from configparser import ConfigParser
@ -952,6 +953,32 @@ class TestTenantExtra(TenantParserTestCase):
], ordered=False)
class TestTenantExtraConfigsInvalidType(TenantParserTestCase):
tenant_config_file = 'config/tenant-parser/extra_invalid_type.yaml'
def setUp(self):
err = "Expected str or list of str for extra-config-paths.*"
with testtools.ExpectedException(vs.MultipleInvalid, err):
super().setUp()
def test_tenant_extra_configs_invalid_type(self):
# The magic is in setUp
pass
class TestTenantExtraConfigsInvalidValue(TenantParserTestCase):
tenant_config_file = 'config/tenant-parser/extra_invalid_value.yaml'
def setUp(self):
err = "Default zuul configs are not allowed in extra-config-paths.*"
with testtools.ExpectedException(vs.MultipleInvalid, err):
super().setUp()
def test_tenant_extra_configs_invalid_value(self):
# The magic is in setUp
pass
class TestTenantDuplicate(TenantParserTestCase):
tenant_config_file = 'config/tenant-parser/duplicate.yaml'

View File

@ -838,8 +838,9 @@ class Client(zuul.cmd.ZuulApp):
self.connections, zk_client, zuul_globals)
sched = SchedulerConfig(self.config, self.connections)
tenant_config, script = sched._checkTenantSourceConf(self.config)
unparsed_abide = loader.readConfig(tenant_config, from_script=script)
try:
unparsed_abide = loader.readConfig(
tenant_config, from_script=script)
for conf_tenant in unparsed_abide.tenants.values():
loader.tenant_parser.getSchema()(conf_tenant)
print("Tenants config validated with success")

View File

@ -51,6 +51,24 @@ def as_list(item):
return [item]
def no_dup_config_paths(v):
if isinstance(v, list):
for x in v:
check_config_path(x)
elif isinstance(v, str):
check_config_path(x)
else:
raise vs.Invalid("Expected str or list of str for extra-config-paths")
def check_config_path(path):
if not isinstance(path, str):
raise vs.Invalid("Expected str or list of str for extra-config-paths")
elif path in ["zuul.yaml", "zuul.d/", ".zuul.yaml", ".zuul.d/"]:
raise vs.Invalid("Default zuul configs are not "
"allowed in extra-config-paths")
class ConfigurationSyntaxError(Exception):
pass
@ -1468,7 +1486,7 @@ class TenantParser(object):
'exclude': to_list(classes),
'shadow': to_list(str),
'exclude-unprotected-branches': bool,
'extra-config-paths': to_list(str),
'extra-config-paths': no_dup_config_paths,
'load-branch': str,
'allow-circular-dependencies': bool,
}}
@ -1524,6 +1542,13 @@ class TenantParser(object):
def fromYaml(self, abide, conf, ansible_manager, min_ltimes=None,
layout_uuid=None, branch_cache_min_ltimes=None):
# Note: This vs schema validation is not necessary in most cases as we
# verify the schema when loading tenant configs into zookeeper.
# However, it is theoretically possible in a multi scheduler setup that
# one scheduler would load the config into zk with validated schema
# then another newer or older scheduler could load it from zk and fail.
# We validate again to help users debug this situation should it
# happen.
self.getSchema()(conf)
tenant = model.Tenant(conf['name'])
pcontext = ParseContext(self.connections, self.scheduler,
@ -2246,7 +2271,8 @@ class ConfigLoader(object):
config_path)
return config_path
def readConfig(self, config_path, from_script=False):
def readConfig(self, config_path, from_script=False,
tenants_to_validate=None):
config_path = self.expandConfigPath(config_path)
if not from_script:
with open(config_path) as config_file:
@ -2274,6 +2300,16 @@ class ConfigLoader(object):
data = []
unparsed_abide = model.UnparsedAbideConfig()
unparsed_abide.extend(data)
available_tenants = list(unparsed_abide.tenants)
tenants_to_validate = tenants_to_validate or available_tenants
if not set(tenants_to_validate).issubset(available_tenants):
invalid = tenants_to_validate.difference(available_tenants)
raise RuntimeError(f"Invalid tenant(s) found: {invalid}")
for tenant_name in tenants_to_validate:
# Validate the voluptuous schema early when reading the config
# as multiple subsequent steps need consistent yaml input.
self.tenant_parser.getSchema()(unparsed_abide.tenants[tenant_name])
return unparsed_abide
def loadAdminRules(self, abide, unparsed_abide):

View File

@ -764,6 +764,7 @@ class Scheduler(threading.Thread):
self.updateSystemConfig()
else:
self.log.info("Creating initial system config")
# This verifies the voluptuous schema
self.primeSystemConfig()
loader = configloader.ConfigLoader(
@ -798,6 +799,7 @@ class Scheduler(threading.Thread):
# we don't have a layout state.
branch_cache_min_ltimes = defaultdict(lambda: -1)
# This load validates the entire tenant config
tenant = loader.loadTenant(
self.abide, tenant_name, self.ansible_manager,
self.unparsed_abide, min_ltimes=min_ltimes,
@ -1050,14 +1052,12 @@ class Scheduler(threading.Thread):
self.connections, self.zk_client, self.globals, self.statsd,
self, self.merger, self.keystore)
tenant_config, script = self._checkTenantSourceConf(self.config)
unparsed_abide = loader.readConfig(tenant_config,
from_script=script)
unparsed_abide = loader.readConfig(
tenant_config,
from_script=script,
tenants_to_validate=tenants_to_validate)
available_tenants = list(unparsed_abide.tenants)
tenants_to_load = tenants_to_validate or available_tenants
if not set(tenants_to_load).issubset(available_tenants):
invalid = tenants_to_load.difference(available_tenants)
raise RuntimeError(f"Invalid tenant(s) found: {invalid}")
# Use a temporary config cache for the validation
validate_root = f"/zuul/validate/{uuid.uuid4().hex}"