From: Alberto Leiva Popper Date: Sun, 12 Nov 2023 18:27:20 +0000 (-0600) Subject: Improve selection of cache RPP fallback X-Git-Tag: 1.6.0~18 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=9048c96f67b993867e7624c2be46883f4eab4d4e;p=thirdparty%2FFORT-validator.git Improve selection of cache RPP fallback Corner case. Suppose the cache has (for whatever reason) downloaded the two following URLs separately: rsync://a.b.c/d rsync://a.b.c/d/e/f (This might happen if the latter is downloaded in one iteration, then the former is downloaded the next, and the cleanup timer hasn't kicked in yet.) This commit extends the existing priority selection algorithm to this situation. (The old way was to choose the most specific URL, which would go on to lose to a different URL which might have lost to the less specific version.) Honestly, this is a micro-correction. It hopefully slightly increases the chances of the cache fallback being useful in very specific unusual situations, rather than guarantee it. But it's more consistent, future- proof and looks more sensible in the tests. --- diff --git a/src/cache/local_cache.c b/src/cache/local_cache.c index 2780c618..06d3665c 100644 --- a/src/cache/local_cache.c +++ b/src/cache/local_cache.c @@ -80,7 +80,6 @@ static struct cache_node *https; static time_t startup_time; /* When we started the last validation */ -/* Minimizes multiple evaluation */ static struct cache_node * add_child(struct cache_node *parent, char const *basename) { @@ -494,8 +493,41 @@ end: return error; } +/* + * Highest to lowest priority: + * + * 1. Recent Success: !error, CNF_SUCCESS, high ts_success. + * 2. Old Success: !error, CNF_SUCCESS, low ts_success. + * 3. Previous Recent Success: error, CNF_SUCCESS, high ts_success. + * 4. Previous Old Success: error, CNF_SUCCESS, old ts_success. + * 5. No Success: !CNF_SUCCESS (completely unviable) + */ +static struct cache_node * +choose_better(struct cache_node *old, struct cache_node *new) +{ + if (!(new->flags & CNF_SUCCESS)) + return old; + if (old == NULL) + return new; + + /* + * We're gonna have to get subjective here. + * Should we prioritize a candidate that was successfully downloaded a + * long time ago (with no retries since), or one that failed recently? + * Both are terrible, but returning something is still better than + * returning nothing, because the validator might manage to salvage + * remnant cached ROAs that haven't expired yet. + */ + + if (old->error && !new->error) + return new; + if (!old->error && new->error) + return old; + return (difftime(old->ts_success, new->ts_success) < 0) ? new : old; +} + static struct cache_node * -find_uri(struct rpki_uri *uri) +find_node(struct rpki_uri *uri) { char *luri, *token, *saveptr; struct cache_node *parent, *node; @@ -528,79 +560,45 @@ find_uri(struct rpki_uri *uri) if (node == NULL) goto end; if (recursive && (node->flags & CNF_DIRECT)) - result = node; + result = choose_better(result, node); parent = node; } - if ((node != NULL) && (node->flags & CNF_DIRECT)) - result = node; + if (!recursive && (node != NULL) && (node->flags & CNF_DIRECT)) + result = choose_better(result, node); end: free(luri); return result; } -static unsigned int -get_score(struct cache_node *node) -{ - unsigned int score; - - /* - * Highest to lowest priority: - * - * 1. Recent Success: !error, CNF_SUCCESS, high ts_success. - * 2. Old Success: !error, CNF_SUCCESS, low ts_success. - * 3. Previous Recent Success: error, CNF_SUCCESS, high ts_success. - * 4. Previous Old Success: error, CNF_SUCCESS, old ts_success. - * 5. No Success: error, !CNF_SUCCESS (completely unviable) - */ - - if (node == NULL) - return 0; - - score = 0; - if (!node->error) - score |= (1 << 1); - if (node->flags & CNF_SUCCESS) - score |= (1 << 0); - return score; -} - -/* - * Returns true if @n1's success happened earlier than n2's. - */ -static bool -earlier_success(struct cache_node *n1, struct cache_node *n2) -{ - return difftime(n1->ts_success, n2->ts_success) < 0; -} +struct uri_and_node { + struct rpki_uri *uri; + struct cache_node *node; +}; -struct rpki_uri * -cache_recover(struct uri_list *uris, bool use_rrdp) +/* Separated because of unit tests. */ +static void +__cache_recover(struct uri_list *uris, bool use_rrdp, struct uri_and_node *best) { - struct scr { - struct rpki_uri *uri; - struct cache_node *node; - unsigned int score; - }; - struct rpki_uri **uri; - struct scr cursor; - struct scr best = { 0 }; + struct uri_and_node cursor; ARRAYLIST_FOREACH(uris, uri) { cursor.uri = *uri; - cursor.node = find_uri(cursor.uri); - cursor.score = get_score(cursor.node); - if (cursor.score == 0) + cursor.node = find_node(cursor.uri); + if (cursor.node == NULL) continue; - if (cursor.score > best.score) - best = cursor; - else if (cursor.score == best.score - && earlier_success(best.node, cursor.node)) - best = cursor; + if (choose_better(best->node, cursor.node) == cursor.node) + *best = cursor; } +} +struct rpki_uri * +cache_recover(struct uri_list *uris, bool use_rrdp) +{ + struct uri_and_node best = { 0 }; + __cache_recover(uris, use_rrdp, &best); return best.uri; } diff --git a/test/cache/local_cache_test.c b/test/cache/local_cache_test.c index f497cdcb..5908cce0 100644 --- a/test/cache/local_cache_test.c +++ b/test/cache/local_cache_test.c @@ -1336,7 +1336,29 @@ START_TEST(test_recover) ck_assert_ptr_eq(uris.array[14], cache_recover(&uris, false)); uris_cleanup(&uris); - /* FIXME test HTTP (non-recursive) */ + cache_teardown(); + + + struct uri_and_node un = { 0 }; + + rsync = NODE("rsync", 0, 0, + TNODE("1", CNF_SUCCESS, 200, 200, 0, + TNODE("2", CNF_DIRECT, 200, 200, 1, + TNODE("3", CNF_DIRECT | CNF_SUCCESS, 100, 100, 1, + TNODE("4", CNF_DIRECT | CNF_SUCCESS, 200, 200, 1, + TNODE("5", CNF_DIRECT | CNF_SUCCESS, 100, 100, 0, + TNODE("6", CNF_DIRECT | CNF_SUCCESS, 200, 200, 0))))))); + + /* Try them all at the same time */ + PREPARE_URI_LIST(&uris, "rsync://1/2/3/4/5/6"); + __cache_recover(&uris, false, &un); + ck_assert_ptr_eq(uris.array[0], un.uri); + ck_assert_str_eq("6", un.node->basename); + uris_cleanup(&uris); + + /* TODO (test) HTTP (non-recursive) */ + /* TODO (test) more variations */ + /* TODO (test) node with DIRECT, then not direct, then DIRECT */ cache_teardown(); }