Keep Linux LXC DHCP state service-writable - #35
Merged
Conversation
📝 WalkthroughWalkthroughChangesRelease and LXC recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant LXCNetwork
participant Bridge
participant DNSMASQ
participant Nftables
Installer->>LXCNetwork: create network state directories
LXCNetwork->>Bridge: create and configure lxcbr0
LXCNetwork->>DNSMASQ: start with DHCP range and leasefile
LXCNetwork->>Nftables: ensure onehelm_lxc rules
LXCNetwork->>DNSMASQ: validate runtime DHCP options
LXCNetwork->>Nftables: validate expected rules
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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 `@scripts/1helm-lxc-net`:
- Around line 106-127: Update rules_healthy() to verify that IPv4 forwarding is
enabled via the same net.ipv4.ip_forward setting configured by install_rules(),
returning unhealthy when it is not enabled. Add coverage for this failure case
in test/site.mjs, ensuring ensure_rules() and network_healthy() do not report
success when forwarding is disabled.
🪄 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: a39c037c-b06f-4145-952a-d2f0fbe7c80c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdpackage.jsonscripts/1helm-lxc-netscripts/1helm-lxc-runtimesite/public/install-lxc-runtime.shsrc/server/channel-computers.tssrc/server/db.tstest/channel-computers.mjstest/site.mjs
Comment on lines
+106
to
+127
| rules_healthy() { | ||
| [[ -e "$RULE_MARKER" ]] || return 1 | ||
| nft list chain inet onehelm_lxc input 2>/dev/null \ | ||
| | grep -Eq 'iifname "lxcbr0" udp dport \{ 53, 67 \} accept' || return 1 | ||
| nft list chain inet onehelm_lxc input 2>/dev/null \ | ||
| | grep -Eq 'iifname "lxcbr0" tcp dport \{ 53, 67 \} accept' || return 1 | ||
| nft list chain inet onehelm_lxc forward 2>/dev/null \ | ||
| | grep -Fq 'iifname "lxcbr0" accept' || return 1 | ||
| nft list chain inet onehelm_lxc forward 2>/dev/null \ | ||
| | grep -Eq 'oifname "lxcbr0" ct state (established,related|related,established) accept' || return 1 | ||
| nft list chain ip onehelm_lxc postrouting 2>/dev/null \ | ||
| | grep -Eq 'ip saddr 10\.0\.3\.0/24 ip daddr != 10\.0\.3\.0/24 masquerade' || return 1 | ||
| } | ||
|
|
||
| network_healthy() { | ||
| lease_state_writable || return 1 | ||
| bridge_dns_healthy || return 1 | ||
| [[ -e "$RULE_MARKER" ]] || return 1 | ||
| nft list table inet onehelm_lxc >/dev/null 2>&1 || return 1 | ||
| nft list table ip onehelm_lxc >/dev/null 2>&1 || return 1 | ||
| rules_healthy || return 1 | ||
| } | ||
|
|
||
| ensure_rules() { | ||
| if [[ -e "$RULE_MARKER" ]] \ | ||
| && nft list table inet onehelm_lxc >/dev/null 2>&1 \ | ||
| && nft list table ip onehelm_lxc >/dev/null 2>&1; then | ||
| if rules_healthy; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include IPv4 forwarding in the health contract.
install_rules() enables net.ipv4.ip_forward, but rules_healthy() never verifies it. If the sysctl is reset while the nftables marker and rules remain, ensure_rules() skips rebuilding and network_healthy() reports success even though guest forwarding and outbound NAT are broken. Add the sysctl check here and cover it in test/site.mjs.
Suggested fix
rules_healthy() {
+ [[ "$(sysctl -n net.ipv4.ip_forward 2>/dev/null)" == "1" ]] || return 1
[[ -e "$RULE_MARKER" ]] || return 1📝 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.
Suggested change
| rules_healthy() { | |
| [[ -e "$RULE_MARKER" ]] || return 1 | |
| nft list chain inet onehelm_lxc input 2>/dev/null \ | |
| | grep -Eq 'iifname "lxcbr0" udp dport \{ 53, 67 \} accept' || return 1 | |
| nft list chain inet onehelm_lxc input 2>/dev/null \ | |
| | grep -Eq 'iifname "lxcbr0" tcp dport \{ 53, 67 \} accept' || return 1 | |
| nft list chain inet onehelm_lxc forward 2>/dev/null \ | |
| | grep -Fq 'iifname "lxcbr0" accept' || return 1 | |
| nft list chain inet onehelm_lxc forward 2>/dev/null \ | |
| | grep -Eq 'oifname "lxcbr0" ct state (established,related|related,established) accept' || return 1 | |
| nft list chain ip onehelm_lxc postrouting 2>/dev/null \ | |
| | grep -Eq 'ip saddr 10\.0\.3\.0/24 ip daddr != 10\.0\.3\.0/24 masquerade' || return 1 | |
| } | |
| network_healthy() { | |
| lease_state_writable || return 1 | |
| bridge_dns_healthy || return 1 | |
| [[ -e "$RULE_MARKER" ]] || return 1 | |
| nft list table inet onehelm_lxc >/dev/null 2>&1 || return 1 | |
| nft list table ip onehelm_lxc >/dev/null 2>&1 || return 1 | |
| rules_healthy || return 1 | |
| } | |
| ensure_rules() { | |
| if [[ -e "$RULE_MARKER" ]] \ | |
| && nft list table inet onehelm_lxc >/dev/null 2>&1 \ | |
| && nft list table ip onehelm_lxc >/dev/null 2>&1; then | |
| if rules_healthy; then | |
| rules_healthy() { | |
| [[ "$(sysctl -n net.ipv4.ip_forward 2>/dev/null)" == "1" ]] || return 1 | |
| [[ -e "$RULE_MARKER" ]] || return 1 | |
| nft list chain inet onehelm_lxc input 2>/dev/null \ | |
| | grep -Eq 'iifname "lxcbr0" udp dport \{ 53, 67 \} accept' || return 1 | |
| nft list chain inet onehelm_lxc input 2>/dev/null \ | |
| | grep -Eq 'iifname "lxcbr0" tcp dport \{ 53, 67 \} accept' || return 1 | |
| nft list chain inet onehelm_lxc forward 2>/dev/null \ | |
| | grep -Fq 'iifname "lxcbr0" accept' || return 1 | |
| nft list chain inet onehelm_lxc forward 2>/dev/null \ | |
| | grep -Eq 'oifname "lxcbr0" ct state (established,related|related,established) accept' || return 1 | |
| nft list chain ip onehelm_lxc postrouting 2>/dev/null \ | |
| | grep -Eq 'ip saddr 10\.0\.3\.0/24 ip daddr != 10\.0\.3\.0/24 masquerade' || return 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 `@scripts/1helm-lxc-net` around lines 106 - 127, Update rules_healthy() to
verify that IPv4 forwarding is enabled via the same net.ipv4.ip_forward setting
configured by install_rules(), returning unhealthy when it is not enabled. Add
coverage for this failure case in test/site.mjs, ensuring ensure_rules() and
network_healthy() do not report success when forwarding is disabled.
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 the final real-host Linux terminal blockers discovered during 0.0.25 acceptance.
/var/lib/1helm-lxcwritable treeset -eVerification: typecheck, build, npm test (125 native + 112 runnable contracts), onboarding 20/20, focused site/channel-computer tests, and a hardened service-namespace DHCP/guest provisioning probe.
Summary by CodeRabbit
Bug Fixes
Release
0.0.26.Tests