Message ID | 570252CD.20208@redhat.com |
---|---|
State | RFC |
Headers | show |
"dev" <dev-bounces@openvswitch.org> wrote on 04/04/2016 06:41:01 AM: > From: Numan Siddique <nusiddiq@redhat.com> > To: ovs dev <dev@openvswitch.org> > Date: 04/04/2016 06:41 AM > Subject: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp using > 'continuations' > Sent by: "dev" <dev-bounces@openvswitch.org> > > To support native dhcp in ovn > - A new column 'dhcp-options' is added in 'Logical_Switch' north db. > - A logical flow is added for each logical port to handle dhcp packets > if the CMS has defined dhcp options in this column. > > Eg. > action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1, > server_id = 10.0.0.2, mtu = 1300, lease_time = 3600, > netmask = 255.255.255.0); eth.dst = eth.src; eth.src = 00:00:00:00:00:03; > ip4.dst = 10.0.0.2; ip4.src = 10.0.0.2; udp.src = 67; udp.dst = 68; > outport = inport; inport = ""; /* Allow sending out inport. */ output;) > > - ovn-controller will convert this logical flow to a packet-in OF flow with > 'pause' flag set. The dhcp options defined in 'dhcp_offer' action > are stored in the 'userdata' > > - When the ovn-controller receives the packet-in, it would frame a new > dhcp packet with the dhcp options set in the 'userdata' and resume > the packet. > > TODO: Test cases and updating the necessary documentation. > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > --- [snip] First, let me say that I'm a big fan of this idea in general... > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > index bcad318..a175458 100644 > --- a/ovn/controller/lflow.c > +++ b/ovn/controller/lflow.c > @@ -28,6 +28,7 @@ > #include "packets.h" > #include "simap.h" > > + > VLOG_DEFINE_THIS_MODULE(lflow); > > /* Symbol table. */ > @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow); > /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ > static struct shash symtab; > > +/* Contains "struct expr_symbol_dhcp_opts"s for dhcp options */ > +static struct shash dhcp_opt_symtab; > + > static void > add_logical_register(struct shash *symtab, enum mf_field_id id) > { > @@ -156,6 +160,27 @@ lflow_init(void) > expr_symtab_add_predicate(&symtab, "sctp", "ip.proto == 132"); > expr_symtab_add_field(&symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false); > expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false); > + > + /* dhcp options */ > + shash_init(&dhcp_opt_symtab); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name", 15, > + DHCP_OPT_TYPE_STR); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "ip_forward_enable", 19, > + DHCP_OPT_TYPE_BOOL); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26, > + DHCP_OPT_TYPE_UINT16); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51, > + DHCP_OPT_TYPE_UINT32); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54, > + DHCP_OPT_TYPE_IP4); but I'm rather uncomfortable with (a) using the bare option numbers above (I think I'd rather see symbols and values defined in dhcp.h) and (b) not supporting more/all of RFC 2132 (under this heading, isn't opt 0 padding, rather than the offerip?) Ryan
On Mon, Apr 4, 2016 at 8:46 AM, Ryan Moats <rmoats@us.ibm.com> wrote: > > "dev" <dev-bounces@openvswitch.org> wrote on 04/04/2016 06:41:01 AM: > > > From: Numan Siddique <nusiddiq@redhat.com> > > To: ovs dev <dev@openvswitch.org> > > Date: 04/04/2016 06:41 AM > > Subject: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp using > > 'continuations' > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > To support native dhcp in ovn > > - A new column 'dhcp-options' is added in 'Logical_Switch' north db. > > - A logical flow is added for each logical port to handle dhcp packets > > if the CMS has defined dhcp options in this column. > > > > Eg. > > action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1, > > server_id = 10.0.0.2, mtu = 1300, lease_time = 3600, > > netmask = 255.255.255.0); eth.dst = eth.src; eth.src = 00:00:00:00:00:03; > > ip4.dst = 10.0.0.2; ip4.src = 10.0.0.2; udp.src = 67; udp.dst = 68; > > outport = inport; inport = ""; /* Allow sending out inport. */ output;) > > > > - ovn-controller will convert this logical flow to a packet-in OF flow > with > > 'pause' flag set. The dhcp options defined in 'dhcp_offer' action > > are stored in the 'userdata' > > > > - When the ovn-controller receives the packet-in, it would frame a new > > dhcp packet with the dhcp options set in the 'userdata' and resume > > the packet. > > > > TODO: Test cases and updating the necessary documentation. > > > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > --- > > [snip] > > First, let me say that I'm a big fan of this idea in general... > > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > index bcad318..a175458 100644 > > --- a/ovn/controller/lflow.c > > +++ b/ovn/controller/lflow.c > > @@ -28,6 +28,7 @@ > > #include "packets.h" > > #include "simap.h" > > > > + > > VLOG_DEFINE_THIS_MODULE(lflow); > > > > /* Symbol table. */ > > @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow); > > /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ > > static struct shash symtab; > > > > +/* Contains "struct expr_symbol_dhcp_opts"s for dhcp options */ > > +static struct shash dhcp_opt_symtab; > > + > > static void > > add_logical_register(struct shash *symtab, enum mf_field_id id) > > { > > @@ -156,6 +160,27 @@ lflow_init(void) > > expr_symtab_add_predicate(&symtab, "sctp", "ip.proto == 132"); > > expr_symtab_add_field(&symtab, "sctp.src", MFF_SCTP_SRC, "sctp", > false); > > expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp", > false); > > + > > + /* dhcp options */ > > + shash_init(&dhcp_opt_symtab); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name", 15, > > + DHCP_OPT_TYPE_STR); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, > "ip_forward_enable", 19, > > + DHCP_OPT_TYPE_BOOL); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26, > > + DHCP_OPT_TYPE_UINT16); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51, > > + DHCP_OPT_TYPE_UINT32); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54, > > + DHCP_OPT_TYPE_IP4); > > but I'm rather uncomfortable with (a) using the bare option numbers > above (I think I'd rather see symbols and values defined in dhcp.h) > and (b) not supporting more/all of RFC 2132 (under this heading, isn't > opt 0 padding, rather than the offerip?) > I agree that some constants would be an improvement. Maybe I'm misreading your suggestion, but I definitely do not think we should be trying to implement a "full featured" DHCP server in OVN. The intention here is "just enough" DHCP. We only need minimal support to be able to issue DHCP responses based on statically defined IP/MAC mappings.
Russell Bryant <russell@ovn.org> wrote on 04/04/2016 09:27:30 AM: > From: Russell Bryant <russell@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: Numan Siddique <nusiddiq@redhat.com>, ovs dev <dev@openvswitch.org> > Date: 04/04/2016 09:28 AM > Subject: Re: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp > using'continuations' > > On Mon, Apr 4, 2016 at 8:46 AM, Ryan Moats <rmoats@us.ibm.com> wrote: > > "dev" <dev-bounces@openvswitch.org> wrote on 04/04/2016 06:41:01 AM: > > > From: Numan Siddique <nusiddiq@redhat.com> > > To: ovs dev <dev@openvswitch.org> > > Date: 04/04/2016 06:41 AM > > Subject: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp using > > 'continuations' > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > To support native dhcp in ovn > > - A new column 'dhcp-options' is added in 'Logical_Switch' north db. > > - A logical flow is added for each logical port to handle dhcp packets > > if the CMS has defined dhcp options in this column. > > > > Eg. > > action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1, > > server_id = 10.0.0.2, mtu = 1300, lease_time = 3600, > > netmask = 255.255.255.0); eth.dst = eth.src; eth.src = 00:00:00:00:00:03; > > ip4.dst = 10.0.0.2; ip4.src = 10.0.0.2; udp.src = 67; udp.dst = 68; > > outport = inport; inport = ""; /* Allow sending out inport. */ output;) > > > > - ovn-controller will convert this logical flow to a packet-in OF flow > with > > 'pause' flag set. The dhcp options defined in 'dhcp_offer' action > > are stored in the 'userdata' > > > > - When the ovn-controller receives the packet-in, it would frame a new > > dhcp packet with the dhcp options set in the 'userdata' and resume > > the packet. > > > > TODO: Test cases and updating the necessary documentation. > > > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > --- > > [snip] > > First, let me say that I'm a big fan of this idea in general... > > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > index bcad318..a175458 100644 > > --- a/ovn/controller/lflow.c > > +++ b/ovn/controller/lflow.c > > @@ -28,6 +28,7 @@ > > #include "packets.h" > > #include "simap.h" > > > > + > > VLOG_DEFINE_THIS_MODULE(lflow); > > > > /* Symbol table. */ > > @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow); > > /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ > > static struct shash symtab; > > > > +/* Contains "struct expr_symbol_dhcp_opts"s for dhcp options */ > > +static struct shash dhcp_opt_symtab; > > + > > static void > > add_logical_register(struct shash *symtab, enum mf_field_id id) > > { > > @@ -156,6 +160,27 @@ lflow_init(void) > > expr_symtab_add_predicate(&symtab, "sctp", "ip.proto == 132"); > > expr_symtab_add_field(&symtab, "sctp.src", MFF_SCTP_SRC, "sctp", > false); > > expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp", > false); > > + > > + /* dhcp options */ > > + shash_init(&dhcp_opt_symtab); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name", 15, > > + DHCP_OPT_TYPE_STR); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, > "ip_forward_enable", 19, > > + DHCP_OPT_TYPE_BOOL); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26, > > + DHCP_OPT_TYPE_UINT16); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51, > > + DHCP_OPT_TYPE_UINT32); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54, > > + DHCP_OPT_TYPE_IP4); > but I'm rather uncomfortable with (a) using the bare option numbers > above (I think I'd rather see symbols and values defined in dhcp.h) > and (b) not supporting more/all of RFC 2132 (under this heading, isn't > opt 0 padding, rather than the offerip?) > > I agree that some constants would be an improvement. > > Maybe I'm misreading your suggestion, but I definitely do not think > we should be trying to implement a "full featured" DHCP server in > OVN. The intention here is "just enough" DHCP. We only need > minimal support to be able to issue DHCP responses based on > statically defined IP/MAC mappings. I certainly don't want a full DHCP server in OVN and so I'd like to see something somewhere that *says* this isn't a full featured DHCP as it provides some minimal set of functionality (and then listing the functions). However, my sad experience with providing a minimal set of functionality is that it becomes a slippery slope of "just one more thing" (for example, what about static routes?). Ryan
> @@ -89,10 +89,11 @@ enum ovn_stage { > PIPELINE_STAGE(SWITCH, IN, PORT_SEC_L2, 0, "ls_in_port_sec_l2") \ > PIPELINE_STAGE(SWITCH, IN, PORT_SEC_IP, 1, "ls_in_port_sec_ip") \ > PIPELINE_STAGE(SWITCH, IN, PORT_SEC_ND, 2, "ls_in_port_sec_nd") \ > - PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 3, "ls_in_pre_acl") \ > - PIPELINE_STAGE(SWITCH, IN, ACL, 4, "ls_in_acl") \ > - PIPELINE_STAGE(SWITCH, IN, ARP_RSP, 5, "ls_in_arp_rsp") \ > - PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 6, "ls_in_l2_lkup") \ > + PIPELINE_STAGE(SWITCH, IN, DHCP, 3, "ls_in_dhcp") \ > + PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 4, "ls_in_pre_acl") \ > + PIPELINE_STAGE(SWITCH, IN, ACL, 5, "ls_in_acl") \ > + PIPELINE_STAGE(SWITCH, IN, ARP_RSP, 6, "ls_in_arp_rsp") \ > + PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 7, "ls_in_l2_lkup") \ > \ Would it make sense to put DHCP after ACL instead of before - so, some control is provided on the handling of DHCP packets via acl rules. For instance, OpenStack programs these DHCP ACL rules currently allowing client->server communication - and the usecase would be if i want to drop all client->server DHCP traffic for a rogue VM. table=2( ls_in_acl), priority= 2002, match=(inport == "55c0912f-f7aa-4318-82f1-6118032839e3" && ip4 && (ip4.dst == 255.255.255.255 || ip4.dst == 10.10.0.0/16) && udp && udp.src == 68 && udp.dst == 67), action=(ct_commit; next;)
On Mon, Apr 4, 2016 at 8:10 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > Russell Bryant <russell@ovn.org> wrote on 04/04/2016 09:27:30 AM: > > > From: Russell Bryant <russell@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: Numan Siddique <nusiddiq@redhat.com>, ovs dev <dev@openvswitch.org> > > Date: 04/04/2016 09:28 AM > > Subject: Re: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp > > using'continuations' > > > > > On Mon, Apr 4, 2016 at 8:46 AM, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > "dev" <dev-bounces@openvswitch.org> wrote on 04/04/2016 06:41:01 AM: > > > > > From: Numan Siddique <nusiddiq@redhat.com> > > > To: ovs dev <dev@openvswitch.org> > > > Date: 04/04/2016 06:41 AM > > > Subject: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp using > > > 'continuations' > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > To support native dhcp in ovn > > > - A new column 'dhcp-options' is added in 'Logical_Switch' north db. > > > - A logical flow is added for each logical port to handle dhcp packets > > > if the CMS has defined dhcp options in this column. > > > > > > Eg. > > > action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1, > > > server_id = 10.0.0.2, mtu = 1300, lease_time = 3600, > > > netmask = 255.255.255.0); eth.dst = eth.src; eth.src = > 00:00:00:00:00:03; > > > ip4.dst = 10.0.0.2; ip4.src = 10.0.0.2; udp.src = 67; udp.dst = 68; > > > outport = inport; inport = ""; /* Allow sending out inport. */ output;) > > > > > > - ovn-controller will convert this logical flow to a packet-in OF flow > > with > > > 'pause' flag set. The dhcp options defined in 'dhcp_offer' action > > > are stored in the 'userdata' > > > > > > - When the ovn-controller receives the packet-in, it would frame a new > > > dhcp packet with the dhcp options set in the 'userdata' and resume > > > the packet. > > > > > > TODO: Test cases and updating the necessary documentation. > > > > > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > > --- > > > > [snip] > > > > First, let me say that I'm a big fan of this idea in general... > > > > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > > index bcad318..a175458 100644 > > > --- a/ovn/controller/lflow.c > > > +++ b/ovn/controller/lflow.c > > > @@ -28,6 +28,7 @@ > > > #include "packets.h" > > > #include "simap.h" > > > > > > + > > > VLOG_DEFINE_THIS_MODULE(lflow); > > > > > > /* Symbol table. */ > > > @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow); > > > /* Contains "struct expr_symbol"s for fields supported by OVN lflows. > */ > > > static struct shash symtab; > > > > > > +/* Contains "struct expr_symbol_dhcp_opts"s for dhcp options */ > > > +static struct shash dhcp_opt_symtab; > > > + > > > static void > > > add_logical_register(struct shash *symtab, enum mf_field_id id) > > > { > > > @@ -156,6 +160,27 @@ lflow_init(void) > > > expr_symtab_add_predicate(&symtab, "sctp", "ip.proto == 132"); > > > expr_symtab_add_field(&symtab, "sctp.src", MFF_SCTP_SRC, "sctp", > > false); > > > expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp", > > false); > > > + > > > + /* dhcp options */ > > > + shash_init(&dhcp_opt_symtab); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0, > > > + DHCP_OPT_TYPE_IP4); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1, > > > + DHCP_OPT_TYPE_IP4); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3, > > > + DHCP_OPT_TYPE_IP4); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6, > > > + DHCP_OPT_TYPE_IP4); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name", > 15, > > > + DHCP_OPT_TYPE_STR); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, > > "ip_forward_enable", 19, > > > + DHCP_OPT_TYPE_BOOL); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26, > > > + DHCP_OPT_TYPE_UINT16); > > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51, > > > + DHCP_OPT_TYPE_UINT32); > > > + > > dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54, > > > + DHCP_OPT_TYPE_IP4); > > > but I'm rather uncomfortable with (a) using the bare option numbers > > above (I think I'd rather see symbols and values defined in dhcp.h) > > and (b) not supporting more/all of RFC 2132 (under this heading, isn't > > opt 0 padding, rather than the offerip?) > > > Thanks Ryan for pointing this out. I will define them in dhcp.h 'offerip' is actually not a dhcp option. I am using opt 0 since CMS would not define this dhcp option. The userdata would be of the format --------------------------------------------------------------------------------------- |ACTION_OPCODE_DHCP_OFFER (8 bytes) | offer_ip (4 bytes) | <dhcp options> | -------------------------------------------------------------------------------------- > > I agree that some constants would be an improvement. > > > > Maybe I'm misreading your suggestion, but I definitely do not think > > we should be trying to implement a "full featured" DHCP server in > > OVN. The intention here is "just enough" DHCP. We only need > > minimal support to be able to issue DHCP responses based on > > statically defined IP/MAC mappings. > > I certainly don't want a full DHCP server in OVN and so I'd like to > see something somewhere that *says* this isn't a full featured DHCP > as it provides some minimal set of functionality (and then listing > the functions). However, my sad experience with providing a minimal > set of functionality is that it becomes a slippery slope of "just > one more thing" (for example, what about static routes?). > Even I was thinking to identify a minimal set of dhcp options and support them. If the requirement arises for more dhcp options, we can raise a bug. In order to support more dhcp options we just need to define them in 'dhcp_opt_symtab'. In the next version, I will add more dhcp options which are more likely to be used, although I am not sure how to select them :) Thanks Numan > > Ryan > >
On Tue, Apr 5, 2016 at 12:17 AM, Ramu Ramamurthy <ramu.ramamurthy@gmail.com> wrote: > > @@ -89,10 +89,11 @@ enum ovn_stage { > > PIPELINE_STAGE(SWITCH, IN, PORT_SEC_L2, 0, > "ls_in_port_sec_l2") \ > > PIPELINE_STAGE(SWITCH, IN, PORT_SEC_IP, 1, > "ls_in_port_sec_ip") \ > > PIPELINE_STAGE(SWITCH, IN, PORT_SEC_ND, 2, > "ls_in_port_sec_nd") \ > > - PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 3, "ls_in_pre_acl") > \ > > - PIPELINE_STAGE(SWITCH, IN, ACL, 4, "ls_in_acl") > \ > > - PIPELINE_STAGE(SWITCH, IN, ARP_RSP, 5, "ls_in_arp_rsp") > \ > > - PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 6, "ls_in_l2_lkup") > \ > > + PIPELINE_STAGE(SWITCH, IN, DHCP, 3, "ls_in_dhcp") \ > > + PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 4, "ls_in_pre_acl") > \ > > + PIPELINE_STAGE(SWITCH, IN, ACL, 5, "ls_in_acl") > \ > > + PIPELINE_STAGE(SWITCH, IN, ARP_RSP, 6, "ls_in_arp_rsp") > \ > > + PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 7, "ls_in_l2_lkup") > \ > > \ > > Would it make sense to put DHCP after ACL instead of before - so, > some control is > provided on the handling of DHCP packets via acl rules. For instance, > OpenStack programs these DHCP ACL > rules currently allowing client->server communication - and the > usecase would be if i want to drop > all client->server DHCP traffic for a rogue VM. > > table=2( ls_in_acl), priority= 2002, match=(inport == > "55c0912f-f7aa-4318-82f1-6118032839e3" && ip4 && (ip4.dst == > 255.255.255.255 || ip4.dst == 10.10.0.0/16) && udp && udp.src == 68 && > udp.dst == 67), action=(ct_commit; next;) > Since the dhcp traffic is handled by the ovn-controller, I thought there is no need for the ACL checks. I do not have any preference as such. I will test this out once. Thanks Numan
> + /* dhcp options */ > + shash_init(&dhcp_opt_symtab); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6, > + DHCP_OPT_TYPE_IP4); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name", 15, > + DHCP_OPT_TYPE_STR); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "ip_forward_enable", 19, > + DHCP_OPT_TYPE_BOOL); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26, > + DHCP_OPT_TYPE_UINT16); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51, > + DHCP_OPT_TYPE_UINT32); > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54, > + DHCP_OPT_TYPE_IP4); > } Numan, DHCP static-route option (249) is used in the DHCP-offer to enable VM Metadata access. Could you consider adding support for that option - Or I can help to add that option.
On Mon, Apr 04, 2016 at 05:11:01PM +0530, Numan Siddique wrote: > To support native dhcp in ovn > - A new column 'dhcp-options' is added in 'Logical_Switch' north db. > - A logical flow is added for each logical port to handle dhcp packets > if the CMS has defined dhcp options in this column. > > Eg. > action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1, > server_id = 10.0.0.2, mtu = 1300, lease_time = 3600, > netmask = 255.255.255.0); eth.dst = eth.src; eth.src = 00:00:00:00:00:03; > ip4.dst = 10.0.0.2; ip4.src = 10.0.0.2; udp.src = 67; udp.dst = 68; > outport = inport; inport = ""; /* Allow sending out inport. */ output;) > > - ovn-controller will convert this logical flow to a packet-in OF flow with > 'pause' flag set. The dhcp options defined in 'dhcp_offer' action > are stored in the 'userdata' > > - When the ovn-controller receives the packet-in, it would frame a new > dhcp packet with the dhcp options set in the 'userdata' and resume > the packet. > > TODO: Test cases and updating the necessary documentation. > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Thanks for working on this! I am so much happier with this than with the previous generation of DHCP support. It is much more in line with the "OVN philosophy" if I can be forgiven for saying that. I get some warnings from Clang: ../ovn/controller/pinctrl.c:203:6: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align] ../ovn/controller/pinctrl.c:217:7: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align] ../ovn/controller/pinctrl.c:225:7: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align] ../ovn/controller/pinctrl.c:272:28: error: cast from 'char *' to 'struct ip_header *' increases required alignment from 1 to 2 [-Werror,-Wcast-align] ../ovn/controller/pinctrl.c:273:30: error: cast from 'char *' to 'struct udp_header *' increases required alignment from 1 to 2 [-Werror,-Wcast-align] ../ovn/controller/pinctrl.c:275:37: error: cast from 'char *' to 'struct dhcp_header *' increases required alignment from 1 to 4 [-Werror,-Wcast-align] and from sparse: ../ovn/controller/pinctrl.c:203:24: warning: incorrect type in assignment (different base types) ../ovn/controller/pinctrl.c:203:24: expected unsigned int [unsigned] [usertype] <noident> ../ovn/controller/pinctrl.c:203:24: got restricted ovs_be32 ../ovn/controller/pinctrl.c:284:37: warning: incorrect type in argument 1 (different base types) ../ovn/controller/pinctrl.c:284:37: expected restricted ofp_port_t [usertype] ofp_port ../ovn/controller/pinctrl.c:284:37: got int [signed] udp_len ../ovn/controller/pinctrl.c:286:53: warning: incorrect type in argument 1 (different base types) ../ovn/controller/pinctrl.c:286:53: expected restricted ofp_port_t [usertype] ofp_port ../ovn/controller/pinctrl.c:286:53: got int This is less code than I expected; that's great! This is an RFC so I won't nitpick. I do have one bigger idea: what if the collection of DHCP options were to be read from a new table in the southbound database? This would make it easy to add and remove support for particular options by updating just ovn-northd. It would solve the
On Tue, Apr 05, 2016 at 05:08:00PM +0530, Numan Siddique wrote: > Even I was thinking to identify a minimal set of dhcp options and > support them. If the requirement arises for more dhcp options, we can > raise a bug. In order to support more dhcp options we just need to > define them in 'dhcp_opt_symtab'. In the next version, I will add > more dhcp options which are more likely to be used, although I am not > sure how to select them :) It wouldn't be hard to have a new OVN_Southbound table DHCP_Option with columns "name", "code", and "type", and that would give us tons of flexibility.
On Thu, Apr 14, 2016 at 12:22 AM, Ramu Ramamurthy <ramu.ramamurthy@gmail.com > wrote: > > + /* dhcp options */ > > + shash_init(&dhcp_opt_symtab); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6, > > + DHCP_OPT_TYPE_IP4); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name", 15, > > + DHCP_OPT_TYPE_STR); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, > "ip_forward_enable", 19, > > + DHCP_OPT_TYPE_BOOL); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26, > > + DHCP_OPT_TYPE_UINT16); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51, > > + DHCP_OPT_TYPE_UINT32); > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54, > > + DHCP_OPT_TYPE_IP4); > > } > > Numan, DHCP static-route option (249) is used in the DHCP-offer to > enable VM Metadata access. > Could you consider adding support for that option - Or I can help to > add that option. > I have already started into supporting dhcp option 121 for static routes. https://tools.ietf.org/html/rfc3442 I think we can support both. Thanks Numan
diff --git a/lib/dhcp.h b/lib/dhcp.h index 6f97298..c9537e7 100644 --- a/lib/dhcp.h +++ b/lib/dhcp.h @@ -25,6 +25,19 @@ #define DHCP_SERVER_PORT 67 /* Port used by DHCP server. */ #define DHCP_CLIENT_PORT 68 /* Port used by DHCP client. */ +#define DHCP_MAGIC_COOKIE (uint32_t)0x63825363 + +#define DHCP_OP_REQUEST ((uint8_t)1) +#define DHCP_OP_REPLY ((uint8_t)2) + +#define DHCP_MSG_DISCOVER ((uint8_t)1) +#define DHCP_MSG_OFFER ((uint8_t)2) +#define DHCP_MSG_REQUEST ((uint8_t)3) +#define DHCP_MSG_ACK ((uint8_t)5) + +#define DHCP_OPT_MSG_TYPE ((uint8_t)53) +#define DHCP_OPT_END ((uint8_t)255) + #define DHCP_HEADER_LEN 236 struct dhcp_header { uint8_t op; /* DHCP_BOOTREQUEST or DHCP_BOOTREPLY. */ diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index bcad318..a175458 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -28,6 +28,7 @@ #include "packets.h" #include "simap.h" + VLOG_DEFINE_THIS_MODULE(lflow); /* Symbol table. */ @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow); /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ static struct shash symtab; +/* Contains "struct expr_symbol_dhcp_opts"s for dhcp options */ +static struct shash dhcp_opt_symtab; + static void add_logical_register(struct shash *symtab, enum mf_field_id id) { @@ -156,6 +160,27 @@ lflow_init(void) expr_symtab_add_predicate(&symtab, "sctp", "ip.proto == 132"); expr_symtab_add_field(&symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false); expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false); + + /* dhcp options */ + shash_init(&dhcp_opt_symtab); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0, + DHCP_OPT_TYPE_IP4); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1, + DHCP_OPT_TYPE_IP4); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3, + DHCP_OPT_TYPE_IP4); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6, + DHCP_OPT_TYPE_IP4); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name", 15, + DHCP_OPT_TYPE_STR); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "ip_forward_enable", 19, + DHCP_OPT_TYPE_BOOL); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26, + DHCP_OPT_TYPE_UINT16); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51, + DHCP_OPT_TYPE_UINT32); + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54, + DHCP_OPT_TYPE_IP4); } struct lookup_port_aux { @@ -280,6 +305,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, }; struct action_params ap = { .symtab = &symtab, + .dhcp_opt_symtab = &dhcp_opt_symtab, .lookup_port = lookup_port_cb, .aux = &aux, .ct_zones = ct_zones, @@ -443,4 +469,5 @@ void lflow_destroy(void) { expr_symtab_destroy(&symtab); + dhcp_opt_expr_symtab_destroy(&dhcp_opt_symtab); } diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c index 3fcab99..a3df5d8 100644 --- a/ovn/controller/pinctrl.c +++ b/ovn/controller/pinctrl.c @@ -17,9 +17,12 @@ #include "pinctrl.h" + #include "coverage.h" +#include "csum.h" #include "dirs.h" #include "dp-packet.h" +#include "lib/dhcp.h" #include "lport.h" #include "ofp-actions.h" #include "ovn/lib/actions.h" @@ -192,13 +195,118 @@ exit: } static void +compose_dhcp_options(struct ofpbuf *userdata, uint8_t *options, + unsigned int *options_len, uint8_t msg_type) +{ + uint8_t *opts = options; + + *(uint32_t *) opts = htonl(DHCP_MAGIC_COOKIE); + opts += (sizeof (uint32_t)); + + /* Dhcp option - type */ + opts[0] = (uint8_t) DHCP_OPT_MSG_TYPE; + opts[1] = (uint8_t) 1; + opts[2] = msg_type; + + opts += 3; + + memcpy(opts, userdata->data, userdata->size); + opts += userdata->size; + + /* Padding */ + *((uint32_t *) opts) = 0; + opts += 4; + + /* End */ + opts[0] = DHCP_OPT_END; + opts += 1; + + /* Padding */ + *((uint32_t *) opts) = 0; + opts += 4; + + *options_len = (opts - options); +} + +static void +pinctl_handle_dhcp_offer(struct dp_packet *pkt, struct ofputil_packet_in *pin, + struct ofpbuf *userdata, struct ofpbuf *continuation) +{ + /* Compose a DHCP response packet. */ + uint8_t options[256]; + unsigned int options_len = 0; + memset(options, 0, sizeof(options)); + + ovs_be32 *offer_ip = ofpbuf_pull(userdata, sizeof *offer_ip); + const uint8_t *payload = dp_packet_get_udp_payload(pkt); + payload = (payload + sizeof (struct dhcp_header) + sizeof(uint32_t)); + + if (payload[0] != DHCP_OPT_MSG_TYPE) { + return; + } + + uint8_t msg_type; + if (payload[2] == DHCP_MSG_DISCOVER) { + msg_type = DHCP_MSG_OFFER; + } else if(payload[2] == DHCP_MSG_REQUEST) { + msg_type = DHCP_MSG_ACK; + } else { + return; + } + + compose_dhcp_options(userdata, options, &options_len, msg_type); + + size_t new_packet_len = ETH_HEADER_LEN + IP_HEADER_LEN + \ + UDP_HEADER_LEN + DHCP_HEADER_LEN + options_len; + + struct dp_packet packet; + dp_packet_init(&packet, new_packet_len); + dp_packet_clear(&packet); + dp_packet_prealloc_tailroom(&packet, new_packet_len); + + dp_packet_put( + &packet, pin->packet, + new_packet_len < pin->packet_len ? new_packet_len : pin->packet_len); + dp_packet_set_size(&packet, new_packet_len); + + struct ip_header *ip = (struct ip_header *)((char *)dp_packet_data(&packet) + ETH_HEADER_LEN); + struct udp_header *udp = (struct udp_header *)((char *)ip + IP_HEADER_LEN); + + struct dhcp_header *dhcp_data = (struct dhcp_header *)((char *)udp + UDP_HEADER_LEN); + dhcp_data->op = DHCP_OP_REPLY; + dhcp_data->yiaddr = *offer_ip; + + char *dhcp_opt = ((char *)dhcp_data + sizeof (*dhcp_data)); + memcpy(dhcp_opt, options, options_len); + + int udp_len = sizeof (*dhcp_data) + options_len + UDP_HEADER_LEN; + + udp->udp_len = htons(ofp_to_u16(udp_len)); + ip->ip_tos = 0; + ip->ip_tot_len = htons(ofp_to_u16(IP_HEADER_LEN + udp_len)); + udp->udp_csum = 0; + ip->ip_csum = 0; + ip->ip_csum = csum(ip, sizeof *ip); + + enum ofp_version version = rconn_get_version(swconn); + enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version); + pin->packet = dp_packet_data(&packet); + pin->packet_len = dp_packet_size(&packet); + + queue_msg(ofputil_encode_resume(pin, continuation, proto)); + dp_packet_uninit(&packet); +} + +static void process_packet_in(const struct ofp_header *msg) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); struct ofputil_packet_in pin; + struct ofpbuf continuation; enum ofperr error = ofputil_decode_packet_in(msg, true, &pin, - NULL, NULL, NULL); + NULL, NULL, &continuation); + if (error) { VLOG_WARN_RL(&rl, "error decoding packet-in: %s", ofperr_to_string(error)); @@ -230,6 +338,9 @@ process_packet_in(const struct ofp_header *msg) pinctrl_handle_put_arp(&pin.flow_metadata.flow, &headers); break; + case ACTION_OPCODE_DHCP_OFFER: + pinctl_handle_dhcp_offer(&packet, &pin, &userdata, &continuation); + break; default: VLOG_WARN_RL(&rl, "unrecognized packet-in opcode %"PRIu32, ntohl(ah->opcode)); @@ -240,6 +351,7 @@ process_packet_in(const struct ofp_header *msg) static void pinctrl_recv(const struct ofp_header *oh, enum ofptype type) { + if (type == OFPTYPE_ECHO_REQUEST) { queue_msg(make_echo_reply(oh)); } else if (type == OFPTYPE_GET_CONFIG_REPLY) { diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index 44957c7..6bb52cc 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -25,6 +25,7 @@ #include "logical-fields.h" #include "ofp-actions.h" #include "openvswitch/ofpbuf.h" +#include "shash.h" #include "simap.h" /* Context maintained during actions_parse(). */ @@ -185,13 +186,15 @@ add_prerequisite(struct action_context *ctx, const char *prerequisite) } static size_t -start_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode) +start_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode, + bool pause) { size_t ofs = ofpacts->size; struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts); oc->max_len = UINT16_MAX; oc->reason = OFPR_ACTION; + oc->pause = pause; struct action_header ah = { .opcode = htonl(opcode) }; ofpbuf_put(ofpacts, &ah, sizeof ah); @@ -211,7 +214,7 @@ finish_controller_op(struct ofpbuf *ofpacts, size_t ofs) static void put_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode) { - size_t ofs = start_controller_op(ofpacts, opcode); + size_t ofs = start_controller_op(ofpacts, opcode, false); finish_controller_op(ofpacts, ofs); } @@ -245,7 +248,9 @@ parse_arp_action(struct action_context *ctx) * converted to OpenFlow, as its userdata. ovn-controller will convert the * packet to an ARP and then send the packet and actions back to the switch * inside an OFPT_PACKET_OUT message. */ - size_t oc_offset = start_controller_op(ctx->ofpacts, ACTION_OPCODE_ARP); + /* controller. */ + size_t oc_offset = start_controller_op(ctx->ofpacts, ACTION_OPCODE_ARP, + false); ofpacts_put_openflow_actions(inner_ofpacts.data, inner_ofpacts.size, ctx->ofpacts, OFP13_VERSION); finish_controller_op(ctx->ofpacts, oc_offset); @@ -260,6 +265,120 @@ parse_arp_action(struct action_context *ctx) } static bool +parse_dhcp_option(struct action_context *ctx, struct ofpbuf *dhcp_opts) +{ + if (ctx->lexer->token.type != LEX_T_ID) { + action_syntax_error(ctx, NULL); + return false; + } + + enum lex_type lookahead = lexer_lookahead(ctx->lexer); + if (lookahead != LEX_T_EQUALS) { + action_syntax_error(ctx, NULL); + return false; + } + + const struct expr_symbol_dhcp_opts *symbol = shash_find_data( + ctx->ap->dhcp_opt_symtab, ctx->lexer->token.s); + + if (!symbol) { + action_syntax_error(ctx, "expecting valid dhcp option."); + return false; + } + + lexer_get(ctx->lexer); + lexer_get(ctx->lexer); + + struct expr_constant_set cs; + memset(&cs, 0, sizeof(struct expr_constant_set)); + if (!expr_parse_constant_set(ctx->lexer, ctx->ap->dhcp_opt_symtab, &cs)) { + action_syntax_error(ctx, "invalid dhcp option values"); + return false; + } + + if (!lexer_match(ctx->lexer, LEX_T_COMMA) && ( + ctx->lexer->token.type != LEX_T_RPAREN)) { + action_syntax_error(ctx, NULL); + return false; + } + + + if (symbol->opt_code == 0) { + /* offer-ip */ + ofpbuf_push(dhcp_opts, &cs.values[0].value.ipv4, sizeof(ovs_be32)); + goto exit; + } + + uint8_t *opt_header = ofpbuf_put_uninit(dhcp_opts, 2); + opt_header[0] = symbol->opt_code; + + + switch(symbol->opt_type) { + case DHCP_OPT_TYPE_BOOL: + case DHCP_OPT_TYPE_UINT8: + opt_header[1] = 1; + uint8_t val = cs.values[0].value.u8[127]; + ofpbuf_put(dhcp_opts, &val, 1); + break; + + case DHCP_OPT_TYPE_UINT16: + opt_header[1] = 2; + ofpbuf_put(dhcp_opts, &cs.values[0].value.be16[63], 2); + break; + + case DHCP_OPT_TYPE_UINT32: + opt_header[1] = 4; + ofpbuf_put(dhcp_opts, &cs.values[0].value.be32[31], 4); + break; + + case DHCP_OPT_TYPE_IP4: + opt_header[1] = 0; + for (size_t i = 0; i < cs.n_values; i++) { + ofpbuf_put(dhcp_opts, &cs.values[i].value.ipv4, sizeof(ovs_be32)); + opt_header[1] += sizeof(ovs_be32); + } + break; + + case DHCP_OPT_TYPE_STR: + opt_header[1] = strlen(cs.values[0].string); + ofpbuf_put(dhcp_opts, cs.values[0].string, opt_header[1]); + break; + } + +exit: + expr_constant_set_destroy(&cs); + return true; +} + +static void +parse_dhcp_offer_action(struct action_context *ctx) +{ + if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) { + action_syntax_error(ctx, "expecting `('"); + return; + } + + uint64_t dhcp_opts_stub[1024 / 8]; + struct ofpbuf dhcp_opts = OFPBUF_STUB_INITIALIZER(dhcp_opts_stub); + + /* Parse inner actions. */ + while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { + if (!parse_dhcp_option(ctx, &dhcp_opts)) { + return; + } + } + + /* controller. */ + size_t oc_offset = start_controller_op(ctx->ofpacts, + ACTION_OPCODE_DHCP_OFFER, true); + ofpbuf_put(ctx->ofpacts, dhcp_opts.data, dhcp_opts.size); + finish_controller_op(ctx->ofpacts, oc_offset); + + /* Free memory. */ + ofpbuf_uninit(&dhcp_opts); +} + +static bool action_force_match(struct action_context *ctx, enum lex_type t) { if (lexer_match(ctx->lexer, t)) { @@ -473,6 +592,8 @@ parse_action(struct action_context *ctx) parse_get_arp_action(ctx); } else if (lexer_match_id(ctx->lexer, "put_arp")) { parse_put_arp_action(ctx); + } else if (lexer_match_id(ctx->lexer, "dhcp_offer")) { + parse_dhcp_offer_action(ctx); } else { action_syntax_error(ctx, "expecting action"); } diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h index 29af06f..782c5ba 100644 --- a/ovn/lib/actions.h +++ b/ovn/lib/actions.h @@ -44,6 +44,11 @@ enum action_opcode { * MFF_ETH_SRC = mac */ ACTION_OPCODE_PUT_ARP, + + /* "dhcp_offer(...dhcp actions ...}". + * + */ + ACTION_OPCODE_DHCP_OFFER, }; /* Header. */ @@ -58,6 +63,9 @@ struct action_params { * expr_parse()). */ const struct shash *symtab; + /* A table of "struct dhcp_opt_expr_symbol"s to support 'dhcp_offer' */ + const struct shash *dhcp_opt_symtab; + /* Looks up logical port 'port_name'. If found, stores its port number in * '*portp' and returns true; otherwise, returns false. */ bool (*lookup_port)(const void *aux, const char *port_name, diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c index aae5df6..5e1db16 100644 --- a/ovn/lib/expr.c +++ b/ovn/lib/expr.c @@ -404,39 +404,6 @@ expr_print(const struct expr *e) /* Parsing. */ -/* Type of a "union expr_constant" or "struct expr_constant_set". */ -enum expr_constant_type { - EXPR_C_INTEGER, - EXPR_C_STRING -}; - -/* A string or integer constant (one must know which from context). */ -union expr_constant { - /* Integer constant. - * - * The width of a constant isn't always clear, e.g. if you write "1", - * there's no way to tell whether you mean for that to be a 1-bit constant - * or a 128-bit constant or somewhere in between. */ - struct { - union mf_subvalue value; - union mf_subvalue mask; /* Only initialized if 'masked'. */ - bool masked; - - enum lex_format format; /* From the constant's lex_token. */ - }; - - /* Null-terminated string constant. */ - char *string; -}; - -/* A collection of "union expr_constant"s of the same type. */ -struct expr_constant_set { - union expr_constant *values; /* Constants. */ - size_t n_values; /* Number of constants. */ - enum expr_constant_type type; /* Type of the constants. */ - bool in_curlies; /* Whether the constants were in {}. */ -}; - /* A reference to a symbol or a subfield of a symbol. * * For string fields, ofs and n_bits are 0. */ @@ -456,7 +423,6 @@ struct expr_context { struct expr *expr_parse__(struct expr_context *); static void expr_not(struct expr *); -static void expr_constant_set_destroy(struct expr_constant_set *); static bool parse_field(struct expr_context *, struct expr_field *); static bool @@ -837,7 +803,7 @@ parse_constant_set(struct expr_context *ctx, struct expr_constant_set *cs) return ok; } -static void +void expr_constant_set_destroy(struct expr_constant_set *cs) { if (cs) { @@ -2928,3 +2894,42 @@ exit: } return ctx.error; } + +bool +expr_parse_constant_set(struct lexer *lexer, const struct shash *symtab, + struct expr_constant_set *cs) +{ + struct expr_context ctx = { + .lexer = lexer, + .symtab = symtab, + }; + + return parse_constant_set(&ctx, cs); +} + +struct expr_symbol_dhcp_opts * +dhcp_opt_expr_symtab_add_field(struct shash *symtab, const char *opt_name, + uint8_t opt_code, enum dhcp_opt_type type) +{ + struct expr_symbol_dhcp_opts *symbol = xzalloc(sizeof *symbol); + symbol->opt_name = xstrdup(opt_name); + symbol->opt_code = opt_code; + symbol->opt_type = type; + shash_add_assert(symtab, symbol->opt_name, symbol); + + return symbol; +} + +void +dhcp_opt_expr_symtab_destroy(struct shash *symtab) +{ + struct shash_node *node, *next; + + SHASH_FOR_EACH_SAFE (node, next, symtab) { + struct expr_symbol_dhcp_opts *symbol = node->data; + + shash_delete(symtab, node); + free(symbol->opt_name); + free(symbol); + } +} diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h index fa7ccbe..370c614 100644 --- a/ovn/lib/expr.h +++ b/ovn/lib/expr.h @@ -391,4 +391,61 @@ char *expr_parse_field(struct lexer *, int n_bits, bool rw, const struct shash *symtab, struct mf_subfield *, struct expr **prereqsp); +/* Type of a "union expr_constant" or "struct expr_constant_set". */ +enum expr_constant_type { + EXPR_C_INTEGER, + EXPR_C_STRING +}; + +/* A string or integer constant (one must know which from context). */ +union expr_constant { + /* Integer constant. + * + * The width of a constant isn't always clear, e.g. if you write "1", + * there's no way to tell whether you mean for that to be a 1-bit constant + * or a 128-bit constant or somewhere in between. */ + struct { + union mf_subvalue value; + union mf_subvalue mask; /* Only initialized if 'masked'. */ + bool masked; + + enum lex_format format; /* From the constant's lex_token. */ + }; + + /* Null-terminated string constant. */ + char *string; +}; + +/* A collection of "union expr_constant"s of the same type. */ +struct expr_constant_set { + union expr_constant *values; /* Constants. */ + size_t n_values; /* Number of constants. */ + enum expr_constant_type type; /* Type of the constants. */ + bool in_curlies; /* Whether the constants were in {}. */ +}; + +bool expr_parse_constant_set(struct lexer *, const struct shash *symtab, + struct expr_constant_set *cs); +void expr_constant_set_destroy(struct expr_constant_set *cs); + +enum dhcp_opt_type { + DHCP_OPT_TYPE_BOOL, + DHCP_OPT_TYPE_UINT8, + DHCP_OPT_TYPE_UINT16, + DHCP_OPT_TYPE_UINT32, + DHCP_OPT_TYPE_IP4, + DHCP_OPT_TYPE_STR +}; + +struct expr_symbol_dhcp_opts { + char *opt_name; + uint8_t opt_code; + uint8_t opt_type; +}; + +struct expr_symbol_dhcp_opts *dhcp_opt_expr_symtab_add_field( + struct shash *symtab, const char *opt_name, uint8_t opt_code, + enum dhcp_opt_type type); + +void dhcp_opt_expr_symtab_destroy(struct shash *symtab); #endif /* ovn/expr.h */ diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 4b1d611..0e3654a 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -89,10 +89,11 @@ enum ovn_stage { PIPELINE_STAGE(SWITCH, IN, PORT_SEC_L2, 0, "ls_in_port_sec_l2") \ PIPELINE_STAGE(SWITCH, IN, PORT_SEC_IP, 1, "ls_in_port_sec_ip") \ PIPELINE_STAGE(SWITCH, IN, PORT_SEC_ND, 2, "ls_in_port_sec_nd") \ - PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 3, "ls_in_pre_acl") \ - PIPELINE_STAGE(SWITCH, IN, ACL, 4, "ls_in_acl") \ - PIPELINE_STAGE(SWITCH, IN, ARP_RSP, 5, "ls_in_arp_rsp") \ - PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 6, "ls_in_l2_lkup") \ + PIPELINE_STAGE(SWITCH, IN, DHCP, 3, "ls_in_dhcp") \ + PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 4, "ls_in_pre_acl") \ + PIPELINE_STAGE(SWITCH, IN, ACL, 5, "ls_in_acl") \ + PIPELINE_STAGE(SWITCH, IN, ARP_RSP, 6, "ls_in_arp_rsp") \ + PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 7, "ls_in_l2_lkup") \ \ /* Logical switch egress stages. */ \ PIPELINE_STAGE(SWITCH, OUT, PRE_ACL, 0, "ls_out_pre_acl") \ @@ -1324,6 +1325,61 @@ has_stateful_acl(struct ovn_datapath *od) return false; } +static bool +build_dhcp_action(struct ovn_port *op, ovs_be32 offer_ip, struct ds *action) +{ + uint8_t options_bmap = 0; + struct smap_node *node; + char *server_ip= NULL; + char *server_mac = NULL; + + ds_put_format(action, "dhcp_offer(offerip = "IP_FMT", ", + IP_ARGS(offer_ip)); + + SMAP_FOR_EACH(node, &op->od->nbs->dhcp_options) { + if(!strcmp(node->key, "router")) { + options_bmap |= 0x1; + } else if (!strcmp(node->key, "server_id")) { + options_bmap |= 0x2; + server_ip = node->value; + } else if (!strcmp(node->key, "server_mac")) { + options_bmap |= 0x4; + server_mac = node->value; + continue; + } else if (!strcmp(node->key, "lease_time")) { + options_bmap |= 0x8; + } else if (!strcmp(node->key, "netmask")) { + options_bmap |= 0x10; + } + + ds_put_format(action, "%s = %s, ", node->key, node->value); + } + + if (!(options_bmap & 0x08)) { + return false; + } + + SMAP_FOR_EACH(node, &op->nbs->options) { + if(!strncmp(node->key, "dhcp-opt-", 9)) { + ds_put_format(action, "%s = %s, ", &node->key[9], node->value); + } + } + + if (!(options_bmap & 0x10)) { + ds_put_cstr(action, "netmask = 255.255.255.0, "); + } + + ds_chomp(action, ' '); + ds_chomp(action, ','); + + ds_put_format(action, "); eth.dst = eth.src; eth.src = %s; ip4.dst = "IP_FMT"; " + "ip4.src = %s; udp.src = 67; udp.dst = 68; ", + server_mac, IP_ARGS(offer_ip), server_ip); + ds_put_cstr(action, "outport = inport; inport = \"\"; " + "/* Allow sending out inport. */ output;"); + return true; +} + static void build_acls(struct ovn_datapath *od, struct hmap *lflows, struct hmap *ports) { @@ -1541,8 +1597,8 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, } } - /* Ingress table 1 and 2: Port security - IP and ND, by default goto next. - * (priority 0)*/ + /* Ingress table 1 and 2: Port security - IP and ND, tabl3 - DCHP + * by default goto next. (priority 0)*/ HMAP_FOR_EACH (od, key_node, datapaths) { if (!od->nbs) { continue; @@ -1550,9 +1606,51 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_ND, 0, "1", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_IP, 0, "1", "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_IN_DHCP, 0, "1", "next;"); } - /* Ingress table 3: ARP responder, skip requests coming from localnet ports. + /* Logical switch ingress table 3: DHCP priority 50. */ + HMAP_FOR_EACH (op, key_node, ports) { + if (!op->nbs) { + continue; + } + + if (!lport_is_enabled(op->nbs)) { + /* Drop packets from disabled logical ports (since logical flow + * tables are default-drop). */ + continue; + } + + for (size_t i = 0; i < op->nbs->n_addresses; i++) { + struct lport_addresses laddrs; + if (!extract_lport_addresses(op->nbs->addresses[i], &laddrs, + false)) { + continue; + } + + if (!laddrs.n_ipv4_addrs) { + continue; + } + + struct ds action = DS_EMPTY_INITIALIZER; + if (build_dhcp_action(op, laddrs.ipv4_addrs[0].addr, &action)) { + struct ds match = DS_EMPTY_INITIALIZER; + ds_put_format(&match, "inport == %s && eth.src == "ETH_ADDR_FMT + " && ip4.src == 0.0.0.0 && " + "ip4.dst == 255.255.255.255 && udp.src == 68 && " + "udp.dst == 67", op->json_key, + ETH_ADDR_ARGS(laddrs.ea)); + + ovn_lflow_add(lflows, op->od, S_SWITCH_IN_DHCP, 50, + ds_cstr(&match), ds_cstr(&action)); + ds_destroy(&match); + } + ds_destroy(&action); + } + + } + + /* Ingress table 6: ARP responder, skip requests coming from localnet ports. * (priority 100). */ HMAP_FOR_EACH (op, key_node, ports) { if (!op->nbs) { @@ -1567,7 +1665,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, } } - /* Ingress table 5: ARP responder, reply for known IPs. + /* Ingress table 6: ARP responder, reply for known IPs. * (priority 50). */ HMAP_FOR_EACH (op, key_node, ports) { if (!op->nbs) { @@ -1617,7 +1715,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, } } - /* Ingress table 5: ARP responder, by default goto next. + /* Ingress table 6: ARP responder, by default goto next. * (priority 0)*/ HMAP_FOR_EACH (od, key_node, datapaths) { if (!od->nbs) { @@ -1627,7 +1725,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, ovn_lflow_add(lflows, od, S_SWITCH_IN_ARP_RSP, 0, "1", "next;"); } - /* Ingress table 6: Destination lookup, broadcast and multicast handling + /* Ingress table 7: Destination lookup, broadcast and multicast handling * (priority 100). */ HMAP_FOR_EACH (op, key_node, ports) { if (!op->nbs) { @@ -1647,7 +1745,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, "outport = \""MC_FLOOD"\"; output;"); } - /* Ingress table 6: Destination lookup, unicast handling (priority 50), */ + /* Ingress table 7: Destination lookup, unicast handling (priority 50), */ HMAP_FOR_EACH (op, key_node, ports) { if (!op->nbs) { continue; @@ -1684,7 +1782,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, } } - /* Ingress table 6: Destination lookup for unknown MACs (priority 0). */ + /* Ingress table 7: Destination lookup for unknown MACs (priority 0). */ HMAP_FOR_EACH (od, key_node, datapaths) { if (!od->nbs) { continue; diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema index 40a7a97..2088fd5 100644 --- a/ovn/ovn-nb.ovsschema +++ b/ovn/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "2.0.2", - "cksum": "4289495412 4436", + "version": "2.0.3", + "cksum": "776261543 4598", "tables": { "Logical_Switch": { "columns": { @@ -16,6 +16,9 @@ "refType": "strong"}, "min": 0, "max": "unlimited"}}, + "dhcp_options": { + "type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml index e65bc3a..803c213 100644 --- a/ovn/ovn-nb.xml +++ b/ovn/ovn-nb.xml @@ -78,6 +78,12 @@ See <em>External IDs</em> at the beginning of this document. </column> </group> + + <column name="dhcp_options"> + This column provides key/value settings specific to the logical port + <ref column="type"/>. The type-specific options are described + individually below. + </column> </table> <table name="Logical_Port" title="L2 logical switch port"> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index bdad368..5bff15e 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -478,6 +478,52 @@ nbctl_lswitch_list(struct ctl_context *ctx) smap_destroy(&lswitches); free(nodes); } + +static void +nbctl_lswitch_set_dhcp_options(struct ctl_context *ctx) +{ + const char *id = ctx->argv[1]; + const struct nbrec_logical_switch *lswitch; + size_t i; + struct smap dhcp_options = SMAP_INITIALIZER(&dhcp_options); + + lswitch = lswitch_by_name_or_uuid(ctx, id); + if (!lswitch) { + return; + } + + for (i = 2; i < ctx->argc; i++) { + char *key, *value; + value = xstrdup(ctx->argv[i]); + key = strsep(&value, "="); + if (value) { + smap_add(&dhcp_options, key, value); + } + free(key); + } + + nbrec_logical_switch_set_dhcp_options(lswitch, &dhcp_options); + + smap_destroy(&dhcp_options); +} + +static void +nbctl_lswitch_get_dhcp_options(struct ctl_context *ctx) +{ + const char *id = ctx->argv[1]; + const struct nbrec_logical_switch *lswitch; + struct smap_node *node; + + lswitch = lswitch_by_name_or_uuid(ctx, id); + if (!lswitch) { + return; + } + + SMAP_FOR_EACH(node, &lswitch->dhcp_options) { + ds_put_format(&ctx->output, "%s=%s\n", node->key, node->value); + } +} + static const struct nbrec_logical_port * lport_by_name_or_uuid(struct ctl_context *ctx, const char *id) @@ -1311,6 +1357,11 @@ static const struct ctl_command_syntax nbctl_commands[] = { { "lswitch-del", 1, 1, "LSWITCH", NULL, nbctl_lswitch_del, NULL, "", RW }, { "lswitch-list", 0, 0, "", NULL, nbctl_lswitch_list, NULL, "", RO }, + { "lswitch-set-dhcp-options", 1, INT_MAX, + "LSWITCH KEY=VALUE [KEY=VALUE]...", NULL, nbctl_lswitch_set_dhcp_options, + NULL, "", RW }, + { "lswitch-get-dhcp-options", 1, 1, "LSWITCH", NULL, + nbctl_lswitch_get_dhcp_options, NULL, "", RO }, /* acl commands. */ { "acl-add", 5, 5, "LSWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
To support native dhcp in ovn - A new column 'dhcp-options' is added in 'Logical_Switch' north db. - A logical flow is added for each logical port to handle dhcp packets if the CMS has defined dhcp options in this column. Eg. action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1, server_id = 10.0.0.2, mtu = 1300, lease_time = 3600, netmask = 255.255.255.0); eth.dst = eth.src; eth.src = 00:00:00:00:00:03; ip4.dst = 10.0.0.2; ip4.src = 10.0.0.2; udp.src = 67; udp.dst = 68; outport = inport; inport = ""; /* Allow sending out inport. */ output;) - ovn-controller will convert this logical flow to a packet-in OF flow with 'pause' flag set. The dhcp options defined in 'dhcp_offer' action are stored in the 'userdata' - When the ovn-controller receives the packet-in, it would frame a new dhcp packet with the dhcp options set in the 'userdata' and resume the packet. TODO: Test cases and updating the necessary documentation. Signed-off-by: Numan Siddique <nusiddiq@redhat.com> --- lib/dhcp.h | 13 +++++ ovn/controller/lflow.c | 27 ++++++++++ ovn/controller/pinctrl.c | 114 ++++++++++++++++++++++++++++++++++++++++- ovn/lib/actions.c | 127 ++++++++++++++++++++++++++++++++++++++++++++-- ovn/lib/actions.h | 8 +++ ovn/lib/expr.c | 75 ++++++++++++++------------- ovn/lib/expr.h | 57 +++++++++++++++++++++ ovn/northd/ovn-northd.c | 122 +++++++++++++++++++++++++++++++++++++++----- ovn/ovn-nb.ovsschema | 7 ++- ovn/ovn-nb.xml | 6 +++ ovn/utilities/ovn-nbctl.c | 51 +++++++++++++++++++ 11 files changed, 554 insertions(+), 53 deletions(-)