diff mbox series

[ovs-dev] Add support for DHCP option 15 - domain name

Message ID 20190521121303.2286-1-mjozefcz@redhat.com
State Superseded
Headers show
Series [ovs-dev] Add support for DHCP option 15 - domain name | expand

Commit Message

Maciej Jozefczyk May 21, 2019, 12:13 p.m. UTC 
From: Maciej Józefczyk <mjozefcz@redhat.com>

For Openstack Internal DNS functionality we need
to provide support for domain_name option.

Signed-off-by: Maciej Józefczyk <mjozefcz@redhat.com>
---
 ovn/lib/ovn-l7.h                   | 1 +
 ovn/northd/ovn-northd.c            | 1 +
 ovn/ovn-nb.xml                     | 8 ++++++++
 tests/oss-fuzz/expr_parse_target.c | 2 +-
 tests/ovn.at                       | 8 ++++----
 tests/test-ovn.c                   | 2 +-
 6 files changed, 16 insertions(+), 6 deletions(-)

Comments

Numan Siddique May 21, 2019, 6:07 p.m. UTC | #1 
On Tue, May 21, 2019 at 5:42 PM <mjozefcz@redhat.com> wrote:

> From: Maciej Józefczyk <mjozefcz@redhat.com>
>
> For Openstack Internal DNS functionality we need
> to provide support for domain_name option.
>
> Signed-off-by: Maciej Józefczyk <mjozefcz@redhat.com>
>

Acked-by: Numan Siddique <nusiddiq@redhat.com>


