kola: add bootc-base tag for kola tests - #4469
Conversation
|
Skipping CI for Draft Pull Request. |
![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 introduces a --no-ignition flag to kola to allow running tests on pre-baked QCOW2 images, optimizing certain testing scenarios. However, the implementation of the --ssh-user flag introduces a potential SSH configuration injection vulnerability in mantle/platform/cluster.go as the user-supplied SSHUser string is written directly into the ssh-config file without sanitization. It is recommended to sanitize this input to prevent arbitrary SSH option injection. Additionally, consider improving the readability of the machine creation logic in the QEMU platform code.
| if bc.rconf.SSHUser != "" { | ||
| if _, err := fmt.Fprintf(sshBuf, " User %s\n", bc.rconf.SSHUser); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
The SSHUser command-line flag is written directly into the ssh-config file without sanitization. An attacker who can control the command-line arguments to kola can inject arbitrary SSH configuration options by including newlines in the SSHUser string. This can lead to arbitrary command execution if the ssh-config file is used by the user or another tool (e.g., via ProxyCommand).
| qc.mu.Lock() | ||
|
|
||
| conf, err := qc.RenderUserData(userdata, map[string]string{}) | ||
| if err != nil { | ||
| noIgnition := qc.RuntimeConf().NoIgnition | ||
| var conf *conf.Conf | ||
| var confPath string | ||
| var err error | ||
| if noIgnition { | ||
|
|
||
| qc.mu.Unlock() | ||
| return nil, err | ||
| } else { | ||
| conf, err = qc.RenderUserData(userdata, map[string]string{}) | ||
| if err != nil { | ||
| qc.mu.Unlock() | ||
| return nil, err | ||
| } | ||
| qc.mu.Unlock() | ||
|
|
||
| if conf.IsIgnition() { | ||
| confPath = filepath.Join(dir, "ignition.json") | ||
| if err := conf.WriteFile(confPath); err != nil { | ||
| return nil, err | ||
| } | ||
| } else if !conf.IsEmpty() { | ||
| return nil, fmt.Errorf("qemu only supports Ignition or empty configs") | ||
| } | ||
| } |
There was a problem hiding this comment.
This block for handling Ignition is a bit complex and hard to follow due to the locking and branching. It can be simplified by restructuring the if condition and moving the lock to be more tightly scoped around the operation it protects. This will improve readability and maintainability.
noIgnition := qc.RuntimeConf().NoIgnition
var conf *conf.Conf
var confPath string
var err error
if !noIgnition {
qc.mu.Lock()
conf, err = qc.RenderUserData(userdata, map[string]string{})
qc.mu.Unlock()
if err != nil {
return nil, err
}
if conf.IsIgnition() {
confPath = filepath.Join(dir, "ignition.json")
if err := conf.WriteFile(confPath); err != nil {
return nil, err
}
} else if !conf.IsEmpty() {
return nil, fmt.Errorf("qemu only supports Ignition or empty configs")
}
}
Do you have any context for all of this? Running nested container images inside kubernetes/openshift (where we run our pipeline today) isn't trivial so I'm not sure if it will save us much. Also, the description here is contradictory. It says we should be able to run tests against a container, but then you mention a QCOW with an ssh key inject, which is a VM. What's the real goal here? |
Hey Dusty, I’ll send over the task and the context I have. To be honest, I’m still figuring it out myself. Since this is a spike, the goal is to investigate and see what’s actually feasible. The DoD in the jira ticket is to create a POC and document the different approaches. |
7640c85 to
c6612bc
Compare
yasminvalim
changed the title
joelcapitao
left a comment
joelcapitao
left a comment
There was a problem hiding this comment.
It looks good overall, though I think we can already add systemd/SMBIOS support in this PR to implement SSH key provisioning.
So, instead of injecting SSH keys via Ignition, QEMU would be started with:
-smbios type=11,value=io.systemd.credential.binary:tmpfiles.extra=<base64-encoded-tmpfiles-config>
That way, we'd be able to run the kola bootc-base tagged tests against bootc image.
6b43480 to
5917f5d
Compare
|
@joelcapitao I added the support for SMBIOS and I guess it's good since I tested manually and worked fine. I need to test with bootc too. |
jlebon
left a comment
jlebon
left a comment
There was a problem hiding this comment.
Awesome, thanks for working on this!
I think it's OK to try things out by having some of the built-in tests be base bootc-compatible to start. But the real value is in external tests. Once we add enablement for bootc-base for external tests, I would probably even drop all of the internal tags we added to tests here. I don't think it's a good idea to have kola built-in tests be a chokepoint/maintenance burden as we look to scale out kola usage across !CoreOS.
| // Use default builder if none provided | ||
| builder = qc.ensureBuilderDefaults(builder) | ||
|
|
||
| qm, config, err := qc.createMachine(userdata) |
There was a problem hiding this comment.
createMachine seems to already handle the case where userdata could be nil. Would it be cleaner to instead keep using createMachine, and conditionalize whatever else is needed there on nil userdata?
There was a problem hiding this comment.
Not sure I got the point here, but I kept createMachine and moved the Ignition vs SMBIOS split there. For bootc runs we key off --no-ignition rather than userdata == nil, since many normal tests pass nil userdata and still expect default Ignition. When the flag isn’t set, nil userdata still goes through RenderUserDataIfNeeded as before. Is that making sense to you? What would be another approach for this?
| if qc.flight.opts.Arch != "" { | ||
| if err := builder.SetArchitecture(qc.flight.opts.Arch); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
This feels like something that should just already be taken care of when builder was constructed.
There was a problem hiding this comment.
I did this based in what I saw in qemuiso/cluster.go (line 91 - 101). As far I understood NewQemuBuilder only knows host defaults. Is that right? What would be the best approach in this case?
| if qc.flight.opts.Firmware != "" { | ||
| builder.Firmware = qc.flight.opts.Firmware | ||
| } |
5523475 to
c6d020a
Compare
6735d65 to
20a2c9d
Compare
90e9fcd to
6dcbff7
Compare
📝 WalkthroughWalkthroughAdds ChangesNo-Ignition QEMU execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant KolaCLI
participant KolaHarness
participant QemuCluster
participant Credentials
participant QemuGuest
KolaCLI->>KolaHarness: Enable --no-ignition
KolaHarness->>QemuCluster: Pass NoIgnition and SSHUser=root
QemuCluster->>Credentials: Write authorized_keys credentials
QemuCluster->>QemuGuest: Mount credentials through virtiofs
QemuGuest-->>QemuCluster: Start for SSH access
QemuCluster-->>KolaHarness: Connect as root
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
30d8be7 to
3289b46
Compare
62367b8 to
cbcb7c7
Compare
Add kola.BootcBaseTag and tag the coretest basic.* tests that do not set UserData, so they can be selected with kola run --tag bootc-base. Co-authored-by: Cursor <cursoragent@cursor.com>
Add helpers to build and write the tmpfiles.extra credential used to provision SSH keys without Ignition over virtiofs. Co-authored-by: Cursor <cursoragent@cursor.com>
Add MountSystemdCredentialDir() so QEMU shares a host directory with the guest over virtiofs using the io.systemd.credentials tag. Co-authored-by: Cursor <cursoragent@cursor.com>
Add --no-ignition to skip Ignition and provision SSH keys via virtiofs systemd credentials. SSH connects as root. Requires -p qemu and --qemu-image. Co-authored-by: Cursor <cursoragent@cursor.com>
…eos-assembler@5ea4caa Integrate virtiofs systemd credential changes from dustymabe/coreos-assembler 5ea4caa: move credential setup after disk and network configuration, expand cross-arch documentation, and log virtiofs tags in debug output. See https://systemd.io/CREDENTIALS/ for the systemd credentials spec. See systemd/systemd#29175 for upstream discussion of virtiofs as a credential transport. Co-authored-by: Cursor <cursoragent@cursor.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mantle/platform/machine/qemu/cluster.go`:
- Around line 163-174: Update the QEMU setup flow containing systemdCredDir and
qemuBuilder.MountSystemdCredentialDir so the temporary credential directory is
registered with the QEMU instance/builder cleanup path rather than left as an
independent host temp directory. Ensure setup failures after
WriteSystemdSSHCredentialsDir, including later builder configuration errors,
remove the directory, while preserving cleanup on successful
QemuInstance.Destroy().
In `@mantle/platform/qemu.go`:
- Around line 2032-2036: Update the virtiofs command construction around
createVirtiofsCmd to accept the HostMount.readonly value and add virtiofsd’s
read-only option when it is true. Ensure the virtiofs startup loop propagates
readonly for all mounts, including the systemd credential share, while
preserving writable behavior for mounts marked false.
- Around line 2029-2032: Update the allVirtioFSMounts initialization in the
relevant builder flow to create a zero-length slice with capacity for
builder.hostMounts, then copy the existing mounts into it before appending the
systemd credential mount. Preserve the resulting mount order and contents while
avoiding initialization with a nonzero length before append.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6dad89a-b556-4535-9c31-9f67f2828864
📒 Files selected for processing (10)
mantle/cmd/kola/options.gomantle/kola/harness.gomantle/kola/tests/coretest/core.gomantle/platform/cluster.gomantle/platform/credentials.gomantle/platform/credentials_test.gomantle/platform/machine/qemu/cluster.gomantle/platform/machine/qemu/flight.gomantle/platform/platform.gomantle/platform/qemu.go
| systemdCredDir, err := os.MkdirTemp("", "mantle-systemd-credentials-*") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating systemd credential dir: %w", err) | ||
| } | ||
| keys, err := qc.Keys() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting SSH keys: %w", err) | ||
| } | ||
| if err := platform.WriteSystemdSSHCredentialsDir(systemdCredDir, rconf.SSHUser, keys); err != nil { | ||
| return nil, err | ||
| } | ||
| qemuBuilder.MountSystemdCredentialDir(systemdCredDir) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A80 -B5 'func \(.*\*QemuBuilder\) Close|func \(.*\*QemuInstance\) Destroy|systemdCredentialDir' mantle/platformRepository: coreos/coreos-assembler
Length of output: 22953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'mantle/platform/*qemu*' 'mantle/platform/machine/qemu/*' | sort
echo
echo "== cluster.go relevant run sections =="
wc -l mantle/platform/machine/qemu/cluster.go mantle/platform/qemu.go
sed -n '130,195p;210,260p' mantle/platform/machine/qemu/cluster.go
echo
echo "== qemu.go relevant lifecycle sections =="
sed -n '490,575p;1980,2215p' mantle/platform/qemu.go
echo
echo "== usages of qc.Keys and platform.WriteSystemdSSHCredentialsDir =="
rg -n -C4 'WriteSystemdSSHCredentialsDir|systemdCredDir|MountSystemdCredentialDir' mantle/platformRepository: coreos/coreos-assembler
Length of output: 18886
Release the systemd credential directory with the QEMU instance.
The systemdCredDir created by MkdirTemp("", ...) is passed directly to the host machine and is not transferred into builder.tempdir/inst.tempdir, so QemuInstance.Destroy() does not remove it. Also, failures after WriteSystemdSSHCredentialsDir() can return without removing the partial directory. Move the path into the QEMU cleanup path and remove it on later setup failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mantle/platform/machine/qemu/cluster.go` around lines 163 - 174, Update the
QEMU setup flow containing systemdCredDir and
qemuBuilder.MountSystemdCredentialDir so the temporary credential directory is
registered with the QEMU instance/builder cleanup path rather than left as an
independent host temp directory. Ensure setup failures after
WriteSystemdSSHCredentialsDir, including later builder configuration errors,
remove the directory, while preserving cleanup on successful
QemuInstance.Destroy().
| allVirtioFSMounts := make([]HostMount, len(builder.hostMounts)) | ||
| copy(allVirtioFSMounts, builder.hostMounts) | ||
| if builder.systemdCredentialDir != "" { | ||
| allVirtioFSMounts = append(allVirtioFSMounts, HostMount{ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the makezero lint failure.
golangci-lint reports this initialized-length slice is subsequently appended to.
Proposed fix
- allVirtioFSMounts := make([]HostMount, len(builder.hostMounts))
- copy(allVirtioFSMounts, builder.hostMounts)
+ allVirtioFSMounts := make([]HostMount, 0, len(builder.hostMounts)+1)
+ allVirtioFSMounts = append(allVirtioFSMounts, builder.hostMounts...)📝 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.
| allVirtioFSMounts := make([]HostMount, len(builder.hostMounts)) | |
| copy(allVirtioFSMounts, builder.hostMounts) | |
| if builder.systemdCredentialDir != "" { | |
| allVirtioFSMounts = append(allVirtioFSMounts, HostMount{ | |
| allVirtioFSMounts := make([]HostMount, 0, len(builder.hostMounts)+1) | |
| allVirtioFSMounts = append(allVirtioFSMounts, builder.hostMounts...) | |
| if builder.systemdCredentialDir != "" { | |
| allVirtioFSMounts = append(allVirtioFSMounts, HostMount{ |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 2032-2032: append to slice allVirtioFSMounts with non-zero initialized length
(makezero)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mantle/platform/qemu.go` around lines 2029 - 2032, Update the
allVirtioFSMounts initialization in the relevant builder flow to create a
zero-length slice with capacity for builder.hostMounts, then copy the existing
mounts into it before appending the systemd credential mount. Preserve the
resulting mount order and contents while avoiding initialization with a nonzero
length before append.
Source: Linters/SAST tools
| allVirtioFSMounts = append(allVirtioFSMounts, HostMount{ | ||
| src: builder.systemdCredentialDir, | ||
| dest: SystemdCredentialVirtiofsTag, | ||
| readonly: true, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A35 -B5 'func createVirtiofsCmd|HostMount|readonly' mantle/platform/qemu.goRepository: coreos/coreos-assembler
Length of output: 15482
🌐 Web query:
virtiofsd --readonly option read-only mount documentation
💡 Result:
The --readonly option in virtiofsd is used to prevent write access to the shared directory from the guest [1][2]. When this option is enabled, the guest is unable to modify the contents of the shared file system [1][2]. It is important to note that --readonly does not make the underlying host directory an actual read-only mount [1][2]. Consequently, certain metadata, such as file access times (atime), may still be updated on the host when accessed from the guest [1][2]. For scenarios requiring a true read-only configuration, documentation recommends exporting a read-only mount point from the host [3][1][2]. This can be achieved by creating a separate mount point with read-only permissions and then pointing virtiofsd to that directory [3][1][2]. Example usage for read-only sharing: Using the --readonly flag: virtiofsd --shared-dir /path/to/share --readonly... Using a read-only bind mount: mkdir /path/to/ro-share mount -o bind,ro /path/to/share /path/to/ro-share virtiofsd --shared-dir /path/to/ro-share...
Citations:
- 1: https://lib.rs/crates/virtiofsd
- 2: https://docs.rs/crate/virtiofsd/latest
- 3: https://docs.rs/crate/virtiofsd/1.7.2
Enforce readonly for virtiofs mounts before starting virtiofsd.
HostMount.readonly is populated for the systemd credential share, but the loop ignores it and createVirtiofsCmd() never adds virtiofsd’s --readonly flag. Add an option/argument for read-only virtiofs mounts and use it for mounts like the credential dir so the host temp directory cannot be modified by the guest.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 2032-2032: append to slice allVirtioFSMounts with non-zero initialized length
(makezero)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mantle/platform/qemu.go` around lines 2032 - 2036, Update the virtiofs
command construction around createVirtiofsCmd to accept the HostMount.readonly
value and add virtiofsd’s read-only option when it is true. Ensure the virtiofs
startup loop propagates readonly for all mounts, including the systemd
credential share, while preserving writable behavior for mounts marked false.
4 participants
Adds a bootc-base tag for Kola tests that do not set register.Test.UserData (no test-specific Ignition/Butane).
Summary by CodeRabbit
New Features
--no-ignitionoption for QEMU test runs using a supplied disk image.rootuser.Bug Fixes
--no-ignitionis enabled.