Repository: ansible/ansible
Pull request: iptables: Reorder comment postition
##### SUMMARY
Fixes: #71444.

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
iptables

Problem statement:
iptables comment parameter incorrectly positioned in iptables
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
When using **comment** parameter with iptables module, it is not positioned as the right-most object in the actual iptables table.
The outcome looks very bad at the moment.

##### ISSUE TYPE
- Cosmetic Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
**iptables** module, **comment** parameter

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.9.9
  config file = /home/me/project/ansible.cfg
  configured module search path = ['/home/me/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/me/vConstruction/lib/python3.6/site-packages/ansible
  executable location = /home/me/vConstruction/bin/ansible
  python version = 3.6.9 (default, Jul 17 2020, 12:50:27) [GCC 8.4.0]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
DEFAULT_CALLBACK_WHITELIST(/home/me/project/ansible.cfg) = ['profile_tasks']
DEFAULT_HOST_LIST(/home/me/project/ansible.cfg) = ['/home/me/project/inventory']
DEPRECATION_WARNINGS(/home/me/project/ansible.cfg) = False
HOST_KEY_CHECKING(/home/me/project/ansible.cfg) = False
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
Host where I use Ansible: Ubuntu 18.04 via WSL 2.
Target VM where the iptables rules are being created: Ubuntu 18.04.

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
It can be reproduced by using the following task and running `iptables -L` on the target machine.
I added the DNS task as well, because this one looks correct, but I'm sure that's just because I don't have **ctstate** specified.

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: Create HTTPS rule
  iptables:
    chain: INPUT
    in_interface: eth0
    protocol: tcp
    destination_port: https
    ctstate: NEW
    jump: ACCEPT
    comment: Accept new inbound HTTPS connections to eth0 interface

- name: Create DNS rule
  iptables:
    chain: INPUT
    protocol: udp
    destination_port: domain
    jump: ACCEPT
    comment: Accept inbound DNS connections
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
```bash
ACCEPT     tcp  --  anywhere             anywhere             tcp dpt:ssh  ctstate NEW /* Accept new inbound HTTPS connections to eth0 interface */
```
##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```bash
ACCEPT     tcp  --  anywhere             anywhere             tcp dpt:https /* Accept new inbound HTTPS connections to eth0 interface */ ctstate NEW
ACCEPT     udp  --  anywhere             anywhere             udp dpt:domain /* Accept inbound DNS connections */
```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/iptables.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/iptables.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
Does re-ordering comment position like in this commit fix the issue?
https://github.com/aminvakil/ansible/commit/2a816de2cced19b614d3b50704f2ad66dc6be3df
To make sure it's always at the end.
Please tell me to open a PR if that's OK.
Yes, by reordering the position of those two lines, the output is now as it should be. Comment is positioned on the right now.

I don't know what's the purpose of opening a PR, but if it's necessary for the process, sure, you may open it.
@RRohi 
I'll wait for a maintainer to remove the `needs_triage` tag.
@aminvakil Feel free to submit a PR with a fix as well as tests.

Commit message: Add changelog

Patch to review:
diff --git a/changelogs/fragments/71496-iptables-reorder-comment-position.yml b/changelogs/fragments/71496-iptables-reorder-comment-position.yml
new file mode 100644
index 00000000000000..942edb22a7c8dd
--- /dev/null
+++ b/changelogs/fragments/71496-iptables-reorder-comment-position.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - iptables - reorder comment postition to be at the end (https://github.com/ansible/ansible/issues/71444).
diff --git a/lib/ansible/modules/iptables.py b/lib/ansible/modules/iptables.py
index 01741c88fbdbc4..75c979d8ce4783 100644
--- a/lib/ansible/modules/iptables.py
+++ b/lib/ansible/modules/iptables.py
@@ -556,8 +556,6 @@ def construct_rule(params):
         '--set-dscp-class',
         False)
     append_match_flag(rule, params['syn'], '--syn', True)
-    append_match(rule, params['comment'], 'comment')
-    append_param(rule, params['comment'], '--comment', False)
     if 'conntrack' in params['match']:
         append_csv(rule, params['ctstate'], '--ctstate')
     elif 'state' in params['match']:
@@ -589,6 +587,8 @@ def construct_rule(params):
         params['icmp_type'],
         ICMP_TYPE_OPTIONS[params['ip_version']],
         False)
+    append_match(rule, params['comment'], 'comment')
+    append_param(rule, params['comment'], '--comment', False)
     return rule
 
 
diff --git a/test/units/modules/test_iptables.py b/test/units/modules/test_iptables.py
index 68a80d20130884..23fe59d5fe741c 100644
--- a/test/units/modules/test_iptables.py
+++ b/test/units/modules/test_iptables.py
@@ -876,3 +876,39 @@ def test_insert_rule_with_wait(self):
             '-j',
             'ACCEPT'
         ])
+
+    def test_comment_position_at_end(self):
+        """Test flush without parameters"""
+        set_module_args({
+            'chain': 'INPUT',
+            'jump': 'ACCEPT',
+            'action': 'insert',
+            'comment': 'this is a comment',
+            '_ansible_check_mode': True,
+        })
+
+        commands_results = [
+            (0, '', ''),
+        ]
+
+        with patch.object(basic.AnsibleModule, 'run_command') as run_command:
+            run_command.side_effect = commands_results
+            with self.assertRaises(AnsibleExitJson) as result:
+                iptables.main()
+                self.assertTrue(result.exception.args[0]['changed'])
+
+        self.assertEqual(run_command.call_count, 1)
+        self.assertEqual(run_command.call_args_list[0][0][0], [
+            '/sbin/iptables',
+            '-t',
+            'filter',
+            '-C',
+            'INPUT',
+            '-j',
+            'ACCEPT',
+            '-m',
+            'comment',
+            '--comment',
+            'this is a comment'
+        ])
+        self.assertEqual(run_command.call_args[0][0][10], 'this is a comment')


Code review comments:
diff_hunk: @@ -876,3 +876,39 @@ def test_insert_rule_with_wait(self):
             '-j',
             'ACCEPT'
         ])
+
+    def test_comment_position_at_end(self):
+        """Test flush without parameters"""
+        set_module_args({
+            'chain': 'INPUT',
+            'jump': 'ACCEPT',
+            'action': 'insert',
+            'comment': 'this is a comment',
path: test/units/modules/test_iptables.py
text: @user1:
I think this should include some parameters that are problematic when they appear before `comment`.
```suggestion
            'cstate': ['NEW'],
            'comment': 'this is a comment',
```

@author:
Fixed in https://github.com/ansible/ansible/pull/71496/files#diff-a184f210bf607737e23e4930d925ba5fR886.