Add support for custom ports #38

lessfoobar · 2021-05-10T09:44:41Z

No description provided.

lessfoobar

fixed the two typos that I've made

richm · 2021-05-13T16:36:01Z

README.md

@@ -30,6 +27,9 @@ These are the variables that can be passed to the role:
 |`nbde_server_fetch_keys`| `no` | indicates whether we should fetch keys to the control node, in which case they will be placed in `nbde_server_keys_dir`. You **must** set `nbde_server_keys_dir` to use `nbde_server_fetch_keys`.
 |`nbde_server_deploy_keys`| `no` |indicates whether we should deploy the keys located in `nbde_server_keys_dir` directory to the remote hosts. You **must** set `nbde_server_keys_dir` to use `nbde_server_deploy_keys`.
 |`nbde_server_keys_dir`| | specifies a directory in the control node that contains keys to be deployed to the remote hosts. Keys located in the top level directory will be deployed to every remote host, while keys located within subdirectories named after the remote hosts  -- as per the inventory -- will be deployed only to these specific hosts. `nbde_server_keys_dir` **must** be an absolute path. You need to set this to use either `nbde_server_fetch_keys` and/or `nbde_server_deploy_keys`.
+|`nbde_server_port`|`80`| setup custom port which will be enabled in SELinux and firewalld.
+|`nbde_server_zone`|`public`| change the default zone where the port should be opened.


is this the firewalld zone?

yes - this should be nbde_server_firewall_zone

richm · 2021-05-13T16:41:10Z

tasks/tangd-custom-port.yml

+---
+# This task tells SELinux for the port that we want the tangd service to use
+- name: Allow the custom port for tangd_port_t in SELinux
+  community.general.seport:


please use the selinux system role - https://github.com/linux-system-roles/selinux

- name: Allow the custom port for tangd_port_t in SELinux include_role: name: linux-system-roles.selinux vars: selinux_ports: - ports: "{{ nbde_server_port }}" proto: tcp setype: tangd_port_t state: present

richm · 2021-05-13T16:42:45Z

tasks/tangd-custom-port.yml

+    # This tasks reload the daemons so the new changes take effect
+    - name: Reload the daemons so the new changes take effect
+      systemd:
+        daemon_reload: yes


This should be conditional i.e. when: __nbde_server_daemon_reload - then one of the tasks above should set __nbde_server_daemon_reload if the daemon needs to be reloaded

richm · 2021-05-13T16:45:31Z

[citest commit:23c512f81404b0f071d4713af730df2eb9f3bab1]

sergio-correia · 2021-05-20T10:35:28Z

tasks/tangd-custom-port.yml

+        path: /etc/systemd/system/tangd.socket.d/override.conf
+        block: |
+          [Socket]
+          ListenStream={{ nbde_server_port }}


ListenStream is a list, so if you just specify a port here, it will be like you are adding this new port to the list. You should first clear this list with an empty ListenStream= line prior to setting the new port. Check out https://www.freedesktop.org/software/systemd/man/systemd.socket.html#Options for more info.

Also, it would be great if you could add a test to make sure the server is using the new port (and ideally make also sure we are not using in the old one)

lessfoobar · 2021-06-09T08:41:33Z

Hello sorry for the delay I was on vacation. I've added the requested changes. The override file is taken from the rhel docs NBDE 10.3.6. If further changes are required please let me know.

richm · 2021-06-09T23:14:07Z

.cache/roles/nbde_server

@@ -0,0 +1 @@
+../..


What is this?

please remove this

richm · 2021-06-09T23:15:18Z

tasks/tangd-custom-port.yml

+---
+# This task tells SELinux for the port that we want the tangd service to use when distribution Fedora
+- name: Allow the custom port for tangd_port_t in SELinux
+  ansible.builtin.import_role:


Please don't use the FQCN ansible.builtin.import_role form - please use import_role

richm · 2021-06-09T23:22:42Z

tasks/tangd-custom-port.yml

+      - { ports: '{{ nbde_server_port }}', proto: 'tcp', setype: 'tangd_port_t', state: 'present' }
+  when: ansible_selinux.status == "enabled" and ansible_facts["distribution"] == "Fedora"
+
+# This task tells SELinux for the port that we want the tangd service to use when distribution RHEL


You don't need this. In fact, you don't need the when: ansible_selinux.status == "enabled" and ansible_facts["distribution"] == "Fedora" clause above.

The problem is that ansible_facts will return the distribution on the managed host, but the roles will be looked up on the controller host. I think the logic you are hoping for is if the controller host is RedHat, then use the rhel-system-roles.selinux role, but that will be much trickier to do.

And, to further complicate matters, if the user is using the Collection instead of the legacy role format, then the role name will be fedora.linux_system_roles.selinux or redhat.rhel_system_roles.selinux.

I think the best you can do here is to use import_role: name: selinux and hope that role path and collection path/collection keyword are configured appropriately.

Just use name: linux-system-roles.selinux - we have tooling in the rhel rpm spec to change it to rhel-system-roles and similar tooling when we build collections.

richm · 2021-07-06T17:11:04Z

ping - any updates ?

richm · 2021-07-21T14:56:56Z

ping - any updates ?

ping

lessfoobar · 2021-08-23T08:47:30Z

Sorry it took me a while to understand what you meant with the managed and controlled node thus the first commit. I've fixed that in the second one.

richm · 2021-10-11T20:49:48Z

[citest pending]

richm · 2021-10-11T21:43:10Z

[citest pending]

richm · 2021-10-20T15:40:05Z

[citest pending]

richm · 2022-01-10T19:25:30Z

tasks/tangd-custom-port.yml

+
+# This task makes sure that we add the new port to firewalld
+- name: Ensure the desired port is added to firewalld
+  firewalld:


please use the https://github.com/linux-system-roles/firewall role for this instead of the firewalld module, similar to how you used the selinux role for the selinux settings.

richm · 2022-04-27T15:23:20Z

ping

lessfoobar · 2022-08-09T03:30:55Z

@richm hope the role is fine now, sorry for the delay again ... I should edit my email settings to get notifications

Signed-off-by: lessfoobar <lessfoobar@pm.me>

richm · 2022-09-14T22:10:05Z

tasks/main-tang.yml

@@ -26,6 +26,10 @@
  when: (nbde_server_fetch_keys | bool) or (nbde_server_deploy_keys | bool)
  include_tasks: tang-key-management.yml

+- name: Open port in firewalld
+  when: (nbde_server_port != "80") or (nbde_server_firewall_zone != "public")


(nbde_server_port | int != 80)

nbde_server_port can be a string or an int - if nbde_server_port is an int e.g. nbde_server_port: 80 then 80 is not equal to "80"

I've had so much problems with mode and always quoting it, so for me by default all numbers should be quoted. But sure I can fix that also.

ansible/ansible#9196

I've had so much problems with mode and always quoting it, so for me by default all numbers should be quoted. But sure I can fix that also.

If you want to keep everything a string, you can do that too:
(nbde_server_port | string != "80")

But in the examples you added, you are using int instead of string e.g. https://github.com/linux-system-roles/nbde_server/pull/38/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R37 , https://github.com/linux-system-roles/nbde_server/pull/38/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R124, etc.

So whatever you decide to do, please be consistent

ansible/ansible#9196

nhosoi · 2022-09-23T23:21:54Z

tests/tests_tangd_custom_port.yml

+          set -euo pipefail
+          ss -tulpn | grep {{ nbde_server_port }} | awk -F' ' '{print $5}'
+      register: __open_ports_output
+      failed_when: __open_ports_output.stdout != "*:{{ nbde_server_port }}"


@sergio-correia, @lessfoobar, @richm, how must this right-hand side be exactly matched?
The test playbook tests_tangd_custom_port.yml passes on rhel-8 and rhel-9, but it fails on rhel-7 as follows:

TASK [check if port is open] *************************************************** task path: /home/nhosoi/linux-system-roles/nbde_server/tests/tests_tangd_custom_port.yml:12 Friday 23 September 2022 15:52:49 -0700 (0:00:00.651) 0:00:40.444 ****** [WARNING]: conditional statements should not include jinja2 templating delimiters such as {{ }} or {% %}. Found: __open_ports_output.stdout != "*:{{ nbde_server_port }}" fatal: [/home/nhosoi/.cache/linux-system-roles/rhel-7.qcow2]: FAILED! => { "changed": true, "cmd": "set -euo pipefail\nss -tulpn | grep 7500 | awk -F' ' '{print $5}'", "delta": "0:00:00.123563", "end": "2022-09-23 18:52:49.938979", "failed_when_result": true, "rc": 0, "start": "2022-09-23 18:52:49.815416" } STDOUT: [::]:7500

Can we consider it is successful if the ports match?

Please note that rhel-7 is a supported platform both as a control and a managed node.

Suggested change

failed_when: __open_ports_output.stdout != "*:{{ nbde_server_port }}"

failed_when: not __open_ports_output.stdout is search(':' ~ (nbde_server_port | string) ~ '$')

nhosoi · 2022-09-25T20:03:29Z

Thanks, @richm. It passed the rhel-7 CI with a tiny change. The filter str had to be string.

-      failed_when: __open_ports_output.stdout != "*:{{ nbde_server_port }}"
+      failed_when: not __open_ports_output.stdout is search(':' ~ (nbde_server_port | string) ~ '$')

@lessfoobar, could you please apply the change to tests_tangd_custom_port.yml?
Thanks.

richm · 2022-09-25T23:55:01Z

tests/tests_tangd_custom_port.yml

+      failed_when: __open_ports_output.stdout != "tcp"
+
+    - name: check if port is opened in firewall
+      shell:


Suggested change

shell:

command: firewall-cmd --zone {{ nbde_server_firewall_zone }} --query-port {{ nbde_server_port }}/tcp

changed_when: false

Then you don't need shell, awk, grep, register, failed_when, etc.

yeah ... really not sure why I didn't went for the firewall-cmd command ... glad you found that out. Way more simpler than my solution.

richm · 2022-09-25T23:56:04Z

tests/tests_tangd_custom_port.yml

+      register: __firewall_output
+      failed_when: __firewall_output.stdout != "{{ nbde_server_port }}/tcp"
+
+    - name: check if firewall zone is set


I don't think you need this check at all - the firewall-cmd --zone {{ nbde_server_firewall_zone }} --query-port {{ nbde_server_port }}/tcp will check that the given zone exists and that the port is set in the given zone.

nhosoi

lgtm

richm · 2022-09-27T19:42:36Z

tests/tests_tangd_custom_port.yml

+          set -euo pipefail
+          ss -tulpn | grep {{ nbde_server_port }} | awk -F' ' '{print $5}'
+      register: __open_ports_output
+      failed_when: not __open_ports_output.stdout is search(':' ~ (nbde_server_port | string) ~ '$')


Suggested change

failed_when: not __open_ports_output.stdout is search(':' ~ (nbde_server_port | string) ~ '$')

failed_when: not __open_ports_output.stdout is

search(':' ~ (nbde_server_port | string) ~ '$')

richm · 2022-09-27T19:42:56Z

tests/tests_tangd_custom_port.yml

+      failed_when: __open_ports_output.stdout != "tcp"
+
+    - name: check if port is opened in firewall
+      command: firewall-cmd --zone {{ nbde_server_firewall_zone }} --query-port {{ nbde_server_port }}/tcp


Suggested change

command: firewall-cmd --zone {{ nbde_server_firewall_zone }} --query-port {{ nbde_server_port }}/tcp

command: >-

firewall-cmd --zone {{ nbde_server_firewall_zone }} --query-port

{{ nbde_server_port }}/tcp

thing :D

richm · 2022-09-27T20:01:25Z

Sorry about that - it may seem that these linters are pedantic, but they are the gatekeepers to being able to publish on Galaxy and Automation Hub

sergio-correia · 2022-09-28T13:14:55Z

tasks/tangd-custom-port.yml

+    - name: Create a directory if it does not exist
+      file:
+        path: /etc/systemd/system/tangd.socket.d
+        state: directory


This will already create the directory if it does not exist, no? There is no need to check/register in the previous task and run this conditionally here.

richm · 2022-09-28T14:22:22Z

tests/templates

@@ -0,0 +1 @@
+../../../templates


This should be in tests/roles/linux-system-roles.nbde_server/ instead of tests/

Signed-off-by: lessfoobar <lessfoobar@pm.me>

richm · 2022-09-28T17:36:15Z

tasks/main-tang.yml

@@ -26,6 +26,10 @@
  when: (nbde_server_fetch_keys | bool) or (nbde_server_deploy_keys | bool)
  include_tasks: tang-key-management.yml

+- name: Open port in firewalld
+  when: (nbde_server_port != 80) or (nbde_server_firewall_zone != "public")


Suggested change

when: (nbde_server_port != 80) or (nbde_server_firewall_zone != "public")

when: (nbde_server_port | int != 80) or (nbde_server_firewall_zone != "public")

This should fix the error from the log:

ERROR: The following numeric operations found at runtime need int or float cast filters: [nbde_server_port] [ne] [80] at tasks/main-tang.yml:29

the issue is that since nbde_server_port is a public variable, there is no guarantee that the data type will be int e.g. if the user does ansible-playbook -e nbde_server_port=80 ... then the variable will be a string data type and the comparison (nbde_server_port != 80) will always fail. Using | int to "cast" the type will ensure that the comparison is done correctly, and will also ensure that an error is generated if the data type cannot be cast to int.

richm · 2022-09-29T19:52:31Z

[citest]

richm

lgtm - the test failure appears to be unrelated - if there are no objections I'll merge this

to manage the custom ports implemented in "Add support for custom ports (linux-system-roles#38)" - Introduce nbde_server_manage_firewall to enable the firewall role to manage the nbde server port. - Introduce nbde_server_manage_selinux to enable the selinux role to manage the nbde server port.

to manage the custom ports implemented in "Add support for custom ports (linux-system-roles#38)" - Introduce nbde_server_manage_firewall to enable the firewall role to manage the nbde server port. - Introduce nbde_server_manage_selinux to enable the selinux role to manage the nbde server port. Signed-off-by: Noriko Hosoi <nhosoi@redhat.com>

to manage the custom ports implemented in "Add support for custom ports (linux-system-roles#38)" - If nbde_server_manage_firewall is set to true, use the firewall role to manage the nbde server port. - If nbde_server_manage_selinux is set to true, use the selinux role to manage the nbde server port. Signed-off-by: Noriko Hosoi <nhosoi@redhat.com>

[1.2.0] - 2022-11-01 -------------------- ### New Features - Add support for custom ports (linux-system-roles#38) - Introduce nbde_server_manage_firewall and nbde_server_manage_selinux to manage the custom ports implemented in "Add support for custom ports (linux-system-roles#38)" - If nbde_server_manage_firewall is set to true, use the firewall role to manage the nbde server port. - If nbde_server_manage_selinux is set to true, use the selinux role to manage the nbde server port. ### Bug Fixes - none ### Other Changes - none Signed-off-by: Rich Megginson <rmeggins@redhat.com>

[1.2.0] - 2022-11-01 -------------------- ### New Features - Add support for custom ports (#38) - Introduce nbde_server_manage_firewall and nbde_server_manage_selinux to manage the custom ports implemented in "Add support for custom ports (#38)" - If nbde_server_manage_firewall is set to true, use the firewall role to manage the nbde server port. - If nbde_server_manage_selinux is set to true, use the selinux role to manage the nbde server port. ### Bug Fixes - none ### Other Changes - none Signed-off-by: Rich Megginson <rmeggins@redhat.com> Signed-off-by: Rich Megginson <rmeggins@redhat.com>

Ivan Ivanov added 3 commits May 10, 2021 11:32

Add support for custom ports

050939f

typo

01bf854

changed exist to exists now it all works

23c512f

lessfoobar commented May 10, 2021

View reviewed changes

richm reviewed May 13, 2021

View reviewed changes

sergio-correia reviewed May 20, 2021

View reviewed changes

Ivan Ivanov added 2 commits June 9, 2021 10:31

made the requested changes

59aa205

fix

eea9d5e

type-o

99cd26d

richm reviewed Jun 9, 2021

View reviewed changes

Ivan Ivanov added 2 commits August 23, 2021 10:37

requested changes were implemented

f91df23

removed the rhel-system-roles

42af42a

richm reviewed Jan 10, 2022

View reviewed changes

lessfoobar added 2 commits August 9, 2022 05:24

Merge branch 'linux-system-roles:master' into master

740d068

swapped firewalld module with the LSR.firewall

81d01c5

removed FQDN for import_role

9d6defb

add fixes suggested by nhosoi

efb96d3

Signed-off-by: lessfoobar <lessfoobar@pm.me>

richm reviewed Sep 14, 2022

View reviewed changes

Ivan Ivanov added 2 commits September 15, 2022 11:02

fix for port possibility as int

6f22e1c

Merge branch 'master' of https://github.com/lessfoobar/nbde_server

c112230

nhosoi reviewed Sep 23, 2022

View reviewed changes

richm reviewed Sep 25, 2022

View reviewed changes

implement the proposed changes from richm & nhosoi

344462b

nhosoi approved these changes Sep 27, 2022

View reviewed changes

richm reviewed Sep 27, 2022

View reviewed changes

lessfoobar added 2 commits September 27, 2022 21:47

sorry I have word wrap and its hard to follow this 80 chars per line

b515a68

thing :D

yeap forgot the empty space on the two lines. Fixed now

ee98762

sergio-correia reviewed Sep 28, 2022

View reviewed changes

richm reviewed Sep 28, 2022

View reviewed changes

remove the stat and moved the templates shortcut

0b32b1e

Signed-off-by: lessfoobar <lessfoobar@pm.me>

richm reviewed Sep 28, 2022

View reviewed changes

added the int conversion

0dbc506

richm approved these changes Sep 29, 2022

View reviewed changes

richm merged commit df7e844 into linux-system-roles:master Sep 30, 2022

nhosoi mentioned this pull request Oct 10, 2022

Introduce nbde_server_manage_firewall and nbde_server_manage_selinux #64

Merged

richm mentioned this pull request Nov 1, 2022

Version 1.2.0 - CHANGELOG.md [citest skip] #65

Merged

	failed_when: __open_ports_output.stdout != "*:{{ nbde_server_port }}"
	failed_when: not __open_ports_output.stdout is search(':' ~ (nbde_server_port \| string) ~ '$')

	shell:
	command: firewall-cmd --zone {{ nbde_server_firewall_zone }} --query-port {{ nbde_server_port }}/tcp
	changed_when: false

	failed_when: not __open_ports_output.stdout is search(':' ~ (nbde_server_port \| string) ~ '$')
	failed_when: not __open_ports_output.stdout is
	search(':' ~ (nbde_server_port \| string) ~ '$')

	when: (nbde_server_port != 80) or (nbde_server_firewall_zone != "public")
	when: (nbde_server_port \| int != 80) or (nbde_server_firewall_zone != "public")

		@@ -0,0 +1 @@
		../..

		@@ -0,0 +1 @@
		../../../templates

Add support for custom ports #38

Add support for custom ports #38

Conversation

lessfoobar commented May 10, 2021

lessfoobar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm commented May 13, 2021

Choose a reason for hiding this comment

lessfoobar commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm commented Jul 6, 2021

richm commented Jul 21, 2021

lessfoobar commented Aug 23, 2021

richm commented Oct 11, 2021

richm commented Oct 11, 2021

richm commented Oct 20, 2021

Choose a reason for hiding this comment

richm commented Apr 27, 2022

lessfoobar commented Aug 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm Sep 25, 2022 • edited

Choose a reason for hiding this comment

nhosoi commented Sep 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhosoi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm commented Sep 29, 2022

richm left a comment

Choose a reason for hiding this comment

richm Sep 25, 2022 •

edited