From 8c8509ecc8aa523dae24677acb94135affd189c9 Mon Sep 17 00:00:00 2001 From: Jacob Callahan Date: Tue, 17 Sep 2024 13:44:04 -0400 Subject: [PATCH 1/6] Introduction of broker config cli and much more! The initial change of this was to introduce the broker config cli. However, to do this, a number of supporting and related changes needed to be made. The first of these is the new ConfigManager class that manages reading from and writing to the config file. Since so much logic was implemented for this, it made more sense to roll in the import/init functionality, that was originally in the settings module, into the new class. This also gave me the opportunity to separate the test config from the example config. This removal, and other changes in this were supported by ConfigManager's migration functionality. To simplify the nested chunk notation, I moved provider instances out from a list to just being a nested dictionary. This actually simplified the instance logic quite a bit. Related to config simlification was the separation of the host settings to a new ssh chunk of the config. This is because not all users use the host functionality Broker provides. A likely change with this release is going to be removing a default ssh backend from Broker's requirements. While I was at it, I also switched the CLI over to rich-click as part of a larger effort to improve the CLI experience. --- .github/workflows/codeql-analysis.yml | 5 +- broker/commands.py | 153 ++++++++++++-- broker/config_manager.py | 280 ++++++++++++++++++++++++++ broker/config_migrations/v0_6_0.py | 72 +++++++ broker/helpers.py | 19 +- broker/hosts.py | 32 +-- broker/providers/__init__.py | 24 +-- broker/session.py | 2 +- broker/settings.py | 119 ++++------- broker_settings.yaml.example | 55 ++--- pyproject.toml | 1 + tests/data/broker_settings.yaml | 59 ++++++ tests/functional/test_containers.py | 9 +- tests/test_broker.py | 11 +- tests/test_config_manager.py | 60 ++++++ 15 files changed, 714 insertions(+), 187 deletions(-) create mode 100644 broker/config_manager.py create mode 100644 broker/config_migrations/v0_6_0.py create mode 100644 tests/data/broker_settings.yaml create mode 100644 tests/test_config_manager.py diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 1feceb75..792d5ac2 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -50,11 +50,8 @@ jobs: - name: Unit Tests env: - BROKER_DIRECTORY: "${{ github.workspace }}/broker_dir" UV_SYSTEM_PYTHON: 1 run: | - cp broker_settings.yaml.example ${BROKER_DIRECTORY}/broker_settings.yaml - uv pip install "broker[dev,docker] @ ." - ls -l "$BROKER_DIRECTORY" + uv pip install "broker[dev,podman] @ ." broker --version pytest -v tests/ --ignore tests/functional diff --git a/broker/commands.py b/broker/commands.py index 98b9a442..5f17c7c7 100644 --- a/broker/commands.py +++ b/broker/commands.py @@ -4,17 +4,28 @@ import signal import sys -import click from logzero import logger from rich.console import Console +from rich.syntax import Syntax from rich.table import Table +import rich_click as click from broker import exceptions, helpers, settings from broker.broker import Broker +from broker.config_manager import ConfigManager from broker.logger import LOG_LEVEL from broker.providers import PROVIDER_ACTIONS, PROVIDER_HELP, PROVIDERS signal.signal(signal.SIGINT, helpers.handle_keyboardinterrupt) +CONSOLE = Console() # rich console for pretty printing + +click.rich_click.SHOW_ARGUMENTS = True +click.rich_click.COMMAND_GROUPS = { + "broker": [ + {"name": "Core Actions", "commands": ["checkout", "checkin", "inventory"]}, + {"name": "Extras", "commands": ["execute", "extend", "providers", "config"]}, + ] +} def loggedcli(group=None, *cli_args, **cli_kwargs): @@ -47,7 +58,7 @@ def parse_labels(provider_labels): } -class ExceptionHandler(click.Group): +class ExceptionHandler(click.RichGroup): """Wraps click group to catch and handle raised exceptions.""" def __call__(self, *args, **kwargs): @@ -167,13 +178,9 @@ def provider_cmd(ctx, *args, **kwargs): # the actual subcommand def cli(version): """Command-line interface for interacting with providers.""" if version: - from importlib.metadata import version - from packaging.version import Version import requests - broker_version = version("broker") - # Check against the latest version published to PyPi try: latest_version = Version( @@ -181,19 +188,28 @@ def cli(version): "version" ] ) - if latest_version > Version(broker_version): + if latest_version > Version(ConfigManager.version): click.secho( f"A newer version of broker is available: {latest_version}", fg="yellow", ) except requests.exceptions.RequestException as err: logger.warning(f"Unable to check for latest version: {err}") - click.echo(f"Version: {broker_version}") - broker_directory = settings.BROKER_DIRECTORY.absolute() - click.echo(f"Broker Directory: {broker_directory}") - click.echo(f"Settings File: {settings.settings_path.absolute()}") - click.echo(f"Inventory File: {broker_directory}/inventory.yaml") - click.echo(f"Log File: {broker_directory}/logs/broker.log") + + # Create a rich table + table = Table(title=f"Broker {ConfigManager.version}") + + table.add_column("", justify="left", style="cyan", no_wrap=True) + table.add_column("Location", justify="left", style="magenta") + + table.add_row("Broker Directory", str(settings.BROKER_DIRECTORY.absolute())) + table.add_row("Settings File", str(settings.settings_path.absolute())) + table.add_row("Inventory File", f"{settings.BROKER_DIRECTORY.absolute()}/inventory.yaml") + table.add_row("Log File", f"{settings.BROKER_DIRECTORY.absolute()}/logs/broker.log") + + # Print the table + console = Console() + console.print(table) @loggedcli(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) @@ -288,7 +304,6 @@ def inventory(details, curated, sync, filter): inventory = helpers.load_inventory(filter=filter) helpers.emit({"inventory": inventory}) if curated: - console = Console() table = Table(title="Host Inventory") table.add_column("Id", justify="left", style="cyan", no_wrap=True) @@ -302,7 +317,7 @@ def inventory(details, curated, sync, filter): str(host["id"]), host["host"], host["provider"], host["action"], host["os"] ) - console.print(table) + CONSOLE.print(table) return for num, host in enumerate(inventory): if (display_name := host.get("hostname")) is None: @@ -400,3 +415,111 @@ def execute(ctx, background, nick, output_format, artifacts, args_file, provider logger.info(result) elif output_format == "yaml": click.echo(helpers.yaml_format(result)) + + +@cli.group(cls=ExceptionHandler) +def config(): + """View and manage Broker's configuration. + + Note: One important concept of these commands is the concept of a "chunk". + + A chunk is a part of the configuration file that can be accessed or updated. + Chunks are specified by their keys in the configuration file. + Nested chunks are separated by periods. + + e.g. broker config view AnsibleTower.instances.my_instance + """ + + +@loggedcli(group=config) +@click.argument("chunk", type=str, required=False) +@click.option("--no-syntax", is_flag=True, help="Disable syntax highlighting") +def view(chunk, no_syntax): + """View all or part of the broker configuration.""" + result = helpers.yaml_format(ConfigManager(settings.settings_path).get(chunk)) + if no_syntax: + CONSOLE.print(result) + else: + CONSOLE.print(Syntax(result, "yaml", background_color="default")) + + +@loggedcli(group=config) +@click.argument("chunk", type=str, required=False) +def edit(chunk): + """Directly edit the broker configuration file. + + You can define the scope of the edit by specifying a chunk. + Otherwise, the entire configuration file will be opened. + """ + ConfigManager(settings.settings_path).edit(chunk) + + +@loggedcli(group=config, name="set") +@click.argument("chunk", type=str, required=True) +@click.argument("new-value", type=str, required=True) +def _set(chunk, new_value): + """Set a value in the Broker configuration file. + + These updates take the form of ` ` pairs. + You can also pass a yaml or json file containing the new contents of a chunk. + """ + new_value = helpers.resolve_file_args({"nv": new_value})["nv"] + ConfigManager(settings.settings_path).update(chunk, new_value) + + +@loggedcli(group=config) +def restore(): + """Restore the broker configuration file to the last backup.""" + ConfigManager(settings.settings_path).restore() + + +@loggedcli(group=config) +@click.argument("chunk", type=str, required=False) +def init(chunk): + """Initialize the broker configuration file from your local clone or GitHub. + + You can also init specific chunks by passing the chunk name. + """ + ConfigManager(settings.settings_path).init_config_file(chunk) + + +@loggedcli(group=config) +def nicks(): + """Get a list of nicks.""" + result = ConfigManager(settings.settings_path).nicks() + CONSOLE.print("\n".join(result)) + + +@loggedcli(group=config) +@click.argument("nick", type=str, required=True) +@click.option("--no-syntax", is_flag=True, help="Disable syntax highlighting") +def nick(nick, no_syntax): + """Get information about a specific nick.""" + result = helpers.yaml_format(ConfigManager(settings.settings_path).nicks(nick)) + if no_syntax: + CONSOLE.print(result) + else: + CONSOLE.print(Syntax(result, "yaml", background_color="default")) + + +@loggedcli(group=config) +def migrate(): + """Migrate the broker configuration file to the latest version.""" + ConfigManager(settings.settings_path).migrate() + + +@loggedcli(group=config) +@click.argument("chunk", type=str, required=False, default="base") +def validate(chunk): + """Validate top-level chunks of the broker configuration file. + + You can validate against the `base` settings by default or specify a provider. + You can also validate against a specific provider instance with `ProviderClass:instance_name`. + + To validate everything, pass `all` + """ + try: + ConfigManager(settings.settings_path).validate(chunk, PROVIDERS) + logger.info("Validation passed!") + except exceptions.BrokerError as err: + logger.warning(f"Validation failed: {err}") diff --git a/broker/config_manager.py b/broker/config_manager.py new file mode 100644 index 00000000..25f0a2b0 --- /dev/null +++ b/broker/config_manager.py @@ -0,0 +1,280 @@ +"""Module providing the functionality powering the `broker config` command.""" + +import importlib +from importlib.metadata import version +import inspect +import json +import os +from pathlib import Path +import pkgutil +from tempfile import NamedTemporaryFile + +import click +from logzero import logger +from packaging.version import Version +import yaml + +from broker import exceptions + +C_SEP = "." # chunk separator +GH_CFG = "https://raw.githubusercontent.com/SatelliteQE/broker/master/broker_settings.yaml.example" + + +def file_name_to_ver(file_name): + """Convert a version-encoded filename `v0_6_0` to a `Version` object.""" + return Version(file_name[1:].replace("_", ".")) + + +def yaml_format(data): + """Format the data as yaml. + + Duplicating here to avoid circular imports. + """ + return yaml.dump(data, default_flow_style=False, sort_keys=False) + + +class ConfigManager: + """Class to interact with Broker's configuration file. + + One important concept of these commands is the concept of a "chunk". + + A chunk is a part of the configuration file that can be accessed or updated. + Chunks are specified by their keys in the configuration file. + Nested chunks are separated by periods. + + e.g. broker config view AnsibleTower.instances.my_instance + """ + + version = version("broker") + + def __init__(self, settings_path=None): + self._interactive_mode = None + self._settings_path = settings_path + if settings_path: + if settings_path.exists(): + self._cfg = yaml.safe_load(self._settings_path.read_text()) + else: + click.secho( + f"Broker settings file not found at {settings_path.absolute()}.", fg="red" + ) + self.init_config_file() + + @property + def interactive_mode(self): + """Determine if Broker is running in interactive mode.""" + if self._interactive_mode is None: + self._interactive_mode = False + # GitHub action context + if "GITHUB_WORKFLOW" not in os.environ: + # determine if we're being ran from a CLI + for frame in inspect.stack()[::-1]: + if "/bin/broker" in frame.filename: + self._interactive_mode = True + break + return self._interactive_mode + + def _interactive_edit(self, chunk): + """Write the chunk data to a temporary file and open it in an editor.""" + with NamedTemporaryFile(mode="w+", suffix=".yaml") as tmp: + tmp.write(yaml_format(chunk)) + tmp.seek(0) + click.edit(filename=tmp.name) + tmp.seek(0) + new_data = tmp.read() + # first try to load it as yaml + try: + return yaml.safe_load(new_data) + except yaml.YAMLError: # then try json + try: + return json.loads(new_data) + except json.JSONDecodeError: # finally, just return the raw data + return new_data + + def _import_config(self, source, is_url=False): + """Initialize the broker settings file from a source.""" + proceed = True + if self.interactive_mode: + try: + proceed = click.confirm(f"Get example file from {source}?") + except click.core.Abort: + # We're likely in a different non-interactive environment (container?) + self._interactive_mode = False + if not proceed: + return + # get example file from source + if is_url: + import requests + + click.echo(f"Downloading example file from: {source}") + return requests.get(source, timeout=60).text + else: + return source.read_text() + + def _get_migrations(self): + """Construct a list of all applicable migrations.""" + from broker import config_migrations + + config_version = Version(self._cfg.get("_version", "0.0.0")) + migrations = [] + for _, name, _ in pkgutil.iter_modules(config_migrations.__path__): + module = importlib.import_module(f"broker.config_migrations.{name}") + if hasattr(module, "run_migrations") and config_version < file_name_to_ver(name): + migrations.append(module) + return migrations + + def backup(self): + """Backup the current configuration file.""" + logger.debug( + f"Backing up the configuration file to {self._settings_path.with_suffix('.bak')}" + ) + self._settings_path.with_suffix(".bak").write_text(self._settings_path.read_text()) + + def restore(self): + """Restore the configuration file from a backup if it exists.""" + logger.debug( + f"Restoring the configuration file from {self._settings_path.with_suffix('.bak')}" + ) + backup_path = self._settings_path.with_suffix(".bak") + if not backup_path.exists(): + raise exceptions.UserError("No backup file found.") + self._settings_path.write_text(backup_path.read_text()) + + def edit(self, chunk=None, content=None): + """Open the config file in an editor.""" + if not self.interactive_mode: + raise exceptions.UserError( + "Attempted to edit the config in non-interactive mode.\n" + "Did you mean to use the `set` method instead?" + ) + content = content or self.get(chunk=chunk) + new_val = self._interactive_edit(content) + self.update(chunk, new_val) + + def get(self, chunk=None, curr_chunk=None): + """Get a chunk of Broker's config or the whole config.""" + if not curr_chunk: + curr_chunk = self._cfg + if not chunk: + return curr_chunk + if C_SEP in chunk: + curr, chunk = chunk.split(C_SEP, 1) + # curr = int(curr) if curr.isdigit() else curr + return self.get(chunk, curr_chunk=curr_chunk[curr]) + else: + # chunk = int(chunk) if chunk.isdigit() else chunk + try: + return curr_chunk[chunk] + except KeyError: + raise exceptions.UserError(f"Chunk '{chunk}' not found in the config.") + + def update(self, chunk, new_val, curr_chunk=None): + """Update a chunk of Broker's config or the whole config.""" + # Recursive down to find the chunk to update, then propagate the new value back up + if not curr_chunk: # we're at the top level, so update the config directly + if chunk is None: # the whole config is being updated + self._cfg = new_val + elif C_SEP in chunk: # the update needs to happen at a lower level + curr, chunk = chunk.split(C_SEP, 1) + self._cfg[curr] = self.update(chunk, new_val, curr_chunk=self._cfg[curr]) + else: + self._cfg[chunk] = new_val + # update the config file if it exists + if self._settings_path.exists(): + self.backup() + self._settings_path.write_text( + yaml.dump(self._cfg, default_flow_style=False, sort_keys=False) + ) + else: # we're not at the top level, so keep going down + if C_SEP in chunk: + curr, chunk = chunk.split(C_SEP, 1) + curr_chunk[curr] = self.update(chunk, new_val, curr_chunk=curr_chunk[curr]) + else: + curr_chunk[chunk] = new_val + return curr_chunk + + def nicks(self, nick=None): + """Get a list of nicks or single nick information.""" + nicks = self.get("nicks") + if nick: + return nicks[nick] + return list(nicks.keys()) + + def init_config_file(self, chunk=None): + """Check for the existence of the config file and create it if it doesn't exist.""" + if self.interactive_mode and self._settings_path.exists() and not chunk: + # if the file exists, ask the user if they want to overwrite it + if ( + click.prompt( + f"Settings file already exists at {self._settings_path.absolute()}. Overwrite?", + type=click.Choice(["y", "n"]), + default="n", + ) + != "y" + ): + return + # get the example file from the local repo or GitHub + example_path = Path(__file__).parent.parent.joinpath("broker_settings.yaml.example") + raw_data = None + if example_path.exists(): + raw_data = self._import_config(example_path) + if not raw_data: + raw_data = self._import_config(GH_CFG, is_url=True) + if not raw_data: + raise exceptions.ConfigurationError( + f"Broker settings file not found at {self._settings_path.absolute()}." + ) + chunk_data = self.get(chunk, yaml.safe_load(raw_data)) + if self.interactive_mode: + chunk_data = self._interactive_edit(chunk_data) + self.update(chunk, chunk_data) + + def migrate(self): + """Migrate the config from a previous version of Broker.""" + # get all available migrations + if not (migrations := self._get_migrations()): + logger.info("No migrations are applicable to your config.") + return + # run all migrations in order + working_config = self._cfg + for migration in sorted(migrations, key=lambda m: m.TO_VERSION): + working_config = migration.run_migrations(working_config) + self.backup() + self._settings_path.write_text( + yaml.dump(working_config, default_flow_style=False, sort_keys=False) + ) + logger.info("Config migration complete.") + + def validate(self, chunk, providers=None): + """Validate a top-level chunk of Broker's config.""" + if "." in chunk: # only validate the top-level chunk + chunk = chunk.split(".")[0] + if chunk.lower() == "base": # validate the base config + return # this happens before we're called + if chunk.lower() == "ssh": + from broker.settings import settings + + settings.validators.validate(only="SSH") + return + if providers is None: + raise exceptions.UserError( + "Attempted to validate provider settings without passing providers." + ) + if ":" in chunk: + prov_name, instance = chunk.split(":") + providers[prov_name](**{prov_name: instance}) + if chunk == "all": + for prov_name, prov_cls in providers.items(): + if prov_name == "TestProvider": + continue + logger.info(f"Validating {prov_name} provider settings.") + try: # we want to suppress all exceptions here to allow each provider to validate + prov_cls() + except Exception as err: # noqa: BLE001 + logger.warning(f"Provider {prov_name} failed validation: {err}") + return + if chunk not in providers: + raise exceptions.UserError( + "I don't know how to validate that.\n" + "If it's important, it is likely covered in the base validations." + ) + providers[chunk]() diff --git a/broker/config_migrations/v0_6_0.py b/broker/config_migrations/v0_6_0.py new file mode 100644 index 00000000..a1cbce22 --- /dev/null +++ b/broker/config_migrations/v0_6_0.py @@ -0,0 +1,72 @@ +"""Config migrations for versions older than 0.6.0 to 0.6.0.""" +from logzero import logger + +TO_VERSION = "0.6.0" + + +def migrate_instances(config_dict): + """Migrate instances from a list of dicts to a dict of dicts.""" + logger.debug("Migrating instances from a list to a dict.") + for key, val in config_dict.items(): + if not isinstance(val, dict): + continue + if "instances" in val and isinstance(val["instances"], list): + old_instances = val.pop("instances") + val["instances"] = {} + for inst in old_instances: + val["instances"].update(inst) + config_dict[key] = val + return config_dict + + +def remove_testprovider(config_dict): + """Remove the testprovider from the config.""" + logger.debug("Removing the testprovider from the config.") + config_dict.pop("TestProvider", None) + return config_dict + + +def remove_test_nick(config_dict): + """Remove the test nick from the config.""" + logger.debug("Removing the test nick from the config.") + nicks = config_dict.get("nicks", {}) + nicks.pop("test_nick", None) + config_dict["nicks"] = nicks + return config_dict + + +def move_ssh_settings(config_dict): + """Move SSH settings from the top leve into its own chunk.""" + logger.debug("Moving SSH settings into their own section.") + ssh_settings = { + "backend": config_dict.pop("ssh_backend", "ssh2-python312"), + "host_username": config_dict.pop("host_username", "root"), + "host_password": config_dict.pop("host_password", "toor"), + "host_ipv6": config_dict.pop("host_ipv6", False), + "host_ipv4_fallback": config_dict.pop("host_ipv4_fallback", True), + } + if ssh_port := config_dict.pop("host_ssh_port", None): + ssh_settings["ssh_port"] = ssh_port + if ssh_key := config_dict.pop("host_ssh_key_filename", None): + ssh_settings["host_ssh_key_filename"] = ssh_key + config_dict["ssh"] = ssh_settings + return config_dict + + +def add_thread_limit(config_dict): + """Add a thread limit to the config.""" + logger.debug("Adding a thread limit to the config.") + config_dict["thread_limit"] = None + return config_dict + + +def run_migrations(config_dict): + """Run all migrations.""" + logger.info(f"Running config migrations for {TO_VERSION}.") + config_dict = migrate_instances(config_dict) + config_dict = remove_testprovider(config_dict) + config_dict = remove_test_nick(config_dict) + config_dict = move_ssh_settings(config_dict) + config_dict = add_thread_limit(config_dict) + config_dict["_version"] = TO_VERSION + return config_dict diff --git a/broker/helpers.py b/broker/helpers.py index 2ff7e680..835ca633 100644 --- a/broker/helpers.py +++ b/broker/helpers.py @@ -458,19 +458,20 @@ def fork_broker(): def handle_keyboardinterrupt(*args): """Handle keyboard interrupts gracefully. - Offer the user a choice between keeping Broker alive in the background or killing it. + Offer the user a choice between keeping Broker alive in the background, killing it, or resuming execution. """ choice = click.prompt( - "\nEnding Broker while running won't end processes being monitored.\n" - "Would you like to switch Broker to run in the background?\n" - "[y/n]: ", - type=click.Choice(["y", "n"]), - default="n", - ) - if choice == "y": + "\nEnding Broker while running may not end processes being monitored.\n" + "Would you like to switch Broker to run in the Background, Kill it, or Resume execution?\n", + type=click.Choice(["b", "k", "r"]), + default="r", + ).lower() + if choice == "b": fork_broker() - else: + elif choice == "k": raise exceptions.BrokerError("Broker killed by user.") + elif choice == "r": + click.echo("Resuming execution...") def translate_timeout(timeout): diff --git a/broker/hosts.py b/broker/hosts.py index 1149469f..bbc0095d 100644 --- a/broker/hosts.py +++ b/broker/hosts.py @@ -17,9 +17,10 @@ from logzero import logger from broker.exceptions import HostError, NotImplementedError -from broker.session import ContainerSession, Session from broker.settings import settings +SETTINGS_VALIDATED = False + class Host: """Class representing a host that can be accessed via SSH or Bind. @@ -45,6 +46,11 @@ def __init__(self, **kwargs): ipv6 (bool): Whether or not to use IPv6. Defaults to False. ipv4_fallback (bool): Whether or not to fallback to IPv4 if IPv6 fails. Defaults to True. """ + global SETTINGS_VALIDATED # noqa: PLW0603 + if not SETTINGS_VALIDATED: + logger.debug("Validating ssh settings") + settings.validators.validate(only="SSH") + SETTINGS_VALIDATED = True logger.debug(f"Constructing host using {kwargs=}") self.hostname = kwargs.get("hostname") or kwargs.get("ip") if not self.hostname: @@ -56,13 +62,13 @@ def __init__(self, **kwargs): else: raise HostError("Host must be constructed with a hostname or ip") self.name = kwargs.pop("name", None) - self.username = kwargs.pop("username", settings.HOST_USERNAME) - self.password = kwargs.pop("password", settings.HOST_PASSWORD) - self.timeout = kwargs.pop("connection_timeout", settings.HOST_CONNECTION_TIMEOUT) - self.port = kwargs.pop("port", settings.HOST_SSH_PORT) - self.key_filename = kwargs.pop("key_filename", settings.HOST_SSH_KEY_FILENAME) - self.ipv6 = kwargs.pop("ipv6", settings.HOST_IPV6) - self.ipv4_fallback = kwargs.pop("ipv4_fallback", settings.HOST_IPV4_FALLBACK) + self.username = kwargs.pop("username", settings.SSH.HOST_USERNAME) + self.password = kwargs.pop("password", settings.SSH.HOST_PASSWORD) + self.timeout = kwargs.pop("connection_timeout", settings.SSH.HOST_CONNECTION_TIMEOUT) + self.port = kwargs.pop("port", settings.SSH.HOST_SSH_PORT) + self.key_filename = kwargs.pop("key_filename", settings.SSH.HOST_SSH_KEY_FILENAME) + self.ipv6 = kwargs.pop("ipv6", settings.SSH.HOST_IPV6) + self.ipv4_fallback = kwargs.pop("ipv4_fallback", settings.SSH.HOST_IPV4_FALLBACK) self.__dict__.update(kwargs) # Make every other kwarg an attribute self._session = None @@ -79,10 +85,11 @@ def session(self): If the session object does not exist, it will be created by calling the `connect` method. If the host is a non-SSH-enabled container host, a `ContainerSession` object will be created instead. """ - # This attribute may be missing after pickling - if not isinstance(getattr(self, "_session", None), Session): + if self._session is None: # Check to see if we're a non-ssh-enabled Container Host if hasattr(self, "_cont_inst") and not self._cont_inst.ports.get(22): + from broker.session import ContainerSession + runtime = "podman" if "podman" in str(self._cont_inst.client) else "docker" self._session = ContainerSession(self, runtime=runtime) else: @@ -110,6 +117,8 @@ def connect( ipv6 (bool): Whether or not to use IPv6. Defaults to False. ipv4_fallback (bool): Whether or not to fallback to IPv4 if IPv6 fails. Defaults to True. """ + from broker.session import Session + username = username or self.username password = password or self.password timeout = timeout or self.timeout @@ -135,8 +144,7 @@ def connect( def close(self): """Close the SSH connection to the host.""" - # This attribute may be missing after pickling - if isinstance(getattr(self, "_session", None), Session): + if self._session is not None: self._session.disconnect() self._session = None diff --git a/broker/providers/__init__.py b/broker/providers/__init__.py index ae264038..0df8fe75 100644 --- a/broker/providers/__init__.py +++ b/broker/providers/__init__.py @@ -120,7 +120,7 @@ def __init__(self, **kwargs): def _validate_settings(self, instance_name=None): """Load and validate provider settings. - Each provider's settings can include an instances list with specific instance + Each provider's settings can include an instances dict with specific instance details. One instance should have a "default" key set to True, if instances are defined. General provider settings should live on the top level for that provider. @@ -132,19 +132,17 @@ def _validate_settings(self, instance_name=None): if self._fresh_settings.get(section_name).get("instances"): fresh_settings = self._fresh_settings.get(section_name).copy() instance_name = instance_name or getattr(self, "instance", None) - # iterate through the instances and find the one that matches the instance_name - # if no instance matches, use the default instance - for candidate in fresh_settings.instances: - logger.debug("Checking %s against %s", instance_name, candidate) - if instance_name in candidate: - instance = candidate - break - elif candidate.values()[0].get("default") or len(fresh_settings.instances) == 1: - instance = candidate - self.instance, *_ = instance # store the instance name on the provider - fresh_settings.update(inst_vals := instance.values()[0]) + # first check to see if we have a direct match + if not (instance_values := fresh_settings.instances.get(instance_name)): + # if no direct match is found, or no instance is provided, find the default + for name, values in fresh_settings.instances.items(): + if values.get("default") or len(fresh_settings.instances) == 1: + instance_name, instance_values = name, values + break + self.instance = instance_name # store the instance name on the provider + fresh_settings.update(instance_values) settings[section_name] = fresh_settings - if not inst_vals.get("override_envars"): + if not instance_values.get("override_envars"): # if a provider instance doesn't want to override envars, load them settings.execute_loaders(loaders=[dynaconf.loaders.env_loader]) # use selective validation to only validate the instance settings diff --git a/broker/session.py b/broker/session.py index 4abddfd9..44abd610 100644 --- a/broker/session.py +++ b/broker/session.py @@ -19,7 +19,7 @@ from broker.settings import settings SSH_BACKENDS = ("ssh2-python", "ssh2-python312", "ansible-pylibssh", "hussh") -SSH_BACKEND = settings.SSH_BACKEND +SSH_BACKEND = settings.BACKEND logger.debug(f"{SSH_BACKEND=}") diff --git a/broker/settings.py b/broker/settings.py index 0bd758b9..b34ffea9 100644 --- a/broker/settings.py +++ b/broker/settings.py @@ -6,88 +6,28 @@ validate_settings: Function to validate the settings file. INTERACTIVE_MODE: Whether or not Broker is running in interactive mode. BROKER_DIRECTORY: The directory where Broker looks for its files. + TEST_MODE: Whether or not Broker is running in a pytest session. settings_path: The path to the settings file. inventory_path: The path to the inventory file. """ -import inspect import os from pathlib import Path +import sys import click from dynaconf import Dynaconf, Validator from dynaconf.validator import ValidationError +from broker.config_manager import ConfigManager from broker.exceptions import ConfigurationError - -def init_settings(settings_path, source, interactive=False, is_url=False): - """Initialize the broker settings file.""" - proceed = not False - if interactive: - try: - proceed = ( - click.prompt( - f"Get example file from {source}?\n", - type=click.Choice(["y", "n"]), - default="y", - ) - == "y" - ) - except click.core.Abort: - # We're likely in a different non-interactive environment (container?) - global INTERACTIVE_MODE - proceed, INTERACTIVE_MODE = True, False - if proceed: - # get example file from source - if is_url: - import requests - - click.echo(f"Downloading example file from: {source}") - raw_file = requests.get(source, timeout=60) - settings_path.write_text(raw_file.text) - else: - example_file = source.read_text() - settings_path.write_text(example_file) - if INTERACTIVE_MODE: - try: - click.edit(filename=str(settings_path.absolute())) - except click.exceptions.ClickException: - click.secho( - f"Please edit the file {settings_path.absolute()} and add your settings.", - fg="yellow", - ) - return True - - -def init_settings_from_github(settings_path, interactive=False): - """Initialize the broker settings file.""" - raw_url = ( - "https://raw.githubusercontent.com/SatelliteQE/broker/master/broker_settings.yaml.example" - ) - return init_settings(settings_path, raw_url, interactive, is_url=True) - - -def init_settings_from_local_repo(settings_path, interactive=False): - """Initialize the broker settings file.""" - example_path = Path(__file__).parent.parent.joinpath("broker_settings.yaml.example") - if not example_path.exists(): - return - return init_settings(settings_path, example_path, interactive) - - -INTERACTIVE_MODE = False -# GitHub action context -if "GITHUB_WORKFLOW" not in os.environ: - # determine if we're being ran from a CLI - for frame in inspect.stack()[::-1]: - if "/bin/broker" in frame.filename: - INTERACTIVE_MODE = True - break - - +INTERACTIVE_MODE = ConfigManager().interactive_mode BROKER_DIRECTORY = Path.home().joinpath(".broker") +TEST_MODE = "pytest" in sys.modules -if "BROKER_DIRECTORY" in os.environ: +if TEST_MODE: # when in test mode, don't use the real broker directory + BROKER_DIRECTORY = Path("tests/data/") +elif "BROKER_DIRECTORY" in os.environ: envar_location = Path(os.environ["BROKER_DIRECTORY"]) if envar_location.is_dir(): BROKER_DIRECTORY = envar_location @@ -97,23 +37,33 @@ def init_settings_from_local_repo(settings_path, interactive=False): settings_path = BROKER_DIRECTORY.joinpath("broker_settings.yaml") inventory_path = BROKER_DIRECTORY.joinpath("inventory.yaml") - -if not settings_path.exists(): - click.secho(f"Broker settings file not found at {settings_path.absolute()}.", fg="red") - if not (success := init_settings_from_local_repo(settings_path, interactive=INTERACTIVE_MODE)): - success = init_settings_from_github(settings_path, interactive=INTERACTIVE_MODE) - if not success: - raise ConfigurationError(f"Broker settings file not found at {settings_path.absolute()}.") +cfg_manager = ConfigManager(settings_path) + + +if cfg_manager._get_migrations() and not TEST_MODE: + if INTERACTIVE_MODE: + click.secho( + "Broker settings file has pending migrations.\n" + "Continuing without running the migrations may cause errors.", + fg="red", + ) + if click.confirm("Would you like to run the migrations now?"): + cfg_manager.migrate() + else: + click.secho("Continuing without running migrations.", fg="yellow") + else: + cfg_manager.migrate() validators = [ - Validator("HOST_USERNAME", default="root"), - Validator("HOST_PASSWORD", default="toor"), - Validator("HOST_CONNECTION_TIMEOUT", default=60), - Validator("HOST_SSH_PORT", default=22), - Validator("HOST_SSH_KEY_FILENAME", default=None), - Validator("HOST_IPV6", default=False), - Validator("HOST_IPV4_FALLBACK", default=True), - Validator("SSH_BACKEND", default="ssh2-python312"), + Validator("SSH", is_type_of=dict), + Validator("SSH.HOST_USERNAME", default="root"), + Validator("SSH.HOST_PASSWORD", default="toor"), + Validator("SSH.HOST_CONNECTION_TIMEOUT", default=60), + Validator("SSH.HOST_SSH_PORT", default=22), + Validator("SSH.HOST_SSH_KEY_FILENAME", default=None), + Validator("SSH.HOST_IPV6", default=False), + Validator("SSH.HOST_IPV4_FALLBACK", default=True), + Validator("SSH.BACKEND", default="ssh2-python312"), Validator("LOGGING", is_type_of=dict), Validator( "LOGGING.CONSOLE_LEVEL", @@ -125,6 +75,7 @@ def init_settings_from_local_repo(settings_path, interactive=False): is_in=["error", "warning", "info", "debug", "trace", "silent"], default="debug", ), + Validator("THREAD_LIMIT", default=None), ] # temporary fix for dynaconf #751 @@ -141,7 +92,7 @@ def init_settings_from_local_repo(settings_path, interactive=False): settings._loaders = [loader for loader in settings._loaders if "vault" not in loader] try: - settings.validators.validate() + settings.validators.validate(only="LOGGING") except ValidationError as err: raise ConfigurationError( f"Configuration error in {settings_path.absolute()}: {err.args[0]}" diff --git a/broker_settings.yaml.example b/broker_settings.yaml.example index cdb57e30..747d3841 100644 --- a/broker_settings.yaml.example +++ b/broker_settings.yaml.example @@ -1,19 +1,24 @@ # Broker settings +_version: 0.6.0 # different log levels for file and stdout logging: console_level: info file_level: debug -# Host Settings +# Optionally set a limit for the number of threads Broker can use for actions +thread_limit: None +# Host SSH Settings # These can be left alone if you're not using Broker as a library -host_username: root -host_password: "" -host_ssh_port: 22 -host_ssh_key_filename: "" -# Default all host ssh connections to IPv6 -host_ipv6: False -# If IPv6 connection attempts fail, fallback to IPv4 -host_ipv4_fallback: True -ssh_backend: ssh2-python312 +ssh: + # this is the library Broker should use to perform ssh actions + backend: ssh2-python312 + host_username: root + host_password: "" + host_ssh_port: 22 + host_ssh_key_filename: "" + # Default all host ssh connections to IPv6 + host_ipv6: False + # If IPv6 connection attempts fail, fallback to IPv4 + host_ipv4_fallback: True # Provider settings AnsibleTower: base_url: "https:///" @@ -30,14 +35,14 @@ AnsibleTower: results_limit: 50 Container: instances: - - docker: + docker: host_username: "" host_password: "" host_port: None runtime: docker network: null default: True - - remote: + remote: host: "" host_username: "" host_password: "" @@ -49,7 +54,7 @@ Container: auto_map_ports: False Foreman: instances: - - foreman1: + foreman1: foreman_url: https://test.fore.man foreman_username: admin foreman_password: secret @@ -57,7 +62,7 @@ Foreman: location: LOC verify: ./ca.crt default: true - - foreman2: + foreman2: foreman_url: https://other-test.fore.man foreman_username: admin foreman_password: secret @@ -67,25 +72,9 @@ Foreman: Beaker: hub_url: max_job_wait: 24h -TestProvider: - instances: - - test1: - foo: "bar" - default: True - - test2: - foo: "baz" - override_envars: True - - bad: - nothing: False - config_value: "something" # You can set a nickname as a shortcut for arguments nicks: - rhel7: - workflow: "deploy-rhel" - deploy_rhel_version: "7.9" + rhel9: + workflow: deploy-rhel + deploy_rhel_version: 9.4 notes: "Requested by broker" - test_nick: - test_action: "fake" - arg1: "abc" - arg2: 123 - arg3: True diff --git a/pyproject.toml b/pyproject.toml index cb393998..7e9c1ddd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,6 +29,7 @@ dependencies = [ "packaging", "pyyaml", "rich", + "rich_click", "setuptools", "ssh2-python312", ] diff --git a/tests/data/broker_settings.yaml b/tests/data/broker_settings.yaml new file mode 100644 index 00000000..c18e4724 --- /dev/null +++ b/tests/data/broker_settings.yaml @@ -0,0 +1,59 @@ +_version: 0.6.0 +logging: + console_level: info + file_level: debug +ssh: + backend: ssh2-python312 + host_username: root + host_password: + host_ssh_port: 22 + host_ssh_key_filename: + host_ipv6: false + host_ipv4_fallback: true +AnsibleTower: + base_url: https:/// + username: + token: + release_workflow: remove-vm + extend_workflow: extend-vm + new_expire_time: '+172800' + workflow_timeout: 3600 + results_limit: 50 +Container: + host_username: + host_password: + host_port: None + network: null + default: true + runtime: podman + results_limit: 50 + auto_map_ports: false +Foreman: + foreman_url: https://test.fore.man + foreman_username: admin + foreman_password: secret + organization: ORG + location: LOC + verify: ./ca.crt + name_prefix: broker +Beaker: + hub_url: null + max_job_wait: 24h +TestProvider: + instances: + test1: + foo: "bar" + default: True + test2: + foo: "baz" + override_envars: True + bad: + nothing: False + config_value: "something" +# You can set a nickname as a shortcut for arguments +nicks: + test_nick: + test_action: "fake" + arg1: "abc" + arg2: 123 + arg3: True diff --git a/tests/functional/test_containers.py b/tests/functional/test_containers.py index 2e91dca7..0847e868 100644 --- a/tests/functional/test_containers.py +++ b/tests/functional/test_containers.py @@ -20,13 +20,10 @@ def skip_if_not_configured(): @pytest.fixture(scope="module") -def temp_inventory(): - """Temporarily move the local inventory, then move it back when done""" - backup_path = inventory_path.rename(f"{inventory_path.absolute()}.bak") +def checkin_containers(): + """Checkin all containers checkout out by the tests.""" yield CliRunner().invoke(cli, ["checkin", "--all", "--filter", "_broker_provider Date: Fri, 20 Sep 2024 16:19:20 -0400 Subject: [PATCH 2/6] Respect the thread_limit setting included in the previous commit Due to timing, I couldn't cleanly include this in the previous commit, but here it is. Now Broker actions, including checkins will reference this new setting when determining how many threads it should use. --- broker/broker.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/broker/broker.py b/broker/broker.py index e94e280e..f499c9d7 100644 --- a/broker/broker.py +++ b/broker/broker.py @@ -24,6 +24,7 @@ from broker import exceptions, helpers from broker.hosts import Host from broker.providers import PROVIDER_ACTIONS, PROVIDERS, _provider_imports +from broker.settings import settings # load all the provider class so they are registered for _import in _provider_imports: @@ -86,7 +87,8 @@ def _act(self, provider, method, checkout=False): method_obj = getattr(provider_inst, method) logger.debug(f"On {provider_inst=} executing {method_obj=} with params {self._kwargs=}.") # Overkill for a single action, cleaner than splitting the logic - with ThreadPoolExecutor() as workers: + max_workers = min(count, int(settings.thread_limit)) if settings.thread_limit else None + with ThreadPoolExecutor(max_workers=max_workers) as workers: tasks = [workers.submit(method_obj, **self._kwargs) for _ in range(count)] result = [] for task in as_completed(tasks): @@ -202,8 +204,8 @@ def checkin(self, sequential=False, host=None, in_context=False): if not hosts: logger.debug("Checkin called with no hosts, taking no action") return - - with ThreadPoolExecutor(max_workers=1 if sequential else None) as workers: + max_workers = min(len(hosts), int(settings.thread_limit)) if settings.thread_limit else None + with ThreadPoolExecutor(max_workers=1 if sequential else max_workers) as workers: completed_checkins = as_completed( # reversing over a copy of the list to avoid skipping workers.submit(self._checkin, _host) From f130b747d85fe043c533dbea12acb5b6e7e699fc Mon Sep 17 00:00:00 2001 From: Jacob Callahan Date: Fri, 20 Sep 2024 16:37:24 -0400 Subject: [PATCH 3/6] Allow users to init the config (or chunk) from a custom location With this change, users can now specify a `--from` argument with a valid path or url to a valid broker settings file. This will still try to fallback on the current behavior of using a local repo or the main SatelliteQE/broker repo as the source. --- broker/commands.py | 12 ++++++++---- broker/config_manager.py | 38 ++++++++++++++++++++++++++------------ 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/broker/commands.py b/broker/commands.py index 5f17c7c7..96dd2a3c 100644 --- a/broker/commands.py +++ b/broker/commands.py @@ -475,12 +475,15 @@ def restore(): @loggedcli(group=config) @click.argument("chunk", type=str, required=False) -def init(chunk): +@click.option("--from", "_from", type=str, help="A file path or URL to initialize the config from.") +def init(chunk=None, _from=None): """Initialize the broker configuration file from your local clone or GitHub. You can also init specific chunks by passing the chunk name. + Additionally, if you want to initialize from a file or URL, you can pass the `--from` flag. + Keep in mind that the file and url contents need to be valid yaml. """ - ConfigManager(settings.settings_path).init_config_file(chunk) + ConfigManager(settings.settings_path).init_config_file(chunk=chunk, _from=_from) @loggedcli(group=config) @@ -503,9 +506,10 @@ def nick(nick, no_syntax): @loggedcli(group=config) -def migrate(): +@click.option("-f", "--force-version", type=str, help="Force the migration to a specific version") +def migrate(force_version=None): """Migrate the broker configuration file to the latest version.""" - ConfigManager(settings.settings_path).migrate() + ConfigManager(settings.settings_path).migrate(force_version=force_version) @loggedcli(group=config) diff --git a/broker/config_manager.py b/broker/config_manager.py index 25f0a2b0..b4782668 100644 --- a/broker/config_manager.py +++ b/broker/config_manager.py @@ -110,16 +110,22 @@ def _import_config(self, source, is_url=False): else: return source.read_text() - def _get_migrations(self): + def _get_migrations(self, force_version=None): """Construct a list of all applicable migrations.""" from broker import config_migrations config_version = Version(self._cfg.get("_version", "0.0.0")) + if force_version: + force_version = Version(force_version) migrations = [] for _, name, _ in pkgutil.iter_modules(config_migrations.__path__): module = importlib.import_module(f"broker.config_migrations.{name}") - if hasattr(module, "run_migrations") and config_version < file_name_to_ver(name): - migrations.append(module) + if hasattr(module, "run_migrations"): + if force_version and force_version == file_name_to_ver(name): + migrations.append(module) + break + elif config_version < file_name_to_ver(name): + migrations.append(module) return migrations def backup(self): @@ -199,26 +205,34 @@ def nicks(self, nick=None): return nicks[nick] return list(nicks.keys()) - def init_config_file(self, chunk=None): + def init_config_file(self, chunk=None, _from=None): """Check for the existence of the config file and create it if it doesn't exist.""" if self.interactive_mode and self._settings_path.exists() and not chunk: # if the file exists, ask the user if they want to overwrite it if ( click.prompt( - f"Settings file already exists at {self._settings_path.absolute()}. Overwrite?", + f"Overwrite the settings file at {self._settings_path.absolute()}. Overwrite?", type=click.Choice(["y", "n"]), default="n", ) != "y" ): return - # get the example file from the local repo or GitHub - example_path = Path(__file__).parent.parent.joinpath("broker_settings.yaml.example") raw_data = None - if example_path.exists(): - raw_data = self._import_config(example_path) + if _from: + # determine if this is a local file or a URL + if Path(_from).exists(): + raw_data = self._import_config(Path(_from)) + else: + raw_data = self._import_config(_from, is_url=True) + # if we still don't have data, get the example file from the local repo or GitHub if not raw_data: - raw_data = self._import_config(GH_CFG, is_url=True) + # get the example file from the local repo or GitHub + example_path = Path(__file__).parent.parent.joinpath("broker_settings.yaml.example") + if example_path.exists(): + raw_data = self._import_config(example_path) + if not raw_data: + raw_data = self._import_config(GH_CFG, is_url=True) if not raw_data: raise exceptions.ConfigurationError( f"Broker settings file not found at {self._settings_path.absolute()}." @@ -228,10 +242,10 @@ def init_config_file(self, chunk=None): chunk_data = self._interactive_edit(chunk_data) self.update(chunk, chunk_data) - def migrate(self): + def migrate(self, force_version=None): """Migrate the config from a previous version of Broker.""" # get all available migrations - if not (migrations := self._get_migrations()): + if not (migrations := self._get_migrations(force_version)): logger.info("No migrations are applicable to your config.") return # run all migrations in order From 45efe37770e4a3637c0164ec37dcbebced6dac73 Mon Sep 17 00:00:00 2001 From: Jacob Callahan Date: Fri, 27 Sep 2024 10:47:03 -0400 Subject: [PATCH 4/6] Change cli terminology from VMs to Hosts Broker has expanded a while ago with the types of hosts it can support, it's time we updated our terminology to match. --- broker/commands.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/broker/commands.py b/broker/commands.py index 96dd2a3c..885cfbc2 100644 --- a/broker/commands.py +++ b/broker/commands.py @@ -264,22 +264,22 @@ def providers(): @loggedcli() -@click.argument("vm", type=str, nargs=-1) +@click.argument("hosts", type=str, nargs=-1) @click.option("-b", "--background", is_flag=True, help="Run checkin in the background") -@click.option("--all", "all_", is_flag=True, help="Select all VMs") +@click.option("--all", "all_", is_flag=True, help="Select all hosts") @click.option("--sequential", is_flag=True, help="Run checkins sequentially") @click.option("--filter", type=str, help="Checkin only what matches the specified filter") -def checkin(vm, background, all_, sequential, filter): - """Checkin or "remove" a VM or series of VM broker instances. +def checkin(hosts, background, all_, sequential, filter): + """Checkin or "remove" a host or series of hosts. - COMMAND: broker checkin ||--all + COMMAND: broker checkin ||--all """ if background: helpers.fork_broker() inventory = helpers.load_inventory(filter=filter) to_remove = [] for num, host in enumerate(inventory): - if str(num) in vm or host.get("hostname") in vm or host.get("name") in vm or all_: + if str(num) in hosts or host.get("hostname") in hosts or host.get("name") in hosts or all_: to_remove.append(Broker().reconstruct_host(host)) Broker(hosts=to_remove).checkin(sequential=sequential) @@ -294,7 +294,7 @@ def checkin(vm, background, all_, sequential, filter): ) @click.option("--filter", type=str, help="Display only what matches the specified filter") def inventory(details, curated, sync, filter): - """Get a list of all VMs you've checked out showing hostname and local id. + """Get a list of all hosts you've checked out showing hostname and local id. hostname pulled from list of dictionaries. """ @@ -332,9 +332,9 @@ def inventory(details, curated, sync, filter): @loggedcli() -@click.argument("vm", type=str, nargs=-1) +@click.argument("hosts", type=str, nargs=-1) @click.option("-b", "--background", is_flag=True, help="Run extend in the background") -@click.option("--all", "all_", is_flag=True, help="Select all VMs") +@click.option("--all", "all_", is_flag=True, help="Select all hosts") @click.option("--sequential", is_flag=True, help="Run extends sequentially") @click.option("--filter", type=str, help="Extend only what matches the specified filter") @click.option( @@ -346,10 +346,10 @@ def inventory(details, curated, sync, filter): " labels (e.g. '-l k1=v1,k2=v2,k3=v3=z4').", ) @provider_options -def extend(vm, background, all_, sequential, filter, **kwargs): +def extend(hosts, background, all_, sequential, filter, **kwargs): """Extend a host's lease time. - COMMAND: broker extend |||--all + COMMAND: broker extend |||--all """ broker_args = helpers.clean_dict(kwargs) if background: @@ -357,7 +357,7 @@ def extend(vm, background, all_, sequential, filter, **kwargs): inventory = helpers.load_inventory(filter=filter) to_extend = [] for num, host in enumerate(inventory): - if str(num) in vm or host["hostname"] in vm or host.get("name") in vm or all_: + if str(num) in hosts or host["hostname"] in hosts or host.get("name") in hosts or all_: to_extend.append(Broker().reconstruct_host(host)) Broker(hosts=to_extend, **broker_args).extend(sequential=sequential) From 0f937aebe624328b124a11bbf71329c5564c79b0 Mon Sep 17 00:00:00 2001 From: Jacob Callahan Date: Fri, 27 Sep 2024 12:03:23 -0400 Subject: [PATCH 5/6] Rework config manager's validation A number of changes were needed both functionally and stylistically to get this where we want it, but it's pretty good now. I don't know if I like the "Validation passed!" message, since it will likely print every time, but it can stay for now. --- broker/commands.py | 3 +-- broker/config_manager.py | 49 +++++++++++++++++++++------------------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/broker/commands.py b/broker/commands.py index 885cfbc2..fd414618 100644 --- a/broker/commands.py +++ b/broker/commands.py @@ -208,8 +208,7 @@ def cli(version): table.add_row("Log File", f"{settings.BROKER_DIRECTORY.absolute()}/logs/broker.log") # Print the table - console = Console() - console.print(table) + CONSOLE.print(table) @loggedcli(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) diff --git a/broker/config_manager.py b/broker/config_manager.py index b4782668..6a6d6bf5 100644 --- a/broker/config_manager.py +++ b/broker/config_manager.py @@ -2,11 +2,11 @@ import importlib from importlib.metadata import version -import inspect import json import os from pathlib import Path import pkgutil +import sys from tempfile import NamedTemporaryFile import click @@ -67,10 +67,8 @@ def interactive_mode(self): # GitHub action context if "GITHUB_WORKFLOW" not in os.environ: # determine if we're being ran from a CLI - for frame in inspect.stack()[::-1]: - if "/bin/broker" in frame.filename: - self._interactive_mode = True - break + if sys.stdin.isatty(): + self._interactive_mode = True return self._interactive_mode def _interactive_edit(self, chunk): @@ -156,7 +154,7 @@ def edit(self, chunk=None, content=None): new_val = self._interactive_edit(content) self.update(chunk, new_val) - def get(self, chunk=None, curr_chunk=None): + def get(self, chunk=None, curr_chunk=None, suppress=False): """Get a chunk of Broker's config or the whole config.""" if not curr_chunk: curr_chunk = self._cfg @@ -171,6 +169,8 @@ def get(self, chunk=None, curr_chunk=None): try: return curr_chunk[chunk] except KeyError: + if suppress: + return raise exceptions.UserError(f"Chunk '{chunk}' not found in the config.") def update(self, chunk, new_val, curr_chunk=None): @@ -260,35 +260,38 @@ def migrate(self, force_version=None): def validate(self, chunk, providers=None): """Validate a top-level chunk of Broker's config.""" - if "." in chunk: # only validate the top-level chunk - chunk = chunk.split(".")[0] - if chunk.lower() == "base": # validate the base config - return # this happens before we're called + if chunk == "all": + all_settings = [prov for prov in providers if prov != "TestProvider"] + ["base", "ssh"] + for item in all_settings: + self.validate(item, providers) + return + chunk = chunk.split(".")[0] if "." in chunk else chunk + if chunk.lower() == "base": + return if chunk.lower() == "ssh": from broker.settings import settings + logger.info("Validating SSH settings.") settings.validators.validate(only="SSH") return if providers is None: raise exceptions.UserError( "Attempted to validate provider settings without passing providers." ) + instance_settings = {} if ":" in chunk: - prov_name, instance = chunk.split(":") - providers[prov_name](**{prov_name: instance}) - if chunk == "all": - for prov_name, prov_cls in providers.items(): - if prov_name == "TestProvider": - continue - logger.info(f"Validating {prov_name} provider settings.") - try: # we want to suppress all exceptions here to allow each provider to validate - prov_cls() - except Exception as err: # noqa: BLE001 - logger.warning(f"Provider {prov_name} failed validation: {err}") - return + chunk, instance = chunk.split(":") + instance_settings = {chunk: instance} if chunk not in providers: raise exceptions.UserError( "I don't know how to validate that.\n" "If it's important, it is likely covered in the base validations." ) - providers[chunk]() + if not self.get(chunk, suppress=True): + logger.warning(f"No settings found for {chunk} provider.") + return + logger.info(f"Validating {chunk} provider settings.") + try: + providers[chunk](**instance_settings) + except Exception as err: # noqa: BLE001 + logger.warning(f"Provider {chunk} failed validation: {err}") From 23a7025426f1ad1bec18433cf90fd17f1f9f12ac Mon Sep 17 00:00:00 2001 From: Jacob Callahan Date: Fri, 27 Sep 2024 17:09:33 -0400 Subject: [PATCH 6/6] Switch to ruamel to keep comments in yaml files PyYaml is a good library, but had the unfortunate side-effect of stripping comments from yaml files on read/write. Switching to ruamel now let's us retain the comments, and arguably improves usage in some ways. There may be some issues with awxkit objects due to the way the representers are added, but I can't say for certain that is the case yet. --- broker/config_manager.py | 50 +++------ broker/helpers.py | 25 ++--- broker/providers/ansible_tower.py | 27 +++-- broker/session.py | 3 +- broker/settings.py | 6 +- pyproject.toml | 164 ++++++++++++++---------------- tests/conftest.py | 5 + tests/test_config_manager.py | 8 +- 8 files changed, 134 insertions(+), 154 deletions(-) diff --git a/broker/config_manager.py b/broker/config_manager.py index 6a6d6bf5..fff31723 100644 --- a/broker/config_manager.py +++ b/broker/config_manager.py @@ -3,7 +3,6 @@ import importlib from importlib.metadata import version import json -import os from pathlib import Path import pkgutil import sys @@ -12,10 +11,14 @@ import click from logzero import logger from packaging.version import Version -import yaml +from ruamel.yaml import YAML, YAMLError from broker import exceptions +yaml = YAML() +yaml.default_flow_style = False +yaml.sort_keys = False + C_SEP = "." # chunk separator GH_CFG = "https://raw.githubusercontent.com/SatelliteQE/broker/master/broker_settings.yaml.example" @@ -25,14 +28,6 @@ def file_name_to_ver(file_name): return Version(file_name[1:].replace("_", ".")) -def yaml_format(data): - """Format the data as yaml. - - Duplicating here to avoid circular imports. - """ - return yaml.dump(data, default_flow_style=False, sort_keys=False) - - class ConfigManager: """Class to interact with Broker's configuration file. @@ -45,44 +40,31 @@ class ConfigManager: e.g. broker config view AnsibleTower.instances.my_instance """ + interactive_mode = sys.stdin.isatty() version = version("broker") def __init__(self, settings_path=None): - self._interactive_mode = None self._settings_path = settings_path if settings_path: if settings_path.exists(): - self._cfg = yaml.safe_load(self._settings_path.read_text()) + self._cfg = yaml.load(self._settings_path) else: click.secho( f"Broker settings file not found at {settings_path.absolute()}.", fg="red" ) self.init_config_file() - @property - def interactive_mode(self): - """Determine if Broker is running in interactive mode.""" - if self._interactive_mode is None: - self._interactive_mode = False - # GitHub action context - if "GITHUB_WORKFLOW" not in os.environ: - # determine if we're being ran from a CLI - if sys.stdin.isatty(): - self._interactive_mode = True - return self._interactive_mode - def _interactive_edit(self, chunk): """Write the chunk data to a temporary file and open it in an editor.""" with NamedTemporaryFile(mode="w+", suffix=".yaml") as tmp: - tmp.write(yaml_format(chunk)) - tmp.seek(0) + yaml.dump(chunk, tmp) click.edit(filename=tmp.name) tmp.seek(0) new_data = tmp.read() # first try to load it as yaml try: - return yaml.safe_load(new_data) - except yaml.YAMLError: # then try json + return yaml.load(new_data) + except YAMLError: # then try json try: return json.loads(new_data) except json.JSONDecodeError: # finally, just return the raw data @@ -187,9 +169,7 @@ def update(self, chunk, new_val, curr_chunk=None): # update the config file if it exists if self._settings_path.exists(): self.backup() - self._settings_path.write_text( - yaml.dump(self._cfg, default_flow_style=False, sort_keys=False) - ) + yaml.dump(self._cfg, self._settings_path) else: # we're not at the top level, so keep going down if C_SEP in chunk: curr, chunk = chunk.split(C_SEP, 1) @@ -237,7 +217,7 @@ def init_config_file(self, chunk=None, _from=None): raise exceptions.ConfigurationError( f"Broker settings file not found at {self._settings_path.absolute()}." ) - chunk_data = self.get(chunk, yaml.safe_load(raw_data)) + chunk_data = self.get(chunk, yaml.load(raw_data)) if self.interactive_mode: chunk_data = self._interactive_edit(chunk_data) self.update(chunk, chunk_data) @@ -253,9 +233,7 @@ def migrate(self, force_version=None): for migration in sorted(migrations, key=lambda m: m.TO_VERSION): working_config = migration.run_migrations(working_config) self.backup() - self._settings_path.write_text( - yaml.dump(working_config, default_flow_style=False, sort_keys=False) - ) + yaml.dump(working_config, self._settings_path) logger.info("Config migration complete.") def validate(self, chunk, providers=None): @@ -265,7 +243,7 @@ def validate(self, chunk, providers=None): for item in all_settings: self.validate(item, providers) return - chunk = chunk.split(".")[0] if "." in chunk else chunk + chunk = chunk.split(C_SEP)[0] if C_SEP in chunk else chunk if chunk.lower() == "base": return if chunk.lower() == "ssh": diff --git a/broker/helpers.py b/broker/helpers.py index 835ca633..b65ae558 100644 --- a/broker/helpers.py +++ b/broker/helpers.py @@ -7,6 +7,7 @@ from copy import deepcopy import getpass import inspect +from io import BytesIO import json import os from pathlib import Path @@ -18,13 +19,17 @@ import click from logzero import logger -import yaml +from ruamel.yaml import YAML from broker import exceptions, logger as b_log, settings FilterTest = namedtuple("FilterTest", "haystack needle test") INVENTORY_LOCK = threading.Lock() +yaml = YAML() +yaml.default_flow_style = False +yaml.sort_keys = False + def clean_dict(in_dict): """Remove entries from a dict where value is None.""" @@ -167,15 +172,10 @@ def load_file(file, warn=True): if warn: logger.warning(f"File {file.absolute()} is invalid or does not exist.") return [] - loader_args = {} if file.suffix == ".json": - loader = json + return json.loads(file.read_text()) elif file.suffix in (".yaml", ".yml"): - loader = yaml - loader_args = {"Loader": yaml.FullLoader} - with file.open() as f: - data = loader.load(f, **loader_args) or [] - return data + return yaml.load(file) def resolve_file_args(broker_args): @@ -251,8 +251,7 @@ def update_inventory(add=None, remove=None): inv_data.extend(add) settings.inventory_path.touch() - with settings.inventory_path.open("w") as inv_file: - yaml.dump(inv_data, inv_file) + yaml.dump(inv_data, settings.inventory_path) def yaml_format(in_struct): @@ -263,8 +262,10 @@ def yaml_format(in_struct): :return: yaml-formatted string """ if isinstance(in_struct, str): - in_struct = yaml.load(in_struct, Loader=yaml.FullLoader) - return yaml.dump(in_struct, default_flow_style=False, sort_keys=False) + in_struct = yaml.load(in_struct) + output = BytesIO() # ruamel doesn't natively allow for string output + yaml.dump(in_struct, output) + return output.getvalue().decode("utf-8") def flip_provider_actions(provider_actions): diff --git a/broker/providers/ansible_tower.py b/broker/providers/ansible_tower.py index 4973b03a..f7c67e15 100644 --- a/broker/providers/ansible_tower.py +++ b/broker/providers/ansible_tower.py @@ -1,4 +1,5 @@ """Ansible Tower provider implementation.""" + from functools import cache, cached_property import inspect import json @@ -7,7 +8,6 @@ import click from dynaconf import Validator from logzero import logger -import yaml from broker import exceptions from broker.helpers import eval_filter, find_origin @@ -22,6 +22,19 @@ from broker.providers import Provider +def convert_psuedonamespaces(attr_dict): + """Recursively convert PsuedoNamespace objects into dictionaries.""" + out_dict = {} + for key, value in attr_dict.items(): + if isinstance(value, awxkit.utils.PseudoNamespace): + out_dict[key] = dict(value) + elif isinstance(value, dict): + out_dict[key] = convert_psuedonamespaces(value) + else: + out_dict[key] = value + return out_dict + + class JobExecutionError(exceptions.ProviderError): """Raised when a job execution fails.""" @@ -203,11 +216,11 @@ def _set_attributes(self, host_inst, broker_args=None, misc_attrs=None): "release": self._host_release, "_prov_inst": self, "_broker_provider": "AnsibleTower", - "_broker_args": broker_args, + "_broker_args": convert_psuedonamespaces(broker_args), } ) if isinstance(misc_attrs, dict): - host_inst.__dict__.update(misc_attrs) + host_inst.__dict__.update(convert_psuedonamespaces(misc_attrs)) def _translate_inventory(self, inventory): if isinstance(inventory, int): # already an id, silly @@ -776,11 +789,3 @@ def release(self, name, broker_args=None): source_vm=name, **broker_args, ) - - -def awxkit_representer(dumper, data): - """In order to resolve awxkit objects, a custom representer is needed.""" - return dumper.represent_dict(dict(data)) - - -yaml.add_representer(awxkit.utils.PseudoNamespace, awxkit_representer) diff --git a/broker/session.py b/broker/session.py index 44abd610..fd886b2f 100644 --- a/broker/session.py +++ b/broker/session.py @@ -8,6 +8,7 @@ Note: You typically want to use a Host object instance to create sessions, not these classes directly. """ + from contextlib import contextmanager from pathlib import Path import tempfile @@ -19,7 +20,7 @@ from broker.settings import settings SSH_BACKENDS = ("ssh2-python", "ssh2-python312", "ansible-pylibssh", "hussh") -SSH_BACKEND = settings.BACKEND +SSH_BACKEND = settings.SSH.BACKEND logger.debug(f"{SSH_BACKEND=}") diff --git a/broker/settings.py b/broker/settings.py index b34ffea9..20101e6e 100644 --- a/broker/settings.py +++ b/broker/settings.py @@ -10,9 +10,9 @@ settings_path: The path to the settings file. inventory_path: The path to the inventory file. """ + import os from pathlib import Path -import sys import click from dynaconf import Dynaconf, Validator @@ -21,9 +21,9 @@ from broker.config_manager import ConfigManager from broker.exceptions import ConfigurationError -INTERACTIVE_MODE = ConfigManager().interactive_mode +INTERACTIVE_MODE = ConfigManager.interactive_mode BROKER_DIRECTORY = Path.home().joinpath(".broker") -TEST_MODE = "pytest" in sys.modules +TEST_MODE = os.environ.get("BROKER_TEST_MODE", False) if TEST_MODE: # when in test mode, don't use the real broker directory BROKER_DIRECTORY = Path("tests/data/") diff --git a/pyproject.toml b/pyproject.toml index 7e9c1ddd..1734af14 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,9 +8,7 @@ description = "The infrastructure middleman." readme = "README.md" requires-python = ">=3.10" keywords = ["broker", "AnsibleTower", "docker", "podman", "beaker"] -authors = [ - {name = "Jacob J Callahan", email = "jacob.callahan05@gmail.com"} -] +authors = [{ name = "Jacob J Callahan", email = "jacob.callahan05@gmail.com" }] classifiers = [ "Development Status :: 4 - Beta", "Intended Audience :: Developers", @@ -27,33 +25,25 @@ dependencies = [ "dynaconf<4.0.0", "logzero", "packaging", - "pyyaml", "rich", "rich_click", + "ruamel.yaml", "setuptools", "ssh2-python312", ] -dynamic = ["version"] # dynamic fields to update on build - version via setuptools_scm +dynamic = [ + "version", +] # dynamic fields to update on build - version via setuptools_scm [project.urls] Repository = "https://github.com/SatelliteQE/broker" [project.optional-dependencies] beaker = ["beaker-client"] -dev = [ - "pre-commit", - "pytest", - "ruff" -] -docker = [ - "docker", - "paramiko" -] +dev = ["pre-commit", "pytest", "ruff"] +docker = ["docker", "paramiko"] podman = ["podman>=5.2"] -setup = [ - "build", - "twine", -] +setup = ["build", "twine"] ssh2_py311 = ["ssh2-python"] ssh2_python = ["ssh2-python"] @@ -72,7 +62,7 @@ include-package-data = true [tool.setuptools.packages.find] include = ["broker"] -[tool.setuptools_scm] # same as use_scm_version=True in setup.py +[tool.setuptools_scm] # same as use_scm_version=True in setup.py [tool.pytest.ini_options] testpaths = ["tests"] @@ -85,82 +75,82 @@ target-version = "py311" fixable = ["ALL"] select = [ - "B002", # Python does not support the unary prefix increment - "B007", # Loop control variable {name} not used within loop body - "B009", # Do not call getattr with a constant attribute value - "B010", # Do not call setattr with a constant attribute value - "B011", # Do not `assert False`, raise `AssertionError` instead - "B013", # Redundant tuple in exception handler - "B014", # Exception handler with duplicate exception - "B023", # Function definition does not bind loop variable {name} - "B026", # Star-arg unpacking after a keyword argument is strongly discouraged - "BLE001", # Using bare except clauses is prohibited - "C", # complexity - "C4", # flake8-comprehensions - "COM818", # Trailing comma on bare tuple prohibited - "D", # docstrings - "E", # pycodestyle - "F", # pyflakes/autoflake - "G", # flake8-logging-format - "I", # isort - "ISC001", # Implicitly concatenated string literals on one line - "N804", # First argument of a class method should be named cls - "N805", # First argument of a method should be named self - "N815", # Variable {name} in class scope should not be mixedCase - "N999", # Invalid module name: '{name}' - "PERF", # Perflint rules - "PGH004", # Use specific rule codes when using noqa + "B002", # Python does not support the unary prefix increment + "B007", # Loop control variable {name} not used within loop body + "B009", # Do not call getattr with a constant attribute value + "B010", # Do not call setattr with a constant attribute value + "B011", # Do not `assert False`, raise `AssertionError` instead + "B013", # Redundant tuple in exception handler + "B014", # Exception handler with duplicate exception + "B023", # Function definition does not bind loop variable {name} + "B026", # Star-arg unpacking after a keyword argument is strongly discouraged + "BLE001", # Using bare except clauses is prohibited + "C", # complexity + "C4", # flake8-comprehensions + "COM818", # Trailing comma on bare tuple prohibited + "D", # docstrings + "E", # pycodestyle + "F", # pyflakes/autoflake + "G", # flake8-logging-format + "I", # isort + "ISC001", # Implicitly concatenated string literals on one line + "N804", # First argument of a class method should be named cls + "N805", # First argument of a method should be named self + "N815", # Variable {name} in class scope should not be mixedCase + "N999", # Invalid module name: '{name}' + "PERF", # Perflint rules + "PGH004", # Use specific rule codes when using noqa "PLC0414", # Useless import alias. Import alias does not rename original package. - "PLC", # pylint - "PLE", # pylint - "PLR", # pylint - "PLW", # pylint - "PTH", # Use pathlib - "RUF", # Ruff-specific rules - "S103", # bad-file-permissions - "S108", # hardcoded-temp-file - "S110", # try-except-pass - "S112", # try-except-continue - "S113", # Probable use of requests call without timeout - "S306", # suspicious-mktemp-usage - "S307", # suspicious-eval-usage - "S601", # paramiko-call - "S602", # subprocess-popen-with-shell-equals-true - "S604", # call-with-shell-equals-true - "S609", # unix-command-wildcard-injection - "SIM105", # Use contextlib.suppress({exception}) instead of try-except-pass - "SIM117", # Merge with-statements that use the same scope - "SIM118", # Use {key} in {dict} instead of {key} in {dict}.keys() - "SIM201", # Use {left} != {right} instead of not {left} == {right} - "SIM208", # Use {expr} instead of not (not {expr}) - "SIM212", # Use {a} if {a} else {b} instead of {b} if not {a} else {a} - "SIM300", # Yoda conditions. Use 'age == 42' instead of '42 == age'. - "SIM401", # Use get from dict with default instead of an if block - "T100", # Trace found: {name} used - "T20", # flake8-print - "TRY004", # Prefer TypeError exception for invalid type - "TRY302", # Remove exception handler; error is immediately re-raised + "PLC", # pylint + "PLE", # pylint + "PLR", # pylint + "PLW", # pylint + "PTH", # Use pathlib + "RUF", # Ruff-specific rules + "S103", # bad-file-permissions + "S108", # hardcoded-temp-file + "S110", # try-except-pass + "S112", # try-except-continue + "S113", # Probable use of requests call without timeout + "S306", # suspicious-mktemp-usage + "S307", # suspicious-eval-usage + "S601", # paramiko-call + "S602", # subprocess-popen-with-shell-equals-true + "S604", # call-with-shell-equals-true + "S609", # unix-command-wildcard-injection + "SIM105", # Use contextlib.suppress({exception}) instead of try-except-pass + "SIM117", # Merge with-statements that use the same scope + "SIM118", # Use {key} in {dict} instead of {key} in {dict}.keys() + "SIM201", # Use {left} != {right} instead of not {left} == {right} + "SIM208", # Use {expr} instead of not (not {expr}) + "SIM212", # Use {a} if {a} else {b} instead of {b} if not {a} else {a} + "SIM300", # Yoda conditions. Use 'age == 42' instead of '42 == age'. + "SIM401", # Use get from dict with default instead of an if block + "T100", # Trace found: {name} used + "T20", # flake8-print + "TRY004", # Prefer TypeError exception for invalid type + "TRY302", # Remove exception handler; error is immediately re-raised "PLR0911", # Too many return statements ({returns} > {max_returns}) "PLR0912", # Too many branches ({branches} > {max_branches}) "PLR0915", # Too many statements ({statements} > {max_statements}) "PLR2004", # Magic value used in comparison, consider replacing {value} with a constant variable "PLW2901", # Outer {outer_kind} variable {name} overwritten by inner {inner_kind} target - "UP", # pyupgrade - "W", # pycodestyle + "UP", # pyupgrade + "W", # pycodestyle ] ignore = [ - "ANN", # flake8-annotations - "D203", # 1 blank line required before class docstring - "D213", # Multi-line docstring summary should start at the second line - "D406", # Section name should end with a newline - "D407", # Section name underlining - "D413", # Missing blank line after last section - "E501", # line too long - "E731", # do not assign a lambda expression, use a def + "ANN", # flake8-annotations + "D203", # 1 blank line required before class docstring + "D213", # Multi-line docstring summary should start at the second line + "D406", # Section name should end with a newline + "D407", # Section name underlining + "D413", # Missing blank line after last section + "E501", # line too long + "E731", # do not assign a lambda expression, use a def "PLR0913", # Too many arguments to function call ({c_args} > {max_args}) - "RUF012", # Mutable class attributes should be annotated with typing.ClassVar - "D107", # Missing docstring in __init__ + "RUF012", # Mutable class attributes should be annotated with typing.ClassVar + "D107", # Missing docstring in __init__ ] [tool.ruff.flake8-pytest-style] @@ -168,9 +158,7 @@ fixture-parentheses = false [tool.ruff.isort] force-sort-within-sections = true -known-first-party = [ - "broker", -] +known-first-party = ["broker"] combine-as-imports = true [tool.ruff.per-file-ignores] diff --git a/tests/conftest.py b/tests/conftest.py index 52e2e58f..19cc5189 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,11 @@ import pytest +def pytest_sessionstart(session): + """Put Broker into test mode.""" + os.environ["BROKER_TEST_MODE"] = "True" + + @pytest.fixture def set_envars(request): """Set and unset one or more environment variables""" diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 798c53a2..c6ee7033 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -1,9 +1,11 @@ """Test file for broker.config_manager module.""" -import yaml +from ruamel.yaml import YAML from broker.config_manager import ConfigManager, GH_CFG from broker.settings import settings_path -TEST_CFG_DATA = yaml.safe_load(settings_path.read_text()) + +yaml = YAML() +TEST_CFG_DATA = yaml.load(settings_path) def test_basic_assertions(): @@ -19,7 +21,7 @@ def test_import_config(): cfg_mgr = ConfigManager() result = cfg_mgr._import_config(GH_CFG, is_url=True) assert isinstance(result, str) - converted = yaml.safe_load(result) + converted = yaml.load(result) assert isinstance(converted, dict) def test_get_e2e():