[ovs-dev] [PATCH v7 ovn] Add support for DHCP domain search option (119)

Numan Siddique numans at ovn.org
Tue Jun 23 09:13:52 UTC 2020


On Mon, Jun 22, 2020 at 12:09 PM Numan Siddique <numans at ovn.org> wrote:

>
>
> On Fri, Jun 19, 2020 at 11:12 PM Ankur Sharma <svc.mail.git at nutanix.com>
> wrote:
>
>> From: Dhathri Purohith <dhathri.purohith at nutanix.com>
>>
>> Domain search list is encoded according to the specifications in RFC 1035,
>> section 4.1.4.
>>
>> Signed-off-by: Dhathri Purohith <dhathri.purohith at nutanix.com>
>> Signed-off-by: Ankur Sharma <ankur.sharma at nutanix.com>
>>
>
> I have asked a question in v5 of this patch about the encoding.
> Could you please reply to that ?
>
> Thanks
> Numan
>
>

Hi Dhatri,

I've few minor comments. Otherwise the patch LGTM.

I tested locally and it seems to work fine. I compared the encoding between
what this patch is doing
and dnsmasq. I found a little difference.

For example: for the domain_search_list - test.org,ovn.org,abc.ovn.org,
def.ovn.org,ghi.ovn.org

Your patch encodes it as :
77 22 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 c0 0a 03
64 65 66 c0 0a 03 67 68 69 c0 0a

But dnsmasq does encoding a little differently.
77 2e 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 03 6f 76
6e c0 05 03 64 65  66 03 6f 76 6e c0 05 03 67 68 69 03 6f 76 6e c0 05

Can you please check and verify once ?

I'm fine with your patch. But I just want to be sure.


---
>>  lib/actions.c       | 90
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  lib/ovn-l7.h        |  3 ++
>>  northd/ovn-northd.c |  1 +
>>  ovn-nb.xml          | 13 ++++++++
>>  ovn-sb.ovsschema    |  6 ++--
>>  ovn-sb.xml          | 11 +++++++
>>  tests/ovn.at        | 36 +++++++++++++++++++++
>>  tests/test-ovn.c    |  1 +
>>  8 files changed, 157 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/actions.c b/lib/actions.c
>> index 9baa90f..ad7ae78 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -1982,7 +1982,8 @@ parse_gen_opt(struct action_context *ctx, struct
>> ovnact_gen_option *o,
>>          return;
>>      }
>>
>> -    if (!strcmp(o->option->type, "str")) {
>> +    if (!strcmp(o->option->type, "str") ||
>> +        !strcmp(o->option->type, "domains")) {
>>          if (o->value.type != EXPR_C_STRING) {
>>              lexer_error(ctx->lexer, "%s option %s requires string
>> value.",
>>                          opts_type, o->option->name);
>> @@ -2317,6 +2318,93 @@ encode_put_dhcpv4_option(const struct
>> ovnact_gen_option *o,
>>             opt_header[1] = sizeof(ovs_be32);
>>             ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
>>          }
>> +    } else if (!strcmp(o->option->type, "domains")) {
>> +        /* Please refer to RFC 1035, section 4.1.4 for the format of
>> encoding
>> +         * domain names. Below is an example for encoding a search list
>> +         * consisting of the "abc.com" and "xyz.abc.com".
>> +         *
>> +         *
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
>> +         * |119|14 | 3 |'a'|'b'|'c'| 3 |'c'|'o'|'m'| 0
>> |'x'|'y'|'z'|xC0|x00|
>> +         *
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
>> +         *
>> +         * The encoding of "abc.com" ends with 0 to mark the end of the
>> +         * domain name as required by RFC 1035.
>> +         *
>> +         * The encoding of "xyz" (for "xyz.abc.com") ends with the
>> two-octet
>> +         * compression pointer C000 (hex), which points to offset 0 where
>> +         * another validly encoded domain name can be found to complete
>> +         * the name ("abc.com").
>> +         *
>> +         * Encoding adds 2 bytes (one for length and one for delimiter)
>> for
>> +         * every domain name that is unique. If all the domain names are
>> unique
>> +         * (which probably never happens in real world), then encoded
>> string
>> +         * could be longer than the original string. Just to be on the
>> safer
>> +         * side, allocate the (approx.) worst case length here.
>> +         */
>> +        uint8_t *dns_encoded = xzalloc(2 * strlen(c->string));
>> +        uint16_t encode_offset = 0;
>> +        struct shash label_offset_map;
>> +        shash_init(&label_offset_map);
>>
>
You can init shash during declaration as:
struct shash label_offset_map = SHASH_INITIALIZER(&label_offset_map);



> +        char *domain_list = xstrdup(c->string), *dom_ptr = NULL;
>> +        char *suffix = xzalloc(strlen(domain_list));
>> +        for (char *domain = strtok_r(domain_list, ",", &dom_ptr);
>> +             domain != NULL;
>> +             domain = strtok_r(NULL, ",", &dom_ptr)) {
>> +            if (strlen(domain) > DOMAIN_NAME_MAX_LEN) {
>> +                VLOG_WARN("Domain names longer than 255 characters are
>> not"
>> +                          "supported");
>> +                goto out;
>> +            }
>> +            strcpy(suffix, domain);
>>
>
checkpatch complains to use ovs_strlcpy. Please use that instead of strcpy.


> +            char *label;
>> +            for (label = strtok_r(domain, ".", &domain);
>> +                 label != NULL;
>> +                 label = strtok_r(NULL, ".", &domain)) {
>> +                /* Check if we have already encoded this suffix.
>> +                 * If yes, fill in the reference and break. */
>> +                uint16_t *get_offset;
>> +                get_offset  = shash_find_data(&label_offset_map, suffix);
>> +                if (get_offset != NULL) {
>> +                    ovs_be16 temp = htons(0xc000) | htons(*get_offset);
>> +                    memcpy(dns_encoded + encode_offset, &temp,
>> +                        sizeof(temp));
>> +                    encode_offset += sizeof(temp);
>> +                    break;
>> +                } else {
>> +                    /* The suffix was not encoded before, encode it now
>> +                     * and add the offset to the label_offset_map. */
>> +                    uint16_t *set_offset = xzalloc(sizeof(uint16_t));
>> +                    *set_offset = encode_offset;
>> +                    shash_add_once(&label_offset_map, suffix,
>> set_offset);
>> +
>> +                    uint8_t len = strlen(label);
>> +                    memcpy(dns_encoded + encode_offset, &len,
>> sizeof(uint8_t));
>> +                    encode_offset += sizeof(uint8_t);
>> +                    memcpy(dns_encoded + encode_offset, label, len);
>> +                    encode_offset += len;
>> +                }
>> +               strcpy(suffix, domain);
>>
>
Same here.



> +            }
>> +            /* Add the end marker (0 byte) to determine the end of the
>> +             * domain. */
>> +            if (label == NULL) {
>> +                uint8_t end = 0;
>> +                memcpy(dns_encoded + encode_offset, &end,
>> sizeof(uint8_t));
>> +                encode_offset += sizeof(uint8_t);
>> +            }
>> +        }
>> +        opt_header[1] = encode_offset;
>> +        ofpbuf_put(ofpacts, dns_encoded, encode_offset);
>> +
>> +        out:
>> +            free(suffix);
>> +            free(domain_list);
>> +            free(dns_encoded);
>> +            struct shash_node *node, *next;
>> +            SHASH_FOR_EACH_SAFE (node, next, &label_offset_map) {
>> +                shash_delete(&label_offset_map, node);
>> +            }
>> +            shash_destroy(&label_offset_map);
>>
>
I think there is a memory leak here since shash_delete doesn't free the
shash data which you allocated for 'set_offset'.

I think instead of using  SHASH_FOR_EACH_SAFE and then
calling shash_destroy(),
you can just do shash_destroy_free_data().


     }
>>  }
>>
>> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
>> index 22a2153..cea97b9 100644
>> --- a/lib/ovn-l7.h
>> +++ b/lib/ovn-l7.h
>> @@ -34,6 +34,7 @@ struct gen_opts_map {
>>      size_t code;
>>  };
>>
>> +#define DOMAIN_NAME_MAX_LEN 255
>>  #define DHCP_BROADCAST_FLAG 0x8000
>>
>>  #define DHCP_OPTION(NAME, CODE, TYPE) \
>> @@ -81,6 +82,8 @@ struct gen_opts_map {
>>  #define DHCP_OPT_PATH_PREFIX DHCP_OPTION("path_prefix", 210, "str")
>>  #define DHCP_OPT_TFTP_SERVER_ADDRESS \
>>      DHCP_OPTION("tftp_server_address", 150, "ipv4")
>> +#define DHCP_OPT_DOMAIN_SEARCH_LIST \
>> +    DHCP_OPTION("domain_search_list", 119, "domains")
>>
>>  #define DHCP_OPT_ARP_CACHE_TIMEOUT \
>>      DHCP_OPTION("arp_cache_timeout", 35, "uint32")
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index 6a9b097..22891c1 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -11345,6 +11345,7 @@ static struct gen_opts_map supported_dhcp_opts[]
>> = {
>>      DHCP_OPT_DOMAIN_NAME,
>>      DHCP_OPT_ARP_CACHE_TIMEOUT,
>>      DHCP_OPT_TCP_KEEPALIVE_INTERVAL,
>> +    DHCP_OPT_DOMAIN_SEARCH_LIST,
>>  };
>>
>>  static struct gen_opts_map supported_dhcpv6_opts[] = {
>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>> index 8368d51..80e843c 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -2950,6 +2950,19 @@
>>            </p>
>>          </column>
>>        </group>
>> +
>> +      <group title=" DHCP Options of type domains">
>> +        <p>
>> +          These options accept string value which is a comma separated
>> +          list of domain names. The domain names are encoded based on
>> RFC 1035.
>> +        </p>
>> +
>> +        <column name="options" key="domain_search_list">
>> +          <p>
>> +            The DHCPv4 option code for this option is 119.
>> +          </p>
>> +        </column>
>> +      </group>
>>      </group>
>>
>>      <group title="DHCPv6 options">
>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>> index 2ec729b..99c5de8 100644
>> --- a/ovn-sb.ovsschema
>> +++ b/ovn-sb.ovsschema
>> @@ -1,7 +1,7 @@
>>  {
>>      "name": "OVN_Southbound",
>> -    "version": "2.8.1",
>> -    "cksum": "236203406 21905",
>> +    "version": "2.8.2",
>> +    "cksum": "464326363 21916",
>>      "tables": {
>>          "SB_Global": {
>>              "columns": {
>> @@ -218,7 +218,7 @@
>>                          "type": "string",
>>                          "enum": ["set", ["bool", "uint8", "uint16",
>> "uint32",
>>                                           "ipv4", "static_routes", "str",
>> -                                         "host_id"]]}}}},
>> +                                         "host_id", "domains"]]}}}},
>>              "isRoot": true},
>>          "DHCPv6_Options": {
>>              "columns": {
>> diff --git a/ovn-sb.xml b/ovn-sb.xml
>> index 208fb93..a6da80b 100644
>> --- a/ovn-sb.xml
>> +++ b/ovn-sb.xml
>> @@ -3232,6 +3232,17 @@ tcp.flags = RST;
>>            </p>
>>          </dd>
>>
>> +        <dt><code>value: domains</code></dt>
>> +        <dd>
>> +          <p>
>> +            This indicates that the value of the DHCP option is a domain
>> name
>> +            or a comma separated list of domain names.
>> +          </p>
>> +
>> +          <p>
>> +            Example. "name=domain_search_list", "code=119",
>> "type=domains".
>> +          </p>
>> +        </dd>
>>        </dl>
>>      </column>
>>    </table>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 7622b74..6093991 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -1218,6 +1218,9 @@ reg0[15] =
>> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,
>>  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="tftp_server_test");
>>      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 = "tftp_server_test");
>>      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.42.10.74.66.74.70.5f.73.65.72.76.65.72.5f.74.65.73.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",domain_search_list="
>> ovn.org,abc.ovn.org");
>> +    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",
>> domain_search_list = "ovn.org,abc.ovn.org");
>> +    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.77.0f.03.6f.76.6e.03.6f.72.67.00.03.61.62.63.c0.00,pause)
>>
>
Can you add a few more cases here ? Like the above example I gave ?

