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.

3 files changed, 67 insertions, 10 deletions

diff --git a/lib/rules/api.c b/lib/rules/api.c
index 6abc0ac1..03fa1bc5 100644
--- 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 fa50807e..e4995582 100644
--- 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 ccbc9880..b5f8e3de 100644
--- 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.", },