Merge "Prevent duplicate config file entries"

tests/fixtures/config/tenant-parser/extra_invalid_type.yaml

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

tests/fixtures/config/tenant-parser/extra_invalid_value.yaml

@@ -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

tests/unit/test_configloader.py

@@ -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'
 

zuul/cmd/client.py

@@ -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")

zuul/configloader.py

@@ -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
 
@@ -1474,7 +1492,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,
     }}
@@ -1530,6 +1548,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,
@@ -2252,7 +2277,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:
@@ -2280,6 +2306,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):

zuul/scheduler.py

@@ -779,6 +779,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(
@@ -813,6 +814,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,
@@ -1067,14 +1069,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,
+            unparsed_abide = loader.readConfig(
-                                               from_script=script)
+                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}"