-
Notifications
You must be signed in to change notification settings - Fork 2
feat(router): wip ipv6 only network #1589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Claude finished @johnae's task —— View job Pull Request Review: IPv6-Only Network ConfigurationReview Tasks
SummaryThis PR introduces IPv6-only network configuration with NAT64 using Jool and DNS64 using CoreDNS. The implementation looks solid but has some areas that need attention before merging. 🟢 Strengths
🟡 Code Quality & Best Practices
🔴 Potential Bugs
🛡️ Security Concerns
⚡ Performance Considerations
🧪 Test Coverage Recommendations
📝 Additional Recommendations
Action Items Before Merge
This is marked as WIP, so I assume you're aware of some of these issues. The IPv6-only approach with NAT64/DNS64 is a solid architectural choice, but needs some polish before production use. |
9b44b31 to
17a6fe8
Compare
|
Claude finished @johnae's task —— View job Pull Request Review: IPv6-Only Network Configuration (Updated)Review Tasks
SummaryThis updated PR shows progress on the IPv6-only network configuration. Some critical issues from the previous review have been addressed, but several important problems remain that need attention before merging. ✅ Issues Addressed from Previous Review
🔴 Critical Issues Still Present1. Undefined
|
17a6fe8 to
8124b4d
Compare
|
Claude finished @johnae's task —— View job Pull Request Review: IPv6-Only Network Configuration (Updated)Review Tasks
SummaryThis PR introduces IPv6-only network configuration with NAT64 using Jool and DNS64 using CoreDNS. Good progress has been made since the last review, but a critical blocker remains that will cause runtime errors. 🔴 Critical BlockerUndefined
|
8124b4d to
d75407f
Compare
|
Claude finished @johnae's task —— View job Pull Request Review: IPv6-Only Network ConfigurationReview Tasks
SummaryThis PR implements IPv6-only networking with NAT64/DNS64. Great progress since the last review - the critical ✅ Improvements Since Last Review
🔴 Critical Issues1. Unused Option Without Default (modules/router.nix:45-48)upstreamDnsServers = mkOption {
type = listOf str;
description = "List of upstream dns server addresses.";
};This option has no default value and is never used. Either remove it or provide a default. Fix: Delete lines 45-48 since CoreDNS uses hardcoded Cloudflare DNS. 🟡 Code Quality Issues1. Commented Out Code ClutterMultiple files have large blocks of commented code:
Recommendation: For WIP, this is acceptable, but clean up before final merge. 2. Mixed IPv4/IPv6 State (modules/router.nix)
Clarification Needed: Is this truly IPv6-only or dual-stack transitional? 3. Hardcoded Network Assumptions
🛡️ Security Considerations1. DNS Binding (modules/router.nix:169)bind ::Binds to all IPv6 interfaces without explicit access control. Risk: Could become an open resolver if firewall rules aren't properly configured. Recommendation: Ensure 2. NAT64 Pool Configuration (sagittarius.nix:33)Using standard 3. Port Range (sagittarius.nix:40, 45, 50)Port range ⚡ Performance Considerations
🧪 Test Coverage Recommendations
📋 Action ItemsBefore Removing WIP Status
Nice to Have
Overall AssessmentSolid progress on the IPv6-only network configuration. The automatic IPv4 detection for Jool is particularly well done. The main blocker is the unused option without a default value. Once that's fixed and the IPv4/IPv6 strategy is clarified, this should be ready for testing. The NAT64/DNS64 approach using Jool and CoreDNS is architecturally sound and follows best practices for IPv6 migration. |
No description provided.