hints: merge RRs instead of replacing them

We had this behavior in 5.x.
Lua level: affects hints.set() and hints['name'] and hints.add_hosts()
YAML level: /local-data/addresses and /local-data/addresses-files

I considered various approaches when writing this.  This one won because
in /etc/hosts like files a name can be repeated with arbitrary lines
in between, and users can reasonably expect it to collect all addresses.

diff --git a/lib/rules/api.c b/lib/rules/api.c
index 6abc0ac1f23eb95dd59a2ed7e2b560d902d6a014..03fa1bc50c27ba332156373c0ec8d5050bba019a 100644 (file)
--- a/lib/rules/api.c
+++ b/lib/rules/api.c
@@ -570,6 +570,53 @@ int kr_rule_local_data_del(const knot_rrset_t *rrs, kr_rule_tags_t tags)
        knot_db_val_t key = local_data_key(rrs, key_data, RULESET_DEFAULT);
        return ruledb_op(remove, &key, 1);
 }
+int kr_rule_local_data_merge(const knot_rrset_t *rrs, const kr_rule_tags_t tags)
+{
+       ENSURE_the_rules;
+       // Construct the DB key.
+       uint8_t key_data[KEY_MAXLEN];
+       knot_db_val_t key = local_data_key(rrs, key_data, RULESET_DEFAULT);
+       knot_db_val_t val;
+       // Transaction: we assume that we're in a RW transaction already,
+       // so that here we already "have a lock" on the last version.
+       int ret = ruledb_op(read, &key, &val, 1);
+       if (abs(ret) == abs(ENOENT))
+               goto fallback;
+       if (ret)
+               return kr_error(ret);
+       // check tags
+       kr_rule_tags_t tags_old;
+       if (deserialize_fails_assert(&val, &tags_old) || tags_old != tags)
+               goto fallback;
+       // merge TTLs
+       uint32_t ttl;
+       if (deserialize_fails_assert(&val, &ttl))
+               goto fallback;
+       if (ttl > rrs->ttl)
+               ttl = rrs->ttl;
+       knot_rrset_t rrs_new;
+       knot_rrset_init(&rrs_new, rrs->owner, rrs->type, rrs->rclass, ttl);
+       // merge the rdatasets
+       knot_mm_t *mm = mm_ctx_mempool2(MM_DEFAULT_BLKSIZE); // frag. optimization
+       if (!mm)
+               return kr_error(ENOMEM);
+       ret = rdataset_materialize(&rrs_new.rrs, val.data, val.data + val.len, mm);
+       if (kr_fails_assert(ret >= 0)) { // just invalid call or rubbish data
+               mm_ctx_delete(mm);
+               return ret;
+       }
+       ret = knot_rdataset_merge(&rrs_new.rrs, &rrs->rrs, mm);
+       if (ret) { // ENOMEM or hitting 64 KiB limit
+               mm_ctx_delete(mm);
+               return kr_error(ret);
+       }
+       // everything is ready to insert the merged RRset
+       ret = local_data_ins(key, &rrs_new, NULL, tags);
+       mm_ctx_delete(mm);
+       return ret;
+fallback:
+       return local_data_ins(key, rrs, NULL, tags);
+}
 
 /** Empty or NXDOMAIN or NODATA.  Returning kr_error(EAGAIN) means the rule didn't match. */
 static int answer_zla_empty(val_zla_type_t type, struct kr_query *qry, knot_pkt_t *pkt,

diff --git a/lib/rules/api.h b/lib/rules/api.h
index fa50807e565af7f2f25dae396cb8b02f3745c050..e4995582c31647f65f49104a7c1e654bba1d0210 100644 (file)
--- a/lib/rules/api.h
+++ b/lib/rules/api.h
@@ -70,6 +70,15 @@ int kr_view_select_action(const struct kr_request *req, knot_db_val_t *selected)
 KR_EXPORT
 int kr_rule_local_data_ins(const knot_rrset_t *rrs, const knot_rdataset_t *sig_rds,
                                kr_rule_tags_t tags);
+/** Merge RRs into a local data rule.
+ *
+ * - If tags don't match, overwrite the data and return kr_error(EEXIST).
+ * - RRSIGs get dropped, if any were attached.
+ * - We assume that this is called with a RW transaction open already,
+ *   which is always true in normal usage (long RW txn covering whole config).
+ */
+KR_EXPORT
+int kr_rule_local_data_merge(const knot_rrset_t *rrs, kr_rule_tags_t tags);
 
 /** Remove a local data rule.
  *

diff --git a/modules/hints/hints.c b/modules/hints/hints.c
index ccbc988013defb37003d65ecab808ce0ffb5ea79..b5f8e3de007042eba9f1b3984000c041f30b943f 100644 (file)
--- a/modules/hints/hints.c
+++ b/modules/hints/hints.c
@@ -142,11 +142,12 @@ static int add_pair(const struct hints_data *data, const char *name, const char
        } else {
                ret = knot_rrset_add_rdata(&rrs, (const uint8_t *)&ia.ip4.sin_addr, 4, NULL);
        }
-       if (!ret) ret = kr_rule_local_data_ins(&rrs, NULL, KR_RULE_TAGS_ALL);
+       if (!ret) ret = kr_rule_local_data_merge(&rrs, KR_RULE_TAGS_ALL);
        if (!ret && data->use_nodata) {
                rrs.type = KNOT_RRTYPE_CNAME;
                rrs.rrs.count = 0;
                rrs.rrs.size = 0;
+               // no point in the _merge() variant here
                ret = kr_rule_local_data_ins(&rrs, NULL, KR_RULE_TAGS_ALL);
        }
 
@@ -167,7 +168,9 @@ static int add_reverse_pair(const struct hints_data *data, const char *name, con
                return kr_error(EINVAL);
        int ret = knot_rrset_add_rdata(&rrs, ptr_name, knot_dname_size(ptr_name), NULL);
        if (!ret) {
-               ret = kr_rule_local_data_ins(&rrs, NULL, KR_RULE_TAGS_ALL);
+               // We use _merge().  Using multiple PTR RRs is not recommended generally,
+               // but here it seems better than choosing any "arbitrarily".
+               ret = kr_rule_local_data_merge(&rrs, KR_RULE_TAGS_ALL);
                knot_rdataset_clear(&rrs.rrs, NULL);
        }
        return ret;
@@ -481,15 +484,13 @@ int hints_init(struct kr_module *module)
        module->layer = &layer;
 
        static const struct kr_prop props[] = {
-       /* FIXME(decide): .set() and .del() used to work on individual RRs;
-        * now they overwrite or delete whole RRsets.
-        * Also, .get() doesn't work at all.
+       /* TODO(decide): .del() used to work on individual RRs;
+        * now it deletes whole RRsets. Also, .get() doesn't work at all.
+        *
+        * We'll probably be deprecating access direct through these non-declarative
+        * commands (set, get, del) which are also usable dynamically.
         *
-        * It really depends what kind of config/API we'll be exposing to user.
-        *  - Manipulating whole RRsets generally makes more sense to me.
-        *    (But hints.set() currently can't even insert larger sets.)
-        *  - We'll probably be deprecating access through these non-declarative
-        *    commands (set, get, del) which are also usable dynamically.
+        * For .set() and .add_hosts() see the RW transaction note at kr_rule_local_data_merge()
         */
            { &hint_set,    "set", "Set {name, address} hint.", },
            { &hint_del,    "del", "Delete one {name, address} hint or all addresses for the name.", },