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

Container-in-VM: put DNS servers from all ports to resolv.conf #3826

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Mar 19, 2024

When container application has multiple network interfaces, it should be configured to failover between DNS servers collected from all interfaces.

The current behaviour is that we only put DNS servers from the first (eth0) interface into resolv.conf. However, if the uplink port corresponding to the first app interface looses connectivity, name resolution will stop working and app will not try DNS servers from other interfaces (that could be potentially using different uplinks).
However, there is nothing in EVE API that would declare the first app interface as being special and exclusively used for DNS.

Comparing this to Linux or Windows (i.e. VM apps), the default behaviour of the resolver is to iterate over all ports and try every DNS server until one responds. We should therefore replicate the same behaviour in our shim VM created for container applications.

Additionally, in order to make sure that query destined to a user-configured DNS server is sent out through the appropriate application interface, for every NI we use DHCP to propagate host routes (/32) for all DNS (and also NTP) servers (with the NI bridge IP as GW) into applications.

@milan-zededa
Copy link
Contributor Author

@naiming-zededa @gkodali-zededa @eriknordmark I'm not completely sure about this and would like to hear your opinion. Maybe there are some reasons why we pick DNS servers only from eth0 that I'm not aware of.

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.51%. Comparing base (e4f2710) to head (67242c0).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3826   +/-   ##
=======================================
  Coverage   17.51%   17.51%           
=======================================
  Files           3        3           
  Lines         805      805           
=======================================
  Hits          141      141           
  Misses        629      629           
  Partials       35       35           

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

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.

I don't think there was a particular reason to only use eth0.

But as we use DNS from multiple interfaces we might even more depend on that the app instance routing works correctly so that the source IP address (of DNS requests) match the interface over which the request is sent. Does the resolver code always leave the source IP address unset and have the kernel pick based on the route?
If the DNS server is multiple hops away then we can't rely on matching a default route since we might have multiple of those for different interfaces. I don't know what of this we should document (to ask the user to configure the static routes to reach the DNS servers) or add code to set it up.

Some shellcheck issues to fix.

@milan-zededa
Copy link
Contributor Author

milan-zededa commented Mar 21, 2024

But as we use DNS from multiple interfaces we might even more depend on that the app instance routing works correctly so that the source IP address (of DNS requests) match the interface over which the request is sent. Does the resolver code always leave the source IP address unset and have the kernel pick based on the route?

Yes, src IP is unset and routing table decides the next hop and the output interface.

But as we use DNS from multiple interfaces we might even more depend on that the app instance routing works correctly so that the source IP address (of DNS requests) match the interface over which the request is sent.

In most cases DNS IP will be the NI bridge IP so routing will work correctly. In case user configures some DNS server(s) from external networks instead, we could generate and propagate static routes for them automatically into the connected apps. Actually, we already have the same situation with NTP servers. My only worry is that while users are able to add their own static routes, they are not able to delete the automatically created ones. So they will not be able to remove/edit these routes towards DNS/NTP servers that we would add automatically.

Some shellcheck issues to fix.

Fixed

@eriknordmark
Copy link
Contributor

In most cases DNS IP will be the NI bridge IP so routing will work correctly. In case user configures some DNS server(s) from external networks instead, we could generate and propagate static routes for them automatically into the connected apps. Actually, we already have the same situation with NTP servers. My only worry is that while users are able to add their own static routes, they are not able to delete the automatically created ones. So they will not be able to remove/edit these routes towards DNS/NTP servers that we would add automatically.

They can choose to explicitly list the static routes and not import the ones from DHCP into the NI, right?

We should probably provide some examples for how to configure this with off-subnet ntp/dns/other servers.

@milan-zededa
Copy link
Contributor Author

They can choose to explicitly list the static routes and not import the ones from DHCP into the NI, right?

EVE currently propagates to applications static IP routes (if any configured) + connected routed (if enabled). Routes from the DHCP of the uplink port are not propagated to applications (only added to the NI routing table on the host side).
If we would automatically generate routes for DNS/NTP and also propagate them to apps, then these routes would not be editable for the user (unless we add some API knobs).

@naiming-zededa
Copy link
Contributor

My only worry is that while users are able to add their own static routes, they are not able to delete the automatically created ones. So they will not be able to remove/edit these routes towards DNS/NTP servers that we would add automatically.

User can add a /32 route to their destination of DNS/NTP which should work unless the dhcp propagated routes is also /32, which is not likely.

@milan-zededa
Copy link
Contributor Author

My only worry is that while users are able to add their own static routes, they are not able to delete the automatically created ones. So they will not be able to remove/edit these routes towards DNS/NTP servers that we would add automatically.

User can add a /32 route to their destination of DNS/NTP which should work unless the dhcp propagated routes is also /32, which is not likely.

But in this case we are talking about EVE automatically adding routes for DNS and NTP servers configured by the user for the network instance. Since these are IP addresses without subnet prefix, we would exactly use the /32 prefix for the generated routes. What we can perhaps do, is that if the user also creates /32 static routes for those IPs, then EVE will not be generating those routes and will install the user ones instead.

When container application has multiple network interfaces, it should be
configured to failover between DNS servers collected from all
interfaces.

The current behaviour is that we only put DNS servers from the first (eth0)
interface into resolv.conf. However, if the uplink port corresponding to
the first app interface looses connectivity, name resolution will stop
working and app will not try DNS servers from other interfaces (that
could be potentially using different uplinks).
However, there is nothing in EVE API that would declare the first app
interface as being special and exclusively used for DNS.

Comparing this to Linux or Windows (i.e. VM apps), the default behaviour
of the resolver is to iterate over all ports and try every DNS server
until one responds. We should therefore replicate the same behaviour
in our shim VM created for container applications.

Additionally, in order to make sure that query destined to a user-configured
DNS server is sent out through the appropriate application interface,
for every NI we use DHCP to propagate host routes (/32) for all DNS
(and also NTP) servers (with the NI bridge IP as GW) into applications.

Signed-off-by: Milan Lenco <milan@zededa.com>
@milan-zededa milan-zededa marked this pull request as ready for review March 26, 2024 11:15
@milan-zededa
Copy link
Contributor Author

milan-zededa commented Mar 26, 2024

@eriknordmark @naiming-zededa I have added propagation of /32 routes for user-configured DNS and NTP servers. I cannot envision a realistic scenario where this could cause some problems for the user. After all, it would not make much sense if query towards a DNS/NTP server was sent through a different network instance that it is configured for.

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.

Do we allow users to specify not servers using names? Or do we require that they be IP addresses

@milan-zededa
Copy link
Contributor Author

Do we allow users to specify not servers using names? Or do we require that they be IP addresses

We require IP addresses for NTP servers: https://github.com/lf-edge/eve/blob/master/pkg/pillar/cmd/zedagent/parseconfig.go#L2052-L2058

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.

LGTM

@eriknordmark eriknordmark merged commit 5477648 into lf-edge:master Apr 9, 2024
32 of 36 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