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

Add SNAT random-fully flag. #1901

Merged
merged 6 commits into from Oct 10, 2018

Conversation

Projects
None yet
2 participants
@fasaxc
Member

fasaxc commented Oct 9, 2018

Description

Detect iptables version and pass feature flags around.

  • SNAT --random-fully was aded in iptables v1.6.1
  • MASQUERADE --random-fully was added in iptables v1.6.2

Use feature flag to control whether the --random-fully flag is included.

Fixes projectcalico/calico#2073

Todos

  • Check kernel compat?
  • Unit tests (full coverage)
  • Integration tests (delete as appropriate) In plan/Not needed/Done
  • Documentation
  • Backport
  • Release note

Release Note

Where supported, Felix now uses the iptables --random-fully option when programming SNAT and MASQUERADE rules.

@fasaxc fasaxc requested a review from projectcalico/core-maintainers as a code owner Oct 9, 2018

@fasaxc fasaxc requested a review from neiljerram Oct 9, 2018

@neiljerram

A few nits here; but also I'm tempted to think that having Features per-table is unnecessary, and that this change could be much smaller if it used a single global Features variable that is populated at start of day. (Then it doesn't need to be passed through everywhere.) WDYT? Am I missing an important benefit of the current arrangement?

return &merged
}

// VersionToFeatures convers an iptables version line as returned by iptables --version into a set of feature flags.

This comment has been minimized.

@neiljerram

neiljerram Oct 9, 2018

Member

*converts

@@ -731,7 +754,7 @@ func describeDirtyDataplaneTests(appendMode bool) {
It("with no errors, it should get to correct final state", func() {
table.Apply()
checkFinalState()
Expect(len(dataplane.Cmds)).To(Equal(2)) // a save and a restore
Expect(dataplane.Cmds).To(HaveLen(3)) // a save and a restore

This comment has been minimized.

@neiljerram

neiljerram Oct 9, 2018

Member

need comment update

@@ -742,7 +765,7 @@ func describeDirtyDataplaneTests(appendMode bool) {
checkFinalState()
})
It("it should retry once", func() {
Expect(len(dataplane.Cmds)).To(Equal(3)) // 2 saves and a restore
Expect(dataplane.Cmds).To(HaveLen(4)) // 2 saves and a restore

This comment has been minimized.

@neiljerram

neiljerram Oct 9, 2018

Member

here too

}

func (d *versionCmd) String() string {
return "saveCmd"

This comment has been minimized.

@neiljerram

neiljerram Oct 9, 2018

Member

Is that right? (As opposed to "versionCmd")

@fasaxc

This comment has been minimized.

Member

fasaxc commented Oct 9, 2018

I went that direction to make it easier to test in fully-deterministic way. A global variable doesn't fit well with ginkgo tests at all and once the table needs to be passed it, it needs to be passed down to the rules too (since the rules shouldn't be carrying around a feature flag pointer).

@neiljerram

OK, good point about the global variable not working with ginkgo. LGTM now.

@fasaxc fasaxc referenced this pull request Oct 10, 2018

Merged

Use iptables-restore xtables lock #1903

3 of 5 tasks complete

fasaxc added some commits Oct 9, 2018

Add SNAT fully-random flag.
Detect iptables version and pass feature flags around.

Use feature flag to control whether the --fully-random flag is included.

@fasaxc fasaxc force-pushed the fasaxc:snat-fully-random branch from 92e2b80 to 2739c44 Oct 10, 2018

@fasaxc fasaxc changed the title from Add SNAT fully-random flag. to Add SNAT random-fully flag. Oct 10, 2018

@fasaxc fasaxc force-pushed the fasaxc:snat-fully-random branch from dafaae6 to 6920797 Oct 10, 2018

@fasaxc fasaxc added this to the Calico v3.3.0 milestone Oct 10, 2018

@fasaxc fasaxc merged commit af16b72 into projectcalico:master Oct 10, 2018

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@fasaxc fasaxc deleted the fasaxc:snat-fully-random branch Oct 10, 2018

fasaxc added a commit to fasaxc/felix that referenced this pull request Oct 17, 2018

@fasaxc fasaxc referenced this pull request Oct 17, 2018

Merged

Backport SNAT --random-fully and xtables lock support #1907

0 of 5 tasks complete

fasaxc added a commit to fasaxc/felix that referenced this pull request Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment