feat(otdf-local): add CLI for local test environment management#406
feat(otdf-local): add CLI for local test environment management#406dmihalcik-virtru wants to merge 4 commits intoopentdf:mainfrom
Conversation
setup-uv already handles Python installation, making the separate setup-python action redundant. This also removes the pip install step in resolve-versions, which will use uv instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Rewrite AGENTS.md as comprehensive agent guide with test framework overview, key concepts, debugging workflow, and best practices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Add new Python package for managing Docker services, platform, KAS instances, health checks, log viewing, and environment export. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
|
Summary of ChangesHello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a dedicated Python CLI tool, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the otdf-local Python package, a CLI for managing the local test environment, significantly improving upon shell script usage. While the code is generally well-structured, a potential path traversal vulnerability has been identified when setting up golden keys, and sensitive directories/files are created with world-readable default permissions. The review also addresses improving overall robustness, fixing a critical dependency issue, and ensuring consistency across documentation, configuration, and implementation, including corrections to file paths, improved path resolution, and suggestions for performance and portability.
| try: | ||
| import requests | ||
|
|
||
| platform = get_platform_service(settings) | ||
| if platform.is_running(): | ||
| resp = requests.get( | ||
| f"{settings.platform_url}/.well-known/opentdf-configuration", | ||
| timeout=5, | ||
| ) | ||
| if resp.ok: | ||
| config = resp.json() | ||
| if "version" in config: | ||
| env_vars["PLATFORM_VERSION"] = config["version"] | ||
| except Exception: | ||
| # If we can't get the version, that's okay | ||
| pass |
There was a problem hiding this comment.
The requests library is imported and used here, but it is not listed as a dependency in pyproject.toml. This will cause a runtime error if the library is not installed. The httpx library is already a dependency, so it should be used here instead for consistency.
Please add import httpx to the top of the file and use it here. Also, it's better to catch more specific exceptions than the broad Exception.
| try: | |
| import requests | |
| platform = get_platform_service(settings) | |
| if platform.is_running(): | |
| resp = requests.get( | |
| f"{settings.platform_url}/.well-known/opentdf-configuration", | |
| timeout=5, | |
| ) | |
| if resp.ok: | |
| config = resp.json() | |
| if "version" in config: | |
| env_vars["PLATFORM_VERSION"] = config["version"] | |
| except Exception: | |
| # If we can't get the version, that's okay | |
| pass | |
| # Try to get platform version from API | |
| try: | |
| platform = get_platform_service(settings) | |
| if platform.is_running(): | |
| resp = httpx.get( | |
| f"{settings.platform_url}/.well-known/opentdf-configuration", | |
| timeout=5, | |
| ) | |
| resp.raise_for_status() | |
| config = resp.json() | |
| if "version" in config: | |
| env_vars["PLATFORM_VERSION"] = config["version"] | |
| except (httpx.RequestError, json.JSONDecodeError): | |
| # If we can't get the version, that's okay | |
| pass |
| def _find_xtest_root() -> Path: | ||
| """Find the xtest/tests root directory by walking up from this file.""" | ||
| current = Path(__file__).resolve() | ||
| while current != current.parent: | ||
| # Check if we're in the otdf_local package within tests | ||
| if (current / "otdf_local").exists() and current.name == "tests": | ||
| return current | ||
| # Legacy: check for xtest directory | ||
| if (current / "conftest.py").exists() and current.name == "xtest": | ||
| return current | ||
| # Also check if we're in the otdf_local package within xtest | ||
| if (current / "otdf_local").exists() and ( | ||
| current.parent / "conftest.py" | ||
| ).exists(): | ||
| return current.parent | ||
| current = current.parent | ||
| # Fallback to assuming we're in tests/otdf-local | ||
| return Path(__file__).resolve().parent.parent.parent.parent |
There was a problem hiding this comment.
The logic in _find_xtest_root seems fragile and might not work correctly depending on where the code is checked out. It makes assumptions about the directory structure (e.g., that otdf-local is inside a tests or xtest directory) which don't seem to hold given the file structure in this PR. The fallback logic is also likely incorrect, as it resolves to the otdf-local directory itself, not the repository root. This will lead to incorrect paths for logs and other resources.
A more robust approach would be to find the project root based on a marker file like .git or by looking for the repo root, and then construct paths from there.
| ## Environment Setup for pytest | ||
|
|
||
| ```bash | ||
| cd tests/otdf-local |
There was a problem hiding this comment.
The path to the otdf-local package seems to be incorrect. Based on the file structure in this pull request, the package is located at otdf-local/, not tests/otdf-local/. This should be corrected throughout the documentation to avoid confusion.
| cd tests/otdf-local | |
| cd otdf-local |
| private_path = platform_dir / f"{kid}-private.pem" | ||
| cert_path = platform_dir / f"{kid}-cert.pem" |
There was a problem hiding this comment.
The setup_golden_keys function uses the kid field from extra-keys.json to construct file paths for writing private keys and certificates without proper sanitization. A malicious kid value containing path traversal sequences (e.g., ../../) could allow an attacker to write files to arbitrary locations on the filesystem, potentially overwriting sensitive system files if the tool is run with sufficient privileges.
| tail -f tests/xtest/logs/platform.log | ||
| tail -f tests/xtest/logs/kas-alpha.log | ||
| tail -f tests/xtest/logs/kas-km1.log |
There was a problem hiding this comment.
The log file paths mentioned here appear to be inconsistent with the implementation and other documentation. The settings.py file defines the log directory as xtest_root / "tmp" / "logs", and the .gitignore is updated for /xtest/logs/. Please ensure all paths are consistent across the implementation and documentation for clarity.
| self.logs_dir.mkdir(parents=True, exist_ok=True) | ||
| self.config_dir.mkdir(parents=True, exist_ok=True) | ||
| self.keys_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
The ensure_directories method creates directories for storing sensitive information, such as private keys and configuration files containing the root_key, using default system permissions. On many systems, this results in these directories and the files within them being world-readable, which could allow other users on the same system to access sensitive cryptographic material.
| @lru_cache | ||
| def get_settings() -> Settings: | ||
| """Get cached settings instance.""" | ||
| platform_url = os.environ.get("OTDF_LOCAL_PLATFORM_URL", "http://localhost:8080") | ||
| keycloak_url = os.environ.get("OTDF_LOCAL_KEYCLOAK_URL", "http://localhost:8888") | ||
| return Settings(platform_url=platform_url, keycloak_url=keycloak_url) |
There was a problem hiding this comment.
The get_settings function manually reads environment variables, which is redundant as pydantic-settings is designed to handle this automatically. You can simplify this by letting Pydantic manage the environment variable loading.
In addition to this change, you can simplify the field definitions in the Settings class (lines 124-125) to:
platform_url: str = "http://localhost:8080"
keycloak_url: str = "http://localhost:8888"pydantic-settings will automatically look for OTDF_LOCAL_PLATFORM_URL and OTDF_LOCAL_KEYCLOAK_URL and fall back to these defaults if the environment variables are not set. This makes the code cleaner and relies on the library's intended usage.
| @lru_cache | |
| def get_settings() -> Settings: | |
| """Get cached settings instance.""" | |
| platform_url = os.environ.get("OTDF_LOCAL_PLATFORM_URL", "http://localhost:8080") | |
| keycloak_url = os.environ.get("OTDF_LOCAL_KEYCLOAK_URL", "http://localhost:8888") | |
| return Settings(platform_url=platform_url, keycloak_url=keycloak_url) | |
| @lru_cache | |
| def get_settings() -> Settings: | |
| """Get cached settings instance.""" | |
| return Settings() |
| def read_tail(self, n: int = 50) -> list[LogEntry]: | ||
| """Read the last n lines from the log file.""" | ||
| if not self.log_file.exists(): | ||
| return [] | ||
|
|
||
| # Simple tail implementation | ||
| with open(self.log_file) as f: | ||
| lines = f.readlines() | ||
|
|
||
| return [self._parse_line(line) for line in lines[-n:]] |
There was a problem hiding this comment.
The read_tail implementation reads the entire file into memory to get the last n lines. This can be inefficient for large log files. For better performance and memory usage, consider a more efficient implementation that seeks near the end of the file and reads backwards.
| def kill_process_on_port(port: int) -> bool: | ||
| """Kill any process listening on the specified port. | ||
|
|
||
| Args: | ||
| port: Port number | ||
|
|
||
| Returns: | ||
| True if a process was killed | ||
| """ | ||
| try: | ||
| # Use lsof to find process on port | ||
| result = subprocess.run( | ||
| ["lsof", "-ti", f":{port}"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| return False | ||
|
|
||
| pids = result.stdout.strip().split("\n") | ||
| for pid in pids: | ||
| try: | ||
| os.kill(int(pid), signal.SIGKILL) | ||
| except (ProcessLookupError, ValueError): | ||
| pass | ||
| return True | ||
| except FileNotFoundError: | ||
| # lsof not available | ||
| return False |
There was a problem hiding this comment.
The kill_process_on_port function relies on lsof, which is not available on Windows. This limits the portability of this feature. While FileNotFoundError is handled, you might consider adding a platform-specific implementation for Windows (e.g., using netstat -ano and taskkill) if Windows support is a goal for this tool. The same applies to find_pid_by_name which uses pgrep.
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| cwd=str(__file__).rsplit("/tests/", 1)[0], |
There was a problem hiding this comment.
The cwd path is constructed with str(__file__).rsplit("/tests/", 1)[0]. This is a bit fragile as it assumes a specific directory structure. It would be more robust to determine the project root dynamically, for example by searching upwards for the pyproject.toml file. This would make the tests less dependent on the exact location of the test file.
1 participant
Summary
otdf-localPython package for managing Docker services, platform, KAS instances, health checks, log viewing, and environment export/xtest/logs/to.gitignoreParent PRs
chore/ci-replace-setup-python)docs/cleanup-agents-md)Test plan
cd otdf-local && uv sync && uv run ruff check . && uv run ruff format --check . && uv run pyrightcd otdf-local && uv run pytest tests/🤖 Generated with Claude Code
Part of stacked PR series decomposing
chore/the-claudiest-day-tmux