Add composefs backend#61
Merged
Merged
Conversation
The composefs backend mounts root as read-only, so CRI-O fails to mkdir /opt/cni. Move Calico and CRI-O CNI paths to /var/lib/cni/bin which is writable on both ostree and composefs backends. Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Alice Frosi <afrosi@redhat.com>
Wire bcvk --composefs-backend through the build system and CI so both ostree and composefs disk images are built and pushed to the registry. Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Alice Frosi <afrosi@redhat.com>
Reviewer's GuideAdds support for building and publishing a composefs-backed node disk image for Fedora node images, while standardizing CNI paths from /opt/cni to /var/lib/cni and /etc/cni/net.d, and making cloud-init/user-data and cluster manifests consistent with the new layout. Sequence diagram for composefs-backed disk image build and pushsequenceDiagram
participant GitHubActions
participant Makefile
participant bcvk
participant Registry
GitHubActions->>Makefile: make build-disk-image-composefs BOOTC_IMAGE BOOTC_DIGEST
Makefile->>Makefile: build-disk-image BCVK_EXTRA_ARGS=--composefs-backend NODE_IMAGE=NODE_IMAGE_COMPOSEFS
Makefile->>bcvk: bcvk to-disk --composefs-backend
bcvk-->>Makefile: composefs qcow2 image
GitHubActions->>Makefile: make print-node-image-composefs
Makefile-->>GitHubActions: NODE_IMAGE_COMPOSEFS
GitHubActions->>Registry: podman tag NODE_IMAGE_COMPOSEFS :TAG-disk-composefs
GitHubActions->>Registry: podman push :TAG-disk-composefs
GitHubActions->>Registry: podman tag NODE_IMAGE_COMPOSEFS :latest-disk-composefs
GitHubActions->>Registry: podman push :latest-disk-composefs
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:
Security issues:
- Container should not be privileged (link)
General comments:
- The
cleantarget innode-images/fedora/Makefileonly removes$(BOOTC_IMAGE)and$(NODE_IMAGE)and should be updated to also remove$(NODE_IMAGE_COMPOSEFS)to avoid leaving composefs images behind. - The GitHub Actions workflow now has nearly duplicated logic for pushing disk images (standard vs composefs); consider extracting the common tagging/pushing commands into a shared snippet or composite action to reduce drift between the two paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `clean` target in `node-images/fedora/Makefile` only removes `$(BOOTC_IMAGE)` and `$(NODE_IMAGE)` and should be updated to also remove `$(NODE_IMAGE_COMPOSEFS)` to avoid leaving composefs images behind.
- The GitHub Actions workflow now has nearly duplicated logic for pushing disk images (standard vs composefs); consider extracting the common tagging/pushing commands into a shared snippet or composite action to reduce drift between the two paths.
## Individual Comments
### Comment 1
<location path="internal/node/templates/user-data.yaml.tmpl" line_range="82" />
<code_context>
- systemctl daemon-reload
- systemctl enable --now var-mnt-cluster_images.mount
- - systemctl enable --now ostree-state-overlay@opt.service
+ - systemctl enable --now ostree-state-overlay@opt.service || true
- systemctl enable --now qemu-guest-agent
- nmcli connection modify "cloud-init enp2s0" ipv4.dns-search "~{{.ClusterDomain}} {{.ClusterDomain}}"
</code_context>
<issue_to_address>
**issue (bug_risk):** Silencing `systemctl` failures may hide real issues; consider at least logging failures explicitly
Using `|| true` here means any failure from `systemctl enable --now ostree-state-overlay@opt.service` (e.g., missing or misconfigured unit) will be silently ignored, making provisioning issues hard to detect. If this service is optional, guard the call with an explicit check (e.g., verifying the unit file exists or checking `systemctl list-unit-files`) and/or log a clear message when `systemctl` fails so it’s obvious when the overlay is not active.
</issue_to_address>
### Comment 2
<location path="internal/cluster/calico.yaml" line_range="5026-5032" />
<code_context>
</code_context>
<issue_to_address>
**security (CKV_K8S_16):** Container should not be privileged
Privileged containers are containers that have all of the root capabilities of a host machine, allowing access to resources that are not accessible in ordinary containers.
Common uses of privileged containers include: running a Docker daemon inside a Docker container, running a container with direct hardware access, and automating CI/CD tasks in the open-source automation server Jenkins.
Running a container with a privileged flag allows users to have critical access to the host's resources.
If a privileged container is compromised, it does not necessarily entail remote code execution, but it implies that an attacker will be able to run full host root with all of the available capabilities, including CAP_SYS_ADMIN.
<details>
<summary>Fix</summary>
*Kubernetes*
* *Resource:* Container
* *Arguments:* privileged (Optional)
If true, processes in the privileged containers are essentially equivalent to root on the host.
Default to false.
```yaml
apiVersion: v1
kind: Pod
metadata:
name: <Pod name>
spec:
containers:
- name: <container name>
image: <image>
securityContext:
- privileged: true
```
</details>
*Source: checkov*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - systemctl daemon-reload | ||
| - systemctl enable --now var-mnt-cluster_images.mount | ||
| - systemctl enable --now ostree-state-overlay@opt.service | ||
| - systemctl enable --now ostree-state-overlay@opt.service || true |
There was a problem hiding this comment.
issue (bug_risk): Silencing systemctl failures may hide real issues; consider at least logging failures explicitly
Using || true here means any failure from systemctl enable --now ostree-state-overlay@opt.service (e.g., missing or misconfigured unit) will be silently ignored, making provisioning issues hard to detect. If this service is optional, guard the call with an explicit check (e.g., verifying the unit file exists or checking systemctl list-unit-files) and/or log a clear message when systemctl fails so it’s obvious when the overlay is not active.
Comment on lines
5026
to
5032
| # Used to install CNI. | ||
| - name: cni-bin-dir | ||
| hostPath: | ||
| path: /opt/cni/bin | ||
| path: /var/lib/cni/bin | ||
| - name: cni-net-dir | ||
| hostPath: | ||
| path: /etc/cni/net.d |
There was a problem hiding this comment.
security (CKV_K8S_16): Container should not be privileged
Privileged containers are containers that have all of the root capabilities of a host machine, allowing access to resources that are not accessible in ordinary containers.
Common uses of privileged containers include: running a Docker daemon inside a Docker container, running a container with direct hardware access, and automating CI/CD tasks in the open-source automation server Jenkins.
Running a container with a privileged flag allows users to have critical access to the host's resources.
If a privileged container is compromised, it does not necessarily entail remote code execution, but it implies that an attacker will be able to run full host root with all of the available capabilities, including CAP_SYS_ADMIN.
Fix
Kubernetes
- Resource: Container
- Arguments: privileged (Optional)
If true, processes in the privileged containers are essentially equivalent to root on the host.
Default to false.
apiVersion: v1
kind: Pod
metadata:
name: <Pod name>
spec:
containers:
- name: <container name>
image: <image>
securityContext:
- privileged: trueSource: checkov
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #9
Summary by Sourcery
Add support for building and publishing Fedora node disk images using a composefs backend and align CNI paths with /var/lib/cni/bin across the system.
New Features:
Enhancements:
CI: