From 17f44ac919431da7b503a84f941364250655e006 Mon Sep 17 00:00:00 2001 From: mibe Date: Tue, 26 Mar 2024 09:20:50 +0000 Subject: [PATCH 1/4] Add documentation build folder to .gitignore --- .gitignore | 1 + {changes => doc/changes}/changelog.md | 0 {changes => doc/changes}/changes_0.1.0.md | 0 exasol/python_extension_common/__init__.py | 0 .../deployment/language_container_deployer.py | 291 ++++++++++++++++++ .../language_container_deployer_cli.py | 168 ++++++++++ pyproject.toml | 15 + .../test_language_container_deployer.py | 133 ++++++++ .../test_language_container_deployer_cli.py | 29 ++ 9 files changed, 637 insertions(+) create mode 100644 .gitignore rename {changes => doc/changes}/changelog.md (100%) rename {changes => doc/changes}/changes_0.1.0.md (100%) create mode 100644 exasol/python_extension_common/__init__.py create mode 100644 exasol/python_extension_common/deployment/language_container_deployer.py create mode 100644 exasol/python_extension_common/deployment/language_container_deployer_cli.py create mode 100644 pyproject.toml create mode 100644 test/unit/deployment/test_language_container_deployer.py create mode 100644 test/unit/deployment/test_language_container_deployer_cli.py diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..8568120 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +.html-documentation diff --git a/changes/changelog.md b/doc/changes/changelog.md similarity index 100% rename from changes/changelog.md rename to doc/changes/changelog.md diff --git a/changes/changes_0.1.0.md b/doc/changes/changes_0.1.0.md similarity index 100% rename from changes/changes_0.1.0.md rename to doc/changes/changes_0.1.0.md diff --git a/exasol/python_extension_common/__init__.py b/exasol/python_extension_common/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/exasol/python_extension_common/deployment/language_container_deployer.py b/exasol/python_extension_common/deployment/language_container_deployer.py new file mode 100644 index 0000000..c260965 --- /dev/null +++ b/exasol/python_extension_common/deployment/language_container_deployer.py @@ -0,0 +1,291 @@ +from enum import Enum +from textwrap import dedent +from typing import List, Optional +from pathlib import Path, PurePosixPath +import logging +import tempfile +import requests +import ssl +import pyexasol +from exasol_bucketfs_utils_python.bucketfs_location import BucketFSLocation +from exasol_bucketfs_utils_python.bucket_config import BucketConfig, BucketFSConfig +from exasol_bucketfs_utils_python.bucketfs_connection_config import BucketFSConnectionConfig + +logger = logging.getLogger(__name__) + + +def create_bucketfs_location( + bucketfs_name: str, bucketfs_host: str, bucketfs_port: int, + bucketfs_use_https: bool, bucketfs_user: str, bucketfs_password: str, + bucket: str, path_in_bucket: str) -> BucketFSLocation: + _bucketfs_connection = BucketFSConnectionConfig( + host=bucketfs_host, port=bucketfs_port, user=bucketfs_user, + pwd=bucketfs_password, is_https=bucketfs_use_https) + _bucketfs_config = BucketFSConfig( + bucketfs_name=bucketfs_name, connection_config=_bucketfs_connection) + _bucket_config = BucketConfig( + bucket_name=bucket, bucketfs_config=_bucketfs_config) + return BucketFSLocation( + bucket_config=_bucket_config, + base_path=PurePosixPath(path_in_bucket)) + + +def get_websocket_sslopt(use_ssl_cert_validation: bool = True, + ssl_trusted_ca: Optional[str] = None, + ssl_client_certificate: Optional[str] = None, + ssl_private_key: Optional[str] = None) -> dict: + """ + Returns a dictionary in the winsocket-client format + (see https://websocket-client.readthedocs.io/en/latest/faq.html#what-else-can-i-do-with-sslopts) + """ + + # Is server certificate validation required? + sslopt: dict[str, object] = {"cert_reqs": ssl.CERT_REQUIRED if use_ssl_cert_validation else ssl.CERT_NONE} + + # Is a bundle with trusted CAs provided? + if ssl_trusted_ca: + trusted_ca_path = Path(ssl_trusted_ca) + if trusted_ca_path.is_dir(): + sslopt["ca_cert_path"] = ssl_trusted_ca + elif trusted_ca_path.is_file(): + sslopt["ca_certs"] = ssl_trusted_ca + else: + raise ValueError(f"Trusted CA location {ssl_trusted_ca} doesn't exist.") + + # Is client's own certificate provided? + if ssl_client_certificate: + if not Path(ssl_client_certificate).is_file(): + raise ValueError(f"Certificate file {ssl_client_certificate} doesn't exist.") + sslopt["certfile"] = ssl_client_certificate + if ssl_private_key: + if not Path(ssl_private_key).is_file(): + raise ValueError(f"Private key file {ssl_private_key} doesn't exist.") + sslopt["keyfile"] = ssl_private_key + + return sslopt + + +class LanguageActivationLevel(Enum): + f""" + Language activation level, i.e. + ALTER SET SCRIPT_LANGUAGES=... + """ + Session = 'SESSION' + System = 'SYSTEM' + + +def get_language_settings(pyexasol_conn: pyexasol.ExaConnection, alter_type: LanguageActivationLevel) -> str: + """ + Reads the current language settings at the specified level. + + pyexasol_conn - Opened database connection. + alter_type - Activation level - SYSTEM or SESSION. + """ + result = pyexasol_conn.execute( + f"""SELECT "{alter_type.value}_VALUE" FROM SYS.EXA_PARAMETERS WHERE + PARAMETER_NAME='SCRIPT_LANGUAGES'""").fetchall() + return result[0][0] + + +class LanguageContainerDeployer: + + def __init__(self, + pyexasol_connection: pyexasol.ExaConnection, + language_alias: str, + bucketfs_location: BucketFSLocation) -> None: + + self._bucketfs_location = bucketfs_location + self._language_alias = language_alias + self._pyexasol_conn = pyexasol_connection + logger.debug(f"Init {LanguageContainerDeployer.__name__}") + + def download_and_run(self, url: str, + bucket_file_path: str, + alter_system: bool = True, + allow_override: bool = False) -> None: + """ + Downloads the language container from the provided url to a temporary file and then deploys it. + See docstring on the `run` method for details on what is involved in the deployment. + + url - Address where the container will be downloaded from. + bucket_file_path - Path within the designated bucket where the container should be uploaded. + alter_system - If True will try to activate the container at the System level. + allow_override - If True the activation of a language container with the same alias will be + overriden, otherwise a RuntimeException will be thrown. + """ + + with tempfile.NamedTemporaryFile() as tmp_file: + response = requests.get(url, stream=True) + response.raise_for_status() + tmp_file.write(response.content) + + self.run(Path(tmp_file.name), bucket_file_path, alter_system, allow_override) + + def run(self, container_file: Optional[Path] = None, + bucket_file_path: Optional[str] = None, + alter_system: bool = True, + allow_override: bool = False) -> None: + """ + Deploys the language container. This includes two steps, both of which are optional: + - Uploading the container into the database. This step can be skipped if the container + has already been uploaded. + - Activating the container. This step may have to be skipped if the user does not have + System Privileges in the database. In that case two alternative activation SQL commands + will be printed on the console. + + container_file - Path of the container tar.gz file in a local file system. + If not provided the container is assumed to be uploaded already. + bucket_file_path - Path within the designated bucket where the container should be uploaded. + If not specified the name of the container file will be used instead. + alter_system - If True will try to activate the container at the System level. + allow_override - If True the activation of a language container with the same alias will be + overriden, otherwise a RuntimeException will be thrown. + """ + + if not bucket_file_path: + if not container_file: + raise ValueError('Either a container file or a bucket file path must be specified.') + bucket_file_path = container_file.name + + if container_file: + self.upload_container(container_file, bucket_file_path) + + if alter_system: + self.activate_container(bucket_file_path, LanguageActivationLevel.System, allow_override) + else: + message = dedent(f""" + In SQL, you can activate the SLC of the Transformers Extension + by using the following statements: + + To activate the SLC only for the current session: + {self.generate_activation_command(bucket_file_path, LanguageActivationLevel.Session, True)} + + To activate the SLC on the system: + {self.generate_activation_command(bucket_file_path, LanguageActivationLevel.System, True)} + """) + print(message) + + def upload_container(self, container_file: Path, + bucket_file_path: Optional[str] = None) -> None: + """ + Upload the language container to the BucketFS. + + container_file - Path of the container tar.gz file in a local file system. + bucket_file_path - Path within the designated bucket where the container should be uploaded. + """ + if not container_file.is_file(): + raise RuntimeError(f"Container file {container_file} " + f"is not a file.") + with open(container_file, "br") as f: + self._bucketfs_location.upload_fileobj_to_bucketfs( + fileobj=f, bucket_file_path=bucket_file_path) + logging.debug("Container is uploaded to bucketfs") + + def activate_container(self, bucket_file_path: str, + alter_type: LanguageActivationLevel = LanguageActivationLevel.Session, + allow_override: bool = False) -> None: + """ + Activates the language container at the required level. + + bucket_file_path - Path within the designated bucket where the container is uploaded. + alter_type - Language activation level, defaults to the SESSION. + allow_override - If True the activation of a language container with the same alias will be overriden, + otherwise a RuntimeException will be thrown. + """ + alter_command = self.generate_activation_command(bucket_file_path, alter_type, allow_override) + self._pyexasol_conn.execute(alter_command) + logging.debug(alter_command) + + def generate_activation_command(self, bucket_file_path: str, + alter_type: LanguageActivationLevel, + allow_override: bool = False) -> str: + """ + Generates an SQL command to activate the SLC container at the required level. The command will + preserve existing activations of other containers identified by different language aliases. + Activation of a container with the same alias, if exists, will be overwritten. + + bucket_file_path - Path within the designated bucket where the container is uploaded. + alter_type - Activation level - SYSTEM or SESSION. + allow_override - If True the activation of a language container with the same alias will be overriden, + otherwise a RuntimeException will be thrown. + """ + path_in_udf = self._bucketfs_location.generate_bucket_udf_path(bucket_file_path) + new_settings = \ + self._update_previous_language_settings(alter_type, allow_override, path_in_udf) + alter_command = \ + f"ALTER {alter_type.value} SET SCRIPT_LANGUAGES='{new_settings}';" + return alter_command + + def _update_previous_language_settings(self, alter_type: LanguageActivationLevel, + allow_override: bool, + path_in_udf: PurePosixPath) -> str: + prev_lang_settings = get_language_settings(self._pyexasol_conn, alter_type) + prev_lang_aliases = prev_lang_settings.split(" ") + self._check_if_requested_language_alias_already_exists( + allow_override, prev_lang_aliases) + new_definitions_str = self._generate_new_language_settings( + path_in_udf, prev_lang_aliases) + return new_definitions_str + + def get_language_definition(self, bucket_file_path: str): + """ + Generate a language definition (ALIAS=URL) for the specified bucket file path. + + bucket_file_path - Path within the designated bucket where the container is uploaded. + """ + path_in_udf = self._bucketfs_location.generate_bucket_udf_path(bucket_file_path) + result = self._generate_new_language_settings(path_in_udf=path_in_udf, prev_lang_aliases=[]) + return result + + def _generate_new_language_settings(self, path_in_udf: PurePosixPath, + prev_lang_aliases: List[str]) -> str: + other_definitions = [ + alias_definition for alias_definition in prev_lang_aliases + if not alias_definition.startswith(self._language_alias + "=")] + path_in_udf_without_buckets = PurePosixPath(*path_in_udf.parts[2:]) + new_language_alias_definition = \ + f"{self._language_alias}=localzmq+protobuf:///" \ + f"{path_in_udf_without_buckets}?lang=python#" \ + f"{path_in_udf}/exaudf/exaudfclient_py3" + new_definitions = other_definitions + [new_language_alias_definition] + new_definitions_str = " ".join(new_definitions) + return new_definitions_str + + def _check_if_requested_language_alias_already_exists( + self, allow_override: bool, + prev_lang_aliases: List[str]) -> None: + definition_for_requested_alias = [ + alias_definition for alias_definition in prev_lang_aliases + if alias_definition.startswith(self._language_alias + "=")] + if not len(definition_for_requested_alias) == 0: + warning_message = f"The requested language alias {self._language_alias} is already in use." + if allow_override: + logging.warning(warning_message) + else: + raise RuntimeError(warning_message) + + @classmethod + def create(cls, bucketfs_name: str, bucketfs_host: str, bucketfs_port: int, + bucketfs_use_https: bool, bucketfs_user: str, + bucketfs_password: str, bucket: str, path_in_bucket: str, + dsn: str, db_user: str, db_password: str, language_alias: str, + use_ssl_cert_validation: bool = True, ssl_trusted_ca: Optional[str] = None, + ssl_client_certificate: Optional[str] = None, + ssl_private_key: Optional[str] = None) -> "LanguageContainerDeployer": + + websocket_sslopt = get_websocket_sslopt(use_ssl_cert_validation, ssl_trusted_ca, + ssl_client_certificate, ssl_private_key) + + pyexasol_conn = pyexasol.connect( + dsn=dsn, + user=db_user, + password=db_password, + encryption=True, + websocket_sslopt=websocket_sslopt + ) + + bucketfs_location = create_bucketfs_location( + bucketfs_name, bucketfs_host, bucketfs_port, bucketfs_use_https, + bucketfs_user, bucketfs_password, bucket, path_in_bucket) + + return cls(pyexasol_conn, language_alias, bucketfs_location) diff --git a/exasol/python_extension_common/deployment/language_container_deployer_cli.py b/exasol/python_extension_common/deployment/language_container_deployer_cli.py new file mode 100644 index 0000000..fb0b222 --- /dev/null +++ b/exasol/python_extension_common/deployment/language_container_deployer_cli.py @@ -0,0 +1,168 @@ +from typing import Optional, Any +import os +import re +import click +from enum import Enum +from pathlib import Path +from exasol.python_transformers_extension.deployment import deployment_utils as utils +from exasol_transformers_extension.deployment.language_container_deployer import LanguageContainerDeployer + + +class CustomizableParameters(Enum): + """ + Parameters of the cli that can be programmatically customised by a developer + of a specialised version of the cli. + The names in the enum list should match the parameter names in language_container_deployer_main. + """ + container_url = 1 + container_name = 2 + + +class _ParameterFormatters: + """ + Class facilitating customization of the cli. + + The idea is that some of the cli parameters can be programmatically customized based + on values of other parameters and externally supplied formatters. For example a specialized + version of the cli may want to provide its own url. Furthermore, this url will depend on + the user supplied parameter called "version". The solution is to set a formatter for the + url, for instance "http://my_stuff/{version}/my_data". If the user specifies non-empty version + parameter the url will be fully formed. + + A formatter may include more than one parameter. In the previous example the url could, + for instance, also include a username: "http://my_stuff/{version}/{user}/my_data". + + Note that customized parameters can only be updated in a callback function. There is no + way to inject them directly into the cli. Also, the current implementation doesn't perform + the update if the value of the parameter dressed with the callback is None. + + IMPORTANT! Please make sure that the formatters are set up before the call to the cli function, + e.g. language_container_deployer_main, is executed. + """ + def __init__(self): + self._formatters = {} + + def __call__(self, ctx: click.Context, param: click.Parameter, value: Optional[Any]) -> Optional[Any]: + + def update_parameter(parameter_name: str, formatter: str) -> None: + param_formatter = ctx.params.get(parameter_name, formatter) + if param_formatter: + # Enclose in double curly brackets all other parameters in the formatting string, + # to avoid the missing parameters' error. Below is an example of a formatter string + # before and after applying the regex, assuming the current parameter is 'version'. + # 'something-with-{version}/tailored-for-{user}' => 'something-with-{version}/tailored-for-{{user}}' + # We were looking for all occurrences of a pattern '{some_name}', where some_name is not version. + pattern = r'\{(?!' + param.name + r'\})\w+\}' + param_formatter = re.sub(pattern, lambda m: f'{{{m.group(0)}}}', param_formatter) + kwargs = {param.name: value} + ctx.params[parameter_name] = param_formatter.format(**kwargs) + + if value is not None: + for prm_name, prm_formatter in self._formatters.items(): + update_parameter(prm_name, prm_formatter) + + return value + + def set_formatter(self, custom_parameter: CustomizableParameters, formatter: str) -> None: + """ Sets a formatter for a customizable parameter. """ + self._formatters[custom_parameter.name] = formatter + + def clear_formatters(self): + """ Deletes all formatters, mainly for testing purposes. """ + self._formatters.clear() + + +# Global cli customization object. +# Specialized versions of this cli should use this object to set custom parameter formatters. +slc_parameter_formatters = _ParameterFormatters() + + +@click.command(name="language-container") +@click.option('--bucketfs-name', type=str, required=True) +@click.option('--bucketfs-host', type=str, required=True) +@click.option('--bucketfs-port', type=int, required=True) +@click.option('--bucketfs-use-https', type=bool, default=False) +@click.option('--bucketfs-user', type=str, required=True, default="w") +@click.option('--bucketfs-password', prompt='bucketFS password', hide_input=True, + default=lambda: os.environ.get(utils.BUCKETFS_PASSWORD_ENVIRONMENT_VARIABLE, "")) +@click.option('--bucket', type=str, required=True) +@click.option('--path-in-bucket', type=str, required=True, default=None) +@click.option('--container-file', + type=click.Path(exists=True, file_okay=True), default=None) +@click.option('--version', type=str, default=None, expose_value=False, + callback=slc_parameter_formatters) +@click.option('--dsn', type=str, required=True) +@click.option('--db-user', type=str, required=True) +@click.option('--db-pass', prompt='db password', hide_input=True, + default=lambda: os.environ.get(utils.DB_PASSWORD_ENVIRONMENT_VARIABLE, "")) +@click.option('--language-alias', type=str, default="PYTHON3_TE") +@click.option('--ssl-cert-path', type=str, default="") +@click.option('--ssl-client-cert-path', type=str, default="") +@click.option('--ssl-client-private-key', type=str, default="") +@click.option('--use-ssl-cert-validation/--no-use-ssl-cert-validation', type=bool, default=True) +@click.option('--upload-container/--no-upload_container', type=bool, default=True) +@click.option('--alter-system/--no-alter-system', type=bool, default=True) +@click.option('--allow-override/--disallow-override', type=bool, default=False) +def language_container_deployer_main( + bucketfs_name: str, + bucketfs_host: str, + bucketfs_port: int, + bucketfs_use_https: bool, + bucketfs_user: str, + bucketfs_password: str, + bucket: str, + path_in_bucket: str, + container_file: str, + dsn: str, + db_user: str, + db_pass: str, + language_alias: str, + ssl_cert_path: str, + ssl_client_cert_path: str, + ssl_client_private_key: str, + use_ssl_cert_validation: bool, + upload_container: bool, + alter_system: bool, + allow_override: bool, + container_url: str = None, + container_name: str = None): + + deployer = LanguageContainerDeployer.create( + bucketfs_name=bucketfs_name, + bucketfs_host=bucketfs_host, + bucketfs_port=bucketfs_port, + bucketfs_use_https=bucketfs_use_https, + bucketfs_user=bucketfs_user, + bucketfs_password=bucketfs_password, + bucket=bucket, + path_in_bucket=path_in_bucket, + dsn=dsn, + db_user=db_user, + db_password=db_pass, + language_alias=language_alias, + ssl_trusted_ca=ssl_cert_path, + ssl_client_certificate=ssl_client_cert_path, + ssl_private_key=ssl_client_private_key, + use_ssl_cert_validation=use_ssl_cert_validation) + + if not upload_container: + deployer.run(alter_system=alter_system, allow_override=allow_override) + elif container_file: + deployer.run(container_file=Path(container_file), alter_system=alter_system, allow_override=allow_override) + elif container_url and container_name: + deployer.download_and_run(container_url, container_name, alter_system=alter_system, + allow_override=allow_override) + else: + # The error message should mention the parameters which the callback is specified for being missed. + raise ValueError("To upload a language container you should specify either its " + "release version or a path of the already downloaded container file.") + + +if __name__ == '__main__': + import logging + + logging.basicConfig( + format='%(asctime)s - %(module)s - %(message)s', + level=logging.DEBUG) + + language_container_deployer_main() diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..aa8c864 --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,15 @@ +[tool.poetry] +name = "python-extension-common" +version = "0.1.0" +description = "A collection of common utilities for Exasol extensions." +authors = ["Your Name "] +license = "MIT" +readme = "README.md" + +[tool.poetry.dependencies] +python = "[^3.8]" + + +[build-system] +requires = ["poetry-core"] +build-backend = "poetry.core.masonry.api" diff --git a/test/unit/deployment/test_language_container_deployer.py b/test/unit/deployment/test_language_container_deployer.py new file mode 100644 index 0000000..746c442 --- /dev/null +++ b/test/unit/deployment/test_language_container_deployer.py @@ -0,0 +1,133 @@ +######################################################### +# To be migrated to the script-languages-container-tool # +######################################################### +from pathlib import Path, PurePosixPath +from unittest.mock import create_autospec, MagicMock, patch + +import pytest +from exasol_bucketfs_utils_python.bucketfs_location import BucketFSLocation +from pyexasol import ExaConnection + +from exasol_transformers_extension.deployment.language_container_deployer import ( + LanguageContainerDeployer, LanguageActivationLevel) + + +@pytest.fixture(scope='module') +def container_file_name() -> str: + return 'container_xyz.tag.gz' + + +@pytest.fixture(scope='module') +def container_file_path(container_file_name) -> Path: + return Path(container_file_name) + + +@pytest.fixture(scope='module') +def language_alias() -> str: + return 'PYTHON3_TEST' + + +@pytest.fixture(scope='module') +def container_bfs_path(container_file_name) -> str: + return f'bfsdefault/default/container/{container_file_name[:-7]}' + + +@pytest.fixture(scope='module') +def mock_pyexasol_conn() -> ExaConnection: + return create_autospec(ExaConnection) + + +@pytest.fixture(scope='module') +def mock_bfs_location(container_bfs_path) -> BucketFSLocation: + mock_loc = create_autospec(BucketFSLocation) + mock_loc.generate_bucket_udf_path.return_value = PurePosixPath(f'/buckets/{container_bfs_path}') + return mock_loc + + +@pytest.fixture +def container_deployer(mock_pyexasol_conn, mock_bfs_location, language_alias) -> LanguageContainerDeployer: + deployer = LanguageContainerDeployer(pyexasol_connection=mock_pyexasol_conn, + language_alias=language_alias, + bucketfs_location=mock_bfs_location) + + deployer.upload_container = MagicMock() + deployer.activate_container = MagicMock() + return deployer + + +def test_slc_deployer_deploy(container_deployer, container_file_name, container_file_path): + container_deployer.run(container_file=container_file_path, bucket_file_path=container_file_name, alter_system=True, + allow_override=True) + container_deployer.upload_container.assert_called_once_with(container_file_path, container_file_name) + container_deployer.activate_container.assert_called_once_with(container_file_name, LanguageActivationLevel.System, + True) + + +def test_slc_deployer_upload(container_deployer, container_file_name, container_file_path): + container_deployer.run(container_file=container_file_path, alter_system=False) + container_deployer.upload_container.assert_called_once_with(container_file_path, container_file_name) + container_deployer.activate_container.assert_not_called() + + +def test_slc_deployer_activate(container_deployer, container_file_name, container_file_path): + container_deployer.run(bucket_file_path=container_file_name, alter_system=True, allow_override=True) + container_deployer.upload_container.assert_not_called() + container_deployer.activate_container.assert_called_once_with(container_file_name, LanguageActivationLevel.System, + True) + + +@patch('exasol_transformers_extension.deployment.language_container_deployer.get_language_settings') +def test_slc_deployer_generate_activation_command(mock_lang_settings, container_deployer, language_alias, + container_file_name, container_bfs_path): + mock_lang_settings.return_value = 'R=builtin_r JAVA=builtin_java PYTHON3=builtin_python3' + + alter_type = LanguageActivationLevel.Session + expected_command = f"ALTER {alter_type.value.upper()} SET SCRIPT_LANGUAGES='" \ + "R=builtin_r JAVA=builtin_java PYTHON3=builtin_python3 " \ + f"{language_alias}=localzmq+protobuf:///{container_bfs_path}?" \ + f"lang=python#/buckets/{container_bfs_path}/exaudf/exaudfclient_py3';" + + command = container_deployer.generate_activation_command(container_file_name, alter_type) + assert command == expected_command + + +@patch('exasol_transformers_extension.deployment.language_container_deployer.get_language_settings') +def test_slc_deployer_generate_activation_command_override(mock_lang_settings, container_deployer, language_alias, + container_file_name, container_bfs_path): + current_bfs_path = 'bfsdefault/default/container_abc' + mock_lang_settings.return_value = \ + 'R=builtin_r JAVA=builtin_java PYTHON3=builtin_python3 ' \ + f'{language_alias}=localzmq+protobuf:///{current_bfs_path}?' \ + f'lang=python#/buckets/{current_bfs_path}/exaudf/exaudfclient_py3' + + alter_type = LanguageActivationLevel.Session + expected_command = f"ALTER {alter_type.value.upper()} SET SCRIPT_LANGUAGES='" \ + "R=builtin_r JAVA=builtin_java PYTHON3=builtin_python3 " \ + f"{language_alias}=localzmq+protobuf:///{container_bfs_path}?" \ + f"lang=python#/buckets/{container_bfs_path}/exaudf/exaudfclient_py3';" + + command = container_deployer.generate_activation_command(container_file_name, alter_type, allow_override=True) + assert command == expected_command + + +@patch('exasol_transformers_extension.deployment.language_container_deployer.get_language_settings') +def test_slc_deployer_generate_activation_command_failure(mock_lang_settings, container_deployer, language_alias, + container_file_name): + current_bfs_path = 'bfsdefault/default/container_abc' + mock_lang_settings.return_value = \ + 'R=builtin_r JAVA=builtin_java PYTHON3=builtin_python3 ' \ + f'{language_alias}=localzmq+protobuf:///{current_bfs_path}?' \ + f'lang=python#/buckets/{current_bfs_path}/exaudf/exaudfclient_py3' + + with pytest.raises(RuntimeError): + container_deployer.generate_activation_command(container_file_name, LanguageActivationLevel.Session, + allow_override=False) + + +def test_slc_deployer_get_language_definition(container_deployer, language_alias, + container_file_name, container_bfs_path): + expected_command = f"{language_alias}=localzmq+protobuf:///{container_bfs_path}?" \ + f"lang=python#/buckets/{container_bfs_path}/exaudf/exaudfclient_py3" + + command = container_deployer.get_language_definition(container_file_name) + assert command == expected_command diff --git a/test/unit/deployment/test_language_container_deployer_cli.py b/test/unit/deployment/test_language_container_deployer_cli.py new file mode 100644 index 0000000..dcaf837 --- /dev/null +++ b/test/unit/deployment/test_language_container_deployer_cli.py @@ -0,0 +1,29 @@ +import click +from exasol_transformers_extension.deployment.language_container_deployer_cli import ( + _ParameterFormatters, CustomizableParameters) + + +def test_parameter_formatters_1param(): + cmd = click.Command('a_command') + ctx = click.Context(cmd) + opt = click.Option(['--version']) + formatters = _ParameterFormatters() + formatters.set_formatter(CustomizableParameters.container_url, 'http://my_server/{version}/my_stuff') + formatters.set_formatter(CustomizableParameters.container_name, 'downloaded') + formatters(ctx, opt, '1.3.2') + assert ctx.params[CustomizableParameters.container_url.name] == 'http://my_server/1.3.2/my_stuff' + assert ctx.params[CustomizableParameters.container_name.name] == 'downloaded' + + +def test_parameter_formatters_2params(): + cmd = click.Command('a_command') + ctx = click.Context(cmd) + opt1 = click.Option(['--version']) + opt2 = click.Option(['--user']) + formatters = _ParameterFormatters() + formatters.set_formatter(CustomizableParameters.container_url, 'http://my_server/{version}/{user}/my_stuff') + formatters.set_formatter(CustomizableParameters.container_name, 'downloaded-{version}') + formatters(ctx, opt1, '1.3.2') + formatters(ctx, opt2, 'cezar') + assert ctx.params[CustomizableParameters.container_url.name] == 'http://my_server/1.3.2/cezar/my_stuff' + assert ctx.params[CustomizableParameters.container_name.name] == 'downloaded-1.3.2' From 0fae7c001547b9a1914d1fe8d7903b752a331b65 Mon Sep 17 00:00:00 2001 From: mibe Date: Mon, 7 Sep 2026 17:00:42 +0100 Subject: [PATCH 2/4] #168 - Fixed hyphen in secret problem --- doc/changes/unreleased.md | 7 + .../cli/std_options.py | 99 ++++++++++- .../test_language_container_deployer_cli.py | 24 --- test/unit/cli/test_std_options.py | 160 +++++++++++++++++- 4 files changed, 256 insertions(+), 34 deletions(-) diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index fb47370..8fca1f9 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -1,3 +1,10 @@ # Unreleased ## Summary + +## Bug Fixes + +* #168: Fixed `get_cli_arg`/`kwargs_to_cli_args` raising `NoSuchOption` for secret + option values starting with `-`/`--` (e.g. SaaS/DB credentials and ids). +* #173: Fixed `secret_callback`'s env-var fallback using a hyphenated name (e.g. + `DB-PASSWORD`) instead of the documented `DB_PASSWORD`. diff --git a/exasol/python_extension_common/cli/std_options.py b/exasol/python_extension_common/cli/std_options.py index 76047e6..6ba70b9 100644 --- a/exasol/python_extension_common/cli/std_options.py +++ b/exasol/python_extension_common/cli/std_options.py @@ -1,5 +1,6 @@ import os import re +import shlex from enum import ( Enum, Flag, @@ -106,6 +107,46 @@ def clear_formatters(self): # This text will be displayed instead of the actual value for a "secret" option. SECRET_DISPLAY = "***" +# A lookalike character used as a reserved delimiter in encode_secret_value's output +# (see there). Click's parser only recognizes the ASCII hyphen-minus (U+002D) as an +# option prefix, so this is invisible to it. +_ESCAPE_BOUNDARY = "‐" + + +def encode_secret_value(value: str) -> str: + """ + Encodes value so that a secret option's value can be put on the command line + without click's parser mistaking it for a new option (see get_cli_arg's + docstring). Use decode_secret_value to reverse this. + + A value that neither starts with "-" (which click's parser would choke on) nor + with _ESCAPE_BOUNDARY (which decode_secret_value would otherwise mistake for its + own encoding) is returned unchanged. Otherwise, the value is encoded as + _ESCAPE_BOUNDARY + + _ESCAPE_BOUNDARY + , e.g. "--secret" -> "‐2‐secret". This length-prefixed form is + an exact inverse for every possible input, including a value that itself starts + with "-" and/or _ESCAPE_BOUNDARY, since decoding only ever needs the first two + occurrences of _ESCAPE_BOUNDARY to recover n and the remainder verbatim. + """ + if not value.startswith("-") and not value.startswith(_ESCAPE_BOUNDARY): + return value + stripped = value.lstrip("-") + n = len(value) - len(stripped) + return f"{_ESCAPE_BOUNDARY}{n}{_ESCAPE_BOUNDARY}{stripped}" + + +def decode_secret_value(value: str) -> str: + """ + Inverse of encode_secret_value. Applied automatically by secret_callback for + options built through this module (see get_cli_arg). Call this explicitly only + if you parse a kwargs_to_cli_args()/get_cli_arg() args string some other way, + e.g. with a different parser or in another program/language. + """ + if not value.startswith(_ESCAPE_BOUNDARY): + return value + _, n, rest = value.split(_ESCAPE_BOUNDARY, 2) + return "-" * int(n) + rest + def secret_callback(ctx: click.Context, param: click.Option, value: Any): """ @@ -115,8 +156,16 @@ def secret_callback(ctx: click.Context, param: click.Option, value: Any): be no way of altering this behaviour. """ if value == SECRET_DISPLAY: - envar_name = param.opts[0][2:].upper() + # Derived from param.opts[0] (the CLI flag itself) rather than param.name, so this + # keeps tracking the flag actually shown to the user (e.g. in --help) even for an + # option declared directly via make_option_secret with a custom internal name. + # Hyphens are converted to underscores because POSIX environment variable names + # can't contain them (#173) - matching the underscored names documented in + # user-guide.md, e.g. "--db-password" -> "DB_PASSWORD". + envar_name = param.opts[0][2:].upper().replace("-", "_") return os.environ.get(envar_name) + if isinstance(value, str): + return decode_secret_value(value) return value @@ -179,7 +228,7 @@ def _get_param_name(std_param: StdParamOrName) -> str: Standard options defined in the form of key-value pairs, where key is the option's StaParam key and the value is a kwargs for creating the click.Options(...). """ -_std_options = { +_std_options: dict[StdParams, dict[str, Any]] = { StdParams.bucketfs_name: {"type": str}, StdParams.bucketfs_host: {"type": str}, StdParams.bucketfs_port: {"type": int}, @@ -249,6 +298,30 @@ def get_bool_opt_name(std_param: StdParamOrName) -> str: return f"--{opt_name}/--no-{opt_name}" +def is_secret_param(std_param: StdParamOrName) -> bool: + """ + True if std_param is a StdParams member defined with hide_input=True in + _std_options. A plain string name is only considered secret if it happens to match + the name of such a StdParams member; any other string name can never be secret, + since it has no entry in _std_options. + + Note: this only reflects the default hide_input in _std_options, not any + hide_input a caller passed directly to create_std_option or via + select_std_options(override=...). get_cli_arg (the only caller) is only ever + given a param name, not the click.Option that was actually constructed, so it + has no way to see such an override. + """ + if isinstance(std_param, StdParams): + member = std_param + elif std_param in StdParams.__members__: + member = StdParams[std_param] + else: + return False + if member not in _std_options: + return False + return bool(_std_options[member].get("hide_input", False)) + + def create_std_option(std_param: StdParamOrName, **kwargs) -> click.Option: """ Creates a Click option. @@ -332,12 +405,32 @@ def get_cli_arg(std_param: StdParamOrName, param_value: Any) -> str: Makes a CLI args string from an option and its value. An option can be given as either an StdParams or its string name. For boolean values the args string takes the form --option-name/--no-option-name. + A non-boolean value is quoted with shlex.quote, so the returned string can be + split back into args with shlex.split (as click.testing.CliRunner.invoke does for + a string args) regardless of what characters the value contains. + + For a "secret" (hide_input) standard parameter, click's parser can't tell an + option value starting with "-"/"--" apart from the option being given with no + value at all, since such an option allows omitting its value (which is how it + lets its value be entered interactively instead) - this holds no matter how the + option and its value are joined in the returned string. To avoid that, such a + value is encoded with encode_secret_value before being put on the command line. + + This is decoded back automatically only if the resulting args string is parsed by + a click.Option built through this module (create_std_option, select_std_options, + make_option_secret), since decoding happens in their shared secret_callback. A + caller who instead parses this string themselves, or hands it to a different + program/language, must call decode_secret_value explicitly to recover the + original value. """ option_name = _get_param_name(std_param).replace("_", "-") if isinstance(param_value, bool): return f"--{option_name}" if param_value else f"--no-{option_name}" - return f'--{option_name} "{param_value}"' + str_value = str(param_value) + if is_secret_param(std_param): + str_value = encode_secret_value(str_value) + return f"--{option_name} {shlex.quote(str_value)}" def kwargs_to_cli_args(**kwargs) -> str: diff --git a/test/integration/cli/test_language_container_deployer_cli.py b/test/integration/cli/test_language_container_deployer_cli.py index 88d9543..62705a3 100644 --- a/test/integration/cli/test_language_container_deployer_cli.py +++ b/test/integration/cli/test_language_container_deployer_cli.py @@ -11,7 +11,6 @@ import pytest from click.testing import CliRunner -from exasol.python_extension_common.cli import std_options from exasol.python_extension_common.cli.language_container_deployer_cli import ( LanguageContainerDeployerCli, ) @@ -30,29 +29,6 @@ CONTAINER_NAME_ARG = "container_name" -@pytest.fixture(autouse=True) -def _patch_get_cli_arg_for_dash_prefixed_values(monkeypatch): - """ - SaaS database ids are randomly generated and may themselves start with "-" - (e.g. "--dI0m90RUKefql382tsWA"). `get_cli_arg` joins an option and its - value with a space, which click's parser can mistake for a new option - when the value itself looks like one. This is patched here, rather than - in `get_cli_arg` itself, to avoid changing that function's behavior for - its other, non-test callers. See - https://github.com/exasol/python-extension-common/issues/168 - """ - original_get_cli_arg = std_options.get_cli_arg - - def patched_get_cli_arg(std_param, param_value): - if isinstance(param_value, bool) or not str(param_value).startswith("-"): - return original_get_cli_arg(std_param, param_value) - option_name = std_param if isinstance(std_param, str) else std_param.name - option_name = option_name.replace("_", "-") - return f'--{option_name}="{param_value}"' - - monkeypatch.setattr(std_options, "get_cli_arg", patched_get_cli_arg) - - @pytest.fixture(scope="session") def onprem_cli_args( backend_aware_onprem_database, exasol_config, bucketfs_config, language_alias diff --git a/test/unit/cli/test_std_options.py b/test/unit/cli/test_std_options.py index babd474..b12a672 100644 --- a/test/unit/cli/test_std_options.py +++ b/test/unit/cli/test_std_options.py @@ -1,3 +1,5 @@ +import shlex + import click import pytest from click.testing import CliRunner @@ -9,9 +11,12 @@ StdTags, check_params, create_std_option, + decode_secret_value, + encode_secret_value, get_bool_opt_name, get_cli_arg, get_opt_name, + is_secret_param, kwargs_to_cli_args, select_std_options, ) @@ -147,29 +152,37 @@ def test_hidden_opt_with_envar(monkeypatch): """ This test checks the mechanism of providing a value of a confidential parameter via an environment variable. + + Regression test for #173: the env var name must be underscored (DB_PASSWORD), not + the hyphenated form of the CLI flag (DB-PASSWORD), since the latter can't even be + set via `export` in a real shell. """ std_param = StdParams.db_password - envar_name = std_param.name.upper() + envar_name = "DB_PASSWORD" param_value = "my_password" + captured = {} + def func(**kwargs): - assert std_param.name in kwargs - assert kwargs[std_param.name] == param_value + captured.update(kwargs) opt = create_std_option(std_param, type=str, hide_input=True) cmd = click.Command("do_something", params=[opt], callback=func) runner = CliRunner() monkeypatch.setenv(envar_name, param_value) - runner.invoke(cmd) + result = runner.invoke(cmd, catch_exceptions=False, standalone_mode=False) + assert result.exit_code == 0 + assert captured[std_param.name] == param_value @pytest.mark.parametrize( ["std_param", "param_value", "expected_result"], [ - (StdParams.db_user, "Me", '--db-user "Me"'), - ("user_rating", 5, '--user-rating "5"'), + (StdParams.db_user, "Me", "--db-user Me"), + ("user_rating", 5, "--user-rating 5"), (StdParams.use_ssl_cert_validation, True, "--use-ssl-cert-validation"), (StdParams.use_ssl_cert_validation, False, "--no-use-ssl-cert-validation"), + (StdParams.db_user, 'quote"inside', f'--db-user {shlex.quote("quote\"inside")}'), ], ) def test_get_cli_arg(std_param, param_value, expected_result): @@ -179,10 +192,143 @@ def test_get_cli_arg(std_param, param_value, expected_result): def test_kwargs_to_cli_args(): arg_string = kwargs_to_cli_args(use_rgb=True, colour="Blue", compress_image=False) arg_set = set(arg_string.split()) - expected_set = {"--use-rgb", "--colour", '"Blue"', "--no-compress-image"} + expected_set = {"--use-rgb", "--colour", "Blue", "--no-compress-image"} assert arg_set == expected_set +def test_get_cli_arg_value_with_double_quote_survives_shlex_round_trip(): + """ + Regression test: get_cli_arg used to wrap the value in unescaped literal double + quotes, so a value containing '"' produced an args string that shlex/click can't + parse (the same class of bug as #168, just triggered by a different character). + """ + value = 'pa"ss' + arg = get_cli_arg(StdParams.db_user, value) + assert shlex.split(arg) == ["--db-user", value] + + +@pytest.mark.parametrize( + ["std_param", "expected"], + [ + (StdParams.saas_database_id, True), + (StdParams.db_password, True), + (StdParams.bucketfs_password, True), + (StdParams.saas_account_id, True), + (StdParams.saas_token, True), + (StdParams.db_user, False), + ("saas_database_id", True), + ("db_user", False), + ("not_a_std_param", False), + ], +) +def test_is_secret_param(std_param, expected): + assert is_secret_param(std_param) is expected + + +@pytest.mark.parametrize( + "value", + [ + "--dI0m90RUKefql382tsWA", + "-dashy", + "---triple-dash", + "-", + "", + # Values that themselves contain the reserved escape-boundary character + # (U+2010), which decode_secret_value used to always treat as its own + # encoding, corrupting a value that legitimately starts with it. + "‐2-", + "‐‐realtoken", + "-‐‐foo", + "‐", + ], +) +def test_encode_decode_secret_value_roundtrip(value): + assert decode_secret_value(encode_secret_value(value)) == value + + +def test_encode_secret_value_leaves_unremarkable_values_unchanged(): + assert encode_secret_value("regular_value") == "regular_value" + + +def test_get_cli_arg_secret_param_with_dash_prefixed_value(): + """ + Regression test for #168. A secret option's value that itself starts with "-" + breaks click's parser (see docstring of get_cli_arg for why), regardless of + whether it's joined to the option with a space or "=". get_cli_arg works around + this by encoding the leading dash(es) instead of putting them on the command + line literally. + """ + dashy_value = "--dI0m90RUKefql382tsWA" + arg = get_cli_arg(StdParams.saas_database_id, dashy_value) + assert arg == f"--saas-database-id {shlex.quote(encode_secret_value(dashy_value))}" + + +def test_get_cli_arg_secret_param_end_to_end_via_click(): + """ + End-to-end regression test for #168: builds the real saas_database_id option and + invokes it through click, with a value that used to raise NoSuchOption. + """ + dashy_value = "--dI0m90RUKefql382tsWA" + opt = create_std_option(StdParams.saas_database_id, type=str, hide_input=True) + + captured = {} + + def func(**kwargs): + captured.update(kwargs) + + cmd = click.Command("do_something", params=[opt], callback=func) + arg_string = kwargs_to_cli_args(saas_database_id=dashy_value) + + runner = CliRunner() + result = runner.invoke(cmd, args=arg_string, catch_exceptions=False, standalone_mode=False) + + assert result.exit_code == 0 + assert captured["saas_database_id"] == dashy_value + + +def test_get_cli_arg_secret_params_survive_full_saas_option_set(): + """ + Regression test for #168, exercised through the full option set built the same + way LanguageContainerDeployerCli's SaaS CLI is (select_std_options over DB|SAAS, + BFS|SAAS, SLC tags), to guard against a secret param being silently dropped when + mixed in with many other options. + """ + opts = select_std_options([StdTags.DB | StdTags.SAAS, StdTags.BFS | StdTags.SAAS, StdTags.SLC]) + captured = {} + + def func(**kwargs): + captured.update(kwargs) + + cmd = click.Command("deploy_slc", params=opts, callback=func) + + saas_cli_args = { + StdParams.saas_url.name: "https://cloud.exasol.com", + StdParams.saas_account_id.name: "--saas-acct-dashy", + StdParams.saas_database_id.name: "--dI0m90RUKefql382tsWA", + StdParams.saas_token.name: "--saas-token-dashy", + StdParams.path_in_bucket.name: "container", + StdParams.language_alias.name: "PYTHON3_MY_LANG", + } + slc_cli_args = { + StdParams.alter_system.name: True, + StdParams.allow_override.name: True, + StdParams.wait_for_completion.name: True, + } + extra_cli_args = {StdParams.version.name: "1.2.3"} + + arg_string = kwargs_to_cli_args(**saas_cli_args, **slc_cli_args, **extra_cli_args) + runner = CliRunner() + result = runner.invoke(cmd, args=arg_string, catch_exceptions=False, standalone_mode=False) + + assert result.exit_code == 0 + for name in ( + StdParams.saas_account_id.name, + StdParams.saas_database_id.name, + StdParams.saas_token.name, + ): + assert captured[name] == saas_cli_args[name] + + @pytest.mark.parametrize( ["std_params", "param_kwargs", "expected_result"], [ From bc6354b1dbce8e3027270fde8e0b4f674bb7c282 Mon Sep 17 00:00:00 2001 From: mibe Date: Mon, 7 Sep 2026 17:17:37 +0100 Subject: [PATCH 3/4] #168 - Fixed quote inside --- test/unit/cli/test_std_options.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/unit/cli/test_std_options.py b/test/unit/cli/test_std_options.py index b12a672..4f12188 100644 --- a/test/unit/cli/test_std_options.py +++ b/test/unit/cli/test_std_options.py @@ -175,6 +175,9 @@ def func(**kwargs): assert captured[std_param.name] == param_value +_QUOTE_INSIDE_VALUE = 'quote"inside' + + @pytest.mark.parametrize( ["std_param", "param_value", "expected_result"], [ @@ -182,7 +185,11 @@ def func(**kwargs): ("user_rating", 5, "--user-rating 5"), (StdParams.use_ssl_cert_validation, True, "--use-ssl-cert-validation"), (StdParams.use_ssl_cert_validation, False, "--no-use-ssl-cert-validation"), - (StdParams.db_user, 'quote"inside', f'--db-user {shlex.quote("quote\"inside")}'), + ( + StdParams.db_user, + _QUOTE_INSIDE_VALUE, + f"--db-user {shlex.quote(_QUOTE_INSIDE_VALUE)}", + ), ], ) def test_get_cli_arg(std_param, param_value, expected_result): From 8b3b25ac65e12760d3e887672676d2fcbd0c38a7 Mon Sep 17 00:00:00 2001 From: mibe Date: Thu, 10 Sep 2026 12:07:07 +0100 Subject: [PATCH 4/4] #168 - Added more unit tests --- test/unit/cli/test_std_options.py | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/unit/cli/test_std_options.py b/test/unit/cli/test_std_options.py index 4f12188..c616a6d 100644 --- a/test/unit/cli/test_std_options.py +++ b/test/unit/cli/test_std_options.py @@ -257,6 +257,39 @@ def test_encode_secret_value_leaves_unremarkable_values_unchanged(): assert encode_secret_value("regular_value") == "regular_value" +@pytest.mark.parametrize( + ["value", "expected_encoded"], + [ + # Plain dash-prefixed values: the leading "-" run is replaced by the + # escape-boundary character (U+2010, a lookalike click's parser doesn't + # recognize as an option prefix) followed by its length, so the encoded + # value no longer starts with an ASCII "-". + ("--dI0m90RUKefql382tsWA", "‐2‐dI0m90RUKefql382tsWA"), + ("-dashy", "‐1‐dashy"), + ("---triple-dash", "‐3‐triple-dash"), + ("-", "‐1‐"), + # Values that themselves start with the escape-boundary character (but + # not with an ASCII "-") still get the same two-part prefix, with a + # leading-dash count of 0, so decode_secret_value can still tell them + # apart from a "real" encoding of a dash-prefixed value. + ("‐2-", "‐0‐‐2-"), + ("‐‐realtoken", "‐0‐‐‐realtoken"), + ("-‐‐foo", "‐1‐‐‐foo"), + ("‐", "‐0‐‐"), + ], +) +def test_encode_secret_value_escapes_leading_dashes(value, expected_encoded): + """ + Regression test for the PR #174 review comment: test_encode_decode_secret_value_roundtrip + only proves encode_secret_value and decode_secret_value are inverses of each other, not + that encoding actually strips the leading "-"/_ESCAPE_BOUNDARY that click's parser + chokes on. This pins down the exact encoded form instead. + """ + encoded = encode_secret_value(value) + assert encoded == expected_encoded + assert not encoded.startswith("-") + + def test_get_cli_arg_secret_param_with_dash_prefixed_value(): """ Regression test for #168. A secret option's value that itself starts with "-"