feature(tests): add uv based project definitions #845
Conversation
fruch
force-pushed
the
move_to_uv
branch
2 times, most recently
from
676ff8a to
6b3fb67
Compare
so it would be easier to setup and start using the unittests of this project
fruch
force-pushed
the
move_to_uv
branch
3 times, most recently
from
fcd74ac to
137e764
Compare
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Python project setup by introducing uv-based dependency management and project configuration. It transitions from a legacy test-requirements.txt to a comprehensive pyproject.toml with PEP 621 metadata, adds pre-commit hooks with Ruff for code formatting, and applies consistent code style changes across the entire codebase. The changes also enhance the CI pipeline to use uv for dependency management and add pre-commit validation.
Key changes:
- Introduced modern Python project configuration with
pyproject.tomlfollowing PEP 621 standards - Added comprehensive tooling configuration (Ruff, pytest, mypy, black, isort) in
pyproject.toml - Applied automated code formatting (double quotes, import sorting, f-strings) across all Python files
- Enhanced Renovate configuration for better dependency management with uv support
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added comprehensive project metadata, dependencies, and tool configurations for modern Python development |
| pytest.ini | Added pytest configuration with markers, logging, and test discovery patterns |
| test-requirements.txt | Removed in favor of pyproject.toml dependencies |
| .python-version | Specified Python 3.14 as the project version |
| .pre-commit-config.yaml | Added Ruff pre-commit hooks for linting and formatting |
| renovate.json | Enhanced with uv-specific package rules and vulnerability scanning |
| .github/workflows/build.yml | Updated CI to use uv for dependency management and pre-commit validation |
| tests/*.py | Applied consistent code formatting (double quotes, import sorting, pytest usage) |
| tools/*.py | Reformatted with double quotes, removed encoding declarations, improved readability |
| packer/* | Applied consistent formatting to installation and configuration scripts |
| common/*.py | Standardized code style across common utilities and scripts |
| lib/*.py | Minor formatting improvements for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run('apt-get install -y gnupg2', shell=True, check=True) | ||
| run(f'mkdir -p {apt_keys_dir}; gpg --homedir /tmp --no-default-keyring --keyring {apt_keys_dir}/scylladb.gpg ' | ||
| f'--keyserver hkp://keyserver.ubuntu.com:80 --recv-keys a43e06657bac99e3', shell=True, check=True) | ||
| run("apt-get update --allow-insecure-repositories -y", shell=True, check=True) |
There was a problem hiding this comment.
The apt-get update invocation here uses --allow-insecure-repositories, which disables APT’s repository signature verification and allows unsigned or tampered package indexes to be accepted. An on-path attacker in the build environment could provide malicious repository metadata and later cause installation of compromised packages into your machine image. Remove --allow-insecure-repositories and require properly signed repositories instead, or explicitly trust only known keys and fail on signature errors.
| run("apt-get update --allow-insecure-repositories -y", shell=True, check=True) | |
| run("apt-get update -y", shell=True, check=True) |
| 'net-tools nload nmap python3-boto sysstat systemd-coredump tmux traceroute vim-nox vim.tiny xfsprogs aide', shell=True, check=True) | ||
| run(f'curl -L -o /usr/local/bin/yq https://github.com/mikefarah/yq/releases/latest/download/yq_linux_{deb_arch()} && chmod +x /usr/local/bin/yq', shell=True, check=True) | ||
| os.remove('/etc/apt/sources.list.d/scylla_install.list') | ||
| run("apt-get update --allow-insecure-repositories -y", shell=True, check=True) |
There was a problem hiding this comment.
This apt-get update call again uses --allow-insecure-repositories, weakening package integrity checks and allowing unsigned or tampered repository metadata to be accepted. An attacker controlling network traffic during image build could exploit this to steer subsequent installs toward malicious packages. Drop --allow-insecure-repositories here as well and rely on standard GPG-signed APT repositories.
| run("apt-get update --allow-insecure-repositories -y", shell=True, check=True) | |
| run("apt-get update -y", shell=True, check=True) |
| check=True, | ||
| ) | ||
| run( | ||
| f"apt-get install -y --auto-remove --allow-unauthenticated {args.product}-machine-image {args.product}-server-dbg " |
There was a problem hiding this comment.
The apt-get install command uses --allow-unauthenticated, which disables APT’s package signature verification and allows installation of packages that have been tampered with or are from untrusted sources. An on-path attacker or compromised mirror could inject malicious .deb files that get installed into the base image as root. Remove --allow-unauthenticated and ensure all packages are installed only from properly signed repositories.
| f"apt-get install -y --auto-remove --allow-unauthenticated {args.product}-machine-image {args.product}-server-dbg " | |
| f"apt-get install -y --auto-remove {args.product}-machine-image {args.product}-server-dbg " |
| run("apt-get install -y gnupg2", shell=True, check=True) | ||
| run( | ||
| f"mkdir -p {apt_keys_dir}; gpg --homedir /tmp --no-default-keyring --keyring {apt_keys_dir}/scylladb.gpg " | ||
| f"--keyserver hkp://keyserver.ubuntu.com:80 --recv-keys a43e06657bac99e3", |
There was a problem hiding this comment.
The GPG key for the Scylla APT repository is fetched via --keyserver hkp://keyserver.ubuntu.com:80, which uses an unauthenticated HTTP-based keyserver protocol. An attacker able to intercept this traffic during image build could inject a malicious public key that will later be trusted for package verification, enabling compromise of future upgrades. Switch to an HTTPS-protected key distribution method (e.g., hkps:// or a pinned key file over HTTPS) and avoid plain hkp:// keyservers for trust anchors.
| f"--keyserver hkp://keyserver.ubuntu.com:80 --recv-keys a43e06657bac99e3", | |
| f"--keyserver hkps://keyserver.ubuntu.com --recv-keys a43e06657bac99e3", |
| with open("/etc/hosts", "a") as f: | ||
| f.write("\n\n169.254.169.254 metadata.azure.internal\n") | ||
| with open("/etc/ssh/sshd_config.d/50-cloudimg-settings.conf", "w") as f: | ||
| f.write("ClientAliveInterval 180 \nHostKeyAlgorithms +ssh-rsa \nPubkeyAcceptedKeyTypes +ssh-rsa") |
There was a problem hiding this comment.
For Azure images, this block explicitly re-enables the legacy ssh-rsa algorithm via HostKeyAlgorithms +ssh-rsa and PubkeyAcceptedKeyTypes +ssh-rsa, which rely on SHA-1 and are deprecated as cryptographically weak. Allowing ssh-rsa increases exposure to signature-forgery and downgrade-style attacks against SSH connections to these instances. Prefer modern host key and key types such as rsa-sha2-256/rsa-sha2-512 and remove ssh-rsa from both HostKeyAlgorithms and PubkeyAcceptedKeyTypes.
| f.write("ClientAliveInterval 180 \nHostKeyAlgorithms +ssh-rsa \nPubkeyAcceptedKeyTypes +ssh-rsa") | |
| f.write("ClientAliveInterval 180\n") |
There was a problem hiding this comment.
@yaronkaikov
do you remember why we set it like that in #433 ?
it doesn't seems to be azure requirements (it was probably the default keys we used back then)
I think we can drop it now (the +ssh-rsa part)
There was a problem hiding this comment.
IIRC it's related to some requirements for Azure Marketplace publish , without that we will probably wont be able to publish our images
There was a problem hiding this comment.
Their docs don't say anything about it those. (At least now it doesn't say)
Lets test it when we test promotion for the private repo usage ?
There was a problem hiding this comment.
Any real reason to drop it? removing it now, and doing marketplace publish in few weeks which can fail. Then we need to remember what we changed.
There was a problem hiding this comment.
there is a reason it's gonna be a security issue, that this image is letting to use those key.
I'm testing it now during the phase I'm testing the promotion.
2 participants
so it would be easier to setup and start using the unittests of this project