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

MODULES-1636: Add --checksum-fill support. #460

Merged
merged 1 commit into from Mar 26, 2015

Conversation

Zlo
Copy link

@Zlo Zlo commented Jan 5, 2015

This is a.o. needed to fill checksums in UDP packets for
DHCP in VMs using virtual network mode.


it 'should contain the rule' do
shell('iptables-save -t mangle') do |r|
expect(r.stdout).to match(/-A POSTROUTING -o virbr0 -p udp -m multiport --dports 68 -m comment --comment "576 - test" -j CHECKSUM --checksumfill/)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the actual option is --checksum-fill, so I think this is going to fail.

@underscorgan
Copy link
Contributor

@Zlo Since it looks like the option in iptables is checksum-fill, so for consistency could you make this parameter named checksum_fill instead of checksumfill?

Also, from my googling it looks like --checksum-fill is also supported by ip6tables, so could you make the corresponding changes to the ip6tables provider and add a test case with provider => 'ip6tables'?

Thanks!

@Zlo
Copy link
Author

Zlo commented Jan 8, 2015

For some reason test runs fail during vagrant initialization here, so I have not actually run the tests.

@ogni90
Copy link

ogni90 commented Jan 21, 2015

Zlo, I tried your changes in Ubuntu 14.10 but I still get an error when I apply my puppet config.

Error: Execution of '/sbin/iptables -I POSTROUTING 1 -t mangle -p udp -m multiport --dports 68 -m comment --comment 100 fix bootp checksum -j CHECKSUM' returned 2: iptables v1.4.21: CHECKSUM: option "--checksum-fill" must be specified

Try `iptables -h' or 'iptables --help' for more information.
Error: /Stage[main]/Tradebyte_dev::Lxc/Firewall[100 fix bootp checksum]/ensure: change from absent to present failed: Execution of '/sbin/iptables -I POSTROUTING 1 -t mangle -p udp -m multiport --dports 68 -m comment --comment 100 fix bootp checksum -j CHECKSUM' returned 2: iptables v1.4.21: CHECKSUM: option "--checksum-fill" must be specified

Try `iptables -h' or 'iptables --help' for more information.

My Config:

firewall { '100 fix bootp checksum':
table => 'mangle',
chain => 'POSTROUTING',
proto => 'udp',
jump => 'CHECKSUM',
checksum_fill => true,
dport => '68',
}

Am I missing something here?

@Zlo
Copy link
Author

Zlo commented Jan 21, 2015

@ogni90: the merging made me drop a newvalue() line in the checksum_fill property. I fixed that and merged master again.
I'll see if I can get my vagrant problem solved so I can actually run the beaker tests.

@jonnytdevops
Copy link
Contributor

Hi There
Thanks for your submission - we really appreciated your efforts!

Please remember to contain the commits of your PR to the feature being implemented. For example, alphabetising the arrays in the provider, if desired, should be done in a separate PR :)

Also, once all your tests pass and you are happy that your code is in a ready state, please remember to squash your commits.

Thanks!

@Zlo
Copy link
Author

Zlo commented Mar 5, 2015

Ok, like this then ?

@jonnytdevops
Copy link
Contributor

@Zlo Thanks for the squashing :)

It would be great if you could add some validation in the provide that checks to see if :jump is set to CHECKSUM if checksum_fill is used and vice versa.

Thanks!

@Zlo
Copy link
Author

Zlo commented Mar 11, 2015

Ok, I added validation in type/firewall.rb

jonnytdevops added a commit that referenced this pull request Mar 26, 2015
MODULES-1636: Add --checksum-fill support.
@jonnytdevops jonnytdevops merged commit eaa2fac into puppetlabs:master Mar 26, 2015
cegeka-jenkins pushed a commit to cegeka/puppet-firewall that referenced this pull request Oct 23, 2017
MODULES-1636: Add --checksum-fill support.
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.

None yet

5 participants