On Thu, May 19, 2022 at 10:05 PM Numan Siddique <[email protected]> wrote:
>
> On Thu, May 19, 2022 at 4:11 AM Lucas Alvares Gomes <[email protected]>
> wrote:
> >
> > Hi,
> >
> > Thanks Numan for the review. See the replies below.
> >
> > On Thu, May 19, 2022 at 12:36 AM Numan Siddique <[email protected]> wrote:
> > >
> > > On Wed, May 11, 2022 at 11:34 AM <[email protected]> wrote:
> > > >
> > > > From: Lucas Alvares Gomes <[email protected]>
> > > >
> > > > In order to PXE boot a baremetal server using the OVN DHCP server we
> > > > need to allow users to set the "next-server" (siaddr) [0] field in the
> > > > DHCP header.
> > > >
> > > > While investigating this issue by comparing the DHCPOFFER and DHCPACK
> > > > packets sent my dnsmasq and OVN we saw that the "next-server" field
> > > > was the problem for OVN, without it PXE booting was timing out while
> > > > fetching the iPXE image from the TFTP server (see the bugzilla ticket
> > > > below for reference).
> > > >
> > > > To confirm this problem we created a bogus patch hardcoding the TFTP
> > > > address in the siaddr of the DHCP header (see the discussion in the
> > > > maillist below) and with this in place we were able to deploy a
> > > > baremetal node using the OVN DHCP end-to-end.
> > > >
> > > > This patch is a proper implementation that creates a new DHCP
> > > > configuration option called "next_server" to allow users to set this
> > > > field dynamically. This patch uses the DHCP code 253 which is a unsed
> > > > code for DHCP specification as this is not a normal DHCP option but a
> > > > special use case in OVN.
> > > >
> > > > [0]
> > > > https://github.com/openvswitch/ovs/blob/9dd3031d2e0e9597449e95428320ccaaff7d8b3d/lib/dhcp.h#L42
> > > >
> > > > Reported-by: https://bugzilla.redhat.com/show_bug.cgi?id=2083629
> > > > Reported-by:
> > > > https://mail.openvswitch.org/pipermail/ovs-discuss/2022-May/051821.html
> > > > Signed-off-by: Lucas Alvares Gomes <[email protected]>
> > > > ---
> > > > controller/pinctrl.c | 69 ++++++++++++++++++++++++++++++--------------
> > > > lib/actions.c | 13 ++++++++-
> > > > lib/ovn-l7.h | 8 +++++
> > > > northd/ovn-northd.c | 1 +
> > > > ovn-nb.xml | 7 +++++
> > > > tests/ovn.at | 6 ++--
> > > > tests/test-ovn.c | 1 +
> > > > 7 files changed, 80 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > > index ae3da332c..428863293 100644
> > > > --- a/controller/pinctrl.c
> > > > +++ b/controller/pinctrl.c
> > > > @@ -2203,30 +2203,56 @@ pinctrl_handle_put_dhcp_opts(
> > > > *| 4 Bytes padding | 1 Byte (option end 0xFF ) | 4 Bytes padding|
> > > > * --------------------------------------------------------------
> > > > */
> > > > - struct dhcp_opt_header *in_dhcp_opt =
> > > > - (struct dhcp_opt_header *)reply_dhcp_opts_ptr->data;
> > > > - if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_CODE) {
> > > > - unsigned char *ptr = (unsigned char *)in_dhcp_opt;
> > > > - int len = sizeof *in_dhcp_opt + in_dhcp_opt->len;
> > > > - struct dhcp_opt_header *next_dhcp_opt =
> > > > - (struct dhcp_opt_header *)(ptr + len);
> > > > -
> > > > - if (next_dhcp_opt->code == DHCP_OPT_BOOTFILE_ALT_CODE) {
> > > > - if (!ipxe_req) {
> > > > - ofpbuf_pull(reply_dhcp_opts_ptr, len);
> > > > - next_dhcp_opt->code = DHCP_OPT_BOOTFILE_CODE;
> > > > - } else {
> > > > - char *buf = xmalloc(len);
> > > > + ovs_be32 next_server = in_dhcp_data->siaddr;
> > > > + bool bootfile_name_set = false;
> > > > + in_dhcp_ptr = reply_dhcp_opts_ptr->data;
> > > > + end = (const char *)reply_dhcp_opts_ptr->data +
> > > > reply_dhcp_opts_ptr->size;
> > > > +
> > >
> > > Hi Lucas,
> > >
> > > Thanks for adding this support.
> > >
> > > It seems to me this while loop can be avoided since lib/actions.c
> > > always encodes offer_ip, DHCP_OPT_BOOTFILE_CODE,
> > > DHCP_OPT_BOOTFILE_ALT_CODE
> > > and now DHCP_OPT_NEXT_SERVER_CODE in the userdata buffer before
> > > encoding other dhcp options.
> > >
> > > Since it is deterministic, can't we do something like below instead
> > > of the while loop ?
> > >
> > > struct dhcp_opt_header *in_dhcp_opt =
> > > (struct dhcp_opt_header *)reply_dhcp_opts_ptr->data;
> > > if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_CODE) {
> > > ....
> > > advance in_dhcp_opt to the next option
> > > } else if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_ALT_CODE) {
> > > ..
> > > advance in_dhcp_opt to the next option
> > > }
> > >
> > > if (in_dhcp_opt->code == DHCP_OPT_NEXT_SERVER_CODE) {
> > > next_server = get_unaligned_be32(DHCP_OPT_PAYLOAD(in_dhcp_opt));
> > > }
> > >
> > > Let me know if this gets complicated because of my above suggestion.
> > > In that case,I'm fine to run the below while loop.
> > >
> >
> > That's how I coded it the first time when I was testing the patch, but
> > I found a problem where if more than one option was set, for example:
> > bootfile_name and next_server only the first encoded option would be
> > processed. In order to process all options I needed to offset to the
> > next one like:
> >
> > unsigned char *ptr = (unsigned char *)in_dhcp_opt;
> > int len = sizeof *in_dhcp_opt + in_dhcp_opt->len;
> > struct dhcp_opt_header *next_dhcp_opt = (struct dhcp_opt_header *)(ptr +
> > len);
> >
> > But by then the code wasn't really looking great anymore. That's why I
> > thought that the looping + switch case just looks better. Plus, it's more
> > future proof as if we need to add another option to this (and we might
> > with iPXE for IPv6) all we need to do is add another "case <opt>".
> >
>
> Ok. Thanks for the detailed explanation.
>
> > >
> > > Also it would be great if you can enhance the test "dhcpv4 : 1 HV, 2
> > > LS, 2 LSPs/LS" in ovn.at with the newly added option.
> > >
> >
> > Will take a look at it.
> >
> > > From what I understand, the dhcp response will also include this new
> > > option - 253 along with setting the dhcp_header->siaddr if
> > > CMS has defined next_server in the dhcp_options right ? If so, the
> > > dhcp clients would ignore this option gracefully right ?
> > >
> >
> > That's right, the option is simply ignored. It won't cause any harm.
> >
>
> Hi Lucas,
>
> Since we will be branching soon, I applied this patch to the main branch.
> I'd appreciate if you can add the test in the follow up patch.
>
> I also updated the NEWS item
>
Thanks very much for this, Numan!
I will look into the follow up patch enhancing the test for this
change. Appreciate it!
> diff --git a/NEWS b/NEWS
> index 1cdba4bfc6..8a5604a3b9 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -21,6 +21,8 @@ Post v22.03.0
> - --n-threads=<N> in northd cmdline.
> - set-n-threads/get-n-threads unixctls.
> - --ovn-northd-n-threads command line argument in ovn-ctl
> + - Added support for setting the Next server IP in the DHCP header
> + using the private DHCP option - 253 in native OVN DHCPv4 responder.
>
> OVN v22.03.0 - 11 Mar 2022
> --------------------------
>
> Thanks
> Numan
>
>
> >
> > > Thanks
> > > Numan
> > >
> > >
> > > > + while (in_dhcp_ptr < end) {
> > > > + struct dhcp_opt_header *in_dhcp_opt =
> > > > + (struct dhcp_opt_header *)in_dhcp_ptr;
> > > > +
> > > > + switch (in_dhcp_opt->code) {
> > > > + case DHCP_OPT_NEXT_SERVER_CODE:
> > > > + next_server =
> > > > get_unaligned_be32(DHCP_OPT_PAYLOAD(in_dhcp_opt));
> > > > + break;
> > > > + case DHCP_OPT_BOOTFILE_CODE: ;
> > > > + unsigned char *ptr = (unsigned char *)in_dhcp_opt;
> > > > + int len = sizeof *in_dhcp_opt + in_dhcp_opt->len;
> > > > + struct dhcp_opt_header *next_dhcp_opt =
> > > > + (struct dhcp_opt_header *)(ptr + len);
> > > > +
> > > > + if (next_dhcp_opt->code == DHCP_OPT_BOOTFILE_ALT_CODE) {
> > > > + if (!ipxe_req) {
> > > > + ofpbuf_pull(reply_dhcp_opts_ptr, len);
> > > > + next_dhcp_opt->code = DHCP_OPT_BOOTFILE_CODE;
> > > > + } else {
> > > > + char *buf = xmalloc(len);
> > > >
> > > > - memcpy(buf, in_dhcp_opt, len);
> > > > - ofpbuf_pull(reply_dhcp_opts_ptr,
> > > > - sizeof *in_dhcp_opt + next_dhcp_opt->len);
> > > > - memcpy(reply_dhcp_opts_ptr->data, buf, len);
> > > > - free(buf);
> > > > + memcpy(buf, in_dhcp_opt, len);
> > > > + ofpbuf_pull(reply_dhcp_opts_ptr,
> > > > + sizeof *in_dhcp_opt +
> > > > next_dhcp_opt->len);
> > > > + memcpy(reply_dhcp_opts_ptr->data, buf, len);
> > > > + free(buf);
> > > > + }
> > > > + }
> > > > + bootfile_name_set = true;
> > > > + break;
> > > > + case DHCP_OPT_BOOTFILE_ALT_CODE:
> > > > + if (!bootfile_name_set) {
> > > > + in_dhcp_opt->code = DHCP_OPT_BOOTFILE_CODE;
> > > > }
> > > > + break;
> > > > + }
> > > > +
> > > > + in_dhcp_ptr += sizeof *in_dhcp_opt;
> > > > + if (in_dhcp_ptr > end) {
> > > > + break;
> > > > + }
> > > > + in_dhcp_ptr += in_dhcp_opt->len;
> > > > + if (in_dhcp_ptr > end) {
> > > > + break;
> > > > }
> > > > - } else if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_ALT_CODE) {
> > > > - in_dhcp_opt->code = DHCP_OPT_BOOTFILE_CODE;
> > > > }
> > > >
> > > > uint16_t new_l4_size = UDP_HEADER_LEN + DHCP_HEADER_LEN + 16;
> > > > @@ -2259,6 +2285,7 @@ pinctrl_handle_put_dhcp_opts(
> > > >
> > > > if (*in_dhcp_msg_type != OVN_DHCP_MSG_INFORM) {
> > > > dhcp_data->yiaddr = (msg_type == DHCP_MSG_NAK) ? 0 : *offer_ip;
> > > > + dhcp_data->siaddr = (msg_type == DHCP_MSG_NAK) ? 0 :
> > > > next_server;
> > > > } else {
> > > > dhcp_data->yiaddr = 0;
> > > > }
> > > > diff --git a/lib/actions.c b/lib/actions.c
> > > > index a3cf747db..11b349368 100644
> > > > --- a/lib/actions.c
> > > > +++ b/lib/actions.c
> > > > @@ -2862,10 +2862,21 @@ encode_PUT_DHCPV4_OPTS(const struct
> > > > ovnact_put_opts *pdo,
> > > > opt_header[1] = strlen(c->string);
> > > > ofpbuf_put(ofpacts, c->string, opt_header[1]);
> > > > }
> > > > + /* Encode next_server opt (253) */
> > > > + const struct ovnact_gen_option *next_server_opt = find_opt(
> > > > + pdo->options, pdo->n_options, DHCP_OPT_NEXT_SERVER_CODE);
> > > > + if (next_server_opt) {
> > > > + uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> > > > + const union expr_constant *c = next_server_opt->value.values;
> > > > + opt_header[0] = next_server_opt->option->code;
> > > > + opt_header[1] = sizeof(ovs_be32);
> > > > + ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
> > > > + }
> > > >
> > > > for (size_t i = 0; i < pdo->n_options; i++) {
> > > > const struct ovnact_gen_option *o = &pdo->options[i];
> > > > - if (o != offerip_opt && o != boot_opt && o != boot_alt_opt) {
> > > > + if (o != offerip_opt && o != boot_opt && o != boot_alt_opt && \
> > > > + o != next_server_opt) {
> > > > encode_put_dhcpv4_option(o, ofpacts);
> > > > }
> > > > }
> > > > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
> > > > index 49ecea81f..0b2da9f7b 100644
> > > > --- a/lib/ovn-l7.h
> > > > +++ b/lib/ovn-l7.h
> > > > @@ -145,6 +145,14 @@ struct gen_opts_map {
> > > > #define DHCP_OPT_TCP_KEEPALIVE_INTERVAL \
> > > > DHCP_OPTION("tcp_keepalive_interval", 38, "uint32")
> > > >
> > > > +/* Use unused 253 option for DHCP next-server DHCP option.
> > > > + * This option is used for setting the "Next server IP address"
> > > > + * field in the DHCP header.
> > > > + */
> > > > +#define DHCP_OPT_NEXT_SERVER_CODE 253
> > > > +#define DHCP_OPT_NEXT_SERVER \
> > > > + DHCP_OPTION("next_server", DHCP_OPT_NEXT_SERVER_CODE, "ipv4")
> > > > +
> > > > static inline uint32_t
> > > > gen_opt_hash(char *opt_name)
> > > > {
> > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > > index 0a0f85010..6e01679e1 100644
> > > > --- a/northd/ovn-northd.c
> > > > +++ b/northd/ovn-northd.c
> > > > @@ -246,6 +246,7 @@ static struct gen_opts_map supported_dhcp_opts[] = {
> > > > DHCP_OPT_BROADCAST_ADDRESS,
> > > > DHCP_OPT_NETBIOS_NAME_SERVER,
> > > > DHCP_OPT_NETBIOS_NODE_TYPE,
> > > > + DHCP_OPT_NEXT_SERVER,
> > > > };
> > > >
> > > > static struct gen_opts_map supported_dhcpv6_opts[] = {
> > > > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > > > index 9010240a8..a9c784865 100644
> > > > --- a/ovn-nb.xml
> > > > +++ b/ovn-nb.xml
> > > > @@ -3579,6 +3579,13 @@
> > > > </p>
> > > > </column>
> > > >
> > > > + <column name="options" key="next_server">
> > > > + <p>
> > > > + The DHCPv4 option code for setting the "Next server IP
> > > > + address" field in the DHCP header.
> > > > + </p>
> > > > + </column>
> > > > +
> > > > </group>
> > > >
> > > > <group title="Boolean DHCP Options">
> > > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > > index 799a6aabd..17a8b7d31 100644
> > > > --- a/tests/ovn.at
> > > > +++ b/tests/ovn.at
> > > > @@ -1438,9 +1438,9 @@ reg0[0] = lookup_arp_ip(inport, eth.dst);
> > > > # put_dhcp_opts
> > > > reg1[0] = put_dhcp_opts(offerip = 1.2.3.4, router = 10.0.0.1);
> > > > encodes as
> > > > controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.40.01.02.03.04.03.04.0a.00.00.01,pause)
> > > > -reg2[5] =
> > > > put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400,domain_name="ovn.org",wpad="https://example.org",bootfile_name="https://127.0.0.1/boot.ipxe",path_prefix="/tftpboot";);
> > > > - formats as reg2[5] = put_dhcp_opts(offerip = 10.0.0.4, router =
> > > > 10.0.0.1, netmask = 255.255.254.0, mtu = 1400, domain_name = "ovn.org",
> > > > wpad = "https://example.org";, bootfile_name =
> > > > "https://127.0.0.1/boot.ipxe";, path_prefix = "/tftpboot");
> > > > - encodes as
> > > > controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.25.0a.00.00.04.43.1b.68.74.74.70.73.3a.2f.2f.31.32.37.2e.30.2e.30.2e.31.2f.62.6f.6f.74.2e.69.70.78.65.03.04.0a.00.00.01.01.04.ff.ff.fe.00.1a.02.05.78.0f.07.6f.76.6e.2e.6f.72.67.fc.13.68.74.74.70.73.3a.2f.2f.65.78.61.6d.70.6c.65.2e.6f.72.67.d2.09.2f.74.66.74.70.62.6f.6f.74,pause)
> > > > +reg2[5] =
> > > > put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400,domain_name="ovn.org",wpad="https://example.org",bootfile_name="https://127.0.0.1/boot.ipxe",path_prefix="/tftpboot";,
> > > > next_server=10.0.0.9);
> > > > + formats as reg2[5] = put_dhcp_opts(offerip = 10.0.0.4, router =
> > > > 10.0.0.1, netmask = 255.255.254.0, mtu = 1400, domain_name = "ovn.org",
> > > > wpad = "https://example.org";, bootfile_name =
> > > > "https://127.0.0.1/boot.ipxe";, path_prefix = "/tftpboot", next_server =
> > > > 10.0.0.9);
> > > > + encodes as
> > > > controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.25.0a.00.00.04.43.1b.68.74.74.70.73.3a.2f.2f.31.32.37.2e.30.2e.30.2e.31.2f.62.6f.6f.74.2e.69.70.78.65.fd.04.0a.00.00.09.03.04.0a.00.00.01.01.04.ff.ff.fe.00.1a.02.05.78.0f.07.6f.76.6e.2e.6f.72.67.fc.13.68.74.74.70.73.3a.2f.2f.65.78.61.6d.70.6c.65.2e.6f.72.67.d2.09.2f.74.66.74.70.62.6f.6f.74,pause)
> > > > reg0[15] =
> > > > put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,mtu=1400,ip_forward_enable=1,default_ttl=121,dns_server={8.8.8.8,7.7.7.7},classless_static_route={30.0.0.0/24,10.0.0.4,40.0.0.0/16,10.0.0.6,0.0.0.0/0,10.0.0.1},ethernet_encap=1,router_discovery=0,tftp_server_address={10.0.0.4,10.0.0.5},arp_cache_timeout=10,tcp_keepalive_interval=10);
> > > > formats as reg0[15] = put_dhcp_opts(offerip = 10.0.0.4, router =
> > > > 10.0.0.1, netmask = 255.255.255.0, mtu = 1400, ip_forward_enable = 1,
> > > > default_ttl = 121, dns_server = {8.8.8.8, 7.7.7.7},
> > > > classless_static_route = {30.0.0.0/24, 10.0.0.4, 40.0.0.0/16, 10.0.0.6,
> > > > 0.0.0.0/0, 10.0.0.1}, ethernet_encap = 1, router_discovery = 0,
> > > > tftp_server_address = {10.0.0.4, 10.0.0.5}, arp_cache_timeout = 10,
> > > > tcp_keepalive_interval = 10);
> > > > encodes as
> > > > controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.6f.0a.00.00.04.03.04.0a.00.00.01.01.04.ff.ff.ff.00.1a.02.05.78.13.01.01.17.01.79.06.08.08.08.08.08.07.07.07.07.79.14.18.1e.00.00.0a.00.00.04.10.28.00.0a.00.00.06.00.0a.00.00.01.24.01.01.1f.01.00.96.08.0a.00.00.04.0a.00.00.05.23.04.00.00.00.0a.26.04.00.00.00.0a,pause)
> > > > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > > > index d79c6a5bc..844905797 100644
> > > > --- a/tests/test-ovn.c
> > > > +++ b/tests/test-ovn.c
> > > > @@ -196,6 +196,7 @@ create_gen_opts(struct hmap *dhcp_opts, struct hmap
> > > > *dhcpv6_opts,
> > > > dhcp_opt_add(dhcp_opts, "broadcast_address", 28, "ipv4");
> > > > dhcp_opt_add(dhcp_opts, "netbios_name_server", 44, "ipv4");
> > > > dhcp_opt_add(dhcp_opts, "netbios_node_type", 46, "uint8");
> > > > + dhcp_opt_add(dhcp_opts, "next_server", 253, "ipv4");
> > > >
> > > > /* DHCPv6 options. */
> > > > hmap_init(dhcpv6_opts);
> > > > --
> > > > 2.36.1
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > [email protected]
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > >
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
