Skip to content
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

Implement propagation of IP routes into apps via DHCP #3690

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Dec 22, 2023

Most of the edge deployments will deploy applications (VMs or containers) that will have
connectivity to both WAN (Internet) and LAN (shop floor, machine floor, intranet).
There may be a single WAN interface and multiple LAN interfaces. With the IP route
configurability of Network instances implemented by this PR (and APIs extensions already
added here), we can automatically (zero-touch) provision routing for applications that have
access to both the Internet and one or more LANs.
This is now possible because EVE allows to:

  • use DHCP to propagate the connected routes to applications (routes for external networks
    that uplink ports are connected to)
  • configure a set of static IP routes for a network instance and have them propagated
    to applications

Another common case is using one application as a network gateway for other applications
running on the same device. The gateway application may provide some network function(s),
such as firewall, IDS, network monitoring, etc. Such application will connect on one side with
the external network(s) using directly attached network adapter(s) or via switch network
instance(s), and the other side will make use of an air-gap local network instance to connect
with other applications running on the device. Propagated static IP routes are necessary
to make the application traffic flow through the gateway app. In theory, multiple network
functions can be chained together in this way using several air-gap network instances
with static IP routes.

PR with examples for eden: lf-edge/eden#938
They allow to easily and quickly perform manual testing of this functionality.
Eventually, I would like to prepare automated tests, but for now examples will do.

@milan-zededa milan-zededa changed the title Implement propagation of routes into apps via DHCP Implement propagation of IP routes into apps via DHCP Dec 22, 2023
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: 150 lines in your changes are missing coverage. Please review.

Comparison is base (03b8972) 19.60% compared to head (0237f6b) 19.82%.

❗ Current head 0237f6b differs from pull request most recent head c823cd1. Consider uploading reports for the commit c823cd1 to get more accurate results

Files Patch % Lines
pkg/pillar/nireconciler/linux_config.go 63.29% 44 Missing and 14 partials ⚠️
pkg/pillar/cmd/zedagent/parseconfig.go 0.00% 37 Missing ⚠️
pkg/pillar/nireconciler/genericitems/dnsmasq.go 72.07% 29 Missing and 2 partials ⚠️
pkg/pillar/types/zedroutertypes.go 0.00% 6 Missing ⚠️
pkg/pillar/utils/generics/generics.go 0.00% 6 Missing ⚠️
pkg/pillar/dpcreconciler/genericitems/dhcpcd.go 28.57% 5 Missing ⚠️
pkg/pillar/cmd/zedrouter/pubsubhandlers.go 0.00% 3 Missing ⚠️
pkg/pillar/cmd/zedagent/reportinfo.go 0.00% 1 Missing ⚠️
pkg/pillar/cmd/zedrouter/networkinstance.go 0.00% 1 Missing ⚠️
pkg/pillar/dpcreconciler/linux.go 0.00% 0 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3690      +/-   ##
==========================================
+ Coverage   19.60%   19.82%   +0.21%     
==========================================
  Files         232      231       -1     
  Lines       50789    51047     +258     
==========================================
+ Hits         9959    10118     +159     
- Misses      40104    40190      +86     
- Partials      726      739      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +667 to +673
// From RFC 3442:
// If the DHCP server returns both a Classless Static Routes option and
// a Router option, the DHCP client MUST ignore the Router option.
//
// This means that we have to include a default "classless" route
// even though the Router option is provided.
// We keep the Router option for clients that do not support the Classless
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to test this with a range of guest VMs including some version(s) of Windows, to make sure no client is upset by seeing both options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Copy link
Contributor Author

@milan-zededa milan-zededa Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with Windows 10 - looks good, IP routes are installed as expected.

}
var withDefaultRoute bool
airGap := ni.bridge.Uplink.IfName == ""
if !airGap && (ni.bridge.Uplink.IsMgmt || r.niHasDefRoute(ni)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the need to check IsMgmt? Is this for a case when the management interface does not have a default route but somehow received all necessary prefix routes to reach the controller etc?

Note that the comment on L820 doesn't talk about IsMgmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsMgmt = ! app-shared

This if-statement is a negation of that comment (we propagate default route if this if-statement is true).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but I still don't understand why you need to check IsMgmt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For management uplink we always want to propagate default route to apps. For app-shared uplink (IsMgmt=false) only if the uplink has default route.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - your terminology might be confusing me.
We have two cases: inject a default route for the app instance (independent of routing on the external network)
propagate a default and/or other routes from the external network to the app instance.

Is it that the same code does the injection (for the management uplinks) and does the propagation?
If so it makes perfect sense, but is hard to understand from reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code here (getIntendedDnsmasqCfg function) only decides if app should receive default route for this network instance or not. If yes, then we propagate 0.0.0.0/0 via bridgeIP - i.e. in this function we do not care what the actual gateway is and whether it is received from an external network or injected by user.
In function getIntendedNIL3Cfg we then prepare config for the NI routing table (for the host network stack). There we decide if we should copy default route from uplink routing table or put there a user-injected default route.

if subnet != nil {
staticRoutes = append(staticRoutes, types.IPRoute{
DstNetwork: subnet,
Gateway: net.IP{0, 0, 0, 0},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the gateway is 0.0.0.0 here?

Copy link
Contributor Author

@milan-zededa milan-zededa Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a link-local route for the whole NI subnet, such as: 10.10.10.0/24 dev eth0 proto dhcp scope link

if isAppGW {
outputIf = linux.RouteOutIf{BridgeIfName: ni.brIfName}
}
intendedL3Cfg.PutItem(linux.Route{
Copy link
Contributor

@naiming-zededa naiming-zededa Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we check here to prevent the app's static route is 0/0 to be installed into host side for IsMgmt?

Copy link
Contributor Author

@milan-zededa milan-zededa Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is no such check.

How should we handle the case when NI uplink has default route and at the same time user configures static default route (with possibly different GW IP)? Which one should take precedence? I would think that the static route, since it would give the user the option to override the default route received from an external DHCP server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, i would think the user explicit one should take precedence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, PR is updated - user-configured default route will override the default route of the uplink.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kick off tests

Signed-off-by: Milan Lenco <milan@zededa.com>
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run tests

Static and connected routes of network instances can be propagated to
applications using DHCP option 121.

Signed-off-by: Milan Lenco <milan@zededa.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
@eriknordmark eriknordmark merged commit cf3dbde into lf-edge:master Jan 9, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants