Use the firewall role and the selinux role from the cockpit role #76

nhosoi · 2022-09-28T22:00:22Z

Introduce cockpit_manage_firewall to use the firewall role to manage the cockpit service. Default to false - means the firewall role is not used.
Introduce cockpit_manage_selinux to use the selinux role to manage the ports in the cockpit service. Assign websm_port_t to the cockpit service ports. Default to false - means the selinux role is not used.
Add the test check task tasks/check_firewall_selinux.yml for verify the ports status.
Add meta/collection-requirements.yml.

nhosoi · 2022-09-28T22:14:17Z

[citest]

martinpitt

Thanks @nhosi! Test-wise you got a lot further than me, I'm happy for you to take over, and I'll close my PR #73. I have several small comments, and I'd really appreciate if you could split this up in to separate commits (one for public, one for firewalld, one for selinux). Cheers!

martinpitt · 2022-09-29T14:28:42Z

README.md

+    `fedora.linux_system_roles` collection, if `cockpit_manage_firewall`
+    and `cockpit_manage_selinux` is set to true, respectively.
+    (Please see also `cockpit_manage_firewall` and `cockpit_manage_selinux`
+     in [`Role Variables`](#role-variables).


Missing ) at the end -- the ) ist for closing the link, but you start the sentence with a (. Or alternatively, drop that; I even think that's better.

martinpitt · 2022-09-29T14:30:13Z

README.md

+    (Please see also `cockpit_manage_firewall` and `cockpit_manage_selinux`
+     in [`Role Variables`](#role-variables).
+
+    If the `cockpit` is a role from the `fedora.linux_system_roles`


The first "the" sounds weird. "If cockpit is a role..." perhaps.

README.md

martinpitt · 2022-09-29T14:32:17Z

README.md

@@ -81,17 +96,35 @@ Configure settings in the /etc/cockpit/cockpit.conf file.  See [`man cockpit.con
    cockpit_port: 9090
 Cockpit runs on port 9090 by default. You can change the port with this option.

-Note that the default SELinux policy does not allow Cockpit to listen to anything else than port 9090, so you need to allow that first, with e.g.
+    cockpit_manage_firewall: false


The bool options above use "yes/no", this should be consistent. Either change them to true/false, or use yes/no here.

We really should be using true and false everywhere - https://github.com/redhat-cop/automation-good-practices/blob/main/coding_style/README.adoc#yaml-and-jinja2-syntax "Use true and false for boolean values in playbooks."

but yes, it will look strange here if we are not consistent, but we may want to convert everything in the future - note that some of the other roles have already done this - linux-system-roles/ha_cluster#60

Ok. yes/no for cockpit for now. Let me update README and tests.
I assume the change is not needed in the files in tasks and defaults... Right?

Ok. yes/no for cockpit for now. Let me update README and tests. I assume the change is not needed in the files in tasks and defaults... Right?

Please use yes and no in all of the code you are adding.

martinpitt · 2022-09-29T14:37:20Z

README.md

+Boolean flag allowing to configure firewall using the firewall role.
+Manage the cockpit firewall service.


This sounds a bit redundant and confusing. Perhaps something like

Boolean variable to control the cockpit firewalld service with the firewall role.

(I'm not sure if you prefer "firewalld" as in the product, or "firewall" as in the concept - IMHO the product is a bit more specific)

martinpitt · 2022-09-29T14:48:01Z

tasks/selinux.yml

+                         'setype': 'websm_port_t',
+                         'state': 'present', 'local': 'true'}] }}"
+  when:
+    - cockpit_manage_selinux | bool


I'd add an additional condition that cockpit_port is defined (and remove that from line 7), so that this isn't even invoked when the port doesn't get customized (the common case). Avoids traps and is faster.

hmm - since cockpit_port is part of the public api of the role, it should be defined in defaults/main.yml as null - so check for cockpit_port is not none (null in YAML == none in Jinja)

hmm - since cockpit_port is part of the public api of the role, it should be defined in defaults/main.yml as null - so check for cockpit_port is not none (null in YAML == none in Jinja)

@richm, @martinpitt, README.md describes cockpit_port as follows.

cockpit_port: 9090 Cockpit runs on port 9090 by default. You can change the port with this option.

~~This means, it should be defined as 9090 in defaults/main.yml?~~

We should distinguish the value was set by customers or not.

hmm - since cockpit_port is part of the public api of the role, it should be defined in defaults/main.yml as null - so check for cockpit_port is not none (null in YAML == none in Jinja)

Is it ok for me to make this change in this pr?

Yes, please.

tasks/firewall.yml

martinpitt · 2022-09-29T14:50:27Z

tests/tests_certificate.yml

@@ -23,7 +23,12 @@
  vars:
    cockpit_packages: minimal
  roles:
-    - linux-system-roles.cockpit
+    - role: linux-system-roles.cockpit
+      public: true


This public is an unrelated change, and should be a separate commit. Same for the other instances below.

I haven't checked, but it looks like check_firewall_selinux.yml requires role private variables in order to check the values? If so, then I think public: true is justified

Right. check_firewall_selinux.yml needs it. And I'm going to drop the unnecessary public: true other than tests_config.yml, tests_packages_full.yml, and tests_port.yml along with check_firewall_selinux.yml.

@nhosoi : I see -- I didn't realize that it's really the new tasks/* files which require public:. That's fine then.

martinpitt · 2022-09-29T14:51:35Z

tests/tests_certificate_runafter.yml

+        - name: test - ensure cockpit_port is configured for firewall and selinux
+          include_tasks: tasks/check_firewall_selinux.yml


Please don't add this to all tests -- separation of concerns, this is unnecessary redundancy. The one place where it really matters is tests_port.yml, and perhaps tests_default.yml in addition, but none of the others.

You are right. In the beginning, I was going to check if the manage variables are false, the port is not configured, which I dropped as it's not very useful. I'm going to delete the unnecessary checks.

martinpitt · 2022-09-29T14:53:24Z

tests/tests_default.yml

+        - name: test - ensure cockpit_port is configured for firewall and selinux
+          include_tasks: tasks/check_firewall_selinux.yml


Note that the default Fedora and RHEL configuration is fine by itself -- this should not mangle anything in the default case. The tasks file does not do anything harmful, so it's fine to call it here; I just wanted to point it out.

richm · 2022-09-29T15:18:55Z

hmm - also looks like we need to change the tests so that you can run multiple tests at the same time on the same controller:

fatal: destination path '/WORKDIR/dist-git-cockpit-use_roles-hJbYsU/tests/roles/linux-system-roles.certificate' already exists and is not an empty directory.

but this is unrelated to this PR - we had to change some of the other roles to do something similar e.g. linux-system-roles/nbde_client#80 so that each test gets its own temp directory

nhosoi · 2022-09-29T15:30:10Z

preciate if you could split this up in to separate commits (one for public, one for firewalld, one for selinux). Cheers!

~~How about the test script, tests/tasks/check_firewall_selinux.yml? Do you want to make it into two files; one for firewall and another for selinux?~~

~~And should README.md needs to be separated, as well? Some parts are a bit difficult to split clearly, especially the Requirements section...~~

Let me split this pr into 1) firewall, 2) selinux, 3) README, and 4) tests.
Hope it's ok.
@martinpitt, ^^^^^^

martinpitt · 2022-09-29T18:06:04Z

@nhosoi : a new feature and a test should always be in the same commit, as they belong together. Same for the documentation. The point is not to make commits as small as possible, but only do one thing/feature/bug fix/change per commit (including the corresponding docs, tests, etc.). That makes them easier to understand/review, possible to bisect, and backport.

I leave it up to you whether you want to split tests/tasks/check_firewall_selinux.yml; IMHO the functionality belongs together, and I'd even rename it to check_port.yml. Then the "firewall" commit would do the firewalld check there, and the "selinux" commit would add the web_sm_t check to the same file.

nhosoi · 2022-09-30T23:19:50Z

[citest]

nhosi · 2022-10-01T07:41:17Z

Thanks @nhosi! Test-wise you got a lot further than me, I'm happy for you to take over, and I'll close my PR #73. I have several small comments, and I'd really appreciate if you could split this up in to separate commits (one for public, one for firewalld, one for selinux). Cheers!

Hi! You tagged a wrong GitHub account. I will unsubscribe this chat. Br, @nhosi

nhosoi · 2022-10-02T23:48:02Z

[citest]

nhosoi · 2022-10-03T01:39:14Z

[citest]

nhosoi · 2022-10-03T05:07:54Z

[citest]

richm · 2022-10-03T14:02:03Z

tests/tasks/check_port.yml

+          package:
+            name:
+              - libselinux-python3
+              - policycoreutils-python3


looks like semanage on Fedora is provided by policycoreutils-python-utils

nhosoi · 2022-10-03T16:36:00Z

[citest]

richm · 2022-10-03T19:02:14Z

ok - looks good - please rebase on top of the latest master branch and we can merge

in defaults/main.yml as null.

- Introduce cockpit_manage_firewall to use the firewall role to manage the cockpit service. Default to false - means the firewall role is not used. - Add the test check task tasks/check_port.yml for verifying the ports status. - Add meta/collection-requirements.yml.

- Introduce cockpit_manage_selinux to use the selinux role to manage the ports in the cockpit service. Assign websm_port_t to the cockpit service ports. Default to false - means the selinux role is not used. - Add a test playbook tests_port2.yml to check port 443 when cockpit_manage_firewall and cockpit_manage_selinux are yes.

nhosoi · 2022-10-03T20:02:46Z

ok - looks good - please rebase on top of the latest master branch and we can merge

Thank you, @richm. Done!

richm · 2022-10-03T20:55:53Z

spoke too soon - looks like merge is blocked - @martinpitt needs to review, and clear the requested changes flag.

martinpitt

Very cool, thanks @nhosoi !

[1.4.0] - 2022-11-01 -------------------- ### New Features - Use the firewall role and the selinux role from the cockpit role (linux-system-roles#76) Since cockpit_port is a public api of the cockpit role, define it in defaults/main.yml as null. - Introduce cockpit_manage_firewall to use the firewall role to manage the cockpit service. Default to false - means the firewall role is not used. - Add the test check task tasks/check_port.yml for verifying the ports status. - Add meta/collection-requirements.yml. - Introduce cockpit_manage_selinux to use the selinux role to manage the ports in the cockpit service. Assign websm_port_t to the cockpit service ports. Default to false - means the selinux role is not used. - Use the certificate role to create the cert and the key (linux-system-roles#78) - Introduce a variable cockpit_certificates to set the certificate_requests. - Update README so that using the certificate role is recommended. Add a check and README note for not supporting creating a self-signed certificate on RHEL/CentOS-7. ### Bug Fixes - none ### Other Changes - workflows: Add integration-tests for Ubuntu 22.04 (linux-system-roles#68) The current LTS 22.04 is the more interesting target. This detects bugs like [1]. Keep 20.04 running as well for the time being. [1] linux-system-roles/certificate#130 - Clone the certificate role in the temporary dir. (linux-system-roles#77) Signed-off-by: Rich Megginson <[email protected]>

[1.4.0] - 2022-11-01 -------------------- ### New Features - Use the firewall role and the selinux role from the cockpit role (#76) Since cockpit_port is a public api of the cockpit role, define it in defaults/main.yml as null. - Introduce cockpit_manage_firewall to use the firewall role to manage the cockpit service. Default to false - means the firewall role is not used. - Add the test check task tasks/check_port.yml for verifying the ports status. - Add meta/collection-requirements.yml. - Introduce cockpit_manage_selinux to use the selinux role to manage the ports in the cockpit service. Assign websm_port_t to the cockpit service ports. Default to false - means the selinux role is not used. - Use the certificate role to create the cert and the key (#78) - Introduce a variable cockpit_certificates to set the certificate_requests. - Update README so that using the certificate role is recommended. Add a check and README note for not supporting creating a self-signed certificate on RHEL/CentOS-7. ### Bug Fixes - none ### Other Changes - workflows: Add integration-tests for Ubuntu 22.04 (#68) The current LTS 22.04 is the more interesting target. This detects bugs like [1]. Keep 20.04 running as well for the time being. [1] linux-system-roles/certificate#130 - Clone the certificate role in the temporary dir. (#77) Signed-off-by: Rich Megginson <[email protected]> Signed-off-by: Rich Megginson <[email protected]>

nhosoi force-pushed the use_roles branch from 114aef9 to 29bcf00 Compare September 28, 2022 22:06

nhosoi force-pushed the use_roles branch from 29bcf00 to aeef11d Compare September 28, 2022 22:22

nhosoi mentioned this pull request Sep 28, 2022

Automatically handle SELinux for port customization #73

Closed

This comment was marked as resolved.

Sign in to view

martinpitt requested changes Sep 29, 2022

View reviewed changes

nhosoi mentioned this pull request Sep 29, 2022

Clone the certificate role in the temporary dir. #77

Merged

nhosoi force-pushed the use_roles branch from aeef11d to bb0f0a3 Compare September 30, 2022 23:15

nhosoi force-pushed the use_roles branch from bb0f0a3 to 3024999 Compare October 2, 2022 22:48

nhosoi force-pushed the use_roles branch from 3024999 to 97c0a14 Compare October 3, 2022 01:31

nhosoi force-pushed the use_roles branch from 97c0a14 to 1c3a476 Compare October 3, 2022 04:58

richm reviewed Oct 3, 2022

View reviewed changes

nhosoi force-pushed the use_roles branch 2 times, most recently from b9624b8 to c0989b5 Compare October 3, 2022 16:29

nhosoi added 3 commits October 3, 2022 13:01

Since cockpit_port is a public api of the cockpit role, define it

a02873d

in defaults/main.yml as null.

nhosoi force-pushed the use_roles branch from c0989b5 to 0852ffe Compare October 3, 2022 20:01

martinpitt approved these changes Oct 4, 2022

View reviewed changes

richm merged commit b3ad839 into linux-system-roles:master Oct 4, 2022

richm mentioned this pull request Nov 1, 2022

Version 1.4.0 - CHANGELOG.md [citest skip] #79

Merged

		Boolean flag allowing to configure firewall using the firewall role.
		Manage the cockpit firewall service.

		- name: test - ensure cockpit_port is configured for firewall and selinux
		include_tasks: tasks/check_firewall_selinux.yml

Use the firewall role and the selinux role from the cockpit role #76

Use the firewall role and the selinux role from the cockpit role #76

Conversation

nhosoi commented Sep 28, 2022

nhosoi commented Sep 28, 2022

This comment was marked as resolved.

martinpitt 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

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

nhosoi Sep 29, 2022 • edited Loading

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

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

nhosoi commented Sep 29, 2022 • edited Loading

martinpitt commented Sep 29, 2022

nhosoi commented Sep 30, 2022

nhosi commented Oct 1, 2022

nhosoi commented Oct 2, 2022

nhosoi commented Oct 3, 2022

nhosoi commented Oct 3, 2022

Choose a reason for hiding this comment

nhosoi commented Oct 3, 2022

richm commented Oct 3, 2022

nhosoi commented Oct 3, 2022

richm commented Oct 3, 2022

martinpitt left a comment

Choose a reason for hiding this comment

nhosoi Sep 29, 2022 •

edited

Loading

nhosoi commented Sep 29, 2022 •

edited

Loading