Teach rkforge to find bundled aardvark-dns#589
Teach rkforge to find bundled aardvark-dns#589erasernoob wants to merge 1 commit intork8s-dev:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c41b1e5a8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| for candidate in aardvark_candidates() { | ||
| if candidate.is_file() { |
There was a problem hiding this comment.
default_aardvark_bin now accepts the first candidate that is only is_file(), including entries discovered from PATH. In environments where an earlier PATH directory contains a non-executable regular file named aardvark-dns, rkforge will select that path and netavark setup/teardown will fail, even if a valid executable exists later in PATH or in system locations. Filtering candidates by executability (or resolving via command lookup semantics) avoids this regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Refactors rkforge’s aardvark-dns discovery to support “bundled”/install-prefix layouts and centralizes the logic so container + compose networking share the same path resolution.
Changes:
- Introduces
commands::network_pathsto resolve netavark config dir and locateaardvark-dnsvia env vars, exe-relative locations, PATH, and system fallbacks. - Wires the new shared helpers into both container networking and compose networking code paths.
- Adds a build script and README guidance to improve local build + runtime discovery expectations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
project/rkforge/src/commands/network_paths.rs |
New centralized discovery logic for netavark config dir and aardvark-dns binary, plus a basic unit test. |
project/rkforge/src/commands/mod.rs |
Exposes the new network_paths module within the crate. |
project/rkforge/src/commands/container/network.rs |
Switches container networking to use shared aardvark/netavark path helpers. |
project/rkforge/src/commands/compose/network.rs |
Switches compose networking to use shared aardvark/netavark path helpers. |
project/rkforge/build.rs |
Attempts to embed a build-time default aardvark-dns path for discovery. |
project/rkforge/README.md |
Documents how to build aardvark-dns with rkforge and the intended runtime lookup order. |
| if let Some(path) = std::env::var_os("PATH") { | ||
| candidates.extend( | ||
| path_entries(&path) | ||
| .into_iter() | ||
| .map(|dir| dir.join(AARDVARK_BINARY_NAME)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Searching PATH for aardvark-dns can result in executing an unexpected binary when rkforge is run with elevated privileges (e.g., via sudo), especially if PATH contains relative entries like .. Consider filtering PATH entries to absolute/trusted directories (and/or moving PATH behind the hard-coded system locations) so discovery can’t be influenced by an untrusted working directory or user-controlled PATH ordering.
| #[test] | ||
| fn path_entries_are_searchable_for_aardvark() { | ||
| let tmp = tempdir().unwrap(); | ||
| let bin = tmp.path().join(AARDVARK_BINARY_NAME); | ||
| fs::write(&bin, b"#!/bin/sh\n").unwrap(); | ||
|
|
||
| let path = env::join_paths([tmp.path()]).unwrap(); | ||
| let found = path_entries(&path) | ||
| .into_iter() | ||
| .map(|dir| dir.join(AARDVARK_BINARY_NAME)) | ||
| .find(|candidate| candidate.is_file()); | ||
|
|
||
| assert_eq!(found, Some(bin)); | ||
| } |
There was a problem hiding this comment.
The new discovery behavior (exe-relative ../libexec/... paths and overall precedence) is largely untested: the current unit test only exercises PATH splitting, not that append_exe_relative_candidates produces the intended install-prefix candidates or that default_aardvark_bin prefers env vars vs. exe-relative vs. PATH/system paths. Adding targeted tests around candidate ordering/selection would help prevent regressions in this refactor.
| 4. `PATH` | ||
| 5. System paths such as `/usr/libexec/podman/aardvark-dns` and `/usr/bin/aardvark-dns` |
There was a problem hiding this comment.
The documented search order for aardvark-dns doesn’t fully match the code: network_paths also checks ../libexec/podman/aardvark-dns relative to the rkforge install prefix (and may include a build-time default via RKFORGE_AARDVARK_DNS_BUILD_DEFAULT). Please update this list to reflect the actual candidates (or adjust the code to match the documented order).
| 4. `PATH` | |
| 5. System paths such as `/usr/libexec/podman/aardvark-dns` and `/usr/bin/aardvark-dns` | |
| 4. `../libexec/podman/aardvark-dns` relative to the `rkforge` install prefix | |
| 5. A build-time default path, if `RKFORGE_AARDVARK_DNS_BUILD_DEFAULT` is set | |
| 6. `PATH` | |
| 7. System paths such as `/usr/libexec/podman/aardvark-dns` and `/usr/bin/aardvark-dns` |
Refactor the aardvark-dns discover logic in rkforge.