Skip to content

Commit c9cbf93

Browse files
martinpittrichm
authored andcommitted
fix: Fix "interface_pci_id" role option
Cause: The introduction of this feature in commit 2b0bb1b was broken: The new `interface_pci_id` option was not passed to `firewall_lib`. The integration test case was very shallow and did not verify the success of the operation. Consequence: Calling the role with `interface_pci_id` had no effect, that setting was ignored. Fix: Pass the option to the module. Rework the test, which was pretty much completely broken: - Add a positive assertion that the interface mapped to the given PCI ID actually appears in the nftables/iptables rules. - Replace the hardcoded 1af4:0001 ID in the test with runtime detection. That *happens* to be the ID of QEMU's virtio card, but this is neither obvious nor reliable nor does it apply to other environments such as Testing Farm. - Container environments don't have any "real" (PCI) interface, so skip the test if we can't find any. - The iptables vs. nftables detection was also broken on RHEL > 9 and Fedora, as `dbus-send` is not installed by default any more (`busctl` is more modern, but not yet available in RHEL 7), and the D-Bus property does not exist on RHEL 7 either. This was shadowed by the bug above (the actually checked backend didn't matter). The dynamic detection is too hard/brittle to fix, and there is no surprise about the backend anyway: RHEL 7 uses iptables, everything else nftables. So put that condition into the test directly, which is much more robust. Fixes #272
1 parent 547b7fd commit c9cbf93

File tree

2 files changed

+52
-25
lines changed

2 files changed

+52
-25
lines changed

tasks/main.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@
9898
source: "{{ item.source | default(omit) }}"
9999
destination: "{{ item.destination | default(omit) }}"
100100
interface: "{{ item.interface | default(omit) }}"
101+
interface_pci_id: "{{ item.interface_pci_id | default(omit) }}"
101102
icmp_block: "{{ item.icmp_block | default(omit) }}"
102103
icmp_block_inversion: "{{ item.icmp_block_inversion | default(omit) }}"
103104
timeout: "{{ item.timeout | default(omit) }}"

tests/tests_interface_pci.yml

Lines changed: 51 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -7,32 +7,46 @@
77
- linux-system-roles.firewall
88

99
tasks:
10-
- name: Get backend from dbus
11-
command: >-
12-
dbus-send --system --print-reply --type=method_call
13-
--dest=org.fedoraproject.FirewallD1
14-
/org/fedoraproject/FirewallD1/config
15-
org.freedesktop.DBus.Properties.Get
16-
string:"org.fedoraproject.FirewallD1.config"
17-
string:"FirewallBackend"
18-
ignore_errors: true # noqa ignore-errors
19-
register: result
20-
changed_when: false
21-
22-
- name: Get backend from result
10+
- name: Set expected backend
2311
set_fact:
2412
nftables_backend:
25-
"{{ result is not failed and 'nftables' in result.stdout }}"
13+
"{{ true
14+
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
15+
and ansible_distribution_major_version is version('8', '>=')
16+
else false }}"
17+
18+
- name: Find ethernet interface
19+
shell: |
20+
set -euo pipefail
21+
I=$(find /sys/class/net -name 'e*' | head -n1)
22+
# containers only have virtual devices; for PCI we need a real one
23+
if [ -n "$I" ] && [ -e "$I/device/vendor" ]; then
24+
echo "$I"
25+
fi
26+
register: find_iface
27+
changed_when: false
2628

2729
- name: Test interfaces with PCI ids
30+
# this can't be tested in containers or similar envs without any real
31+
# ethernet devices
32+
when: find_iface.stdout != ""
2833
block:
34+
- name: Determine interface vendor/product ID
35+
shell: |
36+
set -euo pipefail
37+
VID="$(sed 's/^0x//' < {{ find_iface.stdout | quote }}/device/vendor)"
38+
PID="$(sed 's/^0x//' < {{ find_iface.stdout | quote }}/device/device)"
39+
echo "$VID:$PID"
40+
register: pci_id
41+
changed_when: false
42+
2943
- name: Add pci device ethernet controller
3044
include_role:
3145
name: linux-system-roles.firewall
3246
vars:
3347
firewall:
3448
zone: internal
35-
interface_pci_id: 1af4:0001
49+
interface_pci_id: "{{ pci_id.stdout }}"
3650
state: enabled
3751
permanent: true
3852

@@ -42,31 +56,43 @@
4256
vars:
4357
firewall:
4458
zone: internal
45-
interface_pci_id: 1af4:0001
59+
interface_pci_id: "{{ pci_id.stdout }}"
4660
state: enabled
4761
permanent: true
4862

49-
- name: Assert pcid not in nftable ruleset
63+
- name: Get nftable ruleset
5064
command: nft list ruleset
51-
register: result
52-
failed_when: result is failed or '1af4:0001' in result.stdout
53-
when: nftables_backend | bool
65+
register: nft_list
5466
changed_when: false
67+
when: nftables_backend | bool
68+
69+
- name: Assert that interface is in nftable ruleset
70+
assert:
71+
that:
72+
- find_iface.stdout | basename in nft_list.stdout
73+
- pci_id.stdout | trim not in nft_list.stdout
74+
when: nftables_backend | bool
5575

56-
- name: Assert pcid not in iptables rules
76+
- name: Get iptables ruleset
5777
command: iptables -S
58-
register: result
59-
failed_when: result is failed or '1af4:0001' in result.stdout
60-
when: not nftables_backend | bool
78+
register: ipt_list
6179
changed_when: false
80+
when: not nftables_backend | bool
81+
82+
- name: Assert that interface is in iptables ruleset
83+
assert:
84+
that:
85+
- find_iface.stdout | basename in ipt_list.stdout
86+
- pci_id.stdout | trim not in ipt_list.stdout
87+
when: not nftables_backend | bool
6288

6389
- name: Remove interface from internal
6490
include_role:
6591
name: linux-system-roles.firewall
6692
vars:
6793
firewall:
6894
zone: internal
69-
interface_pci_id: 1af4:0001
95+
interface_pci_id: "{{ pci_id.stdout }}"
7096
state: disabled
7197
permanent: true
7298
always:

0 commit comments

Comments
 (0)