Draft Add ability to override network interface#12
Draft Add ability to override network interface#12evgkrsk wants to merge 5 commits intocozystack:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded a CLI flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @evgkrsk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the network configuration capabilities by adding support for VLAN-tagged interfaces. It allows the system to correctly identify and configure network settings for interfaces that are part of a Virtual Local Area Network, ensuring proper network connectivity in VLAN environments by generating the necessary kernel boot parameters. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds support for VLAN configuration by parsing interface names with a .vid suffix. The implementation is a good start, but there are a few important issues to address. The new getVID function compiles a regular expression on every call, which is inefficient, and the regex pattern itself is too restrictive for common Linux interface names. Additionally, the function's logic can be simplified for better readability. Most critically, an error returned from getVID for invalid VLAN IDs is ignored, which will lead to silent network configuration failures at boot time. I've provided specific comments and suggestions to resolve these issues.
![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 (2)
main.go (2)
290-290: Move regex compilation to package level for efficiency.Compiling the regex pattern on every function call is inefficient. Since the pattern is constant, compile it once at package level.
🔎 Proposed refactor
Add this near the top of the file (e.g., after the flag variables section):
var vlanRegex = regexp.MustCompile(`^([a-z0-9]+)\.(\d+)$`)Then update the function:
func getVID(name string) (int, string, error) { - re := regexp.MustCompile(`^([a-z0-9]+)\.(\d+)$`) - matches := re.FindStringSubmatch(name) + matches := vlanRegex.FindStringSubmatch(name) phy := ""
292-303: Consider simplifying the control flow.The else block could be eliminated by initializing
phybefore the conditional.🔎 Proposed refactor
func getVID(name string) (int, string, error) { re := regexp.MustCompile(`^([a-z0-9]+)\.(\d+)$`) matches := re.FindStringSubmatch(name) - phy := "" + phy := name if len(matches) == 3 { vid, err := strconv.Atoi(matches[2]) phy = matches[1] if err != nil || vid < 1 || vid > 4094 { return 0, phy, fmt.Errorf("invalid VID: %s", matches[2]) } return vid, phy, nil - } else { - phy = name } return 0, phy, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.go
🔇 Additional comments (2)
main.go (2)
18-19: LGTM: Standard library imports for VLAN parsing.The addition of
regexpandstrconvpackages appropriately supports the new VLAN ID parsing functionality.
325-327: The kernel VLAN parameter format is correct as written.The code generates
vlan=<vlan-ifname>:<physical-dev>format (e.g.,vlan=eth0.100:eth0), which matches the standard Linux kernel boot parameter syntax. ThegetVID()function correctly extracts the physical device name from the VLAN interface name (e.g.,eth0frometh0.100), and the conditional appends the parameter only when a valid VLAN ID is detected. No changes needed.
evgkrsk
force-pushed
the
add-vlan-support
branch
from
8c6fa28 to
9554ab7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for VLAN interfaces. The changes correctly parse VLAN interface names to extract the VLAN ID and physical device, and then add the appropriate vlan= kernel boot parameter. My review focuses on improving the robustness of the VLAN interface parsing and enhancing error messages for a better user experience.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
main.go (1)
291-291: Regex pattern is too restrictive for Linux interface names.The pattern
^([a-z0-9]+)\.(\d+)$doesn't match many valid Linux interface names. This was flagged in previous reviews but remains unaddressed.Valid interface names that will fail:
- Names with uppercase: Some USB/virtual devices
- Names with hyphens:
br-lan,eth-wan- Names with underscores: Custom interface names
- Modern predictable names:
enp0s3(actually this works, butenP0s3wouldn't)This means VLAN detection will silently fail for these interfaces, and the
vlan=kernel argument won't be added even when needed.🔎 Proposed fix
-var vlanInterfaceRegex = regexp.MustCompile(`^([a-z0-9]+)\.(\d+)$`) +var vlanInterfaceRegex = regexp.MustCompile(`^([a-zA-Z0-9._-]+)\.(\d+)$`)This allows uppercase letters, dots, hyphens, and underscores in the physical interface name, covering the vast majority of valid Linux interface names.
🧹 Nitpick comments (1)
main.go (1)
293-308: Consider early-return pattern for clearer flow.The logic is correct, but readability could be improved by handling the non-match case with an early return, making the happy path less nested.
🔎 Proposed refactor
func getVID(name string) (int, string, error) { matches := vlanInterfaceRegex.FindStringSubmatch(name) - phy := name - if len(matches) == 3 { - vid, err := strconv.Atoi(matches[2]) - phy = matches[1] - if err != nil { - return 0, phy, fmt.Errorf("invalid VID in interface name: %q is not a valid integer", matches[2]) - } - if vid < 1 || vid > 4094 { - return 0, phy, fmt.Errorf("invalid VID in interface name: %d is out of the valid range (1-4094)", vid) - } - return vid, phy, nil + if len(matches) < 3 { + return 0, name, nil } - return 0, phy, nil + + phy := matches[1] + vid, err := strconv.Atoi(matches[2]) + if err != nil { + return 0, phy, fmt.Errorf("invalid VID in interface name: %q is not a valid integer", matches[2]) + } + if vid < 1 || vid > 4094 { + return 0, phy, fmt.Errorf("invalid VID in interface name: %d is out of the valid range (1-4094)", vid) + } + return vid, phy, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.go
🔇 Additional comments (6)
main.go (6)
18-19: LGTM!The new imports are appropriate for VLAN parsing functionality.
38-38: LGTM!The flag variable declaration is appropriate.
48-48: LGTM!The flag binding is clear and provides a useful override mechanism.
316-318: LGTM!The override logic is straightforward and provides a useful escape hatch for users.
324-327: Good error handling improvement.The error from
getVIDis now properly captured and handled, addressing a critical issue from previous reviews. The program will fail fast with a descriptive error message if an invalid VLAN interface is provided.
335-337: LGTM!The conditional appending of the
vlan=kernel argument is correct. It only adds the argument when a valid VID is detected (vid > 0), following the kernel's VLAN configuration convention.
|
Thank you for quick reaction, @kvaps I have compared 0.5.1 vs 0.6.0 official linux/amd64 binaries on one node with VLAN and official siderolabs/installer:v1.12.1 image: Pro
Contra
Summary: in 0.6.0, there is still no way to non-interactive run boot-to-talos with proper args to get working VLAN network. Please, consider (re)adding --override-interface or similar flag to combine in with -yes. Perhaps it is worth to add ability to override any internal variable? |
evgkrsk
force-pushed
the
add-vlan-support
branch
from
d6a51a2 to
c00c571
Compare
evgkrsk
force-pushed
the
add-vlan-support
branch
from
c00c571 to
93aea02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @main.go:
- Around line 306-308: The override of the interface (dev = devFlag) currently
happens after calling ifaceAddr(), causing IP/mask to be fetched from the wrong
interface; move the dev override to occur before the call to ifaceAddr() so that
ifaceAddr(dev) queries the overridden interface (use symbols dev, devFlag, and
ifaceAddr) and ensure non-interactive flows (-yes) return consistent dev plus
its IP/mask.
🧹 Nitpick comments (1)
main.go (1)
36-36: Consider a more descriptive variable name (optional).While
devFlagis consistent with the existing naming pattern,interfaceFlagoroverrideInterfaceFlagwould be more self-documenting given that the flag is named--override-interface.📝 Suggested naming improvement
- devFlag string + interfaceFlag stringThen update the binding and usage accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.go
🔇 Additional comments (1)
main.go (1)
46-46: Flag binding looks good.The flag name and help text are clear, and the implementation follows the established pattern.
evgkrsk
changed the title
|
Hey @kvaps , can you review/merge new (minimalistic) version of the PR? This is last reason not to use non-interactive (-yes) version of boot-to-talos for me. Sadly, I have not enough experience or ideas how to debug why current (0.6.0) version fails to reproduce working VLAN network setup in this mode. |
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.