-
-
Notifications
You must be signed in to change notification settings - Fork 34
feat(docker): Comprehensive Docker Setup Enhancement with Performance & Security Improvements #890
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
base: main
Are you sure you want to change the base?
Conversation
…curity improvements Add comprehensive performance testing and monitoring scripts to track Docker build and runtime efficiency. Introduce a new GitHub Actions workflow for automated Docker performance testing, including nightly runs and PR comments. Implement security best practices in Docker setup, such as non-root user execution, read-only filesystems, and resource limits. Update Dockerfile and docker-compose files to reflect these changes. These enhancements aim to ensure the Docker setup is efficient, secure, and maintainable. By automating performance testing and monitoring, potential regressions can be identified early, and security vulnerabilities can be mitigated. The changes also facilitate a more robust CI/CD pipeline, improving overall development workflow.
…old handling Update the environment setup to distinguish between development and production configurations by introducing separate environment variables for database URLs and bot tokens. This change allows for more flexible testing scenarios by simulating different environments. Refactor the performance threshold checks to use a single failure flag (`THRESHOLD_FAILED`) instead of multiple environment variables. This simplifies the logic and improves readability. The step to fail the job if thresholds are exceeded is now integrated into the threshold checking logic, providing immediate feedback and reducing redundancy. These changes improve the workflow's maintainability and adaptability to different testing environments, ensuring more robust and clear performance testing.
…mance and flexibility - Update `.dockerignore` to include all markdown files except `README.md` and `requirements.md`, ensuring essential documentation is included in the build context. - Modify `Dockerfile` to enable parallel installation with Poetry, initialize a git repository for build processes, and perform aggressive cleanup for optimized production images. This reduces image size and enhances security by removing unnecessary files and metadata. - Add labels to the production image for better traceability and metadata management. - Introduce a non-root user in the production stage for enhanced security. - Update `scripts/test-docker.sh` to support command-line arguments for cache control and aggressive cleanup, improving test flexibility and performance. - Implement a cleanup function to manage Docker resources efficiently, preventing resource leaks and ensuring a clean test environment. - Adjust Docker run commands to use `--entrypoint=""` for more controlled execution during tests. These changes aim to optimize the Docker build process, reduce image size, and enhance security by removing unnecessary files. The test script improvements provide more control over the testing environment, allowing for more accurate performance assessments.
Update the Ruff version in the pre-commit configuration to v0.11.13 to ensure consistency with the formatter version. Change the hook id from 'ruff' to 'ruff-check' to align with the updated configuration. refactor(Dockerfile): update image metadata for clarity Modify the Dockerfile to update the image description and author/vendor fields for better clarity and readability. This change ensures that the metadata accurately reflects the project's branding as "All Things Linux". feat(cli/docker): enhance Docker CLI with additional commands and options Introduce new commands and options to the Docker CLI for improved functionality and user experience. Add commands for checking Docker availability, managing Tux-related resources, and executing various Docker operations. These enhancements provide more control and flexibility in managing Docker services and resources, especially for development and testing purposes.
Remove the `security_opt: no-new-privileges:true` option from the docker-compose development configuration. This change is made to simplify the configuration and avoid potential issues with services that require elevated privileges during development. The removal ensures that the development environment is less restrictive, which can be beneficial for debugging and testing purposes.
…ble thresholds Update the performance benchmarks section to include default performance thresholds that can be configured via environment variables. This change provides flexibility for different environments and hardware capabilities, allowing users to set custom thresholds for build time, startup time, Prisma generation, and memory usage. test(test-docker.sh): add performance threshold checks and memory parsing Enhance the test script to include performance threshold checks based on configurable environment variables. Add detailed logging for each performance metric, and implement memory usage parsing to handle various units (B, KiB, MiB, etc.). This ensures that performance metrics are within acceptable ranges and provides feedback for optimization if thresholds are exceeded.
…sting documentation Improve Docker cleanup scripts to ensure only test-related resources are removed, preserving system images and containers. Introduce detailed documentation for Docker testing strategies, including safety guidelines, comprehensive testing scenarios, and recovery procedures. The changes aim to prevent accidental removal of critical system resources during Docker cleanup processes, ensuring that only test-specific images and containers are affected. This enhances the reliability and safety of the CI/CD pipeline and local development environments. Comprehensive testing documentation provides clear guidance on executing various test scenarios, ensuring robust Docker functionality across all developer workflows.
Remove outdated Docker-related documentation and scripts to streamline the project and reduce maintenance overhead. The removed files include guides on Docker cleanup safety, security, testing strategies, and comprehensive testing scripts. These documents and scripts were replaced by a new unified Docker guide (DOCKER.md) that consolidates all relevant information and provides a more efficient and updated approach to Docker management. The new guide offers a comprehensive overview of the Docker setup, including performance improvements, testing strategies, security measures, and practical usage, making the old documents redundant. This change simplifies the documentation structure and ensures that all Docker-related information is current and easily accessible. feat(scripts): introduce unified docker management script Add `docker-toolkit.sh` to consolidate Docker operations such as testing, monitoring, and management into a single script. This script replaces multiple individual scripts (`monitor-resources.sh`, `quick-docker-test.sh`, `test-docker.sh`) to streamline Docker management tasks. The new script provides a comprehensive set of commands for quick validation, standard performance testing, comprehensive testing, resource monitoring, and safe cleanup of Docker resources. It also includes detailed logging and metrics collection for performance analysis. The change aims to simplify Docker operations by providing a single entry point for all related tasks, improving maintainability and usability for developers. The unified script ensures consistent execution and reporting across different Docker operations.
This comment was marked as outdated.
This comment was marked as outdated.
Replace `exec poe try run tux --dev start` with `exec poetry run tux --dev start` to ensure the correct execution of the development server. The previous command contained a typo or incorrect command sequence, which could lead to runtime errors or unexpected behavior during the development phase. This change ensures that the application starts correctly in development mode using the intended command.
…tch configuration
… logs behavior and healthcheck functionality
…nce - addresses Sourcery AI performance suggestion
…LW1510 linter rule
This comment was marked as resolved.
This comment was marked as resolved.
The PERFORMANCE-MONITORING.md file is deleted to reduce redundancy and streamline the documentation. The information contained in this file may have been moved to a more appropriate location or deemed unnecessary for the current project scope. This change helps in maintaining a cleaner and more focused documentation set, ensuring that only relevant and up-to-date information is available to the developers and users.
This comment was marked as outdated.
This comment was marked as outdated.
Introduce a streamlined set of GitHub Actions workflows to replace previous fragmented configurations. This change consolidates multiple workflows into a more organized and efficient setup, aligning with industry standards. - Add `release-drafter.yml` for automated release notes generation. - Introduce `ci.yml` for code quality checks, replacing separate linting and type-checking workflows. - Create `docker.yml` for Docker build, test, and deployment, merging previous Docker-related workflows. - Add `maintenance.yml` for routine tasks like TODO conversion and Docker image cleanup. - Implement `security.yml` for comprehensive security checks, including CodeQL analysis and dependency reviews. - Remove redundant workflows: `codeql.yml`, `docker-image.yml`, `docker-test.yml`, `linting.yml`, `pyright.yml`, `remove-old-images.yml`, and `todo.yml`. These changes reduce complexity, improve maintainability, and enhance security and performance monitoring. The new setup also provides cost savings by optimizing resource usage and execution time.
This comment was marked as outdated.
This comment was marked as outdated.
…environment Replace the container start test with a more comprehensive smoke test that verifies the bot's main module imports and checks the availability of essential Python modules like sqlite3 and asyncio. This ensures that the container not only starts but also has the necessary environment to run the bot. The changes improve the reliability of the tests by focusing on the bot's functionality rather than just the container's ability to start.
…fix capabilities Enable validation for GitHub Actions and security scanning with Gitleaks to ensure the integrity and security of the codebase. Introduce auto-fix capabilities for YAML, JSON, and Markdown using Prettier to maintain consistent code formatting and reduce manual intervention. These changes aim to improve code quality and security while streamlining the development process.
Switching from `python` to `python3` ensures compatibility with environments where Python 3 is the default or only version available. This change prevents potential issues in environments where `python` might point to Python 2. ci(workflows): add 'edited' event type and fix release drafter config path Including the 'edited' event type in release.yml ensures that changes to pull requests trigger the workflow, improving automation and consistency. The path to the release drafter config is corrected to `.github/release-drafter.yml` to ensure the workflow uses the correct configuration file. Additionally, the checkout step is added to ensure the repository is available for the workflow.
… of 'python3' The change updates the command used to run Python scripts in the Docker workflow from 'python3' to 'python'. This ensures compatibility with environments where 'python' is the default command for Python 3, which is increasingly common. This change helps avoid potential issues in environments where 'python3' is not explicitly available, improving the robustness of the CI workflow.
Add `--entrypoint python` to the docker run commands in the GitHub Actions workflow. This change ensures that the container uses Python as the entrypoint, which is necessary for executing the subsequent Python commands. This adjustment enhances the reliability of the smoke tests and production image tests by explicitly defining the entrypoint, preventing potential issues if the default entrypoint is not Python.
Condense the smoke test and production test scripts into single-line commands to improve readability and maintainability. This change eliminates the need for multi-line string formatting, reducing potential errors and making the workflow file cleaner and easier to understand. The functionality remains the same, ensuring that the necessary imports and basic checks are performed successfully.
Setting `fetch-depth` to 0 ensures that the entire git history is fetched during the checkout process. This is necessary for workflows that rely on the complete commit history, such as those that generate changelogs or perform versioning based on commit messages.
… and Markdown Switching to Prettier for file format validation allows for auto-fixing of style issues, improving code consistency and reducing manual intervention. This change enhances the workflow by leveraging Prettier's capabilities to maintain a clean and standardized codebase.
…t check Quote the $GITHUB_PATH variable to ensure proper handling of paths with spaces or special characters. Enhance the check for the Prisma client by iterating over potential directories and setting a flag to determine if the client is already cached. This change improves the reliability of the CI workflow by ensuring the correct path is added and by making the Prisma client check more robust.
Remove unnecessary whitespace in `renovate.json` and add `workflow_dispatch` to `ci.yml` to allow manual triggering of workflows. Add timeout and logging settings to improve debugging and execution control. Update Dockerfile to specify exact versions for dependencies, ensuring consistent builds and reducing potential issues from upstream changes. The changes improve maintainability and reliability of the CI/CD pipeline by allowing manual workflow triggers and enhancing debugging capabilities. Specifying exact package versions in the Dockerfile ensures consistent and reproducible builds, reducing the risk of unexpected behavior due to upstream changes.
style(ci.yml, docker.yml): standardize quotes and remove trailing spaces The GitHub token now defaults to a placeholder value, allowing for local testing without a real token. The log level is set to DEBUG for more detailed output during CI runs. Consistent use of double quotes improves readability and reduces potential errors. Trailing spaces are removed to maintain clean and consistent formatting across workflow files.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@sourcery-ai review |
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.
Hey @kzndotsh - I've reviewed your changes and they look great!
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
def _validate_docker_command(cmd: list[str]) -> bool: | ||
"""Validate that a Docker command contains only allowed components.""" | ||
# Define allowed Docker format strings for security | ||
allowed_format_strings = { |
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.
suggestion (performance): Move allowed_format_strings
out of hot path
Extract the set to a module-level constant to avoid recreating it on each function call and improve efficiency.
Suggested implementation:
ALLOWED_DOCKER_FORMAT_STRINGS = {
"{{.Repository}}:{{.Tag}}",
"{{.Names}}",
"{{.Name}}",
"{{.State.Status}}",
"{{.State.Health.Status}}",
"{{.Repository}}",
"{{.Tag}}",
"{{.ID}}",
"{{.Image}}",
"{{.Command}}",
"{{.CreatedAt}}",
"{{.Status}}",
}
def _validate_docker_command(cmd: list[str]) -> bool:
"""Validate that a Docker command contains only allowed components."""
# Use module-level allowed Docker format strings for security
allowed_format_strings = ALLOWED_DOCKER_FORMAT_STRINGS
@@ -87,9 +458,10 @@ | |||
|
|||
|
|||
@command_registration_decorator(docker_group, name="exec") | |||
@click.argument("service", default="tux", required=False) | |||
@click.option("-it", "--interactive", is_flag=True, default=True, help="Keep STDIN open and allocate a TTY.") |
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.
suggestion: Use a standard short option for interactive flag
Using -it
as a short option is non-standard in click; it will be parsed as -i
and -t
. Prefer -i
(or -I
) for the interactive flag, with TTY allocation implied when interactive is enabled.
@click.option("-it", "--interactive", is_flag=True, default=True, help="Keep STDIN open and allocate a TTY.") | |
@click.option("-i", "--interactive", is_flag=True, default=True, help="Keep STDIN open and allocate a TTY (implied when interactive).") |
healthcheck: | ||
test: ["CMD", "python", "-c", "import sys; sys.exit(0)"] | ||
interval: 30s | ||
timeout: 10s | ||
retries: 3 | ||
start_period: 40s |
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.
issue: Healthcheck always returns success
The current healthcheck always succeeds, so Docker can't detect actual failures. Use a check that verifies real service health, such as an HTTP request or status command.
check_flag = final_kwargs.pop("check", True) | ||
|
||
try: | ||
return subprocess.run(sanitized_cmd, check=check_flag, **final_kwargs) # type: ignore[return-value] |
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.
security (dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Summary by Sourcery
Enhance Docker integration across the project by adding robust security validation, resource cleanup, and new CLI commands, refactoring the Dockerfile for performance and security, updating compose configurations with limits and healthchecks, introducing a unified docker-toolkit script with extensive testing and monitoring capabilities, enriching user documentation, and consolidating GitHub Actions workflows into a streamlined CI/CD pipeline.
New Features:
Enhancements:
CI:
Documentation: