On Tue, Jun 23, 2020 at 7:36 PM Mark Michelson <[email protected]> wrote:
> On 6/23/20 5:13 AM, Numan Siddique wrote: > > On Mon, Jun 22, 2020 at 12:09 PM Numan Siddique <[email protected]> wrote: > > > >> > >> > >> On Fri, Jun 19, 2020 at 11:12 PM Ankur Sharma <[email protected] > > > >> wrote: > >> > >>> From: Dhathri Purohith <[email protected]> > >>> > >>> Domain search list is encoded according to the specifications in RFC > 1035, > >>> section 4.1.4. > >>> > >>> Signed-off-by: Dhathri Purohith <[email protected]> > >>> Signed-off-by: Ankur Sharma <[email protected]> > >>> > >> > >> 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 ? > > > Hi Numan, they both appear to be correct to me. dnsmasq uses fewer > domain compression pointers than this patch does. dnsmasq compresses > every occurrence of ".org" after the first into "c0 05". However, it > repeats the explicit use of "ovn" throughout. > > This patch takes it a step further by encoding ".org" into "c0 05" but > also encoding "ovn.org" into "c0 0a" (which itself compresses the ".org" > into "c0 05"). This results in a smaller message that still decodes to > the same value. > > Thanks Mark for verifying it further. I was just making sure that the present patch is correct taking dnsmasq as reference. Thanks Numan > > 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 > >>> [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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
