feat: self-contained container image build #514
Conversation
WalkthroughThe changes update the CI/CD workflow, build automation, Dockerfile, and packaging script. The GitHub Actions workflow now extracts and exports versioning details, removes legacy binary steps, and adds a secure image build job based on tag conditions. The Makefile introduces new variables and targets for Docker Buildx setup and image publishing workflows. The Dockerfile is refactored with a multi-stage build that compiles the application in a builder stage and then prepares the final image. The packaging script now has clearer variable naming and logic to select the build command and assign image tags. Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as GitHub Trigger
participant Build as Build Job
participant Secure as Secure Image Job
Trigger->>Build: Initiate build event
Build->>Build: Execute "Declare build info" to set version outputs
Build->>Build: Run "Build and publish image" step
alt Tag condition met
Build->>Secure: Conditionally trigger secure image build
Secure->>Secure: Checkout code, read secrets, build & push secure image
end
sequenceDiagram
participant Builder as Builder Stage
participant Final as Final Stage
Builder->>Builder: Copy code and run build script
Builder-->>Final: Provide compiled binary
Final->>Final: Copy binary and set executable permissions
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Makefile (1)
19-21: Enhance buildx machine setup resilience.The buildx machine setup should handle the case where the machine exists but is unhealthy.
- docker buildx ls | grep $(MACHINE) >/dev/null || \ - docker buildx create --name=$(MACHINE) --platform=$(DEFAULT_PLATFORMS) + @if ! docker buildx inspect $(MACHINE) >/dev/null 2>&1; then \ + echo "Creating buildx instance $(MACHINE)..."; \ + docker buildx create --name=$(MACHINE) --platform=$(DEFAULT_PLATFORMS); \ + elif ! docker buildx inspect $(MACHINE) | grep -q "Status: running"; then \ + echo "Removing unhealthy buildx instance $(MACHINE)..."; \ + docker buildx rm $(MACHINE); \ + echo "Creating new buildx instance $(MACHINE)..."; \ + docker buildx create --name=$(MACHINE) --platform=$(DEFAULT_PLATFORMS); \ + fipackage/Dockerfile (1)
35-36: Consider using COPY with mode instead of separate chmod.The separate chmod command can be combined with COPY for better efficiency.
-COPY --from=builder /app/bin/backing-image-manager-${ARCH} /usr/local/bin/backing-image-manager -RUN chmod +x /usr/local/bin/backing-image-manager +COPY --from=builder --chmod=755 /app/bin/backing-image-manager-${ARCH} /usr/local/bin/backing-image-manager
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(2 hunks)Makefile(2 hunks)package/Dockerfile(2 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[warning] 92-92: wrong indentation: expected 4 but found 6
(indentation)
🪛 actionlint (1.7.4)
.github/workflows/build.yml
90-90: got unexpected character '"' while lexing expression, expecting 'a'..'z', 'A'..'Z', '_', '0'..'9', ''', '}', '(', ')', '[', ']', '.', '!', '<', '>', '=', '&', '|', '*', ',', ' '. do you mean string literals? only single quotes are available for string delimiter
(expression)
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 15-15: scripts/package#L15
This variable is assigned to itself, so the assignment does nothing. (SC2269)
[notice] 18-18: scripts/package#L18
This variable is assigned to itself, so the assignment does nothing. (SC2269)
[notice] 19-19: scripts/package#L19
This variable is assigned to itself, so the assignment does nothing. (SC2269)
[notice] 20-20: scripts/package#L20
This variable is assigned to itself, so the assignment does nothing. (SC2269)
[notice] 21-21: scripts/package#L21
This variable is assigned to itself, so the assignment does nothing. (SC2269)
[notice] 17-17: scripts/package#L17
This variable is assigned to itself, so the assignment does nothing. (SC2269)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
scripts/package (2)
37-38: LGTM! Secure build configuration.The secure build configuration correctly adds SBOM generation and provenance attestation for enhanced supply chain security.
10-10: Verify buildx availability and handle errors.The command check for buildx should handle the case where neither
buildxnordocker buildxis available.✅ Verification successful
Verified buildx availability check.
- The command chain correctly attempts to locate standalone
buildxordocker buildx.- In the test, neither command is available, causing the script to echo "buildx not found" as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if buildx is available either as standalone or docker plugin command -v buildx >/dev/null || command -v docker >/dev/null && docker buildx version >/dev/null 2>&1 || echo "buildx not found"Length of output: 82
Makefile (2)
29-32: LGTM! Well-structured build targets.The build targets are well-organized with clear separation between secure and non-secure builds.
2-6: Verify platform compatibility.The
DEFAULT_PLATFORMSincludes darwin targets which may not be supported by all components. Verify compatibility.✅ Verification successful
Darwin Support in Vendor Code Confirmed
The search revealed a reference in
vendor/golang.org/x/sys/unix/syscall_bsd.goindicating that the vendor library includes code for darwin targets. This confirms that darwin is explicitly supported in a critical dependency, so theDEFAULT_PLATFORMSincluding darwin variants is in line with the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if darwin platforms are supported in the codebase rg -l 'GOOS.*darwin|darwin.*GOOS' || echo "No explicit darwin support found"Length of output: 79
package/Dockerfile (1)
2-13: LGTM! Secure multi-stage build implementation.The multi-stage build correctly isolates build dependencies from the final image, reducing attack surface.
.github/workflows/build.yml (1)
95-102: LGTM! Secure secrets handling.The vault secrets are correctly accessed and stored in environment variables, not exposed in logs.
derekbit
requested review from
FrankYang0529,
PhanLe1010,
c3y1huang and
shuo-wu
COLDTURNIP
force-pushed
the
feat-secure_build
branch
from
fbe3324 to
fcc3050
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
24-54:⚠️ Potential issueFix version extraction syntax and style issues.
The version extraction has syntax errors and style issues.
Apply these fixes:
- version=$(sed -E 's/^v([0-9.]*)$/\1') <<<$ref ) + version=$(echo "$ref" | sed -E 's/^refs\/tags\/v([0-9.]+)$/\1/') version_major=$(cut -d. -f1 <<<$version) version_minor=$(cut -d. -f2 <<<$version) version_build=$(cut -d. -f3 <<<$version) image_tag="$version" - elif [[ "$ref" =~ 'refs/heads/' ]]; then + elif [[ "$ref" =~ 'refs/heads/' ]]; then image_tag=${{ env.branch }}-head - fi + fi - + echo "version_major=${version_major}" >>$GITHUB_OUTPUT echo "version_minor=${version_minor}" >>$GITHUB_OUTPUT echo "version_build=${version_build}" >>$GITHUB_OUTPUT echo "image_tag=${image_tag}" >>$GITHUB_OUTPUT - +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (2)
scripts/package (1)
43-52: Consider conditional cache usage for faster builds.While using
--no-cacheensures clean builds, it might increase build times unnecessarily in CI/CD pipelines.Consider making cache usage configurable:
+USE_CACHE=${USE_CACHE:-'false'} +cache_args=() +[[ $USE_CACHE != 'true' ]] && cache_args+=('--no-cache') + -echo "${build_cmd[@]}" build --no-cache \ +echo "${build_cmd[@]}" build "${cache_args[@]}" \ "${builder_args[@]}" \ "${iid_file_args[@]}" \ "${buildx_args[@]}" \ -t "$image" -f package/Dockerfile . -"${build_cmd[@]}" build --no-cache \ +"${build_cmd[@]}" build "${cache_args[@]}" \ "${builder_args[@]}" \ "${iid_file_args[@]}" \ "${buildx_args[@]}" \ -t "$image" -f package/Dockerfile .package/Dockerfile (1)
2-13: Consider optimizing build context.While the current setup works, copying the entire context might include unnecessary files.
Consider using a
.dockerignorefile to exclude unnecessary files from the build context:+# Add .dockerignore file with: +.git/ +.github/ +.gitignore +*.md +coverage.out +bin/ +dist/
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(2 hunks)Makefile(2 hunks)package/Dockerfile(2 hunks)scripts/package(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[warning] 92-92: wrong indentation: expected 4 but found 6
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (10)
scripts/package (5)
8-21: LGTM! Well-structured variable declarations.The script follows shell scripting best practices with lowercase variable names and robust command detection.
23-29: Enhance version extraction error handling.While basic error handling exists, the version extraction could be more robust.
Apply this diff to improve error handling:
if [[ -z $TAG ]]; then - if api_version=$(./bin/backing-image-manager version --client-only | jq ".clientVersion.backingImageManagerAPIVersion"); then + if ! ./bin/backing-image-manager version --client-only >/dev/null 2>&1; then + echo "Error: Failed to get version information" >&2 + TAG="unknown_$(date -u +%Y%m%d)" + elif ! api_version=$(./bin/backing-image-manager version --client-only | jq -e ".clientVersion.backingImageManagerAPIVersion" 2>/dev/null); then + echo "Error: Failed to extract API version" >&2 + TAG="unknown_$(date -u +%Y%m%d)" + else TAG="v${api_version}_$(date -u +%Y%m%d)" - else - TAG="unknown_$(date -u +%Y%m%d)" fi fi
31-31: LGTM! Clear image name construction.The image name is constructed correctly using the provided variables.
54-56: LGTM! Clear output handling.The image tag is properly saved and output messages are clear.
33-41: LGTM! Secure build features are well implemented.The secure build features (SBOM and provenance) are correctly added when
IS_SECUREis true. The array handling for build arguments is robust.Run this script to verify the secure build features:
✅ Verification successful
Secure Build Features Verified
The secure build features (SBOM and attest with provenance mode) are added correctly when
IS_SECUREis true, as confirmed by the search output. No issues were found with their implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify secure build features in the codebase # Test: Search for secure build feature usage rg -A 5 'sbom|provenance|attest' .Length of output: 414
package/Dockerfile (3)
15-24: LGTM! Clear platform validation.The platform validation ensures the image is built only for supported architectures with clear error messages.
35-36: LGTM! Secure binary setup.The binary is securely copied from the builder stage and properly configured with executable permissions.
25-33: LGTM! Comprehensive package setup.The repository setup and package installation are well-structured.
Run this script to check for any known vulnerabilities in the installed packages:
✅ Verification successful
Approval: Comprehensive Package Setup Verified
The package installation and repository setup have been confirmed. Our preliminary security check script listed the installed packages (with some extraneous tokens from the extraction method), and no known vulnerabilities were immediately apparent for the intended packages. A manual review of the extracted package list (focusing on the actual packages installed such as fuse, libaio-devel, libibverbs, librdmacm1, librdmacm-utils, perl-Config-General, sg3_utils, qemu-tools, etc.) is still advised to be thorough.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for vulnerabilities in installed packages # Test: Extract package versions and check against known vulnerabilities rg -A 1 'zypper.*install' . | grep -v 'zypper' | tr -s ' ' '\n' | sort -u > packages.txt echo "Please check these packages for known vulnerabilities:" cat packages.txtLength of output: 530
.github/workflows/build.yml (2)
14-18: LGTM! Clear output structure.The output variables are well-defined and follow a logical structure.
86-114: LGTM! Secure image build implementation.The secure image build job correctly implements the security requirements with proper secret handling.
Run this script to verify the secure build configuration:
✅ Verification successful
Secure build configuration verified.
- The workflow uses the
workflow-image-build-push-securetarget as intended.- The grep output confirms that both secure build settings (via
IS_SECURE=true) and secret references (e.g.,PRIME_REGISTRYandprime-password) are present in the Makefile.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify secure build configuration # Test: Check for secure build features and secret usage rg -A 5 'workflow-image-build-push-secure|PRIME_REGISTRY|prime-password' .Length of output: 514
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 92-92: wrong indentation: expected 4 but found 6
(indentation)
COLDTURNIP
force-pushed
the
feat-secure_build
branch
from
fcc3050 to
0bae999
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build.yml (2)
42-47: Nitpick: Remove Trailing SpacesStatic analysis has detected trailing spaces on these lines (42 and 47). Removing these will align the file with YAML best practices and avoid linting errors.
A suggested diff:
(Ensure no extraneous whitespace after the indentation.)
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
86-114: Review of Secure Image Build and Push JobThe secure image build job correctly conditions its execution on the tag reference and a non-zero
version_build. The steps correctly obtain secrets and build the image using those secrets.Please note a formatting issue identified by the static analysis:
- Indentation Issue: On line 92 the
Checkout codestep is indented with 6 spaces, whereas the linter expects 4 spaces. This inconsistency may trigger warnings. Consider aligning the step with the community image job's indentation. For example:This adjustment will help conform to YAML indentation expectations.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 92-92: wrong indentation: expected 4 but found 6
(indentation)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(2 hunks)Makefile(2 hunks)package/Dockerfile(2 hunks)scripts/package(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- scripts/package
- package/Dockerfile
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[warning] 92-92: wrong indentation: expected 4 but found 6
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (5)
.github/workflows/build.yml (5)
14-19: Review of Build Job Outputs DeclarationThe new outputs are declared correctly and reference the values from the
build_infostep. Verify that all downstream jobs are updated to use these outputs.
24-31: Review of Build Info Declaration StepThe
build_infostep initializes the required variables before they’re populated. The default empty-string initializations are acceptable, though you may want to add comments if future maintainers might question why defaults are set.
48-54: Review of Version Info Echo BlockThe block echoing the version info to
$GITHUB_OUTPUTand printing it via a heredoc is clear. Just ensure that this version information is not sensitive. Otherwise, the implementation is straightforward.
66-84: Review of Community Image Build and Push StepThe configuration for building and publishing the community image is consistent with the new outputs and appears correct. All the required inputs, such as image details and credentials, are properly referenced.
32-41:⚠️ Potential issueCritical Issue: Version Extraction Logic
The version extraction in line 34 uses a regex (
s/^v([0-9.]*)$/\1) that expects the version string to start withv. However, the Git reference (github.ref) typically contains the full string (e.g.,refs/tags/v1.2.3), so this regex will not match correctly. Please update the command to extract the version correctly. For example:- version=$(sed -E 's/^v([0-9.]*)$/\1' <<<$ref ) + version=$(echo "$ref" | sed -E 's/^refs\/tags\/v([0-9.]+)$/\1/')This change ensures the prefix is properly stripped.
Likely invalid or redundant comment.
COLDTURNIP
force-pushed
the
feat-secure_build
branch
from
0bae999 to
5dd142e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/package (1)
23-29: 🛠️ Refactor suggestionImprove version extraction error handling.
The version extraction logic could be more robust by adding proper error messages and validation.
Apply this diff to improve error handling:
if [[ -z $TAG ]]; then - if api_version=$(./bin/backing-image-manager version --client-only | jq ".clientVersion.backingImageManagerAPIVersion"); then + if ! api_version=$(./bin/backing-image-manager version --client-only | jq -e ".clientVersion.backingImageManagerAPIVersion" 2>/dev/null); then + echo "Error: Failed to extract API version" >&2 TAG="unknown_$(date -u +%Y%m%d)" + else + TAG="v${api_version}_$(date -u +%Y%m%d)" fi - TAG="v${api_version}_$(date -u +%Y%m%d)" - else - TAG="unknown_$(date -u +%Y%m%d)" - fi fi.github/workflows/build.yml (1)
32-41:⚠️ Potential issueFix version extraction syntax and add validation.
The version extraction has syntax issues and lacks proper validation.
Apply this diff to fix the syntax and add validation:
- ref=${{ github.ref }} + ref="${{ github.ref }}" if [[ "$ref" =~ 'refs/tags/' ]]; then - version=$(sed -E 's/^v([0-9.]*)$/\1' <<<$ref ) + version=$(echo "$ref" | sed -E 's/^refs\/tags\/v([0-9.]+)$/\1/') + if [[ ! "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + echo "Error: Invalid version format: $version" >&2 + exit 1 + fi version_major=$(cut -d. -f1 <<<$version) version_minor=$(cut -d. -f2 <<<$version) version_build=$(cut -d. -f3 <<<$version) image_tag="$version" - elif [[ "$ref" =~ 'refs/heads/' ]] + elif [[ "$ref" =~ 'refs/heads/' ]]; then image_tag=${{ env.branch }}-head fi
🧹 Nitpick comments (3)
scripts/package (1)
39-41: Enhance security attestation flags.The secure build flags are correctly implemented but could benefit from additional security attestations.
Consider adding
--provenance=trueflag for enhanced supply chain security:IFS=' ' read -r -a buildx_args <<<"$OUTPUT_ARGS" -[[ $IS_SECURE == 'true' ]] && buildx_args+=('--sbom=true' '--attest' 'type=provenance,mode=max') +[[ $IS_SECURE == 'true' ]] && buildx_args+=('--sbom=true' '--provenance=true' '--attest' 'type=provenance,mode=max') [[ $TARGET_PLATFORMS ]] && buildx_args+=('--platform' "$TARGET_PLATFORMS")package/Dockerfile (1)
35-36: Consider using more restrictive permissions.The executable permissions are correctly set, but consider using more specific permissions for better security.
Apply this diff to set more restrictive permissions:
COPY --from=builder /app/bin/backing-image-manager-${ARCH} /usr/local/bin/backing-image-manager -RUN chmod +x /usr/local/bin/backing-image-manager +RUN chmod 755 /usr/local/bin/backing-image-manager.github/workflows/build.yml (1)
42-42: Remove trailing whitespace.There are trailing spaces on these lines.
Apply this diff to remove trailing whitespace:
- +Also applies to: 47-47
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(2 hunks)Makefile(2 hunks)package/Dockerfile(2 hunks)scripts/package(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (1)
package/Dockerfile (1)
2-13:⚠️ Potential issueVerify Go version compatibility.
The Dockerfile uses
golang:1.23which seems incorrect as Go 1.23 hasn't been released yet.Update the Go version to a stable release:
-FROM registry.suse.com/bci/golang:1.23 AS builder +FROM registry.suse.com/bci/golang:1.21 AS builder
COLDTURNIP
changed the title
Signed-off-by: Raphanus Lo <[email protected]>
COLDTURNIP
force-pushed
the
feat-secure_build
branch
from
5dd142e to
815ffbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
scripts/package (1)
43-57: Secure sensitive build arguments.The echo of the build command could expose sensitive information. Consider:
- Adding a warning about sensitive information.
- Redacting sensitive values in the echo.
- Making the echo conditional.
+# Warning: Build command may contain sensitive information +if [[ "${DEBUG:-}" == "true" ]]; then echo "${build_cmd[@]}" build --no-cache \ "${builder_args[@]}" \ "${iid_file_args[@]}" \ "${buildx_args[@]}" \ -t "$image" -f package/Dockerfile . +fi "${build_cmd[@]}" build --no-cache \ "${builder_args[@]}" \ "${iid_file_args[@]}" \ "${buildx_args[@]}" \ -t "$image" -f package/Dockerfile . echo "Built $image" -echo "$image" > ./bin/latest_image +# Add build metadata +cat > ./bin/latest_image <<EOF +image: $image +built_at: $(date -u +"%Y-%m-%dT%H:%M:%SZ") +secure_build: $IS_SECURE +EOF
🧹 Nitpick comments (5)
package/Dockerfile (1)
2-13: Multi-Stage Build: Builder Stage Initialization
The introduction of the builder stage usingregistry.suse.com/bci/golang:1.23and setting the working directory to/appstreamlines the build process and helps minimize the final image size. Be sure that the build script at/app/scripts/builddoes not print or expose any sensitive information during execution..github/workflows/build.yml (2)
43-47: Outputting Build Information
The script correctly appends the extracted version components to$GITHUB_OUTPUTfor use by later steps. If the finalcat <<EOFblock is only meant for logging purposes, consider removing or limiting its output—especially if any sensitive details might be present—to keep the build logs lean and secure.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
42-42: Remove Trailing Whitespace
Static analysis has flagged trailing spaces (on lines 42 and 47). Removing these will improve readability and maintain consistency across the workflow file.Also applies to: 47-47
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
scripts/package (2)
2-2: Enhance error handling and project name validation.
- Add more bash safety flags to catch common errors.
- Consider validating the project name against a known value.
-set -e +set -euo pipefail + +# Validate project name +expected_project="backing-image-manager" +if [[ "$project" != "$expected_project" ]]; then + echo "Error: Unexpected project name: $project (expected: $expected_project)" >&2 + exit 1 +fiAlso applies to: 8-8
31-31: Validate image name components.Add validation for image name components to ensure they meet Docker naming requirements.
+# Validate image name components +if [[ ! "$REPO" =~ ^[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)*$ ]]; then + echo "Error: Invalid repository name format: $REPO" >&2 + exit 1 +fi +if [[ ! "$IMAGE_NAME" =~ ^[a-z0-9]+([._-][a-z0-9]+)*$ ]]; then + echo "Error: Invalid image name format: $IMAGE_NAME" >&2 + exit 1 +fi image="${REPO}/${IMAGE_NAME}:${TAG}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(2 hunks)Makefile(2 hunks)package/Dockerfile(2 hunks)scripts/package(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (7)
package/Dockerfile (1)
35-36: Secure Transfer of the Compiled Binary
UsingCOPY --from=builderto bring in thebacking-image-managerbinary helps isolate build-time dependencies from the production image. Confirm that the${ARCH}variable is resolved consistently with the environment defined earlier (viaENV ARCH ${TARGETPLATFORM#linux/})..github/workflows/build.yml (3)
14-19: Exposing Build Outputs
The outputs forversion_major,version_minor,version_build, andimage_tagare now properly defined for downstream jobs. Ensure that the values extracted later in thebuild_infostep match your expectations.
80-84: Validate Image Tag and Repository Configuration
The “Build and publish image” step now uses theimage_tagoutput from the build_info step. Double-check that the derived tag conforms to Docker tag naming conventions and that the repository (docker.io/longhornio) is correct for your secure build process.
32-41:⚠️ Potential issueFix Version Extraction Logic in build_info Step
The current version extraction via sed does not account for the full ref format (refs/tags/v...). This may result in an empty or incorrect version string. Consider revising the command as follows:- version=$(sed -E 's/^v([0-9.]*)$/\1' <<<$ref ) + version=$(echo "$ref" | sed -E 's#^refs/tags/v([0-9.]+)$#\1#')Also, verify that
env.branchis defined when the ref indicates a branch.Likely invalid or redundant comment.
scripts/package (3)
23-29: Improve version extraction error handling.While error handling has improved, the error case could be more informative.
if [[ -z $TAG ]]; then - if api_version=$(./bin/backing-image-manager version --client-only | jq ".clientVersion.backingImageManagerAPIVersion"); then + if ! api_version=$(./bin/backing-image-manager version --client-only | jq -e ".clientVersion.backingImageManagerAPIVersion" 2>/dev/null); then + echo "Error: Failed to extract API version" >&2 + echo "Warning: Using fallback tag" >&2 TAG="unknown_$(date -u +%Y%m%d)" + else + TAG="v${api_version}_$(date -u +%Y%m%d)" fi fi
39-41: LGTM! Good security feature implementation.The implementation correctly enables SBOM generation and provenance attestation for secure builds, which aligns with the PR objectives for enhancing build security.
12-21: 🛠️ Refactor suggestionAdd parameter validation and documentation.
Security-sensitive parameters should be documented and validated to prevent misuse.
Add parameter validation and documentation:
+# Configuration parameters: +# REPO: Container registry repository (default: longhornio) +# IMAGE_NAME: Name of the container image (default: project name) +# TAG: Image tag (default: auto-generated from version) +# OUTPUT_ARGS: Docker buildx output arguments (default: --load) +# IS_SECURE: Enable security features like SBOM and attestation (default: false) +# MACHINE: Builder instance name (default: '') +# TARGET_PLATFORMS: Target build platforms (default: '') +# IID_FILE: Image ID file path (default: '') +# IID_FILE_FLAG: Custom image ID file flag (default: '') +# Validate security parameters +if [[ "$IS_SECURE" != "true" && "$IS_SECURE" != "false" ]]; then + echo "Error: IS_SECURE must be 'true' or 'false'" >&2 + exit 1 +fi✅ Verification successful
Error: No shell scripts or web queries found
Parameter validation and documentation implemented correctly.
The changes now include clear inline documentation for all configuration parameters (with default values) and a validation check to ensure the security-sensitive parameter
IS_SECUREhas an acceptable value ("true"or"false"). This prevents misuse and improves clarity.
- Documentation comments for parameters have been added.
- A validation block is included for
IS_SECURE.
|
|
||
| if [ ! -x ./bin/longhorn ]; then | ||
| ./scripts/build | ||
| command -v buildx >/dev/null && build_cmd=(buildx) || build_cmd=(docker buildx) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve build command validation.
The current command check could fail silently and doesn't verify docker availability in the fallback case.
-command -v buildx >/dev/null && build_cmd=(buildx) || build_cmd=(docker buildx)
+if command -v buildx >/dev/null 2>&1; then
+ build_cmd=(buildx)
+elif command -v docker >/dev/null 2>&1; then
+ build_cmd=(docker buildx)
+else
+ echo "Error: Neither buildx nor docker commands are available" >&2
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| command -v buildx >/dev/null && build_cmd=(buildx) || build_cmd=(docker buildx) | |
| if command -v buildx >/dev/null 2>&1; then | |
| build_cmd=(buildx) | |
| elif command -v docker >/dev/null 2>&1; then | |
| build_cmd=(docker buildx) | |
| else | |
| echo "Error: Neither buildx nor docker commands are available" >&2 | |
| exit 1 | |
| fi |
Which issue(s) this PR fixes:
Supports security build.
What this PR does / why we need it:
Special notes for your reviewer:
The secrets provided by EIO MUST NOT present in any visible output.
Additional documentation or context
Guidance on achieving SLSA Level 3