feat: self-contained container image build #863
Conversation
COLDTURNIP
requested review from
innobead,
FrankYang0529,
yangchiu,
a110605,
derekbit,
PhanLe1010,
shuo-wu and
houhoucoop
|
Warning Rate limit exceeded@COLDTURNIP has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe pull request introduces enhancements to the CI/CD pipeline by adding a new job for collecting versioning information and modifying existing jobs for Docker image building. The Makefile is updated with new variables and targets to manage Docker buildx machines and to invoke a packaging script. Additionally, a new Bash script automates the Docker image build process by constructing and executing build commands based on various parameters and versioning logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as "Developer"
participant GH as "GitHub Actions"
participant BInfo as "build_info job"
participant CommImg as "Community Image Job"
Dev->>GH: Push commit or tag
GH->>BInfo: Trigger version collection
BInfo-->>GH: Return version variables
GH->>CommImg: Trigger build & push community image
sequenceDiagram
participant User as "Developer"
participant Make as "Makefile Targets"
participant Script as "Packaging Script"
participant Docker as "Docker Buildx"
User->>Make: Invoke buildx-machine target
Make-->>User: Buildx machine prepared
User->>Make: Run workflow-image-build-push target
Make->>Script: Execute packaging script
Script->>Docker: Build Docker image
Docker-->>Script: Return build result
Script-->>User: Output latest image info
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/package (1)
40-42: Enhance security attestation handlingThe secure build mode correctly adds SBOM and provenance attestation, but consider adding additional security measures.
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' + '--attest' 'type=provenance,mode=max' + '--attest' 'type=vuln,scan-severity=CRITICAL' +) [[ $TARGET_PLATFORMS ]] && buildx_args+=('--platform' "$TARGET_PLATFORMS")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)Makefile(2 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
64-64: property "build" is not defined in object type {build_info: {outputs: {image_tag: string; version_build: string; version_major: string; version_minor: string}; result: string}}
(expression)
76-76: property "build" is not defined in object type {build_info: {outputs: {image_tag: string; version_build: string; version_major: string; version_minor: string}; result: string}}
(expression)
93-93: property "build" is not defined in object type {build_info: {outputs: {image_tag: string; version_build: string; version_major: string; version_minor: string}; result: string}}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
scripts/package (1)
1-2: LGTM: Good error handling setupThe script properly sets
-eto exit on any error, which is crucial for build reliability.Makefile (1)
34-37: LGTM: Well-structured secure build targetsThe workflow targets properly separate secure and non-secure builds, with appropriate parameter passing.
.github/workflows/build.yml (1)
81-88: LGTM: Secure vault secret handlingThe workflow properly uses vault secrets for registry credentials, keeping sensitive information secure.
COLDTURNIP
force-pushed
the
feat-secure_build
branch
from
7bb0ef3 to
b948b14
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
55-55: Review Redundant Condition in Build Job
The condition on line 55 (if: ${{ startsWith(github.ref, 'refs/heads/') || startsWith(github.ref, 'refs/tags/') }}) appears to allow both tag and branch builds. Confirm that this is intentional and that no unintended build triggers occur. If the condition is meant only as a safeguard, a brief comment in the YAML might help future maintainers understand its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)Makefile(2 hunks)scripts/package(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- scripts/package
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (5)
.github/workflows/build.yml (5)
36-36: Verify Environment Variable for Branch Handling
In the branch condition,image_tagis set using${{ env.branch }}-head. Ensure that thebranchenvironment variable is defined in the workflow context (or via another mechanism) so that the constructed tag is valid.
54-64: Correct Usage of Build Info Outputs in Image Publishing
The updated reference usingneeds.build_info.outputs.image_tag(line 64) correctly replaces the previousneeds.build.outputs.image_tag, aligning with the output from the newbuild_infojob.
Great job updating the dependency!
76-76: Ensure Accurate Trigger for Secure Image Build
The conditionif: ${{ startsWith(github.ref, 'refs/tags/') && needs.build_info.outputs.version_build != '0' }}enforces that the secure image build runs only for tagged builds with a non-zero build version. Please verify that this logic fully captures the intended release criteria and thatversion_buildis always set correctly by the version extraction step.
81-88: Secure Handling of Vault Secrets
The step that reads secrets usingrancher-eio/read-vault-secrets@mainappears well integrated. Just ensure that the secrets are never echoed or printed to the logs later in the process, maintaining their confidentiality as required by the PR's security objectives.
28-34: 🛠️ Refactor suggestion
⚠️ Potential issueSecure and Robust Version Extraction
The current version extraction using a simplesedsubstitution may fail when processing the full ref (e.g."refs/tags/v1.2.3"). Moreover, there is no explicit validation of the expected tag format, which could lead to unintended behavior if an unexpected ref is encountered.
Consider validating the ref format and then extracting the version using parameter expansion. For example:- ref=${{ github.ref }} - if [[ "$ref" =~ 'refs/tags/' ]]; then - version=$(sed -E 's/^v([0-9.]*)$/\1' <<<$ref ) - version_major=$(cut -d. -f1 <<<$version) - version_minor=$(cut -d. -f2 <<<$version) - version_build=$(cut -d. -f3 <<<$version) - image_tag="$version" + ref=${{ github.ref }} + if [[ "$ref" =~ ^refs/tags/v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + version=${ref#refs/tags/v} + 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 + image_tag=${{ env.branch }}-head + else + echo "Unrecognized ref format: $ref" >&2 + exit 1 fiThis change both validates the input and simplifies the extraction.
Likely invalid or redundant comment.
| workflow-image-build-push: buildx-machine | ||
| MACHINE=$(MACHINE) OUTPUT_ARGS='--push' VERSION=$(VERSION) bash scripts/package | ||
| workflow-image-build-push-secure: buildx-machine | ||
| MACHINE=$(MACHINE) OUTPUT_ARGS='--push' VERSION=$(VERSION) IS_SECURE=true bash scripts/package |
There was a problem hiding this comment.
Cam we allow engineers to use make IS_SECURE=true run to test the production build locally?
There was a problem hiding this comment.
We still provide build target for local development, and the flag can be passed through envvar, e.g.:
export IS_SECURE=true
make build
There was a problem hiding this comment.
Tested that we can retrieve IS_SECURE from the environment and use it in the codebase.
COLDTURNIP
changed the title
COLDTURNIP
force-pushed
the
feat-secure_build
branch
3 times, most recently
from
1f561d1 to
5c42b10
Compare
COLDTURNIP
force-pushed
the
feat-secure_build
branch
5 times, most recently
from
7717615 to
d6c96c3
Compare
Signed-off-by: Raphanus Lo <[email protected]>
COLDTURNIP
force-pushed
the
feat-secure_build
branch
from
d6c96c3 to
39f7a59
Compare
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
Summary by CodeRabbit
New Features
Chores