On 10/1/23 21:27, Mohammad Heib wrote:
> Currently OVN allows users to create DNS records
> and define domains within these records.
>
> These domains can be associated with IPV4 or IPv6
> or both, when the user creates a domain with both
> IPv4 and IPv6 ovn will answer each query for this
> domain immediately and everything works as expected.
>
> But if the user only creates a domain with only IPv4
> or IPv6 this will cause the DNS queries respond take
> longer than the usual since OVN will forward query and
> user will keep waiting for an answer until someone else
> replies for this query or timeout occur.
>
> The above behavior is a bit problematic if the user knows
> that this domain is only configured in OVN we should not
> forward queries for this domain to the outside.
>
> This patch adds an option:ovn-owned for the DNS table
> that can be set to "true" when creating a DNS.
>
> When setting this option to "true" all the domains within
> this table will be treated as local domains only,
> and queries for domains within this table that don't have
> an accurate IP will be refused immediately to save the time
> of waiting for timeout.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1946662
> Signed-off-by: Mohammad Heib <mh...@redhat.com>
> ---
Hi Mohammad,
Thanks for the patch. It looks OK overall, I have a few remark inline.
> NEWS | 5 ++++
> controller/pinctrl.c | 47 ++++++++++++++++++++++++++++++--
> northd/northd.c | 11 ++++++++
> ovn-nb.ovsschema | 9 ++++--
> ovn-nb.xml | 14 ++++++++++
> ovn-sb.ovsschema | 9 ++++--
> ovn-sb.xml | 6 ++++
> tests/ovn.at | 65 ++++++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 160 insertions(+), 6 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 425dfe0a8..465649430 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,10 @@
> Post v23.09.0
> -------------
> + - DNS now have an "options" column for configuration of extra options.
> + - A new DNS option "ovn-owned" has been added to allow defining domains
> + that are owned only by ovn, queries for that domain will not be processed
> + externally.
> +
>
> OVN v23.09.0 - 15 Sep 2023
> --------------------------
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index ff5a3444c..e1e558595 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -179,6 +179,7 @@ struct pinctrl {
> struct latch pinctrl_thread_exit;
> bool mac_binding_can_timestamp;
> bool fdb_can_timestamp;
> + bool dns_can_ovn_owned;
I think I'd call this "dns_supports_ovn_owned".
> };
>
> static struct pinctrl pinctrl;
> @@ -2709,6 +2710,7 @@ struct dns_data {
> uint64_t *dps;
> size_t n_dps;
> struct smap records;
> + struct smap options;
> bool delete;
> };
>
> @@ -2737,6 +2739,7 @@ sync_dns_cache(const struct sbrec_dns_table *dns_table)
> if (!dns_data) {
> dns_data = xmalloc(sizeof *dns_data);
> smap_init(&dns_data->records);
> + smap_init(&dns_data->options);
> shash_add(&dns_cache, dns_id, dns_data);
> dns_data->n_dps = 0;
> dns_data->dps = NULL;
> @@ -2751,6 +2754,12 @@ sync_dns_cache(const struct sbrec_dns_table *dns_table)
> smap_clone(&dns_data->records, &sbrec_dns->records);
> }
>
> + if (pinctrl.dns_can_ovn_owned
> + && !smap_equal(&dns_data->options, &sbrec_dns->options)) {
> + smap_destroy(&dns_data->options);
> + smap_clone(&dns_data->options, &sbrec_dns->options);
> + }
> +
> dns_data->n_dps = sbrec_dns->n_datapaths;
> dns_data->dps = xcalloc(dns_data->n_dps, sizeof(uint64_t));
> for (size_t i = 0; i < sbrec_dns->n_datapaths; i++) {
> @@ -2763,6 +2772,7 @@ sync_dns_cache(const struct sbrec_dns_table *dns_table)
> if (d->delete) {
> shash_delete(&dns_cache, iter);
> smap_destroy(&d->records);
> + smap_destroy(&d->options);
> free(d->dps);
> free(d);
> }
> @@ -2777,6 +2787,7 @@ destroy_dns_cache(void)
> struct dns_data *d = iter->data;
> shash_delete(&dns_cache, iter);
> smap_destroy(&d->records);
> + smap_destroy(&d->options);
> free(d->dps);
> free(d);
> }
> @@ -2850,6 +2861,8 @@ dns_build_ptr_answer(
> free(encoded);
> }
>
> +#define DNS_RCODE_SERVER_REFUSE 0x5
> +
> /* Called with in the pinctrl_handler thread context. */
> static void
> pinctrl_handle_dns_lookup(
> @@ -2863,6 +2876,7 @@ pinctrl_handle_dns_lookup(
> enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
> struct dp_packet *pkt_out_ptr = NULL;
> uint32_t success = 0;
> + bool send_query_rejection = false;
Nit: I think I'd rename this to 'send_refuse'.
Nit: reverse xmas tree.
>
> /* Parse result field. */
> const struct mf_field *f;
> @@ -2962,6 +2976,7 @@ pinctrl_handle_dns_lookup(
>
> uint64_t dp_key = ntohll(pin->flow_metadata.flow.metadata);
> const char *answer_data = NULL;
> + bool ovn_owned = false;
> struct shash_node *iter;
> SHASH_FOR_EACH (iter, &dns_cache) {
> struct dns_data *d = iter->data;
> @@ -2974,6 +2989,7 @@ pinctrl_handle_dns_lookup(
> answer_data = smap_get(&d->records, query_name_lower);
> free(query_name_lower);
> if (answer_data) {
> + ovn_owned = smap_get_bool(&d->options, "ovn-owned",
> false);
> break;
> }
> }
> @@ -3020,10 +3036,22 @@ pinctrl_handle_dns_lookup(
> ancount++;
> }
> }
> +
> + /* DNS is configured with a record for this domain with
> + * an IPv4/IPV6 only, so instead of ignoring this A/AAAA query,
> + * we can reply with RCODE = 5 (server refuses) and that
> + * will speed up the DNS process by not letting the customer
> + * wait for a timeout.
> + */
> + if (ovn_owned && (query_type == DNS_QUERY_TYPE_AAAA ||
> + query_type == DNS_QUERY_TYPE_A) && !ancount) {
> + send_query_rejection = true;
> + }
> +
> destroy_lport_addresses(&ip_addrs);
> }
>
> - if (!ancount) {
> + if (!ancount && !send_query_rejection) {
> ofpbuf_uninit(&dns_answer);
> goto exit;
> }
> @@ -3058,13 +3086,19 @@ pinctrl_handle_dns_lookup(
>
> /* Set the answer RRs. */
> out_dns_header->ancount = htons(ancount);
> + if (send_query_rejection) {
> + /* set RCODE = 5 (server refuses). */
> + out_dns_header->hi_flag |= DNS_RCODE_SERVER_REFUSE;
> + }
> out_dns_header->arcount = 0;
>
> /* Copy the Query section. */
> dp_packet_put(&pkt_out, dp_packet_data(pkt_in), dp_packet_size(pkt_in));
>
> /* Copy the answer sections. */
> - dp_packet_put(&pkt_out, dns_answer.data, dns_answer.size);
> + if (!send_query_rejection) {
We don't really need the "if", right?
"dp_packet_put(&packet_out, foo, 0)" basically a no-op.
> + dp_packet_put(&pkt_out, dns_answer.data, dns_answer.size);
> + }
> ofpbuf_uninit(&dns_answer);
>
> out_udp->udp_len = htons(new_l4_size);
> @@ -3521,6 +3555,15 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
> *br_int_name)
> notify_pinctrl_handler();
> }
>
> + bool dns_can_ovn_owned = sbrec_server_has_dns_table_col_options(idl);
> + if (dns_can_ovn_owned != pinctrl.dns_can_ovn_owned) {
> + pinctrl.dns_can_ovn_owned = dns_can_ovn_owned;
> +
> + /* Notify pinctrl_handler that fdb timestamp column
> + * availability has changed. */
> + notify_pinctrl_handler();
> + }
> +
> ovs_mutex_unlock(&pinctrl_mutex);
> }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 528027d07..70a2922f2 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -17196,6 +17196,17 @@ sync_dns_entries(struct ovsdb_idl_txn *ovnsb_txn,
> free(dns_id);
> }
>
> +
> + /* Copy DNS options to SB*/
> + struct smap options;
> + smap_clone(&options, &dns_info->sb_dns->options);
> +
> + bool ovn_owned = smap_get_bool(&dns_info->nb_dns->options,
> + "ovn-owned", false);
> + smap_replace(&options, "ovn-owned",
> + ovn_owned? "true" : "false");
> + sbrec_dns_set_options(dns_info->sb_dns, &options);
> +
We leak "options" here, CI caught this:
https://github.com/ovsrobot/ovn/actions/runs/6372957589/job/17296298390#step:8:5644
> /* Set the datapaths and records. If nothing has changed, then
> * this will be a no-op.
> */
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index e103360ec..b2e0993e0 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> - "version": "7.1.0",
> - "cksum": "217362582 33949",
> + "version": "7.2.0",
> + "cksum": "1069338687 34162",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -560,6 +560,11 @@
> "value": "string",
> "min": 0,
> "max": "unlimited"}},
> + "options": {
> + "type": {"key": "string",
> + "value": "string",
> + "min": 0,
> + "max": "unlimited"}},
> "external_ids": {"type": {"key": "string",
> "value": "string",
> "min": 0,
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 1de0c3041..4aa8107a5 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -4542,6 +4542,20 @@ or
> <p><b>Example: </b> "4.0.0.10.in-addr.arpa" = "vm1.ovn.org"</p>
> </column>
>
> + <column name="options" key="ovn-owned">
> + If set to true, then the OVN will be the main responsible for
> + <code>DNS Records</code> within this Table.
> +
> + <p> A <code>DNS Table</code> with this option set to <code>true</code>
> + can be created for domains that the user needs to configure
> + locally and don't care about IPv6 only interested in IPv4 or
> + vice versa.
> +
> + This will let ovn send IPv4 DNS reply and reject/ignore IPv6
> + queries to save the waiting for a timeout on those uninteresting
> + queries.</p>
> + </column>
> +
> <column name="external_ids">
> See <em>External IDs</em> at the beginning of this document.
> </column>
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 7b20aa21b..b5a16d231 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> - "version": "20.27.4",
> - "cksum": "3194181501 30531",
> + "version": "20.28.0",
> + "cksum": "160983021 30744",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -363,6 +363,11 @@
> "refTable":
> "Datapath_Binding"},
> "min": 1,
> "max": "unlimited"}},
> + "options": {
> + "type": {"key": "string",
> + "value": "string",
> + "min": 0,
> + "max": "unlimited"}},
> "external_ids": {"type": {"key": "string",
> "value": "string",
> "min": 0,
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 46aedf973..55fcf59b2 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -4500,6 +4500,12 @@ tcp.flags = RST;
> in this column.
> </column>
>
> + <column name="options" key="ovn-owned">
> + This column indicates that all the <code>Domains</code> in this table
> + are owned by OVN, and all <code>DNS queries</code> for those domains
> + will be answered locally by either an IP address or
> + <code>DNS rejection</code>.
> +</column>
> <group title="Common Columns">
> <column name="external_ids">
> See <em>External IDs</em> at the beginning of this document.
> diff --git a/tests/ovn.at b/tests/ovn.at
> index dfe535f36..88357fd33 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -11302,6 +11302,71 @@ reset_pcap_file hv1-vif2 hv1/vif2
> rm -f 1.expected
> rm -f 2.expected
>
> +# send AAAA query for a server known domain that don't have
> +# any IPV6 address associated with this domain, and expected
> +# server refused DNS reply to save the sender time of waiting for timeout.
> +AS_BOX([Test IPv6 (AAAA records) NO timeout.])
> +# Add back the DNS options for ls1-lp1 without ipv6.
> +check ovn-nbctl remove DNS $DNS1 records vm1.ovn.org
> +check ovn-nbctl set DNS $DNS1 records:vm1.ovn.org="10.0.0.4"
> +check ovn-nbctl --wait=hv set DNS $DNS1 options:ovn-owned=true
> +ovn-sbctl list DNS > dns6
> +AT_CAPTURE_FILE([dns6])
> +ovn-sbctl dump-flows > sbflows6
> +AT_CAPTURE_FILE([sbflows6])
> +
> +set_dns_params vm1_ipv6_only
> +src_ip=`ip_to_hex 10 0 0 6`
> +dst_ip=`ip_to_hex 10 0 0 1`
> +dns_reply=1
> +test_dns 2 f00000000002 f000000000f0 $src_ip $dst_ip $dns_reply
> $dns_req_data $dns_resp_data
> +
> +# NXT_RESUMEs should be 13.
> +OVS_WAIT_UNTIL([test 13 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> +
> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
> +# dns hdr with server refuse RCODE
> +echo "01028125" > expout
> +#only check for the DNS HDR flags since we are not getting any DNS answer
> +AT_CHECK([cat 2.packets | cut -c -92 | cut -c 85-], [0], [expout])
> +
> +reset_pcap_file hv1-vif1 hv1/vif1
> +reset_pcap_file hv1-vif2 hv1/vif2
> +rm -f 1.expected
> +rm -f 2.expected
> +
> +# send A query for a server known domain that don't have
> +# any IPv4 address associated with this domain, and expected
> +# server refused DNS reply to save the sender time of waiting for timeout.
> +AS_BOX([Test IPv4 (A records) NO timeout.])
> +# Add back the DNS options for ls1-lp1 without ipv4.
> +check ovn-nbctl remove DNS $DNS1 records vm1.ovn.org
> +check ovn-nbctl set DNS $DNS1 records:vm1.ovn.org="aef0::4"
> +check ovn-nbctl --wait=hv set DNS $DNS1 options:ovn-owned=true
> +ovn-sbctl list DNS > dns7
> +AT_CAPTURE_FILE([dns7])
> +ovn-sbctl dump-flows > sbflows7
> +AT_CAPTURE_FILE([sbflows7])
> +
> +set_dns_params vm1
> +src_ip=`ip_to_hex 10 0 0 6`
> +dst_ip=`ip_to_hex 10 0 0 1`
> +dns_reply=1
> +test_dns 2 f00000000002 f000000000f0 $src_ip $dst_ip $dns_reply
> $dns_req_data $dns_resp_data
> +
> +# NXT_RESUMEs should be 14.
> +OVS_WAIT_UNTIL([test 14 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> +
> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
> +# dns hdr with server refuse RCODE
> +echo "01028125" > expout
> +#only check for the DNS HDR flags since we are not getting any DNS answer
> +AT_CHECK([cat 2.packets | cut -c -92 | cut -c 85-], [0], [expout])
> +
> +reset_pcap_file hv1-vif1 hv1/vif1
> +reset_pcap_file hv1-vif2 hv1/vif2
> +rm -f 1.expected
> +rm -f 2.expected
> OVN_CLEANUP([hv1])
>
> AT_CLEANUP
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
