Firewall: T3907: Revert migration script 6-to-7 and add new 7-to-8

7 files changed, 48 insertions, 58 deletions

diff --git a/interface-definitions/firewall.xml.in b/interface-definitions/firewall.xml.in
index ff8d92a24..94450d808 100644
--- a/interface-definitions/firewall.xml.in
+++ b/interface-definitions/firewall.xml.in
@@ -599,7 +599,7 @@
             </properties>
             <children>
               #include <include/firewall/action-accept-drop-reject.xml.i>
-              #include <include/firewall/log.xml.i>
+              #include <include/firewall/rule-log-level.xml.i>
             </children>
           </node>
           <node name="invalid">
@@ -608,7 +608,7 @@
             </properties>
             <children>
               #include <include/firewall/action-accept-drop-reject.xml.i>
-              #include <include/firewall/log.xml.i>
+              #include <include/firewall/rule-log-level.xml.i>
             </children>
           </node>
           <node name="related">
@@ -617,7 +617,7 @@
             </properties>
             <children>
               #include <include/firewall/action-accept-drop-reject.xml.i>
-              #include <include/firewall/log.xml.i>
+              #include <include/firewall/rule-log-level.xml.i>
             </children>
           </node>
         </children>
diff --git a/interface-definitions/include/firewall/common-rule.xml.i b/interface-definitions/include/firewall/common-rule.xml.i
index 0b8838872..079864122 100644
--- a/interface-definitions/include/firewall/common-rule.xml.i
+++ b/interface-definitions/include/firewall/common-rule.xml.i
@@ -76,6 +76,25 @@
     </leafNode>
   </children>
 </node>
+<leafNode name="log">
+  <properties>
+    <help>Option to log packets matching rule</help>
+    <completionHelp>
+      <list>enable disable</list>
+    </completionHelp>
+    <valueHelp>
+      <format>enable</format>
+      <description>Enable log</description>
+    </valueHelp>
+    <valueHelp>
+      <format>disable</format>
+      <description>Disable log</description>
+    </valueHelp>
+    <constraint>
+      <regex>(enable|disable)</regex>
+    </constraint>
+  </properties>
+</leafNode>
 #include <include/firewall/rule-log-level.xml.i>
 <node name="connection-status">
   <properties>
diff --git a/interface-definitions/include/firewall/name-default-log.xml.i b/interface-definitions/include/firewall/name-default-log.xml.i
index c3f6f0171..1d0ff9497 100644
--- a/interface-definitions/include/firewall/name-default-log.xml.i
+++ b/interface-definitions/include/firewall/name-default-log.xml.i
@@ -1,45 +1,8 @@
 <!-- include start from firewall/name-default-log.xml.i -->
 <leafNode name="enable-default-log">
   <properties>
-    <help>Option to log packets matching default-action</help>
-    <completionHelp>
-      <list>emerg alert crit err warn notice info debug</list>
-    </completionHelp>
-    <valueHelp>
-      <format>emerg</format>
-      <description>Emerg log level</description>
-    </valueHelp>
-    <valueHelp>
-      <format>alert</format>
-      <description>Alert log level</description>
-    </valueHelp>
-    <valueHelp>
-      <format>crit</format>
-      <description>Critical log level</description>
-    </valueHelp>
-    <valueHelp>
-      <format>err</format>
-      <description>Error log level</description>
-    </valueHelp>
-    <valueHelp>
-      <format>warn</format>
-      <description>Warning log level</description>
-    </valueHelp>
-    <valueHelp>
-      <format>notice</format>
-      <description>Notice log level</description>
-    </valueHelp>
-    <valueHelp>
-      <format>info</format>
-      <description>Info log level</description>
-    </valueHelp>
-    <valueHelp>
-      <format>debug</format>
-      <description>Debug log level</description>
-    </valueHelp>
-    <constraint>
-      <regex>(emerg|alert|crit|err|warn|notice|info|debug)</regex>
-    </constraint>
+    <help>Option to log packets hitting default-action</help>
+    <valueless/>
   </properties>
 </leafNode>
 <!-- include end -->
\ No newline at end of file
diff --git a/interface-definitions/include/firewall/rule-log-level.xml.i b/interface-definitions/include/firewall/rule-log-level.xml.i
index 4842b73ca..10c8de5e3 100644
--- a/interface-definitions/include/firewall/rule-log-level.xml.i
+++ b/interface-definitions/include/firewall/rule-log-level.xml.i
@@ -1,7 +1,7 @@
 <!-- include start from firewall/common-rule.xml.i -->
-<leafNode name="log">
+<leafNode name="log-level">
   <properties>
-    <help>Option to log packets matching rule</help>
+    <help>Set log-level. Log must be enable.</help>
     <completionHelp>
       <list>emerg alert crit err warn notice info debug</list>
     </completionHelp>
diff --git a/python/vyos/firewall.py b/python/vyos/firewall.py
index 0c6811d72..dbe5efcb6 100644
--- a/python/vyos/firewall.py
+++ b/python/vyos/firewall.py
@@ -146,10 +146,14 @@ def parse_rule(rule_conf, fw_name, rule_id, ip_name):
 
                     output.append(f'{proto} {prefix}port {operator} $P_{group_name}')
 
-    if 'log' in rule_conf:
+    if 'log' in rule_conf and rule_conf['log'] == 'enable':
         action = rule_conf['action'] if 'action' in rule_conf else 'accept'
-        log_level = rule_conf['log']
-        output.append(f'log prefix "[{fw_name[:19]}-{rule_id}-{action[:1].upper()}]" level {log_level}')
+        output.append(f'log prefix "[{fw_name[:19]}-{rule_id}-{action[:1].upper()}]"')
+
+        if 'log_level' in rule_conf:
+            log_level = rule_conf['log_level']
+            output.append(f'level {log_level}')
+
 
     if 'hop_limit' in rule_conf:
         operators = {'eq': '==', 'gt': '>', 'lt': '<'}
diff --git a/python/vyos/template.py b/python/vyos/template.py
index b41525421..ee82f8f8f 100644
--- a/python/vyos/template.py
+++ b/python/vyos/template.py
@@ -554,8 +554,7 @@ def nft_default_rule(fw_conf, fw_name):
 
     if 'enable_default_log' in fw_conf:
         action_suffix = default_action[:1].upper()
-        log_level = fw_conf['enable_default_log']
-        output.append(f'log prefix "[{fw_name[:19]}-default-{action_suffix}]" level {log_level}')
+        output.append(f'log prefix "[{fw_name[:19]}-default-{action_suffix}]"')
 
     output.append(nft_action(default_action))
     output.append(f'comment "{fw_name} default-action {default_action}"')
@@ -565,8 +564,9 @@ def nft_default_rule(fw_conf, fw_name):
 def nft_state_policy(conf, state, ipv6=False):
     out = [f'ct state {state}']
 
-    if 'log' in conf and 'enable' in conf['log']:
-        out.append('log')
+    if 'log' in conf:
+        log_level = conf['log']
+        out.append(f'log level {log_level}')
 
     out.append('counter')
 
diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py
index 7b9691d9d..61e2598fb 100755
--- a/smoketest/scripts/cli/test_firewall.py
+++ b/smoketest/scripts/cli/test_firewall.py
@@ -91,21 +91,24 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
 
     def test_basic_rules(self):
         self.cli_set(['firewall', 'name', 'smoketest', 'default-action', 'drop'])
-        self.cli_set(['firewall', 'name', 'smoketest', 'enable-default-log', 'info'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'enable-default-log'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'action', 'accept'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'source', 'address', '172.16.20.10'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'destination', 'address', '172.16.10.10'])
-        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'log', 'debug'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'log', 'enable'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'log-level', 'debug'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'action', 'reject'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'protocol', 'tcp'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'destination', 'port', '8888'])
-        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'log', 'err'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'log', 'enable'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'log-level', 'err'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'tcp', 'flags', 'syn'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'tcp', 'flags', 'not', 'ack'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'action', 'accept'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'protocol', 'tcp'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'destination', 'port', '22'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'limit', 'rate', '5/minute'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'log', 'disable'])
 
         self.cli_set(['interfaces', 'ethernet', 'eth0', 'firewall', 'in', 'name', 'smoketest'])
 
@@ -116,7 +119,7 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
             ['saddr 172.16.20.10', 'daddr 172.16.10.10', 'log prefix "[smoketest-1-A]" level debug','return'],
             ['tcp flags & (syn | ack) == syn', 'tcp dport { 8888 }', 'log prefix "[smoketest-2-R]" level err', 'reject'],
             ['tcp dport { 22 }', 'limit rate 5/minute', 'return'],
-            ['log prefix "[smoketest-default-D]" level info','smoketest default-action', 'drop']
+            ['log prefix "[smoketest-default-D]"','smoketest default-action', 'drop']
         ]
 
         nftables_output = cmd('sudo nft list table ip filter')
@@ -131,12 +134,13 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
 
     def test_basic_rules_ipv6(self):
         self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'default-action', 'drop'])
-        self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'enable-default-log', 'emerg'])
+        self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'enable-default-log'])
 
         self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '1', 'action', 'accept'])
         self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '1', 'source', 'address', '2002::1'])
         self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '1', 'destination', 'address', '2002::1:1'])
-        self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '1', 'log', 'crit'])
+        self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '1', 'log', 'enable'])
+        self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '1', 'log-level', 'crit'])
 
         self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '2', 'action', 'reject'])
         self.cli_set(['firewall', 'ipv6-name', 'v6-smoketest', 'rule', '2', 'protocol', 'tcp_udp'])
@@ -150,7 +154,7 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
             ['iifname "eth0"', 'jump NAME6_v6-smoketest'],
             ['saddr 2002::1', 'daddr 2002::1:1', 'log prefix "[v6-smoketest-1-A]" level crit', 'return'],
             ['meta l4proto { tcp, udp }', 'th dport { 8888 }', 'reject'],
-            ['smoketest default-action', 'log prefix "[v6-smoketest-default-D]" level emerg', 'drop']
+            ['smoketest default-action', 'log prefix "[v6-smoketest-default-D]"', 'drop']
         ]
 
         nftables_output = cmd('sudo nft list table ip6 filter')