Bug 1400 - allowing use of global cache on ECS-forwarding unless always-forward?
allowing use of global cache on ECS-forwarding unless always-forward?
Status: ASSIGNED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-11 18:36 CEST by JINMEI Tatuya
Modified: 2017-09-18 17:12 CEST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description JINMEI Tatuya 2017-08-11 18:36:51 CEST
While playing with cachedb, I noticed that data in the cachedb isn't used if the subnetcache module is enabled and placed before cachedb and when unbound forwards a query with a client-supplied ECS.

This is perhaps intentional, and wouldn't matter much in practice anyway if the only cache is the built-in in-memory cache: (assuming client-subnet-always-forward isn't set to 'yes') the fact that the query reaches subnetcache means the answer isn't in the global cache, so the result should be basically the same whether or not we allow looking into the global cache from the iterator (of course, I'm simplifying the story a bit here: I'm aware there are corner cases where data in the global cache could still be used after the query reaches subnetcache and other modules below it.  But I believe this scenario covers most common cases).

The situation is different with cachedb, though.  If the cachedb backend has a persistent storage or is run in a separate process and so the data in it can survive a restart/reload of unbound, it's possible that an answer to a query isn't found in the in-memory cache but it's in cachedb and the query reaches the subnetcache module.  If we could use the cachedb (which is also a global cache) unbound should be able to find an answer there.  But when it's forwarding a client-supplied ECS, it suppresses looking at the global cache (both in-memory and cachedb), so we couldn't benefit from cachedb.

It's a pity, and I wonder if it's intentional and can't be loosened for some critical technical reasons.  As noted above, I suspect in many cases that shouldn't matter or break anything: either unbound doesn't reach subnetcache in the first place or it still finds an answer in the global cache anyway.  One thing that I think may matter is when we enable client-subnet-always-forward.  In this case the lookup at the global cache is suppressed in worker_handle_request(), so it's possible unbound reaches subnetcache while an answer is in the global cache.

So one possible idea is to suppress the global cache lookup only when client-subnet-always-forward is set to yes.  The one-line patch pasted below implements the idea.  I've confirmed it works as I intended with a slightly modified unbound 1.6.4.

Does this change make sense to you?  Or am I missing something critical in uncommon cases?  If it's the latter, is there a way we can forward ECS and still benefit from the persistent nature of cachedb?

Index: subnetmod.c
===================================================================
--- subnetmod.c	(revision 4299)
+++ subnetmod.c	(working copy)
@@ -722,6 +722,7 @@
 		sq->ecs_server_out.subnet_scope_mask = 0;
 		sq->ecs_server_out.subnet_validdata = 1;
 		if(sq->ecs_server_out.subnet_source_mask != 0 &&
+			qstate->env->cfg->client_subnet_always_forward &&
 			sq->subnet_downstream)
 			/* ECS specific data required, do not look at the global
 			 * cache in other modules. */
Comment 1 Ralph Dolmans 2017-09-04 16:18:13 CEST
Hi Jinmei,

Thanks for your suggestion, looks good.

The client_subnet_always_forward value is already used to determine whether the non-module cache lookup should be bypassed. We could apply the same logic to the cache lookup in the module.

Since this only has effect when combined with cachedb, I would like to postpone the commit of your proposed change until after the 1.6.6 release.

Regards,
-- Ralph
Comment 2 JINMEI Tatuya 2017-09-05 17:49:31 CEST
Ralph,

Thanks, that sounds good to me.  Looking forward to seeing it merged.
Comment 3 Ralph Dolmans 2017-09-18 11:11:02 CEST
Hi Jinmei,

I merged the change.

Thanks,
-- Ralph
Comment 4 JINMEI Tatuya 2017-09-18 17:12:08 CEST
looks good to me, thanks!