refactor: use firewall role instead of module#19
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors firewall configuration tasks to use the shared fedora.linux_system_roles.firewall role instead of the ansible.posix.firewalld module, removes direct firewalld service checks, updates README docs, and adds the new collection dependency. Sequence diagram for the new firewall configuration via shared rolesequenceDiagram
actor Operator
participant AnsibleController
participant TrusteeRole
participant FirewallRole as fedora_linux_system_roles_firewall_role
participant TargetHost
Operator->>AnsibleController: Run trustee_server role
AnsibleController->>TrusteeRole: Execute tasks/secret_registration_server.yml
TrusteeRole->>FirewallRole: include_role name=fedora.linux_system_roles.firewall
FirewallRole->>FirewallRole: Process firewall var
FirewallRole->>TargetHost: Ensure port trustee_server_secret_registration_listen_port/tcp enabled
AnsibleController->>TrusteeRole: Execute tasks/trustee_quadlet.yml
TrusteeRole->>FirewallRole: include_role name=fedora.linux_system_roles.firewall
FirewallRole->>FirewallRole: Process firewall var
FirewallRole->>TargetHost: Ensure port 8080/tcp enabled
Flow diagram for updated firewall task in secret_registration_server.ymlflowchart TD
A[Start secret_registration_server tasks] --> B[Run secret registration server systemd service task]
B --> C[Allow secret registration server port in firewall]
C --> D[include_role name=fedora.linux_system_roles.firewall]
D --> E[Set firewall var with trustee_server_secret_registration_listen_port/tcp
permanent=true
runtime=true
state=enabled]
E --> F[Firewall role applies rule on managed host]
F --> G[Append secret_registration_server to __trustee_server_services]
G --> H[End tasks]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
We already have the firewall role in the system roles collection. Use that instead of the firewalld module which adds an extra dependency. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tasks/trustee_quadlet.yml" line_range="65-68" />
<code_context>
- ansible.builtin.service_facts:
- no_log: "{{ ansible_verbosity < 2 }}"
-
- name: Allow port 8080 in firewall
- ansible.posix.firewalld:
- port: "8080/tcp"
- permanent: true
- immediate: true
- state: enabled
- when: (ansible_facts.services | default({})).get('firewalld.service', {}).get('state', '') == 'running'
+ ansible.builtin.include_role:
+ name: fedora.linux_system_roles.firewall
+ vars:
+ firewall:
+ - port: "8080/tcp"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The new firewall role invocation always runs, changing behavior from the previous firewalld-only conditional.
Previously this only changed the firewall when `firewalld.service` was running; now the role runs unconditionally and will manage whichever backend it detects. If you still intend “no-op unless firewalld is active”, consider restoring that behavior via a conditional around the role invocation or by setting role variables to restrict backend selection, to avoid failures or unintended changes on hosts with a different (or no) firewall backend.
Suggested implementation:
```
- name: Allow port 8080 in firewall
ansible.builtin.include_role:
name: fedora.linux_system_roles.firewall
vars:
firewall:
- port: "8080/tcp"
permanent: true
when: (ansible_facts.services | default({})).get('firewalld.service', {}).get('state', '') == 'running'
```
To make this condition work, ensure that service facts are gathered before this task runs, for example via:
- A task like `- ansible.builtin.service_facts:` earlier in this play or
- A play-level `gather_facts: true` plus any existing mechanism you use to populate `ansible_facts.services`.
If `service_facts` was removed in the same PR, it should be reintroduced somewhere appropriate before this task.
</issue_to_address>
### Comment 2
<location path="README.md" line_range="99" />
<code_context>
-1. Downloads the Podman Quadlets from designated repo
+1. Installs the Podman Quadlets provided by the role
2. Generates all required certificates of Trustee server components
3. Add KBS port 8080 to firewalld
4. Enables the services by default
</code_context>
<issue_to_address>
**suggestion (typo):** Use "Adds" instead of "Add" for subject-verb agreement.
To keep subject-verb agreement and align with the other items ("Generates", "Enables"), update this line to: `3. Adds KBS port 8080 to firewalld`.
```suggestion
3. Adds KBS port 8080 to firewalld
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 1. Downloads the Podman Quadlets from designated repo | ||
| 1. Installs the Podman Quadlets provided by the role | ||
| 2. Generates all required certificates of Trustee server components | ||
| 3. Add KBS port 8080 to firewalld |
There was a problem hiding this comment.
suggestion (typo): Use "Adds" instead of "Add" for subject-verb agreement.
To keep subject-verb agreement and align with the other items ("Generates", "Enables"), update this line to: 3. Adds KBS port 8080 to firewalld.
Suggested change
| 3. Add KBS port 8080 to firewalld | |
| 3. Adds KBS port 8080 to firewalld |
richm
force-pushed
the
refactor-use-firewall-role
branch
from
adc51c1 to
2757d0f
Compare
Contributor
Author
|
[citest] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We already have the firewall role in the system roles collection. Use that instead
of the firewalld module which adds an extra dependency.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Replace direct firewalld module usage with the shared firewall system role and update related documentation and collection requirements.
Enhancements:
Build:
Documentation: