-
Notifications
You must be signed in to change notification settings - Fork 837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix some lints and firewalld policies #323
Conversation
You will need to sign all commits submitted. See https://github.com/k3s-io/k3s-ansible/pull/323/checks?check_run_id=23445098776 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at my suggestions. We can default them but allow the end user to modify them.
roles/prereq/tasks/main.yml
Outdated
@@ -73,7 +73,7 @@ | |||
- name: If firewalld enabled, open api port | |||
ansible.posix.firewalld: | |||
port: "{{ api_port }}/tcp" | |||
zone: trusted | |||
zone: internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not be a variable that is managed by the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vrsarin Do you mean fw zone for inter-node traffic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am right, Linux allows the creation of a custom zone, which could be different from an OS-defined zone.
Do you know if hardcoding will benefit you here?
Today, you want to change it to internal. Tomorrow, I may want to revert back to trusted or my custom zone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New ansible role config option is added.
state: enabled | ||
permanent: true | ||
immediate: true | ||
with_items: | ||
- 8472/udp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be left for the user to provide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the internal zone, only the nodes would be added. It is not a big risk to open ports to different network backends. I believe this will generate less hassle for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed, The user can manage adding or removing ports in a separate task.
Fix playbooks folder name according to the ansible collection scheme... Refactor firewalld policy Signed-off-by: Przemyslaw Sztoch <[email protected]>
# List of public services | ||
k3s_firewalld_public_ports: | ||
- 80/tcp | ||
- 443/tcp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K3s does not require any public ports to function, so we should not have any set as default. Only user provided ports will be opened.
# List of public services | |
k3s_firewalld_public_ports: | |
- 80/tcp | |
- 443/tcp | |
# List of public services | |
k3s_firewalld_public_ports: [] |
@psztoch Did you still want to handle this PR, or are you going to just carry patches in your fork? |
Linked Issues
Partially close #321