firewalld-formula icon indicating copy to clipboard operation
firewalld-formula copied to clipboard

[TALK] Why you did'nt use the states firewalld ?

Open ArthurREGNARD opened this issue 5 years ago • 4 comments

I would like to know why you did'nt use the firewalld Modules and States?

For exemple your doing in the init.sls file:

reload_firewalld:
  cmd.wait:
    - name: 'firewall-cmd --reload'
    - require:
      - service: service_firewalld

But the Modules do the same things:

reload_firewalld:
  modules.wait:
    - name: firewalld.reload_rules
    - require:
      - service: service_firewalld

Why you don't use the states firewalld.present ? This states reload firewalld if something changed.

ArthurREGNARD avatar Feb 14 '20 16:02 ArthurREGNARD

@ArthurREGNARD Thanks for raising this issue. If you look at the blame, you can see that this section was added to the formula almost 3 years ago:

  • https://github.com/saltstack-formulas/firewalld-formula/blame/49377870380d327039e0e2fb0e8c6f3aba08b775/firewalld/init.sls#L49-L51
    • I'm not sure @amendlik is around to give more info.

However, if you're willing to provide a PR, we can test if it does the job. We have Kitchen, Travis and InSpec set up for this formula, so I'm sure we can confirm things are working as necessary.


Linking to the state that you've mentioned:

  • https://docs.saltstack.com/en/latest/ref/states/all/salt.states.firewalld.html#salt.states.firewalld.present

myii avatar Feb 14 '20 16:02 myii

Looking at the dates, the systemd.reload_rules was not present until v2016.11 (released in November 2016), and this change to the systemd formula was made in March 2017. I probably just didn't have the very latest version of Salt when I created PR #10.

Moving to the module execution, rather than the command, seems like a good move to me.

amendlik avatar Feb 14 '20 18:02 amendlik

@amendlik Thanks for the feedback, that's appreciated.

myii avatar Feb 14 '20 18:02 myii

@ArthurREGNARD you are correct with the reload module

The saltstack firewalld states are a bit different. Sometimes they are a bit behind the curve, and provide fewer options.

aboe76 avatar Feb 14 '20 20:02 aboe76