Galaxy server config updates (#77106)

* ansible-galaxy configurable timeouts

  - also fixed issues with precedence,
    so --ignore-certs now overrides config
  - made galaxy_timeout generic setting,
    if set, it becomes default for server configs,
    but now specific servers can override
  - updated tests or added notes (some tests ignore/override precedence)

Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>

changelogs/fragments/galaxy_server_timeout.yml

@@ -0,0 +1,5 @@
+minor_changes:
+  - ansible-galaxy now supports a user defined timeout,  instead of existing hardcoded 60s (now the default).
+bugfixes:
+  - ansible-galaxy --ignore-certs now has proper precedence over configuration
+  - GALAXY_IGNORE_CERTS reworked to allow each server entry to override

lib/ansible/cli/galaxy.py

@@ -61,6 +61,7 @@ from ansible.utils.plugin_docs import get_versioned_doclink
 display = Display()
 urlparse = six.moves.urllib.parse.urlparse
 
+# config definition by position: name, required, type
 SERVER_DEF = [
     ('url', True, 'str'),
     ('username', False, 'str'),
@@ -70,8 +71,21 @@ SERVER_DEF = [
     ('v3', False, 'bool'),
     ('validate_certs', False, 'bool'),
     ('client_id', False, 'str'),
+    ('timeout', False, 'int'),
 ]
 
+# config definition fields
+SERVER_ADDITIONAL = {
+    'v3': {'default': 'False'},
+    'validate_certs': {'default': True, 'cli': [{'name': 'validate_certs'}]},
+    'timeout': {'default': '60', 'cli': [{'name': 'timeout'}]},
+    'token': {'default': None},
+}
+
+# override default if the generic is set
+if C.GALAXY_IGNORE_CERTS is not None:
+    SERVER_ADDITIONAL['validate_certs'].update({'default': not C.GALAXY_IGNORE_CERTS})
+
 
 def with_collection_artifacts_manager(wrapped_method):
     """Inject an artifacts manager if not passed explicitly.
@@ -84,7 +98,7 @@ def with_collection_artifacts_manager(wrapped_method):
         if 'artifacts_manager' in kwargs:
             return wrapped_method(*args, **kwargs)
 
-        artifacts_manager_kwargs = {'validate_certs': not context.CLIARGS['ignore_certs']}
+        artifacts_manager_kwargs = {'validate_certs': context.CLIARGS['validate_certs']}
 
         keyring = context.CLIARGS.get('keyring', None)
         if keyring is not None:
@@ -200,8 +214,10 @@ class GalaxyCLI(CLI):
         common.add_argument('--token', '--api-key', dest='api_key',
                             help='The Ansible Galaxy API key which can be found at '
                                  'https://galaxy.ansible.com/me/preferences.')
-        common.add_argument('-c', '--ignore-certs', action='store_true', dest='ignore_certs',
+        common.add_argument('-c', '--ignore-certs', action='store_true', dest='ignore_certs', help='Ignore SSL certificate validation errors.', default=None)
-                            default=C.GALAXY_IGNORE_CERTS, help='Ignore SSL certificate validation errors.')
+        common.add_argument('--timeout', dest='timeout', type=int,
+                            help="The time to wait for operations against the galaxy server, defaults to 60s.")
+
         opt_help.add_verbosity_options(common)
 
         force = opt_help.argparse.ArgumentParser(add_help=False)
@@ -530,6 +546,10 @@ class GalaxyCLI(CLI):
 
     def post_process_args(self, options):
         options = super(GalaxyCLI, self).post_process_args(options)
+
+        # ensure we have 'usable' cli option
+        setattr(options, 'validate_certs', (None if options.ignore_certs is None else not options.ignore_certs))
+
         display.verbosity = options.verbosity
         return options
 
@@ -540,7 +560,7 @@ class GalaxyCLI(CLI):
         self.galaxy = Galaxy()
 
         def server_config_def(section, key, required, option_type):
-            return {
+            config_def = {
                 'description': 'The %s of the %s Galaxy server' % (key, section),
                 'ini': [
                     {
@@ -554,10 +574,13 @@ class GalaxyCLI(CLI):
                 'required': required,
                 'type': option_type,
             }
+            if key in SERVER_ADDITIONAL:
+                config_def.update(SERVER_ADDITIONAL[key])
+
+            return config_def
 
-        validate_certs_fallback = not context.CLIARGS['ignore_certs']
         galaxy_options = {}
-        for optional_key in ['clear_response_cache', 'no_cache']:
+        for optional_key in ['clear_response_cache', 'no_cache', 'timeout']:
             if optional_key in context.CLIARGS:
                 galaxy_options[optional_key] = context.CLIARGS[optional_key]
 
@@ -566,25 +589,25 @@ class GalaxyCLI(CLI):
         # Need to filter out empty strings or non truthy values as an empty server list env var is equal to [''].
         server_list = [s for s in C.GALAXY_SERVER_LIST or [] if s]
         for server_priority, server_key in enumerate(server_list, start=1):
+            # Abuse the 'plugin config' by making 'galaxy_server' a type of plugin
             # Config definitions are looked up dynamically based on the C.GALAXY_SERVER_LIST entry. We look up the
             # section [galaxy_server.<server>] for the values url, username, password, and token.
             config_dict = dict((k, server_config_def(server_key, k, req, ensure_type)) for k, req, ensure_type in SERVER_DEF)
             defs = AnsibleLoader(yaml_dump(config_dict)).get_single_data()
             C.config.initialize_plugin_configuration_definitions('galaxy_server', server_key, defs)
 
+            # resolve the config created options above with existing config and user options
             server_options = C.config.get_plugin_options('galaxy_server', server_key)
+
             # auth_url is used to create the token, but not directly by GalaxyAPI, so
-            # it doesn't need to be passed as kwarg to GalaxyApi
+            # it doesn't need to be passed as kwarg to GalaxyApi, same for others we pop here
-            auth_url = server_options.pop('auth_url', None)
+            auth_url = server_options.pop('auth_url')
-            client_id = server_options.pop('client_id', None)
+            client_id = server_options.pop('client_id')
             token_val = server_options['token'] or NoTokenSentinel
             username = server_options['username']
-            available_api_versions = None
+            v3 = server_options.pop('v3')
-            v3 = server_options.pop('v3', None)
             validate_certs = server_options['validate_certs']
-            if validate_certs is None:
+
-                validate_certs = validate_certs_fallback
-            server_options['validate_certs'] = validate_certs
             if v3:
                 # This allows a user to explicitly indicate the server uses the /v3 API
                 # This was added for testing against pulp_ansible and I'm not sure it has
@@ -596,8 +619,7 @@ class GalaxyCLI(CLI):
             server_options['token'] = None
 
             if username:
-                server_options['token'] = BasicAuthToken(username,
+                server_options['token'] = BasicAuthToken(username, server_options['password'])
-                                                         server_options['password'])
             else:
                 if token_val:
                     if auth_url:
@@ -618,6 +640,10 @@ class GalaxyCLI(CLI):
 
         cmd_server = context.CLIARGS['api_server']
         cmd_token = GalaxyToken(token=context.CLIARGS['api_key'])
+
+        # resolve validate_certs
+        v_config_default = True if C.GALAXY_IGNORE_CERTS is None else not C.GALAXY_IGNORE_CERTS
+        validate_certs = v_config_default if context.CLIARGS['validate_certs'] is None else context.CLIARGS['validate_certs']
         if cmd_server:
             # Cmd args take precedence over the config entry but fist check if the arg was a name and use that config
             # entry, otherwise create a new API entry for the server specified.
@@ -628,7 +654,7 @@ class GalaxyCLI(CLI):
                 self.api_servers.append(GalaxyAPI(
                     self.galaxy, 'cmd_arg', cmd_server, token=cmd_token,
                     priority=len(config_servers) + 1,
-                    validate_certs=validate_certs_fallback,
+                    validate_certs=validate_certs,
                     **galaxy_options
                 ))
         else:
@@ -639,7 +665,7 @@ class GalaxyCLI(CLI):
             self.api_servers.append(GalaxyAPI(
                 self.galaxy, 'default', C.GALAXY_SERVER, token=cmd_token,
                 priority=0,
-                validate_certs=validate_certs_fallback,
+                validate_certs=validate_certs,
                 **galaxy_options
             ))
 

lib/ansible/config/base.yml

@@ -1330,7 +1330,6 @@ FACTS_MODULES:
     - name: ansible_facts_modules
 GALAXY_IGNORE_CERTS:
   name: Galaxy validate certs
-  default: False
   description:
     - If set to yes, ansible-galaxy will not validate TLS certificates.
       This can be useful for testing against a server with a self-signed certificate.

lib/ansible/galaxy/api.py

@@ -269,6 +269,7 @@ class GalaxyAPI:
         self.timeout = timeout
         self._available_api_versions = available_api_versions or {}
         self._priority = priority
+        self._server_timeout = timeout
 
         b_cache_dir = to_bytes(C.GALAXY_CACHE_DIR, errors='surrogate_or_strict')
         makedirs_safe(b_cache_dir, mode=0o700)
@@ -378,7 +379,7 @@ class GalaxyAPI:
         try:
             display.vvvv("Calling Galaxy at %s" % url)
             resp = open_url(to_native(url), data=args, validate_certs=self.validate_certs, headers=headers,
-                            method=method, timeout=self.timeout, http_agent=user_agent(), follow_redirects='safe')
+                            method=method, timeout=self._server_timeout, http_agent=user_agent(), follow_redirects='safe')
         except HTTPError as e:
             raise GalaxyError(e, error_context_msg)
         except Exception as e:
@@ -436,7 +437,14 @@ class GalaxyAPI:
         """
         url = _urljoin(self.api_server, self.available_api_versions['v1'], "tokens") + '/'
         args = urlencode({"github_token": github_token})
-        resp = open_url(url, data=args, validate_certs=self.validate_certs, method="POST", http_agent=user_agent(), timeout=self.timeout)
+
+        try:
+            resp = open_url(url, data=args, validate_certs=self.validate_certs, method="POST", http_agent=user_agent(), timeout=self._server_timeout)
+        except HTTPError as e:
+            raise GalaxyError(e, 'Attempting to authenticate to galaxy')
+        except Exception as e:
+            raise AnsibleError('Unable to authenticate to galaxy: %s' % to_native(e), orig_exc=e)
+
         data = json.loads(to_text(resp.read(), errors='surrogate_or_strict'))
         return data
 

lib/ansible/galaxy/collection/concrete_artifact_manager.py

@@ -206,7 +206,7 @@ class ConcreteArtifactsManager:
                     validate_certs=self._validate_certs,
                     timeout=self.timeout
                 )
-            except URLError as err:
+            except Exception as err:
                 raise_from(
                     AnsibleError(
                         'Failed to download collection tar '

test/units/galaxy/test_collection.py

@@ -363,6 +363,8 @@ def test_validate_certs_with_server_url(global_ignore_certs, monkeypatch):
 
 @pytest.mark.parametrize('global_ignore_certs', [True, False])
 def test_validate_certs_with_server_config(global_ignore_certs, server_config, monkeypatch):
+
+    # test sidesteps real resolution and forces the server config to override the cli option
     get_plugin_options = MagicMock(side_effect=server_config)
     monkeypatch.setattr(C.config, 'get_plugin_options', get_plugin_options)
 
@@ -380,9 +382,10 @@ def test_validate_certs_with_server_config(global_ignore_certs, server_config, m
     monkeypatch.setattr(galaxy_cli, '_execute_install_collection', mock_execute_install)
     galaxy_cli.run()
 
-    assert galaxy_cli.api_servers[0].validate_certs is False
+    # server cfg, so should match def above, if not specified so it should use default (true)
-    assert galaxy_cli.api_servers[1].validate_certs is True
+    assert galaxy_cli.api_servers[0].validate_certs is server_config[0].get('validate_certs', True)
-    assert galaxy_cli.api_servers[2].validate_certs is not global_ignore_certs
+    assert galaxy_cli.api_servers[1].validate_certs is server_config[1].get('validate_certs', True)
+    assert galaxy_cli.api_servers[2].validate_certs is server_config[2].get('validate_certs', True)
 
 
 def test_build_collection_no_galaxy_yaml():