> ---
>  ovn/lib/ovn-l7.h                   | 1 +
>  ovn/northd/ovn-northd.c            | 1 +
>  ovn/ovn-nb.xml                     | 8 ++++++++
>  tests/oss-fuzz/expr_parse_target.c | 2 +-
>  tests/ovn.at                       | 8 ++++----
>  tests/test-ovn.c                   | 2 +-
>  6 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/ovn/lib/ovn-l7.h b/ovn/lib/ovn-l7.h
> index c24201ef0..362f537da 100644
> --- a/ovn/lib/ovn-l7.h
> +++ b/ovn/lib/ovn-l7.h
> @@ -40,6 +40,7 @@ struct gen_opts_map {
>  #define DHCP_OPT_DNS_SERVER  DHCP_OPTION("dns_server", 6, "ipv4")
>  #define DHCP_OPT_LOG_SERVER  DHCP_OPTION("log_server", 7, "ipv4")
>  #define DHCP_OPT_LPR_SERVER  DHCP_OPTION("lpr_server", 9, "ipv4")
> +#define DHCP_OPT_DOMAIN_NAME DHCP_OPTION("domain_name", 15, "str")
>  #define DHCP_OPT_SWAP_SERVER DHCP_OPTION("swap_server", 16, "ipv4")
>
>  #define DHCP_OPT_POLICY_FILTER \
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index de0c06d4b..0b0a96a3a 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -8226,6 +8226,7 @@ static struct gen_opts_map supported_dhcp_opts[] = {
>      DHCP_OPT_BOOTFILE,
>      DHCP_OPT_PATH_PREFIX,
>      DHCP_OPT_TFTP_SERVER_ADDRESS,
> +    DHCP_OPT_DOMAIN_NAME,
>  };
>
>  static struct gen_opts_map supported_dhcpv6_opts[] = {
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index cbaa9495f..318379c1f 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -2355,6 +2355,14 @@
>              that matches with this requirement is option 66 (tftp_server).
>            </p>
>          </column>
> +
> +        <column name="options" key="domain_name">
> +          <p>
> +            The DHCPv4 option code for this option is 15. This option
> +            specifies the domain name that client should use when
> +            resolving hostnames via the Domain Name System.
> +          </p>
> +        </column>
>        </group>
>      </group>
>
> diff --git a/tests/oss-fuzz/expr_parse_target.c
> b/tests/oss-fuzz/expr_parse_target.c
> index 1fdd3895b..e400e52c0 100644
> --- a/tests/oss-fuzz/expr_parse_target.c
> +++ b/tests/oss-fuzz/expr_parse_target.c
> @@ -122,7 +122,7 @@ create_gen_opts(struct hmap *dhcp_opts, struct hmap
> *dhcpv6_opts,
>      dhcp_opt_add(dhcp_opts, "dns_server", 6, "ipv4");
>      dhcp_opt_add(dhcp_opts, "log_server", 7, "ipv4");
>      dhcp_opt_add(dhcp_opts, "lpr_server",  9, "ipv4");
> -    dhcp_opt_add(dhcp_opts, "domain", 15, "str");
> +    dhcp_opt_add(dhcp_opts, "domain_name", 15, "str");
>      dhcp_opt_add(dhcp_opts, "swap_server", 16, "ipv4");
>      dhcp_opt_add(dhcp_opts, "policy_filter", 21, "ipv4");
>      dhcp_opt_add(dhcp_opts, "router_solicitation",  32, "ipv4");
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 6499df3d3..0a08aad17 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1104,8 +1104,8 @@ put_arp(inport, arp.spa, arp.sha);
>  # 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="
> 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 = "ovn.org", wpad =
> "https://example.org", bootfile_name = "https://127.0.0.1/boot.ipxe",
> path_prefix = "/tftpboot");
> +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.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.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.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});
>      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});
> @@ -1125,8 +1125,8 @@ reg1[0] = put_dhcp_opts(offerip=1.2.3.4, xyzzy);
>      Syntax error at `xyzzy' expecting DHCPv4 option name.
>  reg1[0] = put_dhcp_opts(offerip="xyzzy");
>      DHCPv4 option offerip requires numeric value.
> -reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain=1.2.3.4);
> -    DHCPv4 option domain requires string value.
> +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4);
> +    DHCPv4 option domain_name requires string value.
>
>  # nd_ns
>  nd_ns { nd.target = xxreg0; output; };
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 7cce9c2ae..23feec730 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -166,7 +166,7 @@ create_gen_opts(struct hmap *dhcp_opts, struct hmap
> *dhcpv6_opts,
>      dhcp_opt_add(dhcp_opts, "dns_server", 6, "ipv4");
>      dhcp_opt_add(dhcp_opts, "log_server", 7, "ipv4");
>      dhcp_opt_add(dhcp_opts, "lpr_server",  9, "ipv4");
> -    dhcp_opt_add(dhcp_opts, "domain", 15, "str");
> +    dhcp_opt_add(dhcp_opts, "domain_name", 15, "str");
>      dhcp_opt_add(dhcp_opts, "swap_server", 16, "ipv4");
>      dhcp_opt_add(dhcp_opts, "policy_filter", 21, "ipv4");
>      dhcp_opt_add(dhcp_opts, "router_solicitation",  32, "ipv4");
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff May 24, 2019, 4:55 p.m. UTC | #2 
On Tue, May 21, 2019 at 02:13:03PM +0200, mjozefcz@redhat.com wrote:
> From: Maciej Józefczyk <mjozefcz@redhat.com>
> 
> For Openstack Internal DNS functionality we need
> to provide support for domain_name option.
> 
> Signed-off-by: Maciej Józefczyk <mjozefcz@redhat.com>

This seems to be a little different from just adding support for a new
DHCP option, because in some places it's renaming an existing "domain"
option to "domain_name".  That may introduce a cross-version
compatibility issue.  If it does, then it would be better to retain the
existing name.

What's the full story?

Thanks,

Ben.
Numan Siddique May 24, 2019, 5:15 p.m. UTC | #3 
On Fri, May 24, 2019 at 10:26 PM Ben Pfaff <blp@ovn.org> wrote:

> On Tue, May 21, 2019 at 02:13:03PM +0200, mjozefcz@redhat.com wrote:
> > From: Maciej Józefczyk <mjozefcz@redhat.com>
> >
> > For Openstack Internal DNS functionality we need
> > to provide support for domain_name option.
> >
> > Signed-off-by: Maciej Józefczyk <mjozefcz@redhat.com>
>
> This seems to be a little different from just adding support for a new
> DHCP option, because in some places it's renaming an existing "domain"
> option to "domain_name".  That may introduce a cross-version
> compatibility issue.  If it does, then it would be better to retain the
> existing name.
>
> What's the full story?
>

Oops. I never noticed the test code deleting the option - "domain" and
adding "domain_name".

I looked into the code now and this is what has happened. We never
supported the dhcp
option 15 (domain_name) earlier and this patch adds the support for this
option.
But the initial commit [1] which added the DHCP support in OVN has added
the option 15 with the
name - "domain" in the test-ovn.c. I think it was a mistake from my side to
add that option in the
test code even though we never supported that option. My mistake. I think I
might have added that
option in the test code (but not in the actual list) is probably to test
the action - put_dhcp_opts
with an option of type "str".

[1] -
https://github.com/openvswitch/ovs/commit/42814145d70c77462ce28b38841cd160f0486776#diff-e3335c87e2c9ad67b3d403e4f4361771R253

Thanks
Numan


> Thanks,
>
> Ben.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Maciej Jozefczyk May 27, 2019, 9:01 a.m. UTC | #4 
Hello!

>> What's the full story?

I found that option not used in code. It was only used in parser tests, but
with the same flag number - 15 as domain_name.
https://tools.ietf.org/html/rfc2132
As per RFC I decided to rename it in tests and add its support to the code.

Should I add some note to the commit message even the code itself was not
changed?

Thanks!
Maciej

On Fri, May 24, 2019 at 7:21 PM Numan Siddique <nusiddiq@redhat.com> wrote:

>
>
> On Fri, May 24, 2019 at 10:26 PM Ben Pfaff <blp@ovn.org> wrote:
>
>> On Tue, May 21, 2019 at 02:13:03PM +0200, mjozefcz@redhat.com wrote:
>> > From: Maciej Józefczyk <mjozefcz@redhat.com>
>> >
>> > For Openstack Internal DNS functionality we need
>> > to provide support for domain_name option.
>> >
>> > Signed-off-by: Maciej Józefczyk <mjozefcz@redhat.com>
>>
>> This seems to be a little different from just adding support for a new
>> DHCP option, because in some places it's renaming an existing "domain"
>> option to "domain_name".  That may introduce a cross-version
>> compatibility issue.  If it does, then it would be better to retain the
>> existing name.
>>
>> What's the full story?
>>
>
> Oops. I never noticed the test code deleting the option - "domain" and
> adding "domain_name".
>
> I looked into the code now and this is what has happened. We never
> supported the dhcp
> option 15 (domain_name) earlier and this patch adds the support for this
> option.
> But the initial commit [1] which added the DHCP support in OVN has added
> the option 15 with the
> name - "domain" in the test-ovn.c. I think it was a mistake from my side
> to add that option in the
> test code even though we never supported that option. My mistake. I think
> I might have added that
> option in the test code (but not in the actual list) is probably to test
> the action - put_dhcp_opts
> with an option of type "str".
>
> [1] -
> https://github.com/openvswitch/ovs/commit/42814145d70c77462ce28b38841cd160f0486776#diff-e3335c87e2c9ad67b3d403e4f4361771R253
>
> Thanks
> Numan
>
>
>> Thanks,
>>
>> Ben.
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
diff mbox series

Patch

diff --git a/ovn/lib/ovn-l7.h b/ovn/lib/ovn-l7.h
index c24201ef0..362f537da 100644
--- a/ovn/lib/ovn-l7.h
+++ b/ovn/lib/ovn-l7.h
@@ -40,6 +40,7 @@  struct gen_opts_map {
 #define DHCP_OPT_DNS_SERVER  DHCP_OPTION("dns_server", 6, "ipv4")
 #define DHCP_OPT_LOG_SERVER  DHCP_OPTION("log_server", 7, "ipv4")
 #define DHCP_OPT_LPR_SERVER  DHCP_OPTION("lpr_server", 9, "ipv4")
+#define DHCP_OPT_DOMAIN_NAME DHCP_OPTION("domain_name", 15, "str")
 #define DHCP_OPT_SWAP_SERVER DHCP_OPTION("swap_server", 16, "ipv4")
 
 #define DHCP_OPT_POLICY_FILTER \
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index de0c06d4b..0b0a96a3a 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -8226,6 +8226,7 @@  static struct gen_opts_map supported_dhcp_opts[] = {
     DHCP_OPT_BOOTFILE,
     DHCP_OPT_PATH_PREFIX,
     DHCP_OPT_TFTP_SERVER_ADDRESS,
+    DHCP_OPT_DOMAIN_NAME,
 };
 
 static struct gen_opts_map supported_dhcpv6_opts[] = {
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index cbaa9495f..318379c1f 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -2355,6 +2355,14 @@ 
             that matches with this requirement is option 66 (tftp_server).
           </p>
         </column>
+
+        <column name="options" key="domain_name">
+          <p>
+            The DHCPv4 option code for this option is 15. This option
+            specifies the domain name that client should use when
+            resolving hostnames via the Domain Name System.
+          </p>
+        </column>
       </group>
     </group>
 
diff --git a/tests/oss-fuzz/expr_parse_target.c b/tests/oss-fuzz/expr_parse_target.c
index 1fdd3895b..e400e52c0 100644
--- a/tests/oss-fuzz/expr_parse_target.c
+++ b/tests/oss-fuzz/expr_parse_target.c
@@ -122,7 +122,7 @@  create_gen_opts(struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
     dhcp_opt_add(dhcp_opts, "dns_server", 6, "ipv4");
     dhcp_opt_add(dhcp_opts, "log_server", 7, "ipv4");
     dhcp_opt_add(dhcp_opts, "lpr_server",  9, "ipv4");
-    dhcp_opt_add(dhcp_opts, "domain", 15, "str");
+    dhcp_opt_add(dhcp_opts, "domain_name", 15, "str");
     dhcp_opt_add(dhcp_opts, "swap_server", 16, "ipv4");
     dhcp_opt_add(dhcp_opts, "policy_filter", 21, "ipv4");
     dhcp_opt_add(dhcp_opts, "router_solicitation",  32, "ipv4");
diff --git a/tests/ovn.at b/tests/ovn.at
index 6499df3d3..0a08aad17 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1104,8 +1104,8 @@  put_arp(inport, arp.spa, arp.sha);
 # 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="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 = "ovn.org", wpad = "https://example.org", bootfile_name = "https://127.0.0.1/boot.ipxe", path_prefix = "/tftpboot");
+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.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.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.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});
     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});
@@ -1125,8 +1125,8 @@  reg1[0] = put_dhcp_opts(offerip=1.2.3.4, xyzzy);
     Syntax error at `xyzzy' expecting DHCPv4 option name.
 reg1[0] = put_dhcp_opts(offerip="xyzzy");
     DHCPv4 option offerip requires numeric value.
-reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain=1.2.3.4);
-    DHCPv4 option domain requires string value.
+reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4);
+    DHCPv4 option domain_name requires string value.
 
 # nd_ns
 nd_ns { nd.target = xxreg0; output; };
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 7cce9c2ae..23feec730 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -166,7 +166,7 @@  create_gen_opts(struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
     dhcp_opt_add(dhcp_opts, "dns_server", 6, "ipv4");
     dhcp_opt_add(dhcp_opts, "log_server", 7, "ipv4");
     dhcp_opt_add(dhcp_opts, "lpr_server",  9, "ipv4");
-    dhcp_opt_add(dhcp_opts, "domain", 15, "str");
+    dhcp_opt_add(dhcp_opts, "domain_name", 15, "str");
     dhcp_opt_add(dhcp_opts, "swap_server", 16, "ipv4");
     dhcp_opt_add(dhcp_opts, "policy_filter", 21, "ipv4");
     dhcp_opt_add(dhcp_opts, "router_solicitation",  32, "ipv4");