From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Thu, 21 Sep 2023 12:51:42 +0000 (+0200)
Subject: /views/*/options: fix when used with e.g. tags
X-Git-Tag: v6.0.4~1^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=62b10d5432ca40491cba700e4c2d70bcb3408ee5;p=thirdparty%2Fknot-resolver.git

/views/*/options: fix when used with e.g. tags

The issue is not now; it has always been broken in 6.x.
The model is that at most one view applies on any request.
If we need to do more things, they must happen as one meta-action.

test_view_insert_action(): dropped; can't see a suitable replacement
---

diff --git a/manager/knot_resolver_manager/datamodel/templates/macros/view_macros.lua.j2 b/manager/knot_resolver_manager/datamodel/templates/macros/view_macros.lua.j2
index b61cc3f03..2f1a79646 100644
--- a/manager/knot_resolver_manager/datamodel/templates/macros/view_macros.lua.j2
+++ b/manager/knot_resolver_manager/datamodel/templates/macros/view_macros.lua.j2
@@ -5,11 +5,6 @@
 {%- endfor -%}
 {%- endmacro -%}
 
-{% macro view_insert_action(view, subnet, action) -%}
-assert(C.kr_view_insert_action('{{ subnet }}', '{{ view.dst_subnet or '' }}',
-	{{ get_proto_set(view.protocols) }}, {{ action }})==0)
-{%- endmacro %}
-
 {% macro view_flags(options) -%}
 {% if not options.minimize -%}
 "NO_MINIMIZE",
diff --git a/manager/knot_resolver_manager/datamodel/templates/views.lua.j2 b/manager/knot_resolver_manager/datamodel/templates/views.lua.j2
index d4357cbbe..81de8c7b7 100644
--- a/manager/knot_resolver_manager/datamodel/templates/views.lua.j2
+++ b/manager/knot_resolver_manager/datamodel/templates/views.lua.j2
@@ -1,21 +1,24 @@
 {% from 'macros/common_macros.lua.j2' import quotes %}
-{% from 'macros/view_macros.lua.j2' import view_insert_action, view_flags, view_answer %}
+{% from 'macros/view_macros.lua.j2' import get_proto_set, view_flags, view_answer %}
 {% from 'macros/policy_macros.lua.j2' import policy_flags, policy_tags_assign %}
 
 {% if cfg.views %}
 {% for view in cfg.views %}
 {% for subnet in view.subnets %}
 
-{% if view.tags -%}
-{{ view_insert_action(view, subnet, policy_tags_assign(view.tags)) }}
-{% elif view.answer %}
-{{ view_insert_action(view, subnet, view_answer(view.answer)) }}
+assert(C.kr_view_insert_action('{{ subnet }}', '{{ view.dst_subnet or '' }}',
+  {{ get_proto_set(view.protocols) }}, policy.COMBINE({
+{%- set flags = view_flags(view.options) -%}
+{% if flags %}
+	{{ quotes(policy_flags(flags)) }},
 {%- endif %}
 
-{%- set flags = view_flags(view.options) -%}
-{% if flags -%}
-{{ view_insert_action(view, subnet, quotes(policy_flags(flags))) }}
+{% if view.tags %}
+	{{ policy_tags_assign(view.tags) }},
+{% elif view.answer %}
+	{{ view_answer(view.answer) }},
 {%- endif %}
+  })) == 0)
 
 {% endfor %}
 {% endfor %}
diff --git a/manager/knot_resolver_manager/datamodel/view_schema.py b/manager/knot_resolver_manager/datamodel/view_schema.py
index 98d4a1a0d..ad44eb3b3 100644
--- a/manager/knot_resolver_manager/datamodel/view_schema.py
+++ b/manager/knot_resolver_manager/datamodel/view_schema.py
@@ -42,4 +42,4 @@ class ViewSchema(ConfigSchema):
 
     def _validate(self) -> None:
         if bool(self.tags) == bool(self.answer):
-            raise ValueError("only one of 'tags' and 'answer' options must be configured")
+            raise ValueError("exactly one of 'tags' and 'answer' must be configured")
diff --git a/manager/tests/unit/datamodel/templates/test_view_macros.py b/manager/tests/unit/datamodel/templates/test_view_macros.py
index 3a3f35f93..fbc62f49c 100644
--- a/manager/tests/unit/datamodel/templates/test_view_macros.py
+++ b/manager/tests/unit/datamodel/templates/test_view_macros.py
@@ -6,16 +6,6 @@ from knot_resolver_manager.datamodel.config_schema import template_from_str
 from knot_resolver_manager.datamodel.view_schema import ViewOptionsSchema, ViewSchema
 
 
-def test_view_insert_action():
-    subnet = "10.0.0.0/8"
-    action = "policy.DENY"
-    tmpl_str = """{% from 'macros/view_macros.lua.j2' import view_insert_action %}
-{{ view_insert_action(subnet, action) }}"""
-
-    tmpl = template_from_str(tmpl_str)
-    assert tmpl.render(subnet=subnet, action=action) == f"assert(C.kr_view_insert_action('{ subnet }',{ action })==0)"
-
-
 def test_view_flags():
     tmpl_str = """{% from 'macros/view_macros.lua.j2' import view_flags %}
 {{ view_flags(options) }}"""
diff --git a/modules/policy/policy.lua b/modules/policy/policy.lua
index 443fc0b03..60b034783 100644
--- a/modules/policy/policy.lua
+++ b/modules/policy/policy.lua
@@ -856,6 +856,16 @@ function policy.TAGS_ASSIGN(names)
 	return 'policy.tags_assign_bitmap(' .. tostring(bitmap) .. ')'
 end
 
+-- Perform a list of actions sequentially; meant for kr_view_insert_action().
+function policy.COMBINE(list)
+	if #list == 1 then return list[1] end
+	local r = 'function(state,req) '
+		for _, item in ipairs(list) do
+			r = r .. item .. '(state,req); '
+		end
+	return r .. 'end'
+end
+
 --[[ Insert a forwarding rule, i.e. override upstream for one DNS subtree.
 
 Throws lua exceptions when detecting something fishy.