fix: improve pipeline reliability with image verification and retry logic #23
Conversation
Added a comprehensive profiles matrix documenting the five available configurations: - full: Complete toolset with all available tools - minimal: Lightweight profile with essential tools only - k8s: Kubernetes-focused profile - iac: Infrastructure as Code profile with bash-based tools - iac-pwsh: Infrastructure as Code profile with PowerShell support
- Fix CWE-78: Quote variables in seq command to prevent command injection - Fix CWE-78: Add proper quoting for EventId in vmss_monitor.sh - Fix deprecated apt-key usage, replace with gpg --dearmor - Fix insecure curl piping to bash, download scripts first - Add security documentation for privileged mode and NOPASSWD sudo - Improve curl error handling with standardized flags - Quote sensitive variables to prevent word splitting
- Use jq to safely construct JSON payload in vmss_monitor.sh to properly handle special characters in EventId - Add -L flag to curl for Kustomize download to follow redirects
…ogic - Remove continue-on-error from build-push step to catch failures - Add verification step after image push with retry logic - Add retry logic to manifest creation with image existence check - Wait for images to propagate in registry before creating manifests
Summary of ChangesHello @fok666, 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 significantly improves the reliability and error handling of the CI/CD pipeline, specifically addressing issues in the image build, push, and manifest creation processes. By introducing explicit error catching, image verification, and retry mechanisms, it mitigates transient failures and ensures that images are fully propagated before being used, leading to a more stable and predictable pipeline. Additionally, it includes minor script improvements for robustness and documentation updates for clarity. Highlights
Ignored Files
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 significantly improves the reliability and security of the Docker image build process and the runner scripts. The changes to avoid curl | bash, replace the deprecated apt-key, and properly quote variables in shell scripts are excellent. Using jq to construct JSON payloads is also a great improvement for robustness. The added documentation and security notes are very helpful. Overall, these are high-quality changes that make the project more secure and maintainable.
| curl -sLS "https://aka.ms/InstallAzureCLIDeb" -o /tmp/install-azure-cli.sh \ | ||
| && bash /tmp/install-azure-cli.sh \ | ||
| && rm /tmp/install-azure-cli.sh \ |
There was a problem hiding this comment.
This is a great security improvement. Piping curl directly to bash can be risky as it executes remote code without inspection. Downloading the script to a temporary file before executing it is a much safer practice. It also improves debuggability if the download or script execution fails.
| curl -sL https://apt.boltops.com/boltops-key.public | gpg --dearmor -o /usr/share/keyrings/boltops-archive-keyring.gpg \ | ||
| && echo "deb [signed-by=/usr/share/keyrings/boltops-archive-keyring.gpg] https://apt.boltops.com stable main" > /etc/apt/sources.list.d/boltops.list \ |
There was a problem hiding this comment.
Excellent change! apt-key is deprecated and its usage is discouraged for security reasons. Switching to gpg --dearmor and storing the key in /usr/share/keyrings is the modern, recommended, and more secure way to handle repository keys. This prevents the key from being trusted for all repositories on the system.
| curl -sLf "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" -o /tmp/install_kustomize.sh \ | ||
| && bash /tmp/install_kustomize.sh \ | ||
| && install -o root -g root -m 0755 kustomize /usr/local/bin/kustomize \ | ||
| && rm -f kustomize | ||
| && rm -f kustomize /tmp/install_kustomize.sh |
There was a problem hiding this comment.
This change significantly improves security by avoiding the curl | bash anti-pattern. Downloading the script to a temporary file before execution is the recommended and safer approach. Great job on applying this consistently.
|
|
||
| GITHUB_TOKEN_FILE=/runner/.token | ||
| echo -n $GITHUB_TOKEN > "$GITHUB_TOKEN_FILE" | ||
| echo -n "$GITHUB_TOKEN" > "$GITHUB_TOKEN_FILE" |
There was a problem hiding this comment.
Excellent! Quoting the $GITHUB_TOKEN variable is a crucial security and robustness improvement. It prevents potential issues like word splitting or globbing if the token were to contain special characters. This adheres to the shell scripting best practices outlined in the repository style guide.
References
- Variables, especially those containing secrets, must be quoted to prevent unexpected behavior and potential security risks. This is from line 8 of the repository style guide. (link)
| # So, give it some time to finish the job. | ||
| while true; do | ||
| ./config.sh remove --token $(cat "$GITHUB_TOKEN_FILE") && break | ||
| ./config.sh remove --token "$(cat "$GITHUB_TOKEN_FILE")" && break |
There was a problem hiding this comment.
Good catch! Quoting the command substitution $(cat "$GITHUB_TOKEN_FILE") ensures that the token is passed as a single argument to the remove command, even if it contains whitespace or other special characters. This is a great improvement for script robustness and aligns with the style guide.
References
- Command substitutions should be quoted to ensure their output is treated as a single string, which is critical when dealing with secrets or file contents. This is from line 8 of the repository style guide. (link)
| --name "${RUNNER_NAME:-$(hostname)}" \ | ||
| --url "$GITHUB_URL" \ | ||
| --token $(cat "$GITHUB_TOKEN_FILE") \ | ||
| --token "$(cat "$GITHUB_TOKEN_FILE")" \ |
There was a problem hiding this comment.
Another great fix. Quoting the command substitution for the token is essential for script reliability, preventing issues if the token contains special characters. This change correctly follows the repository's style guide on quoting variables and command substitutions.
References
- Command substitutions should be quoted to ensure their output is treated as a single string, which is critical when dealing with secrets or file contents. This is from line 8 of the repository style guide. (link)
| # Use jq to safely construct JSON payload to handle special characters | ||
| JSON_PAYLOAD=$(jq -n --arg eventId "$EventId" '{"StartRequests": [{"EventId": $eventId}]}') | ||
| curl -sf -X POST "$METADATA_ENDPOINT" \ | ||
| -H 'Metadata: true' \ | ||
| -d "{\"StartRequests\": [{\"EventId\": \"${EventId}\"}]}" | ||
| -H 'Content-Type: application/json' \ | ||
| -d "$JSON_PAYLOAD" |
There was a problem hiding this comment.
This is an excellent improvement for script robustness and security. Using jq to construct the JSON payload is much safer than manually creating the JSON string. It correctly handles any special characters that might be present in the EventId, preventing potential errors or injection vulnerabilities.
|
|
||
| # Launch runners | ||
| for R in $(seq 1 $RUNNER_COUNT); do | ||
| for R in $(seq 1 "$RUNNER_COUNT"); do |
There was a problem hiding this comment.
Good job quoting the $RUNNER_COUNT variable. This prevents potential word splitting and globbing issues, making the script more robust. This change aligns with the repository's style guide.
References
- Variables should be quoted to prevent word splitting and globbing. This is from line 8 of the repository style guide. (link)
Summary
This PR fixes the pipeline failures in the create-manifest job by improving reliability and error handling.
Changes
Problem Solved
The pipeline was failing because:
Testing
The changes ensure: