Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

CA-99179: Add other_config param for disabling default gateway advertising on host-internal-management network #1025

Merged
merged 2 commits into from Apr 15, 2013

Conversation

Projects
None yet
5 participants
Member

rdobson commented Feb 14, 2013

Patches XAPI to allow clients to stop XAPI from generating a udhcpd
config file that includes advertising dom0's IP as the default gateway
for the network.

When using the host internal management network with Windows VMs
the network adapter will be allocated a high metric based on its speed,
and so traffic to unkown destinations may be routed via Dom0.

Owner

xen-git commented Feb 14, 2013

Can one of the admins verify this patch?

Collaborator

djs55 commented Feb 15, 2013

Hi Rob,

Could you describe your scenario in a bit more detail? I'm a bit worried that setting this key to make your scenario work, will possibly break someone else's concurrent use where they expect the default route to be present-- so we may end up with features that conflict.

Perhaps we could always remove the route? If we can't I'd like to understand why :-)

Also, I'm worried that other_config keys tend to get forgotten about and get accidentally removed over refactoring. Assuming we're going with this one, could you write a small description in the style of this one:

https://github.com/djs55/xcp-idl/blob/master/xen-api-plugin/service.md

(I put that one in the wrong directory -- I was going to put it into an "apis" directory). My aim is to create and maintain a set of 'mini-API' specs each focused on a separate area.

Thanks!

Member

rdobson commented Feb 15, 2013

Hi Dave,

I agree that this may prevent users from using both the KVP work and other virtual appliances which make use of the host internal management network at the same time. Which is less than ideal.

The context to this problem is that we have a KVP HTTP server running inside a Windows VM, and we plug a VIF into that VM which connects the VM to the Host Internal Management Network.

This extra VIF, is as much as possible meant to be 'unnoticed' in that it should not impact their networking, but rather provide an interface that only our KVP server uses, and listens on.

Due to the fact that we are using DHCP off of the udhcpd server running in Dom0, we run into problems when a default gateway is returned. This is due to the fact that an entry is added to the Windows routing table which is subsequently used to route packets down this added interface - something which shouldn't happen, and breaks the VMs traffic flows to unknown destinations.

It would be possible to pass the responsibility of removing this route onto the in guest service, however I have several concerns about doing this:

  • There is no way to intercept the route being added - so there is likely to be a window of opportunity for traffic to be mis-routed.
  • The guest service is designed to be able to have the VIF plugged/destroyed at will by Dom0 - so the service would potentially have to repeatedly remove the route
  • Relying on the service running inside the guest does not seem ideal - if there were a bug in the service (e.g. crashed/stopped/failing to see route being added), then we would impact the guests network traffic.

For those reasons I thought it would be safer to fix this issue by altering the configuration of the udhcpd server.

I chose to add an extra other_config parameter due to the fact other configuration options used this approach.

I suppose an alternative approach, would be to create and handle a separate network (mimicking the Host Internal Management Network in having a DHCP service, and an IP in Dom0 etc) which would obviously be more work.

@djs55 - What do you think?

If you were happy with this approach, would you like the API spec for all of the udhcpd other_config params? And where would you like me to commit it? (would you like me to create an apis directory?)

Thanks.

Collaborator

djs55 commented Feb 15, 2013

Thanks for the write up! I'd like to get @jonludlam's advice-- he's back on
Monday (as am I, technically :-)

On Friday, February 15, 2013, rdobson wrote:

Hi Dave,

I agree that this may prevent users from using both the KVP work and other
virtual appliances which make use of the host internal management network
at the same time. Which is less than ideal.

The context to this problem is that we have a KVP HTTP server running
inside a Windows VM, and we plug a VIF into that VM which connects the VM
to the Host Internal Management Network.

This extra VIF, is as much as possible meant to be 'unnoticed' in that it
should not impact their networking, but rather provide an interface that
only our KVP server uses, and listens on.

Due to the fact that we are using DHCP off of the udhcpd server running
in Dom0, we run into problems when a default gateway is returned. This is
due to the fact that an entry is added to the Windows routing table which
is subsequently used to route packets down this added interface - something
which shouldn't happen, and breaks the VMs traffic flows to unknown
destinations.

It would be possible to pass the responsibility of removing this route
onto the in guest service, however I have several concerns about doing this:

  • There is no way to intercept the route being added - so there is
    likely to be a window of opportunity for traffic to be mis-routed.
  • The guest service is designed to be able to have the VIF
    plugged/destroyed at will by Dom0 - so the service would potentially have
    to repeatedly remove the route
  • Relying on the service running inside the guest does not seem ideal
  • if there were a bug in the service (e.g. crashed/stopped/failing to see
    route being added), then we would impact the guests network traffic.

For those reasons I thought it would be safer to fix this issue by
altering the configuration of the udhcpd server.

I chose to add an extra other_config parameter due to the fact other
configuration options used this approach.

I suppose an alternative approach, would be to create and handle a
separate network (mimicking the Host Internal Management Network in having
a DHCP service, and an IP in Dom0 etc) which would obviously be more work.

@djs55 https://github.com/djs55 - What do you think?

If you were happy with this approach, would you like the API spec for all
of the udhcpd other_config params? And where would you like me to commit
it? (would you like me to create an apis directory?)

Thanks.


Reply to this email directly or view it on GitHubhttps://github.com/xen-org/xen-api/pull/1025#issuecomment-13604457.

Dave Scott

Owner

robhoes commented Feb 18, 2013

@rdobson As an alternative, would it be possible for the DHCP client in the Windows guest to not request a default gateway from the DHCP server. I am not sure how practical this is in Windows, but the DHCP client in Linux lets you specify exactly what settings you want back from the server.

I believe the default gateway on the HIMN was made to be dom0's IP address, so that the guest knows which IP address to talk to. Does this particular appliance use a different way of identifying dom0's IP?

Member

rdobson commented Feb 18, 2013

@robhoes - You are certainly able to configure an adapter in Windows to have certain properties which I believe would allow you to effectively disable listening for a default gateway (or at least crank the metric so high it will never be preferred). However the problem with this is that we do not have control over the initial properties of the device - and I would like to avoid inadvertently causing any traffic to disappear down the wrong route. In order to do it properly, we would likely need some functionality in the PV drivers I would suspect.

As for detecting Dom0's IP - we actually don't need to do that in this case. The server running in the guest communicates its IP via XenStore, and it talked to from outside, and in the guest. So although the VM obviously learns Dom0s IP, it does not need to initiate communication with it.

@ghost ghost assigned robhoes Feb 21, 2013

Owner

jonludlam commented Feb 21, 2013

ok to test

Owner

xen-git commented Feb 25, 2013

Can one of the admins verify this patch?

Member

rdobson commented Mar 15, 2013

@robhoes - I've now created an API file for configuring the udhcp server. Could you see if your happy with it please?

Thanks.

Owner

xen-git commented Mar 27, 2013

Can one of the admins verify this patch?

Member

rdobson commented Apr 5, 2013

@robhoes - can you please take a look at this?

Thanks.

Owner

robhoes commented Apr 12, 2013

ok to test

@robhoes robhoes commented on an outdated diff Apr 12, 2013

ocaml/xapi/xapi_udhcpd.ml
@@ -114,14 +115,21 @@ module Udhcpd_conf = struct
let string_of_lease l =
Printf.sprintf "static_lease\t%s\t%s # %s\n" l.mac (Ip.string_of l.ip) l.vif in
let leases = List.map string_of_lease t.leases in
- String.concat "\n" (skel :: interface :: subnet :: router :: leases)
+ let network = Helpers.get_guest_installer_network ~__context in
+ let other_config = Db.Network.get_other_config ~__context ~self:network in
+ let config_list =
+ if (List.mem_assoc ip_disable_gw_key other_config)
@robhoes

robhoes Apr 12, 2013

Owner

Here you check only for the presence of the "ip_disable_gw" key, and not for whether it is set to "true". This means that if you set the key to "false", the gateway will also be disabled.

We normally use the following pattern:

if List.mem_assoc ip_disable_gw_key other_config && List.assoc ip_disable_gw_key other_config = "true" then .....
Member

rdobson commented Apr 15, 2013

@robhoes - Thanks - I've updated the patch to include the check for its value. Thanks for spotting that!

Are you happy to merge now?

@robhoes robhoes commented on an outdated diff Apr 15, 2013

ocaml/xapi/xapi_udhcpd.ml
@@ -114,14 +115,21 @@ module Udhcpd_conf = struct
let string_of_lease l =
Printf.sprintf "static_lease\t%s\t%s # %s\n" l.mac (Ip.string_of l.ip) l.vif in
let leases = List.map string_of_lease t.leases in
- String.concat "\n" (skel :: interface :: subnet :: router :: leases)
+ let network = Helpers.get_guest_installer_network ~__context in
+ let other_config = Db.Network.get_other_config ~__context ~self:network in
+ let config_list =
+ if (List.mem_assoc ip_disable_gw_key other_config && List.assoc ip_disable_gw_key other_config == "true")
@robhoes

robhoes Apr 15, 2013

Owner

I'm afraid you need a '=' here, not a '==', which is something different.

Owner

robhoes commented Apr 15, 2013

Also, could you add "Signed-off-by" lines to your patches.

Apart from that, I think it all looks good and ready to merge. Thanks for taking the effort to write the docs!

rdobson added some commits Feb 13, 2013

@rdobson @rdobson rdobson + rdobson CA-99179: Patches XAPI to allow clients to stop XAPI from generating
a udhcpd config file that includes advertising dom0's IP as
the default gateway for the network.

When using the host internal management network with Windows VMs
the network adapter will be allocated a high metric based on its speed,
and so traffic to unkown destinations may be routed via Dom0.

Signed-off-by: Rob Dobson <rob@rdobson.co.uk>
6522e6e
@rdobson rdobson CA-99179: Adding an API for configuring the udhcp server in Dom0.
Signed-off-by: Rob Dobson <rob@rdobson.co.uk>
f0de11c
Member

rdobson commented Apr 15, 2013

Great thanks! I believe I have now made those required modifications.

Thanks.

@robhoes robhoes added a commit that referenced this pull request Apr 15, 2013

@robhoes robhoes Merge pull request #1025 from rdobson/ca-99179
CA-99179: Add other_config param for disabling default gateway advertising on host-internal-management network
399de85

@robhoes robhoes merged commit 399de85 into xapi-project:master Apr 15, 2013

1 check passed

default Merged build finished.
Details
Owner

robhoes commented Apr 15, 2013

Cool, thanks! It is now merged. I'll duplicate your pull request for clearwater as well.

@rdobson rdobson deleted the rdobson:ca-99179 branch Apr 16, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment