feat(podman): add system CA certificate support and pre-install nftables#556
Open
wherka-ama wants to merge 1 commit into
Open
feat(podman): add system CA certificate support and pre-install nftables#556wherka-ama wants to merge 1 commit into
wherka-ama wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR detects and stages system CA certificates into the Podman build context, threads a certsCopied flag into Containerfile generation to conditionally install those certs, ensures nftables is included in workspace images, and wires the workspace image into pod templates for network-guard usage. ChangesEnterprise Proxy CA Support and Workspace Image Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/runtime/podman/network.go (2)
334-335: 💤 Low valueConsider improving the error message to aid debugging.
The error message
"nftables installation failed"is generic and doesn't provide context about potential causes. Given that this PR specifically addresses SSL/certificate issues in enterprise proxy environments, mentioning this potential cause could save debugging time if the fallback installation fails.💡 Suggested error message improvement
- // Verify nft is available after installation attempt - parts = append(parts, "command -v nft >/dev/null 2>&1 || { echo 'nftables installation failed'; exit 1; }") + // Verify nft is available after installation attempt + parts = append(parts, "command -v nft >/dev/null 2>&1 || { echo 'nftables installation failed (this should not happen if workspace image was built correctly; check for SSL/certificate issues if dnf install failed)'; exit 1; }")🤖 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 `@pkg/runtime/podman/network.go` around lines 334 - 335, Update the appended shell check string (the one added to the parts slice that currently reads "command -v nft >/dev/null 2>&1 || { echo 'nftables installation failed'; exit 1; }") to print a more descriptive error that mentions possible enterprise proxy/SSL/certificate issues and diagnostic hints (for example suggest checking proxy settings, certificate trust, or include the command's stderr/exit code). Locate the code that appends to the parts variable in the function handling network setup (the parts = append(parts, "...nft...") line) and replace the simple echo with a message that includes context about proxy/cert causes and a note to include the captured error/exit status for debugging.
330-332: ⚡ Quick winClarify that nftables is pre-installed and this is a defensive fallback.
The comment suggests that the caller should ensure CA certificates are available, but this doesn't accurately reflect the PR's design:
- According to the PR's architecture, nftables is pre-installed in the workspace image during build (Containerfile always includes nftables in the package list).
- The network-guard container uses the workspace image, so nftables should already be present.
- CA certificates are also installed in the workspace image during build.
- This
dnf installcommand should never execute under normal operation—it's a defensive fallback.The current comment implies this is a primary installation mechanism that might fail, when it's actually a safety net for edge cases (e.g., if someone manually removes nftables or uses a non-standard image).
📝 Suggested comment revision
- // Ensure nftables is installed before applying rules. - // Note: This may fail due to SSL certificate issues with enterprise proxies. - // The caller should ensure CA certificates are available in the container. + // Verify nftables is available (should already be pre-installed in workspace image). + // This fallback installation exists for robustness but should rarely execute. parts = append(parts, "command -v nft >/dev/null 2>&1 || dnf install -y nftables")🤖 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 `@pkg/runtime/podman/network.go` around lines 330 - 332, Update the inline comment above the code that appends the fallback install command (the parts variable entry: "command -v nft >/dev/null 2>&1 || dnf install -y nftables") to state that nftables and CA certificates are pre-installed in the workspace image during build and that the network-guard container uses that image, so this dnf install is a defensive fallback for edge cases (e.g., manual removal or non-standard images) and should never run during normal operation; reference the parts variable and the exact command string in the comment so reviewers can see this is a safety-net rather than the primary installation mechanism.pkg/runtime/podman/create_test.go (1)
181-240: ⚡ Quick winCA copy test is non-deterministic and can miss regressions.
This test depends on host machine cert state, so it may pass without validating the intended behavior. Please make
copySystemCACertificatestestable with injected cert path candidates (or an fs abstraction) so both “copied” and “not copied” branches are asserted deterministically.As per coding guidelines, “Tests use the standard
testingpackage and should cover command initialization, execution, and error cases.”🤖 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 `@pkg/runtime/podman/create_test.go` around lines 181 - 240, The test for copySystemCACertificates is non-deterministic because it reads real host system paths; refactor podmanRuntime.copySystemCACertificates to accept either an injected list of candidate cert paths or a filesystem abstraction (e.g., an io/fs or interface) so tests can control presence/absence of cert files, then update create_test.go to create temporary fake cert files and assert certsCopied==true and file contents, and a separate case with no files asserting certsCopied==false; keep the existing function name copySystemCACertificates and the podmanRuntime receiver so callers are easy to update.pkg/runtime/podman/podman_test.go (1)
331-333: ⚡ Quick winTighten the workspace image assertion to avoid false positives.
Checking only for
"kdn-"is too broad. Assert the exact expected image from the fixture (e.g.image: kdn-my-project) so template regressions are caught.🤖 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 `@pkg/runtime/podman/podman_test.go` around lines 331 - 333, The assertion in the test that checks yamlStr for "kdn-" is too permissive; update the test to assert the exact expected workspace image string from the fixture (e.g. check yamlStr contains "image: kdn-my-project" or assert a full line match) instead of just "kdn-". Locate the failing check that references yamlStr in the podman_test.go test block and replace the contains("kdn-") assertion with a contains("image: kdn-my-project") (or equivalent exact match) to prevent false positives and catch template regressions.
🤖 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 `@pkg/runtime/podman/create.go`:
- Around line 159-166: The loop that builds certContent iterates certPaths and
calls os.ReadFile(path), which fails for directory entries (e.g.,
"/etc/pki/ca-trust/source/anchors") and silently skips their contents; update
the logic that populates certContent (the certPaths loop in create.go) to detect
whether path is a directory (using os.Stat or os.ReadDir) and if so iterate the
directory entries (filtering for certificate file extensions like .crt/.pem or
all regular files), read each file's bytes and append them to certContent,
otherwise read the single file as before; keep the same certPaths and
certContent variables and replace the direct os.ReadFile(path) call with this
directory-aware reading flow.
---
Nitpick comments:
In `@pkg/runtime/podman/create_test.go`:
- Around line 181-240: The test for copySystemCACertificates is
non-deterministic because it reads real host system paths; refactor
podmanRuntime.copySystemCACertificates to accept either an injected list of
candidate cert paths or a filesystem abstraction (e.g., an io/fs or interface)
so tests can control presence/absence of cert files, then update create_test.go
to create temporary fake cert files and assert certsCopied==true and file
contents, and a separate case with no files asserting certsCopied==false; keep
the existing function name copySystemCACertificates and the podmanRuntime
receiver so callers are easy to update.
In `@pkg/runtime/podman/network.go`:
- Around line 334-335: Update the appended shell check string (the one added to
the parts slice that currently reads "command -v nft >/dev/null 2>&1 || { echo
'nftables installation failed'; exit 1; }") to print a more descriptive error
that mentions possible enterprise proxy/SSL/certificate issues and diagnostic
hints (for example suggest checking proxy settings, certificate trust, or
include the command's stderr/exit code). Locate the code that appends to the
parts variable in the function handling network setup (the parts = append(parts,
"...nft...") line) and replace the simple echo with a message that includes
context about proxy/cert causes and a note to include the captured error/exit
status for debugging.
- Around line 330-332: Update the inline comment above the code that appends the
fallback install command (the parts variable entry: "command -v nft >/dev/null
2>&1 || dnf install -y nftables") to state that nftables and CA certificates are
pre-installed in the workspace image during build and that the network-guard
container uses that image, so this dnf install is a defensive fallback for edge
cases (e.g., manual removal or non-standard images) and should never run during
normal operation; reference the parts variable and the exact command string in
the comment so reviewers can see this is a safety-net rather than the primary
installation mechanism.
In `@pkg/runtime/podman/podman_test.go`:
- Around line 331-333: The assertion in the test that checks yamlStr for "kdn-"
is too permissive; update the test to assert the exact expected workspace image
string from the fixture (e.g. check yamlStr contains "image: kdn-my-project" or
assert a full line match) instead of just "kdn-". Locate the failing check that
references yamlStr in the podman_test.go test block and replace the
contains("kdn-") assertion with a contains("image: kdn-my-project") (or
equivalent exact match) to prevent false positives and catch template
regressions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 256f3ccc-86d4-4606-bd0a-0fee03308c77
📒 Files selected for processing (11)
.gitignorepkg/runtime/podman/containerfile.gopkg/runtime/podman/containerfile_test.gopkg/runtime/podman/create.gopkg/runtime/podman/create_test.gopkg/runtime/podman/dashboard_test.gopkg/runtime/podman/network.gopkg/runtime/podman/podman_test.gopkg/runtime/podman/pods/onecli-pod.yamlpkg/runtime/podman/start_test.gopkg/runtime/podman/steplogger_test.go
wherka-ama
force-pushed
the
fix/podman-ca-certificates-nftables
branch
from
e9e5893 to
0f260bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/runtime/podman/create_test.go`:
- Around line 253-291: The test TestFindSystemCACertificates is coupled to the
SSL_CERT_FILE env var; update the test to isolate env state by calling
t.Setenv("SSL_CERT_FILE", "") at the start of TestFindSystemCACertificates
(before creating subtests) and stop running the subtests in parallel (remove or
move t.Parallel() out of the individual t.Run closures) so
findSystemCACertificates is not affected by external SSL_CERT_FILE values; keep
references to the same test name TestFindSystemCACertificates and the function
findSystemCACertificates when locating the code to change.
In `@pkg/runtime/podman/create.go`:
- Around line 142-155: The code reads CA bundle paths (caBundlePath from env and
entries in certPaths) using os.Stat/os.ReadFile without normalizing—call
filepath.Abs on caBundlePath and on each path iterated from certPaths (handle
filepath.Abs errors by skipping that candidate) and then use the resulting
absolute path for os.Stat and os.ReadFile so relative-path ambiguity is
eliminated; update the blocks around caBundlePath, certPaths and certContent to
use the absolute path variable before checking/reading.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f6a25d3-900f-49f3-9ea8-e4c64fb2e96c
📒 Files selected for processing (11)
.gitignorepkg/runtime/podman/containerfile.gopkg/runtime/podman/containerfile_test.gopkg/runtime/podman/create.gopkg/runtime/podman/create_test.gopkg/runtime/podman/dashboard_test.gopkg/runtime/podman/network.gopkg/runtime/podman/podman_test.gopkg/runtime/podman/pods/onecli-pod.yamlpkg/runtime/podman/start_test.gopkg/runtime/podman/steplogger_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/runtime/podman/steplogger_test.go
- .gitignore
- pkg/runtime/podman/pods/onecli-pod.yaml
- pkg/runtime/podman/start_test.go
wherka-ama
force-pushed
the
fix/podman-ca-certificates-nftables
branch
from
0f260bf to
dfbc63c
Compare
Add system CA certificate detection and copying to build context for enterprise proxy support. Pre-install nftables in workspace image to avoid runtime installation failures in deny-mode networking. Changes: - Detect and copy system CA certificates from common Linux paths - Conditionally add COPY instructions to Containerfile when certs exist - Pre-install nftables in workspace image during build - Use workspace image for network-guard container instead of base image Signed-off-by: Waldek Herka <87032474+wherka-ama@users.noreply.github.com> Signed-off-by: Waldek Herka <wherka-ama@users.noreply.github.com>
wherka-ama
force-pushed
the
fix/podman-ca-certificates-nftables
branch
from
dfbc63c to
ae70184
Compare
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/runtime/podman/create_test.go (1)
182-251:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIsolate
SSL_CERT_FILEinTestCopySystemCACertificatesto prevent env-coupled flakiness.
copySystemCACertificates()callsfindSystemCACertificates(), which checksSSL_CERT_FILEbeforecertPaths. Witht.Parallel()here, host/CI env can override fixture selection and make assertions nondeterministic.Proposed fix
func TestCopySystemCACertificates(t *testing.T) { - t.Parallel() + // Not parallel: isolate process env for deterministic cert selection. + t.Setenv("SSL_CERT_FILE", "") t.Run("copies certificates when cert file found", func(t *testing.T) { - t.Parallel() - instanceDir := t.TempDir() p := &podmanRuntime{} @@ t.Run("returns false when no certificates found", func(t *testing.T) { - t.Parallel() - instanceDir := t.TempDir() p := &podmanRuntime{} @@ t.Run("skips directories in cert paths", func(t *testing.T) { - t.Parallel() - instanceDir := t.TempDir() p := &podmanRuntime{}As per coding guidelines: “All tests MUST call
t.Parallel()as the first line of the test function, except tests usingt.Setenv()on the parent test function.”🤖 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 `@pkg/runtime/podman/create_test.go` around lines 182 - 251, Remove the top-level t.Parallel() from TestCopySystemCACertificates and instead call t.Setenv("SSL_CERT_FILE", "") at the start of that parent test to isolate environment from findSystemCACertificates(); keep the inner t.Parallel() calls in each subtest so they remain parallel, and ensure references to podmanRuntime.copySystemCACertificates and findSystemCACertificates still operate against the controlled env.
🤖 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.
Duplicate comments:
In `@pkg/runtime/podman/create_test.go`:
- Around line 182-251: Remove the top-level t.Parallel() from
TestCopySystemCACertificates and instead call t.Setenv("SSL_CERT_FILE", "") at
the start of that parent test to isolate environment from
findSystemCACertificates(); keep the inner t.Parallel() calls in each subtest so
they remain parallel, and ensure references to
podmanRuntime.copySystemCACertificates and findSystemCACertificates still
operate against the controlled env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f51abda-8eeb-47b9-ae7c-e6015ffe573c
📒 Files selected for processing (11)
.gitignorepkg/runtime/podman/containerfile.gopkg/runtime/podman/containerfile_test.gopkg/runtime/podman/create.gopkg/runtime/podman/create_test.gopkg/runtime/podman/dashboard_test.gopkg/runtime/podman/network.gopkg/runtime/podman/podman_test.gopkg/runtime/podman/pods/onecli-pod.yamlpkg/runtime/podman/start_test.gopkg/runtime/podman/steplogger_test.go
Contributor
|
Thanks for this contribution. The development of this CLI is for the moment in pause, as we are focusing on another architecture. |
Author
|
Fair enough. Thanks for your comment @feloy 👍 As I mentioned in the issue this PR originated from, I'm not insisting on implementing this feature in any particular way. Just to be sure folks like us can use the Kaiden behind such solutions. Feel free to close it. I've been heard. That's what matters to me :) |
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
2 participants
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.
Add system CA certificate detection and copying to build context for enterprise proxy support.
Pre-install nftables in workspace image to avoid runtime installation failures in deny-mode networking.
Changes:
Fixes: #555
Please note: this is just a first draft/proposal to plug this gap. Feel free to replace it with an entirely different implementation if the conventions or standards used in this PR are not aligned with the project.
Since I've not found any contribution guidelines, I made some assumptions. There is a good chance they were wrong :-)
BTW: Openkaiden rocks 🚀 I hope you guys will make it to the level folks like us will be able to use it to run our agentic workloads safely in the enterprise setup one day!