Fix: Allow DHCP traffic on unmanaged VLAN bridges#1307
Conversation
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 <sankalp.jha9643@gmail.com>
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.
| 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.
| 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.
| 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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hihi maintainers, there has been revamp in DHCP lease handling for unmanaged bridge VLANs This is a major rework of the initial approach to resolve issue #1294 . Key changes:
|
blackdragoon26
force-pushed
the
fix-vlan-dhcp
branch
3 times, most recently
from
89e0021 to
e25dbed
Compare
Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com>
blackdragoon26
force-pushed
the
fix-vlan-dhcp
branch
from
e25dbed to
e31d8ac
Compare
|
@sourcery-ai review |
Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com> # Conflicts: # src/dhcp_proxy/dhcp_service.rs
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
sorry I do not have time to properly review this. Overall this approach here feels wrong, creating a new tokio runtime per container does not scale at all and I am not confident in the way the netns switching is done.
I would much prefer it if we add this via support for the mozim lib to only bind the raw socket in the netns there when we actually open the socket and not have to all this here
Yeah, that makes sense, thanks for that. I’ll rework this by moving the namespace-specific raw socket creation into Then I will update this Netavark PR to remove the custom DHCP worker/runtime changes and just pass the target netns/container iface information into mozim. |
|
Per the feedback about keeping the namespace-sensitive socket work in mozim, I split that out first here: That PR adds scoped netns handling only around DHCP socket open/bind, and restores the original netns before returning to async code. I have the Netavark side reworked locally to use the normal task model again, and I’ll update this PR once the mozim direction is reviewed. |
|
I have the same concern as @Luap99 , I need more detail on what is not working instead of this is how I fix the my problem. |
blackdragoon26
force-pushed
the
fix-vlan-dhcp
branch
from
92327f4 to
f4f063d
Compare
|
Thanks for pushing back on the previous direction. I reworked this to avoid the netns/runtime approach entirely. @cathay4t The issue I reproduced is that for an unmanaged bridge with This update keeps the DHCP I also added an integration test that sets up:
Local verification on x86 Linux:
|
|
@Luap99 , Can you please have a look in this PR now. |
Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com>
blackdragoon26
force-pushed
the
fix-vlan-dhcp
branch
from
f4f063d to
cafbad1
Compare
3 participants
This PR is being reworked in response to review feedback and now depends on:
nispor/mozim#79
The previous implementation here introduced a dedicated per-container DHCP
worker/runtime after entering the target network namespace. Maintainer feedback
was that this was the wrong layer and would not scale well.
The new approach is to move the namespace-sensitive socket binding into mozim,
so Netavark can keep its normal async/task model while still sending DHCP
traffic from the correct target namespace and interface context for unmanaged
bridge VLAN setups.
Once the mozim change is reviewed, this PR will be updated to:
Related dependency PR:
nispor/mozim#79