Thanks
Numan


>
>>  reg1[0..1] = put_dhcp_opts(offerip = 1.2.3.4, router = 10.0.0.1);
>>      Cannot use 2-bit field reg1[0..1] where 1-bit field is required.
>> @@ -1235,6 +1238,8 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy");
>>      DHCPv4 option offerip requires numeric value.
>>  reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4);
>>      DHCPv4 option domain_name requires string value.
>> +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4);
>> +    DHCPv4 option domain_search_list requires string value.
>>
>>  # nd_ns
>>  nd_ns { nd.target = xxreg0; output; };
>> @@ -5614,6 +5619,37 @@ AT_CHECK([cat 2.packets | cut -c -48], [0],
>> [expout])
>>  cat 2.expected | cut -c 53- > expout
>>  AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout])
>>
>> +reset_pcap_file hv1-vif1 hv1/vif1
>> +reset_pcap_file hv1-vif2 hv1/vif2
>> +rm -f 1.expected
>> +rm -f 2.expected
>> +
>> +# Set domain search list option for ls1
>> +echo "------ Set domain search list --------"
>> +ovn-nbctl dhcp-options-set-options $d1 server_id=10.0.0.1 \
>> +server_mac=ff:10:00:00:00:01 lease_time=3600 router=10.0.0.1 \
>> +domain_search_list=\"test1.com,test2.com\"
>> +echo "------------------------------------------"
>> +
>> +# Send DHCPREQUEST in the SELECTING/INIT-REBOOT state with the offered IP
>> +# address in the Requested IP Address option.
>> +offer_ip=`ip_to_hex 10 0 0 6`
>> +server_ip=`ip_to_hex 10 0 0 1`
>> +ciaddr=`ip_to_hex 0 0 0 0`
>> +request_ip=$offer_ip
>>
>> +expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001
>> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0
>> ff1000000001 $server_ip 05 $expected_dhcp_opts
>> +
>> +# NXT_RESUMEs should be 12.
>> +OVS_WAIT_UNTIL([test 12 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
>> +
>> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
>> +cat 2.expected | cut -c -48 > expout
>> +AT_CHECK([cat 2.packets | cut -c -48], [0], [expout])
>> +# Skipping the IPv4 checksum.
>> +cat 2.expected | cut -c 53- > expout
>> +AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout])
>> +
>>  OVN_CLEANUP([hv1])
>>
>>  AT_CLEANUP
>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>> index 4391694..b43f67f 100644
>> --- a/tests/test-ovn.c
>> +++ b/tests/test-ovn.c
>> @@ -189,6 +189,7 @@ create_gen_opts(struct hmap *dhcp_opts, struct hmap
>> *dhcpv6_opts,
>>      dhcp_opt_add(dhcp_opts, "tftp_server_address", 150, "ipv4");
>>      dhcp_opt_add(dhcp_opts, "arp_cache_timeout", 35, "uint32");
>>      dhcp_opt_add(dhcp_opts, "tcp_keepalive_interval", 38, "uint32");
>> +    dhcp_opt_add(dhcp_opts, "domain_search_list", 119, "domains");
>>
>>      /* DHCPv6 options. */
>>      hmap_init(dhcpv6_opts);
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>


More information about the dev mailing list