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

Fix extra quotes in firewall string matching #944

Merged
merged 2 commits into from
Oct 1, 2020
Merged

Fix extra quotes in firewall string matching #944

merged 2 commits into from
Oct 1, 2020

Conversation

IBBoard
Copy link
Contributor

@IBBoard IBBoard commented Sep 23, 2020

As reported by Steve Traylon[1] and @patricknelson[2], the munging adds extra quotes to the string.
This breaks the string matching in iptables, as it looks for literal single quotes in the match.

Removing the munging fixes this.

[1] https://tickets.puppetlabs.com/browse/MODULES-3454?focusedCommentId=686988&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-686988
[2] 3655c6b#diff-379160b60a9bdf297b92a51d20efd8c3R1426

As reported by Steve Traylon[1] and @patricknelson[2], the munging adds extra quotes to the string.
This breaks the string matching in iptables, as it looks for literal single quotes in the match.

Removing the munging fixes this.

[1] https://tickets.puppetlabs.com/browse/MODULES-3454?focusedCommentId=686988&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-686988
[2] 3655c6b#diff-379160b60a9bdf297b92a51d20efd8c3R1426
@IBBoard IBBoard requested a review from a team as a code owner September 23, 2020 19:58
@puppet-community-rangefinder
Copy link

firewall is a type

Breaking changes to this file WILL impact these 124 modules (exact match):
Breaking changes to this file MAY impact these 144 modules (near match):

This module is declared in 108 of 575 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@sanfrancrisko
Copy link
Contributor

Thanks for the fix @IBBoard and references for explanation!

It looks as though some unit tests are now failing as they were testing with the (incorrect!) assertion that the string match values should be wrapped in single quotes. Could you update those tests' data with the new unquoted string values?

I also think you've highlighted the fact that we don't have an acceptance test that will actually test iptables with this flag on real systems. If it has been covered purely by unit tests, and those tests have been built off incorrect assumptions, then it's conceivable this issue was missed. At least with an acceptance test, iptables should either blow up or our test expectations would have shown the expected configuration looked incorrect.

ARGS_TO_HASH was correct (unquoted) but HASH_TO_ARGS
contained the rogue single quotes
@IBBoard
Copy link
Contributor Author

IBBoard commented Sep 26, 2020

That should have fixed the unit/fixture tests. I've not added any acceptance tests because a) I don't know which of several files they go in and b) from what I understand of them then I can't run them without altering my system, and I don't have anywhere to test them at the moment.

I might be able to write an acceptance test in the dark and hope it works, but it doesn't seem like a good use of Travis cycles!

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@a90b565). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #944   +/-   ##
=======================================
  Coverage        ?   81.12%           
=======================================
  Files           ?       11           
  Lines           ?     1664           
  Branches        ?        0           
=======================================
  Hits            ?     1350           
  Misses          ?      314           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a90b565...9c974c0. Read the comment docs.

@adrianiurca
Copy link
Contributor

Hi @IBBoard , thank you for your contribution.

@adrianiurca adrianiurca merged commit 7bfa339 into puppetlabs:main Oct 1, 2020
@IBBoard IBBoard deleted the patch-1 branch October 3, 2020 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants