Fix: Allow DHCP traffic on unmanaged VLAN bridges #1307
Conversation
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
Reviewer's GuideThis PR introduces a netlink-based helper to allow DHCP broadcast packets on VLAN-tagged subinterfaces and integrates its invocation into unmanaged bridge setup when a VLAN ID is specified. Sequence diagram for DHCP allow rule on VLAN during unmanaged bridge setupsequenceDiagram
participant BridgeSetup
participant NetlinkHelper
participant Netlink
BridgeSetup->>NetlinkHelper: Call allow_dhcp_on_vlan(bridge_name, vlan_id)
NetlinkHelper->>Netlink: Create netlink connection
NetlinkHelper->>Netlink: Lookup VLAN interface by name
alt VLAN interface found
NetlinkHelper->>Netlink: (Would) configure VLAN filtering to allow DHCP
else VLAN interface not found or error
NetlinkHelper->>BridgeSetup: Log warning
end
Class diagram for new allow_dhcp_on_vlan helper and Bridge integrationclassDiagram
class Bridge {
+create_interfaces(...)
}
class NetlinkHelper {
+allow_dhcp_on_vlan(bridge_name: str, vlan_id: u16)
}
Bridge --> NetlinkHelper : uses
NetlinkHelper : +allow_dhcp_on_vlan(bridge_name, vlan_id)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Signed-off-by: blackdragoon26 <[email protected]>
blackdragoon26
force-pushed
the
fix-vlan-dhcp
branch
from
dcfc08b to
2f6501b
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @blackdragoon26 - I've reviewed your changes - here's some feedback:
- allow_dhcp_on_vlan currently spins up its own Tokio runtime and netlink connection per call—consider making it async and reusing the existing runtime/handle to avoid nested runtimes.
- The allow_dhcp_on_vlan function has a TODO for actually configuring the VLAN filter—either implement that logic now or remove the placeholder to prevent silent misconfigurations.
- In the unmanaged bridge setup you still hit the
Err(err)return even after calling allow_dhcp_on_vlan, so the control flow never proceeds; adjust the early return so VLAN handling completes as intended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- allow_dhcp_on_vlan currently spins up its own Tokio runtime and netlink connection per call—consider making it async and reusing the existing runtime/handle to avoid nested runtimes.
- The allow_dhcp_on_vlan function has a TODO for actually configuring the VLAN filter—either implement that logic now or remove the placeholder to prevent silent misconfigurations.
- In the unmanaged bridge setup you still hit the `Err(err)` return even after calling allow_dhcp_on_vlan, so the control flow never proceeds; adjust the early return so VLAN handling completes as intended.
## Individual Comments
### Comment 1
<location> `src/network/netlink.rs:637` </location>
<code_context>
+ tokio::spawn(connection);
+
+ // Lookup VLAN interface by name
+ let mut rt = tokio::runtime::Runtime::new().unwrap();
+ let mut links = rt.block_on(
+ handle
+ .link()
</code_context>
<issue_to_address>
Using a new Tokio runtime inside an async context can lead to resource issues.
Instead of creating a new runtime, pass an existing handle or refactor to prevent nested runtimes and potential thread pool contention.
</issue_to_address>
### Comment 2
<location> `src/network/netlink.rs:652` </location>
<code_context>
+ "Found VLAN interface {}, would configure bridge VLAN filtering to allow DHCP (UDP 67/68)",
+ vlan_iface
+ );
+ // TODO: implement actual VLAN filtering adjustment here
+ }
+ Ok(None) => {
</code_context>
<issue_to_address>
Missing implementation for VLAN filtering adjustment.
If this is meant to be a stub, clarify its status or return an error to indicate the feature is not yet implemented.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
info!(
"Found VLAN interface {}, would configure bridge VLAN filtering to allow DHCP (UDP 67/68)",
vlan_iface
);
// TODO: implement actual VLAN filtering adjustment here
=======
info!(
"Found VLAN interface {}, would configure bridge VLAN filtering to allow DHCP (UDP 67/68)",
vlan_iface
);
warn!(
"VLAN filtering adjustment for interface {} is not yet implemented.",
vlan_iface
);
return Err(anyhow::anyhow!(
"VLAN filtering adjustment for interface {} is not yet implemented.",
vlan_iface
));
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+637
to
+638
| let mut rt = tokio::runtime::Runtime::new().unwrap(); | ||
| let mut links = rt.block_on( |
There was a problem hiding this comment.
issue (bug_risk): Using a new Tokio runtime inside an async context can lead to resource issues.
Instead of creating a new runtime, pass an existing handle or refactor to prevent nested runtimes and potential thread pool contention.
Comment on lines
+648
to
+652
| info!( | ||
| "Found VLAN interface {}, would configure bridge VLAN filtering to allow DHCP (UDP 67/68)", | ||
| vlan_iface | ||
| ); | ||
| // TODO: implement actual VLAN filtering adjustment here |
There was a problem hiding this comment.
suggestion: Missing implementation for VLAN filtering adjustment.
If this is meant to be a stub, clarify its status or return an error to indicate the feature is not yet implemented.
Suggested change
| info!( | |
| "Found VLAN interface {}, would configure bridge VLAN filtering to allow DHCP (UDP 67/68)", | |
| vlan_iface | |
| ); | |
| // TODO: implement actual VLAN filtering adjustment here | |
| info!( | |
| "Found VLAN interface {}, would configure bridge VLAN filtering to allow DHCP (UDP 67/68)", | |
| vlan_iface | |
| ); | |
| warn!( | |
| "VLAN filtering adjustment for interface {} is not yet implemented.", | |
| vlan_iface | |
| ); | |
| return Err(anyhow::anyhow!( | |
| "VLAN filtering adjustment for interface {} is not yet implemented.", | |
| vlan_iface | |
| )); |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: blackdragoon26, sourcery-ai[bot] The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In this patch, I am attempting to work on issue of #1294,
Detect unmanaged bridges with VLAN IDs during network setup.
Add netlink rule to allow DHCP broadcast packets (UDP ports 67/68) to pass on VLAN subinterfaces.
Avoids DHCP client timeouts when containers are connected to VLAN-tagged bridge networks
Summary by Sourcery
Apply DHCP allow rules on VLAN-tagged unmanaged bridges to prevent DHCP client timeouts by permitting UDP ports 67/68 on VLAN subinterfaces.
Bug Fixes:
Enhancements: