-
Notifications
You must be signed in to change notification settings - Fork 83
refactor(package): Standardize how configuration and credentials are passed to containers (resolves #1149). #1152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 53 commits
b143237
6cddeda
f9d5b59
489476f
0e3f412
05283d5
34c5e8f
842efac
05715fb
87d07c6
73334ed
866334d
dff9bef
00ceeb8
cc9a7c7
4e1c272
7d82bec
1b7c70c
781bb16
bf63d6e
469bc65
8654a56
03881cc
0c9a035
6f8879c
3850b2d
96735b1
365b5d6
c84836c
04c5c98
33e11ac
6e06740
6ef77dd
36af52d
d85085b
e371aa8
5fa14f8
9eba523
312445e
335f25e
3cc3d44
02f393d
8bcae09
4a9bf99
fc27205
40e387d
ed58dcc
89f001f
95b55ef
fa81a9d
4743566
0a8de41
cbdd9fb
67d048a
f852ca4
0acb960
d21efbf
d446e52
c0949cc
f723512
38c75ee
a11b314
4124b60
3fe8368
ce143c6
e15547e
4bba4ed
6ab2f80
cd4437d
14e0219
33bc068
ae8f581
0e52cb1
f063bd9
04f81b7
34c430f
31342c3
ca2db62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import typing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uuid | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from enum import auto | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import List, Optional, Tuple | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Dict, List, Optional, Tuple | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import yaml | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_py_utils.clp_config import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -95,6 +95,7 @@ def __init__(self, clp_home: pathlib.Path, docker_clp_home: pathlib.Path): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.archives_output_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.stream_output_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.aws_config_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.generated_config_file: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM: generated_config_file mount slot added Good addition. Consider marking the file mount read-only when created to prevent accidental in-container edits. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+99
to
100
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Nit: align typing style (Optional over typing.Optional) You already import Optional; keep usage consistent. - self.generated_config_file: typing.Optional[DockerMount] = None
+ self.generated_config_file: Optional[DockerMount] = None📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _validate_data_directory(data_dir: pathlib.Path, component_name: str) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -285,6 +286,18 @@ def generate_container_config( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.stream_output.get_directory(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not is_path_already_mounted( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_home, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CONTAINER_CLP_HOME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.get_generated_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.get_generated_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docker_mounts.generated_config_file = DockerMount( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DockerMountType.BIND, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.get_generated_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.get_generated_config_file_path(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Only create the mount if the directory exists | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if clp_config.aws_config_directory is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.aws_config_directory = CONTAINER_AWS_CONFIG_DIRECTORY | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -328,13 +341,19 @@ def dump_container_config( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def generate_container_start_cmd( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_name: str, container_mounts: List[Optional[DockerMount]], container_image: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_name: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_mounts: List[Optional[DockerMount]], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_image: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extra_env_vars: Optional[Dict[str, str]] = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generates the command to start a container with the given mounts and name. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generates the command to start a container with the given mounts, environment variables, and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_name: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_mounts: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_image: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param extra_env_vars: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: The command. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Security: avoid adding KEY=VALUE secrets to docker cmd generate_container_start_cmd currently emits -e KEY=VALUE, which leaks secrets via ps and logs. Switch to passing only keys (i.e., -e KEY) and require callers to provide values via subprocess.run(..., env=...). def generate_container_start_cmd(
container_name: str,
container_mounts: List[Optional[DockerMount]],
container_image: str,
- extra_env_vars: Optional[Dict[str, str]] = None,
+ extra_env_vars: Optional[Dict[str, str]] = None,
) -> List[str]:
"""
- Generates the command to start a container with the given mounts, environment variables, and
- name.
+ Generates the command to start a container with the given mounts, environment variables, and
+ name.
:param container_name:
:param container_mounts:
:param container_image:
- :param extra_env_vars:
+ :param extra_env_vars: Additional environment variables as a mapping. Only the keys are emitted
+ with '-e KEY'. Callers must pass actual values via subprocess.run(env=...).
:return: The command.
"""
@@
- if extra_env_vars is not None:
- for key, value in extra_env_vars.items():
- container_start_cmd.extend(["-e", f"{key}={value}"])
+ if extra_env_vars is not None:
+ for key in extra_env_vars.keys():
+ container_start_cmd.extend(["-e", key])Follow through in call sites by merging env dicts when invoking subprocess.run. Also applies to: 371-375 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Docstring update is fine; consider clarifying “extra_env_vars” in the docstring The new arg is correct; adding a note that keys should not include reserved variables (e.g., PYTHONPATH/CLP_HOME) would avoid misuse. """
- Generates the command to start a container with the given mounts, environment variables, and
+ Generates the command to start a container with the given mounts, environment variables, and
name.
:param container_name:
:param container_mounts:
:param container_image:
- :param extra_env_vars:
+ :param extra_env_vars: Additional environment variables as a mapping. Must not include
+ reserved keys like PYTHONPATH or CLP_HOME.
:return: The command.
"""📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_site_packages_dir = CONTAINER_CLP_HOME / "lib" / "python3" / "site-packages" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -350,6 +369,9 @@ def generate_container_start_cmd( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--name", container_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--log-driver", "local" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if extra_env_vars is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kirkrodrigues marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for key, value in extra_env_vars.items(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_start_cmd.extend(["-e", f"{key}={value}"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for mount in container_mounts: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if mount: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_start_cmd.append("--mount") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -428,21 +450,21 @@ def validate_and_load_db_credentials_file( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config: CLPConfig, clp_home: pathlib.Path, generate_default_file: bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_credentials_file_path(clp_config, clp_home, generate_default_file) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.load_database_credentials_from_file() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.database.load_credentials_from_file(clp_config.credentials_file_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_and_load_queue_credentials_file( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config: CLPConfig, clp_home: pathlib.Path, generate_default_file: bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_credentials_file_path(clp_config, clp_home, generate_default_file) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.load_queue_credentials_from_file() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.queue.load_credentials_from_file(clp_config.credentials_file_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_and_load_redis_credentials_file( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config: CLPConfig, clp_home: pathlib.Path, generate_default_file: bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_credentials_file_path(clp_config, clp_home, generate_default_file) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.load_redis_credentials_from_file() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.redis.load_credentials_from_file(clp_config.credentials_file_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_db_config(clp_config: CLPConfig, data_dir: pathlib.Path, logs_dir: pathlib.Path): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -599,3 +621,71 @@ def is_retention_period_configured(clp_config: CLPConfig) -> bool: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def generate_common_environment_variables( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kirkrodrigues marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_clp_home_env_var=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generate a list of common environment variables for Docker containers. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_clp_home_env_var: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: A list of common environment variables for Docker containers in the format "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_site_packages_dir = CONTAINER_CLP_HOME / "lib" / "python3" / "site-packages" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = [f"PYTHONPATH={clp_site_packages_dir}"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_clp_home_env_var: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_HOME={CONTAINER_CLP_HOME}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return env_vars | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def generate_credential_environment_variables( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config: CLPConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_db_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_queue_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| include_redis_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generates a list of credential environment variables for Docker containers. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_clp_config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_db_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_queue_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param include_redis_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: A list of common environment variables for Docker containers in the format "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_db_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_DB_USER={container_clp_config.database.username}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_DB_PASS={container_clp_config.database.password}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_queue_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_QUEUE_USER={container_clp_config.queue.username}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_QUEUE_PASS={container_clp_config.queue.password}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if include_redis_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_REDIS_PASS={container_clp_config.redis.password}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return env_vars | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def generate_celery_connection_environment_variables(container_clp_config: CLPConfig) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generate a list of Celery connection environment variables for Docker containers. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param container_clp_config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: A list of common environment variables for Docker containers in the format "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"BROKER_URL=amqp://" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.queue.username}:{container_clp_config.queue.password}@" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.queue.host}:{container_clp_config.queue.port}", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"RESULT_BACKEND=redis://default:{container_clp_config.redis.password}@" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.redis.query_backend_database}", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+701
to
+707
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion URL-encode credentials in Celery connection strings Credentials containing special characters (e.g., Apply URL encoding to credentials before embedding them in connection strings: +from urllib.parse import quote
def get_celery_connection_env_vars_list(container_clp_config: CLPConfig) -> List[str]:
"""
:param container_clp_config:
:return: A list of Celery connection environment variables for Docker containers, in the format
"KEY=VALUE".
"""
+ queue_user = quote(container_clp_config.queue.username, safe='')
+ queue_pass = quote(container_clp_config.queue.password, safe='')
+ redis_pass = quote(container_clp_config.redis.password, safe='')
env_vars = [
f"BROKER_URL=amqp://"
- f"{container_clp_config.queue.username}:{container_clp_config.queue.password}@"
+ f"{queue_user}:{queue_pass}@"
f"{container_clp_config.queue.host}:{container_clp_config.queue.port}",
- f"RESULT_BACKEND=redis://default:{container_clp_config.redis.password}@"
+ f"RESULT_BACKEND=redis://default:{redis_pass}@"
f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/"
f"{container_clp_config.redis.query_backend_database}",
]📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return env_vars | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def generate_celery_connection_environment_variables(container_clp_config: CLPConfig) -> List[str]: | |
| """ | |
| Generate a list of Celery connection environment variables for Docker containers. | |
| :param container_clp_config: | |
| :return: A list of common environment variables for Docker containers in the format "KEY=VALUE". | |
| """ | |
| env_vars = [ | |
| f"BROKER_URL=amqp://" | |
| f"{container_clp_config.queue.username}:{container_clp_config.queue.password}@" | |
| f"{container_clp_config.queue.host}:{container_clp_config.queue.port}", | |
| f"RESULT_BACKEND=redis://default:{container_clp_config.redis.password}@" | |
| f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/" | |
| f"{container_clp_config.redis.query_backend_database}", | |
| ] | |
| return env_vars | |
| def generate_celery_connection_environment_variables( | |
| container_clp_config: CLPConfig, | |
| redis_database: Optional[int] = None, | |
| ) -> List[str]: | |
| """ | |
| Generate a list of Celery connection environment variables for Docker containers. | |
| :param container_clp_config: Configuration with queue and redis credentials/hosts. | |
| :param redis_database: Optional Redis database index to use; defaults to | |
| container_clp_config.redis.query_backend_database. | |
| :return: A list of environment variable strings in the format "KEY=VALUE". | |
| """ | |
| from urllib.parse import quote | |
| q_user = "" if container_clp_config.queue.username is None else quote( | |
| container_clp_config.queue.username, safe="" | |
| ) | |
| q_pass = "" if container_clp_config.queue.password is None else quote( | |
| container_clp_config.queue.password, safe="" | |
| ) | |
| r_pass = container_clp_config.redis.password | |
| redis_db = ( | |
| container_clp_config.redis.query_backend_database | |
| if redis_database is None | |
| else redis_database | |
| ) | |
| broker = ( | |
| f"amqp://{q_user}:{q_pass}@" | |
| f"{container_clp_config.queue.host}:{container_clp_config.queue.port}" | |
| ) | |
| if r_pass: | |
| backend = ( | |
| "redis://default:" | |
| f"{quote(r_pass, safe='')}@" | |
| f"{container_clp_config.redis.host}:" | |
| f"{container_clp_config.redis.port}/{redis_db}" | |
| ) | |
| else: | |
| backend = ( | |
| f"redis://{container_clp_config.redis.host}:" | |
| f"{container_clp_config.redis.port}/{redis_db}" | |
| ) | |
| env_vars = [f"BROKER_URL={broker}", f"RESULT_BACKEND={backend}"] | |
| return env_vars |
🤖 Prompt for AI Agents
components/clp-package-utils/clp_package_utils/general.py lines 685-701: the
current Celery env builder hardcodes the Redis DB index and embeds credentials
without URL-encoding or handling optional auth; update it to read the DB index
from container_clp_config.redis.redis_database (per start_clp.py call-site
change), URL-encode any username/password (use urllib.parse.quote_plus) for both
BROKER_URL and RESULT_BACKEND, and when Redis password is empty/None omit the
":password@" portion entirely so the URL becomes redis://host:port/db; adjust
call sites (schedulers/workers) to pass redis_database as required.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |||||
| from clp_py_utils.clp_config import ( | ||||||
| CLP_DEFAULT_DATASET_NAME, | ||||||
| StorageEngine, | ||||||
| StorageType, | ||||||
| StorageType, CLP_DB_USER_ENV_VAR_NAME, CLP_DB_PASS_ENV_VAR_NAME, | ||||||
| ) | ||||||
|
|
||||||
| from clp_package_utils.general import ( | ||||||
|
|
@@ -214,14 +214,17 @@ def main(argv: typing.List[str]) -> int: | |||||
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||||||
| container_clp_config, clp_config, container_name | ||||||
| ) | ||||||
|
|
||||||
| necessary_mounts: typing.List[CLPDockerMounts] = [ | ||||||
|
||||||
| necessary_mounts: typing.List[CLPDockerMounts] = [ | |
| necessary_mounts: typing.List[DockerMount] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually have the fix in #1186
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Do not pass secrets on the docker command line; use process env
Including DB creds as -e KEY=VALUE exposes them via ps and (on failure) logs. Emit only -e KEY and provide values via subprocess.run(..., env=...).
Apply:
- extra_env_vars = {
- CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
- CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
- }
- container_start_cmd: typing.List[str] = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ extra_env_vars = {
+ CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
+ CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
+ }
+ # Pass only the keys on the CLI; provide values via subprocess env.
+ container_start_cmd: typing.List[str] = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )
@@
- proc = subprocess.run(cmd)
+ # Merge secrets into the spawned process environment.
+ import os # ensure imported at top if not already
+ proc = subprocess.run(cmd, env={**os.environ, **extra_env_vars})Also consider avoiding logging full command strings anywhere secrets may appear. With the change above, the command string no longer contains DB secrets.
Also applies to: 270-275
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
|
|
||
| from clp_py_utils.clp_config import ( | ||
| CLP_DEFAULT_DATASET_NAME, | ||
| StorageEngine, | ||
| StorageEngine, CLP_DB_USER_ENV_VAR_NAME, CLP_DB_PASS_ENV_VAR_NAME, | ||
| ) | ||
| from job_orchestration.scheduler.job_config import InputType | ||
|
|
||
|
|
@@ -222,8 +222,12 @@ def main(argv): | |
|
|
||
| _generate_logs_list(clp_config.logs_input.type, container_logs_list_path, parsed_args) | ||
|
|
||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
| container_start_cmd = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+228
to
234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainStop leaking DB credentials on the Docker CLI; pass values via subprocess env extra_env_vars currently flows into the docker argv as -e KEY=VALUE, exposing secrets via ps and logs. Emit only -e KEY and inject the values via subprocess env. Action:
Apply within this hunk: - container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ # NOTE: Requires general.generate_container_start_cmd to emit '-e KEY' (no '=VALUE')
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )And update the subprocess invocation to inject secrets via env (outside this hunk): # at top of file
import os- subprocess.run(cmd, check=True)
+ subprocess.run(cmd, check=True, env={**os.environ, **extra_env_vars})Run to locate other CLI injections that still embed secrets: 🏁 Script executed: #!/bin/bash
rg -n -C2 -P '\bgenerate_container_start_cmd\s*\(' --glob '!**/venv/**'
rg -n -C2 -P 'append_docker_options\(.+,\s*env_vars\)' --glob '!**/venv/**'Length of output: 9124 Stop leaking DB credentials via Docker CLI flags Exposing secrets with “-e KEY=VALUE” leaks credentials in process listings and logs. To secure this: • Refactor Add at top of each affected file: import osExample diff for compress.py: - extra_env_vars = {
- CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
- CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
- }
- container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ extra_env_vars = {
+ CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
+ CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
+ }
+ # Emit only “-e KEY” and inject values via env
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )
@@ -240,7 +245,9 @@ def run_compress(...):
- subprocess.run(container_start_cmd, check=True)
+ subprocess.run(
+ container_start_cmd,
+ check=True,
+ env={**os.environ, **extra_env_vars},
+ )🤖 Prompt for AI Agents |
||
| compress_cmd = _generate_compress_cmd( | ||
| parsed_args, dataset, generated_config_path_on_container, logs_list_path_on_container | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| CLP_DEFAULT_DATASET_NAME, | ||
| CLPConfig, | ||
| StorageEngine, | ||
| StorageType, | ||
| StorageType, CLP_DB_USER_ENV_VAR_NAME, CLP_DB_PASS_ENV_VAR_NAME, | ||
| ) | ||
|
|
||
| from clp_package_utils.general import ( | ||
|
|
@@ -122,8 +122,13 @@ def handle_extract_file_cmd( | |
| container_paths_to_extract_file_path, | ||
| ) | ||
| ) | ||
|
|
||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
|
Comment on lines
+129
to
+132
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) DRY: factor out DB env var assembly The extra_env_vars construction is duplicated. Extract a tiny helper to keep it consistent and reduce future drift. Example: def _build_db_env(clp_config: CLPConfig) -> Dict[str, str]:
return {
CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username,
CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password,
}Then: extra_env_vars = _build_db_env(clp_config)Also applies to: 202-205 🤖 Prompt for AI Agents |
||
| container_start_cmd = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+129
to
135
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Do not print DB creds in docker argv; pass via env Emit only the variable names on the docker command line and inject actual values via subprocess env. Apply: - container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ # NOTE: Requires general.generate_container_start_cmd to emit '-e KEY'
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )And later (outside this hunk): import os # at top if missing
# ...
subprocess.run(cmd, check=True, env={**os.environ, **extra_env_vars}) |
||
|
|
||
| # fmt: off | ||
|
|
@@ -194,8 +199,12 @@ def handle_extract_stream_cmd( | |
| container_clp_config, clp_config, container_name | ||
| ) | ||
| necessary_mounts = [mounts.clp_home, mounts.logs_dir] | ||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
| container_start_cmd = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+205
to
211
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Apply the same secret-handling fix to the stream extraction path Mirror the recommended approach here as well: pass only names to Docker and set values in subprocess env to avoid leaking credentials via argv. Apply this localized change (after updating the builder as described above): - container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ env_var_names=list(extra_env_vars.keys()),
+ )And ensure: env = {**os.environ, **extra_env_vars}
subprocess.run(cmd, check=True, env=env) |
||
|
|
||
| # fmt: off | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |
|
|
||
| from clp_py_utils.clp_config import ( | ||
| CLPConfig, | ||
| Database, | ||
| Database, CLP_DB_USER_ENV_VAR_NAME, CLP_DB_PASS_ENV_VAR_NAME, | ||
| ) | ||
| from clp_py_utils.clp_metadata_db_utils import get_files_table_name | ||
| from clp_py_utils.sql_adapter import SQL_Adapter | ||
|
|
@@ -189,6 +189,7 @@ def validate_and_load_config_file( | |
| clp_config = load_config_file(config_file_path, default_config_file_path, clp_home) | ||
| clp_config.validate_archive_output_config() | ||
| clp_config.validate_logs_dir() | ||
| clp_config.database.load_credentials_from_env() | ||
| return clp_config | ||
|
Comment on lines
+194
to
195
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Surface missing DB creds clearly when loading from env
Minimal change: try:
clp_config.database.load_credentials_from_env()
except ValueError as e:
logger.error(f"Database credentials not set in environment: {e}")
return NoneThis keeps stack traces clean and aids operators during setup. 🤖 Prompt for AI Agents |
||
| except Exception: | ||
| logger.exception("Failed to load config.") | ||
|
|
@@ -244,8 +245,8 @@ def handle_extract_file_cmd( | |
| # fmt: on | ||
| extract_env = { | ||
| **os.environ, | ||
| "CLP_DB_USER": clp_db_connection_params["username"], | ||
| "CLP_DB_PASS": clp_db_connection_params["password"], | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_db_connection_params["username"], | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_db_connection_params["password"], | ||
| } | ||
|
|
||
| files_to_extract_list_path = None | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -294,6 +294,7 @@ def main(argv): | |
| config_file_path = pathlib.Path(parsed_args.config) | ||
| clp_config = load_config_file(config_file_path, default_config_file_path, clp_home) | ||
| clp_config.validate_logs_dir() | ||
| clp_config.database.load_credentials_from_env() | ||
| except: | ||
|
Comment on lines
+297
to
298
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Log a targeted error when DB creds are missing As with other scripts, catching the ValueError from Suggested pattern: try:
clp_config.database.load_credentials_from_env()
except ValueError as e:
logger.error(f"Database credentials not set in environment: {e}")
return -1🤖 Prompt for AI Agents |
||
| logger.exception("Failed to load config.") | ||
| return -1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
| from clp_py_utils.clp_config import ( | ||
| CLP_DEFAULT_DATASET_NAME, | ||
| StorageEngine, | ||
| StorageType, | ||
| StorageType, CLP_DB_PASS_ENV_VAR_NAME, CLP_DB_USER_ENV_VAR_NAME, | ||
| ) | ||
|
|
||
| from clp_package_utils.general import ( | ||
|
|
@@ -117,10 +117,13 @@ def main(argv): | |
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||
| container_clp_config, clp_config, container_name | ||
| ) | ||
|
|
||
| necessary_mounts = [mounts.clp_home, mounts.logs_dir] | ||
| extra_env_vars = { | ||
| CLP_DB_USER_ENV_VAR_NAME: clp_config.database.username, | ||
| CLP_DB_PASS_ENV_VAR_NAME: clp_config.database.password, | ||
| } | ||
| container_start_cmd = generate_container_start_cmd( | ||
| container_name, necessary_mounts, clp_config.execution_container | ||
| container_name, necessary_mounts, clp_config.execution_container, extra_env_vars | ||
| ) | ||
|
Comment on lines
+121
to
127
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid exposing DB secrets via “-e KEY=VALUE” on docker argv Like compress.py, this leaks via ps. Emit only env var names on the CLI and provide values via subprocess env. Apply: - container_start_cmd = generate_container_start_cmd(
- container_name, necessary_mounts, clp_config.execution_container, extra_env_vars
- )
+ # NOTE: Requires general.generate_container_start_cmd to emit '-e KEY' (no '=VALUE')
+ container_start_cmd = generate_container_start_cmd(
+ container_name,
+ necessary_mounts,
+ clp_config.execution_container,
+ list(extra_env_vars.keys()),
+ )Also inject env at the call site (outside this hunk): import os # if not already imported
# ...
subprocess.run(cmd, check=True, env={**os.environ, **extra_env_vars})🤖 Prompt for AI Agents |
||
|
|
||
| # fmt: off | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Unify typing style (avoid mixing
typing.OptionalwithOptional).You import
Optional,List, etc., but still usetyping.Optionalelsewhere. Prefer one style for consistency; since you already import from typing, useOptionalconsistently.Apply this refactor in the class where it appears:
class CLPDockerMounts: def __init__(self, clp_home: pathlib.Path, docker_clp_home: pathlib.Path): - self.input_logs_dir: typing.Optional[DockerMount] = None + self.input_logs_dir: Optional[DockerMount] = None self.clp_home: typing.Optional[DockerMount] = DockerMount( DockerMountType.BIND, clp_home, docker_clp_home ) - self.data_dir: typing.Optional[DockerMount] = None - self.logs_dir: typing.Optional[DockerMount] = None - self.archives_output_dir: typing.Optional[DockerMount] = None - self.stream_output_dir: typing.Optional[DockerMount] = None - self.aws_config_dir: typing.Optional[DockerMount] = None + self.data_dir: Optional[DockerMount] = None + self.logs_dir: Optional[DockerMount] = None + self.archives_output_dir: Optional[DockerMount] = None + self.stream_output_dir: Optional[DockerMount] = None + self.aws_config_dir: Optional[DockerMount] = None📝 Committable suggestion
🤖 Prompt for AI Agents