From 732a6616e3bf23e7c91107ac22a3be710cc6addd Mon Sep 17 00:00:00 2001
From: =?utf8?q?Vladim=C3=ADr=20=C4=8Cun=C3=A1t?= <vladimir.cunat@nic.cz>
Date: Mon, 17 Sep 2018 11:34:11 +0200
Subject: [PATCH] view: change :addr to a more natural semantics

Continue executing :addr rules until a non-chain action is executed.
Before this, the only the first match in view:addr rules got a chance,
even though the inner policy rule might not trigger in that case
or be a chain action.
---
 NEWS                    |  5 ++++
 modules/view/README.rst |  2 ++
 modules/view/view.lua   | 66 ++++++++++++++++++++++-------------------
 3 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/NEWS b/NEWS
index 08cc2969d..1c2c42b69 100644
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,11 @@
 Knot Resolver 3.x.y (201y-mm-dd)
 ================================
 
+Incompatible changes
+--------------------
+- view module: keep trying rules until a "non-chain" action is executed.
+  This improves consistency with the policy module.
+
 New features
 ------------
 - module edns_keepalive to implement server side of RFC 7828 (#408)
diff --git a/modules/view/README.rst b/modules/view/README.rst
index b29c52d63..c960daae2 100644
--- a/modules/view/README.rst
+++ b/modules/view/README.rst
@@ -15,6 +15,8 @@ There are two identification mechanisms:
   - identifies the client based on a TSIG key
 
 You can combine this information with :ref:`policy <mod-policy>` rules.
+Just as with policies, the rules for a request get tried until one "non-chain"
+action is executed.
 
 .. code-block:: lua
 
diff --git a/modules/view/view.lua b/modules/view/view.lua
index deb4f3b96..9dfa91902 100644
--- a/modules/view/view.lua
+++ b/modules/view/view.lua
@@ -29,31 +29,46 @@ local function match_subnet(family, subnet, bitlen, addr)
 	return (family == addr:family()) and (C.kr_bitcmp(subnet, addr:ip(), bitlen) == 0)
 end
 
--- @function Find view for given request
-local function evaluate(_, req)
+-- @function Execute a policy callback (may be nil);
+-- return boolean: whether to continue trying further rules.
+local function execute(state, req, match_cb)
+	if match_cb == nil then return false end
+	local action = match_cb(req, req:current())
+	if action == nil then return false end
+	local next_state = action(state, req)
+	if next_state then    -- Not a chain rule,
+		req.state = next_state
+		return true
+	else
+		return false
+	end
+end
+
+-- @function Try all the rules in order, until a non-chain rule gets executed.
+local function evaluate(state, req)
+	-- Try :tsig first.
 	local client_key = req.qsource.packet.tsig_rr
 	local match_cb = (client_key ~= nil) and view.key[client_key:owner()] or nil
-	-- Search subnets otherwise
-	if match_cb == nil then
-		if req.qsource.addr ~= nil then
-			for i = 1, #view.src do
-				local pair = view.src[i]
-				if match_subnet(pair[1], pair[2], pair[3], req.qsource.addr) then
-					match_cb = pair[4]
-					break
-				end
+	if execute(state, req, match_cb) then return end
+	-- Then try :addr by the source.
+	if req.qsource.addr ~= nil then
+		for i = 1, #view.src do
+			local pair = view.src[i]
+			if match_subnet(pair[1], pair[2], pair[3], req.qsource.addr) then
+				match_cb = pair[4]
+				if execute(state, req, match_cb) then return end
 			end
-		elseif req.qsource.dst_addr ~= nil then
-			for i = 1, #view.dst do
-				local pair = view.dst[i]
-				if match_subnet(pair[1], pair[2], pair[3], req.qsource.dst_addr) then
-					match_cb = pair[4]
-					break
-				end
+		end
+	-- Finally try :addr by the destination.
+	elseif req.qsource.dst_addr ~= nil then
+		for i = 1, #view.dst do
+			local pair = view.dst[i]
+			if match_subnet(pair[1], pair[2], pair[3], req.qsource.dst_addr) then
+				match_cb = pair[4]
+				if execute(state, req, match_cb) then return end
 			end
 		end
 	end
-	return match_cb
 end
 
 -- @function Return policy based on source address
@@ -87,17 +102,8 @@ view.layer = {
 	begin = function(state, req)
 		if state == kres.FAIL then return state end
 		req = kres.request_t(req)
-		local match_cb = evaluate(view, req)
-		if match_cb ~= nil then
-			local action = match_cb(req, req:current())
-			if action then
-				local next_state = action(state, req)
-				if next_state then    -- Not a chain rule,
-					return next_state -- stop on first match
-				end
-			end
-		end
-		return state
+		evaluate(state, req)
+		return req.state
 	end
 }
 
-- 
2.47.2