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

(CAT-376) Rework firewall module to use the resource_api #1145

Merged
merged 17 commits into from Aug 30, 2023

Conversation

david22swan
Copy link
Member

@david22swan david22swan commented Jul 18, 2023

Summary

This PR has been created to update the Firewall module to utilize the resource_api, with the intention being to increase the ease with which the module can be updated and managed in the future.
As part of this several backwards incompatible changes where made, including:

  1. The provider attribute with the Firewall type has been renamed protocol, due to both the resource_api forbidding the use of this as an attribute name and to bring it in line with the Firewallchain. Though it continues to accept both iptables and ip6tables alongside IPv4 and IPv6, this may change in the future.
  2. The action attribute has been removed as it managed the same function as the jump attribute, only on a more limited scale. Though this was given a reason, to enforce the use of generic parameters, I found this to be a needlessly complex addition to the code.
  3. Strict types have been declared for all attributes.
  4. The port attribute has been removed as it was deprecated several years ago.
  5. dport/sport ranges should be passed with : as a separator as this is the valid form of input for the flag. Code has been updated to allow the original separator -, though it is not preferred.
  6. Array attributes, such as sport/dport, that require negation to be universal among all passed values have been updated so that rather than negating each and every value passed you now simply negate the first value in the array in order to negate them all, although the original form of negating each and every value is still allowed. Certain array values such as src_type/dst_type differ however and necessitate that each value be negated, or not negated as it were, separately.

**NOTES:

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

@david22swan david22swan requested a review from a team as a code owner July 18, 2023 15:52
@david22swan david22swan force-pushed the CONT-376/Firewall_rework branch 2 times, most recently from e7731c0 to e067e01 Compare July 18, 2023 16:07
@david22swan david22swan changed the title (CONT-376) Rework firewall module to use the resource_api (WIP)(CONT-376) Rework firewall module to use the resource_api Jul 18, 2023
@david22swan david22swan force-pushed the CONT-376/Firewall_rework branch 13 times, most recently from 4143d8d to ccd6486 Compare July 25, 2023 15:52
@david22swan david22swan changed the title (WIP)(CONT-376) Rework firewall module to use the resource_api (WIP)(CAT-376) Rework firewall module to use the resource_api Jul 26, 2023
@david22swan david22swan force-pushed the CONT-376/Firewall_rework branch 9 times, most recently from a8affb5 to 91b2ee4 Compare August 1, 2023 08:42
@david22swan
Copy link
Member Author

david22swan commented Aug 29, 2023

CentOS 8 failures have appeared on nightly,
cause found and accounted for

Remove all old code from the module
- support for IPv4 (CAT-683)
- support for IPv6 (CAT-961)
- support for IPv4 (CAT-684)
- support for IPv6 (CAT-961)
- support for all attributes (CAT-960)
Update acceptance testing to account for changes.
Includes:
- standard_usage_spec.rb
- rules_spec.rb
- resource_cmd_spec.rb
- firewallchain_spec.rb
- firewall_duplicate_comment_spec.rb
Update acceptance testing to account for changes.
Includes:
- class_spec.rb
- firewall_attributes_exceptions_spec.rb
- firewall_attributes_happy_path_spec.rb
- firewall_attributes_ipv6_exceptions_spec.rb
- firewall_attributes_ipv6_happy_path_spec.rb
Unit test coverage for utility functions found within the puppet_x namespace
Test coverage for the firewall and firewallchain Types
Test separated out into three files due to the size and complexity of the provider.
Actionable errors left in file will be handled by a follow-up PR
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a resource_api expert, but the changes look good to me.

Copy link
Contributor

@LukasAud LukasAud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Dont forget to remove the Do Not Merge

@LukasAud LukasAud merged commit cc46895 into puppetlabs:main Aug 30, 2023
38 checks passed
@david22swan david22swan changed the title (DO NOT MERGE)(CAT-376) Rework firewall module to use the resource_api (CAT-376) Rework firewall module to use the resource_api Aug 30, 2023
@traylenator
Copy link
Contributor

The action parameter going ? Is that necessary ?

If it could live till EOL for EL7?

@david22swan
Copy link
Member Author

david22swan commented Aug 30, 2023

@traylenator The functionality behind the attribute is still there, it just means renaming the attribute your setting to jump instead.
I felt it was wasteful to have two attributes for the same iptables value.
It may be inconvenient in the short term but in the long term it helps to simplify the code.

As a side, any reason for E7 in particular?

@traylenator
Copy link
Contributor

traylenator commented Aug 30, 2023

@traylenator The functionality behind the attribute is still there, it just means renaming the attribute your setting to jump instead. I felt it was wasteful to have two attributes for the same iptables value. It may be inconvenient in the short term but in the long term it helps to simplify the code.

Thanks for the reply - will live with the status quo till we launch EL7 into oblivion (and drop this module).

@david22swan david22swan deleted the CONT-376/Firewall_rework branch September 13, 2023 15:23
rdoproject pushed a commit to rdo-packages/puppet-firewall-distgit that referenced this pull request Oct 18, 2023
Add Requires on puppet-resource_api

It's needed since the merge of [1]

[1] puppetlabs/puppetlabs-firewall#1145

(cherry picked from commit 0b245d6)

Change-Id: Id033e1418a4696726382b4ceeb684ae0afa3d253
rdoproject pushed a commit to rdo-packages/puppet-firewall-distgit that referenced this pull request Oct 18, 2023
It's needed since the merge of [1]

[1] puppetlabs/puppetlabs-firewall#1145

Change-Id: If156134ecf968b98578a57dbe14030a1a8c93174
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants