Skip to content
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

Optimize firewalld state #49811

Merged
merged 22 commits into from Oct 16, 2018

Conversation

Projects
None yet
5 participants
@nhavens
Copy link
Contributor

commented Sep 27, 2018

What does this PR do?

This PR removes refactors the firewalld.present state function such that it only calls firewalld execution module functions when necessary. This significantly improves the firewalld.present state function performance.

What issues does this PR fix or reference?

Fixes #44979

Previous Behavior

Previously, the firewalld.present state function called many firewalld execution module functions even if the calls were unnecessary based on the sls file being processed.

New Behavior

Only call firewalld execution module functions if necessary based on the sls file being processed.

Tests written?

No - functionality should not have changed. Existing tests should suffice.

Commits signed with GPG?

Yes

nhavens added some commits Sep 27, 2018

optimize firewalld.present icmp block handling
- Only call firewalld execution module icmp block functions if
necessary.
- Fixes #44979
optimize firewalld.present open port handling
- only call firewalld ports functions if necessary
- Fixes #44979
optimize firewalld.present port forward handling
- Only call firewalld port forward functions if necessary
- Fixes #44979
optimize firewalld.present service handling
- only call firewalld service functions if necessary
- Fixes #44979
optimize firewalld.present interface handling
- only call firewalld interface functions if necessary
- Fixes #44979
optimize firewalld.present source handling
- only call firewalld source functions if necessary
- Fixes #44979
optimize firewalld.present rich rule handling
- only call firewalld rich rule functions if necessary
- Fixes #44979
resolve lint error in firewalld state
- 'String formatting used in logging' for invalid ICMP type
fix invalid icmp type handling in firewalld state
- Previously, it would log an error message for an invalid ICMP type and
not try to add it as an icmp-block. However, it would still report in
ret['changes'] that it had been added.
- Now, we will still log an error, but will not report the invalid
ICMP type in ret['changes']
@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Sep 30, 2018

@hakoerber or @dmyerscough could you help us review this, please?

rallytime and others added some commits Sep 30, 2018

@dmyerscough

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

@cachedout This PR is pretty big to review and I don't have much time free. There are no unit tests for the firewalld states.

@hakoerber

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

At a glance, the changes look good. The indentation fixes are of course a good idea. The functionality also looks good, it should speed up execution significantly when pruning of existing settings is switched off.

I added some inline comments for things I noticed.

Apart from that, I cannot really tell, because I have no way to test the changes at the moment.

@nhavens

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

@hakoerber I apologize if I'm missing something obvious, but I can't seem to find your comments. If you've identified more opportunities for improvement, I'd be happy to incorporate your suggestions.

FWIW, I tested by setting log_level_logfile: info in /etc/salt/minion on my salt master, which is also a minion of itself. This setting is sufficient to get all commands executed by a state.apply (by the firewalld execution module) in the minion logfile. I applied sls files like the following, and watched the logs to ensure only the necessary commands were executed:

basic firewalld test to check block_icmp code improvements:
  firewalld.present:
    - name: public
    - services:
      - ssh
    - block_icmp:
      - echo-request
      - echo-reply
      - lkjfj
Show resolved Hide resolved salt/states/firewalld.py Outdated
Show resolved Hide resolved salt/states/firewalld.py Outdated
@hakoerber

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

Sorry @nhavens, I'm not used to code reviews on github. I guess the comments should show up now.

Mike Place and others added some commits Oct 9, 2018

Mike Place
@nhavens

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

@cachedout The automated Windows checks fail consistently for this PR. Should I be concerned about these failures? My changes were exclusively in the firewalld state module, which will not be used on Windows.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 16, 2018

@nhavens Those are unrelated failures and are not a concern for the fitness of this PR. Thanks!

@cachedout cachedout merged commit 45b6da3 into saltstack:2018.3 Oct 16, 2018

8 of 10 checks passed

jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.