Use iptables-restore to preserve order with MASQ & FORWARD rules #1264

ryarnyah · 2020-03-10T20:04:40Z

Use iptables-restore to preserve order with MASQ & FORWARD rules.

Description

When anything happen when delete/append iptables rules, sometimes you can have your rules out of order.This PR use only iptables-restore to guaranty order in delete/append rules.

This close #1261 & #1146

Todos

Tests
Documentation (Needed?)
Release note

Release Note

None required

rajatchopra · 2020-03-13T21:07:27Z

Looks good. How can we have an end-to-end test for it?
Maybe check on iptables rules after doing a kube e2e in dist/function-test-k8s.sh?

ryarnyah · 2020-03-14T20:34:52Z

e2e tests has been added as requested

ryarnyah · 2020-04-24T14:11:45Z

Do you need something else to have this PR merged?

luthermonson · 2021-01-21T17:07:49Z

@Oats87 take a look would you?

luthermonson · 2021-03-18T15:58:50Z

@Oats87 sign off on this so we can merge it, youre the iptables ninja

Oats87 · 2021-04-01T15:28:43Z

@ryarnyah would you mind rebasing this PR (or just fixing the merge conflict)?

ryarnyah · 2021-04-03T16:03:29Z

@Oats87 i will look to rebase it this weekend if i have time ;)
Edit: Let me have a look at the upgraded code before merging please.

ryarnyah · 2021-04-04T20:53:41Z

@Oats87 It's seem to be good for review now.

network/iptables_restore.go

Oats87 · 2021-04-06T20:20:55Z

network/iptables_restore.go

+}
+
+// ipTablesRestore internal type
+type ipTablesRestore struct {


Is there a reason we don't want to have a timeout for this?

@Oats87 I'm not sur to understood, can you give me more details ?

network/iptables_restore.go

Oats87 · 2021-04-06T21:36:54Z

network/iptables.go

+		return fmt.Errorf("failed to apply partial iptables-restore %v", err)
+	}
+
+	log.Infof("bootstrap done")


Can we provide some more context in this error message? Does this have to be done at the info level?

I'm not sure it's possible

network/iptables.go

Oats87 · 2021-04-06T21:46:13Z

network/iptables_restore.go

@@ -0,0 +1,213 @@
+// Copyright 2015 flannel authors


This probably actually belongs in coreos/go-iptables but having it here should be fine.

So I leave it that way ?

network/iptables_restore.go

rajatchopra · 2021-10-14T15:49:57Z

@ryarnyah could you rebase and respond to the review comments?

louiznk · 2022-01-12T15:15:20Z

@ryarnyah could you rebase and respond to the review comments?

Hello @rajatchopra if you and @ryarnyah are agree I can do this for @ryarnyah
Thanks

luthermonson · 2022-01-12T15:44:47Z

@louiznk please do

luthermonson · 2022-01-13T15:04:09Z

@louiznk any chance you can answer/fix for @Oats87 feedback? can probably wrap this up and merge if those are addressed

louiznk · 2022-01-13T15:32:34Z

@louiznk any chance you can answer/fix for @Oats87 feedback? can probably wrap this up and merge if those are addressed

I just fixe some of this feedback (mark with 👌). But there is some point I'm not sure to understood.

louiznk · 2022-01-14T08:07:57Z

was requested for review

Hello @luthermonson
I don't know if you have the opportunity to see the commit and question/reply for @Oats87 feedback?
Thanks a lot for your help and review.

louiznk

Apply requests

manuelbuil · 2022-05-26T15:57:38Z

Are you still interested in merging this? Could you rebase so that we can review it again? Thanks!

louiznk · 2022-05-30T07:42:44Z

Are you still interested in merging this? Could you rebase so that we can review it again? Thanks!

Hello @manuelbuil ,

Thanks, I can do that this week

louiznk · 2022-05-31T16:10:38Z

Are you still interested in merging this? Could you rebase so that we can review it again? Thanks!

Hello @manuelbuil , the rebase is done, I fixe some trouble with this new versions (usage of comments on iptables rules for exemples)
Thanks for your review and feedback

manuelbuil

IIUC, some users detected a race condition in which these rules: https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L57-L66, are applied in different order. In my opinion, the simplest solution would be adding a lock here https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L30 and then check the lock around here: https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L244 and not allow any other rule to be added until the lock is released. That way the race condition will not exist anymore and the problem would be fixed, or? Using iptables-restore also fixes the problem but it requires adding ~500 lines of code, which makes me wonder if this is an overkill given that the use of iptables in flannel is minimal (if we had some sort of network policy engine using iptables, I would probably think differently).

I'd love to hear more opinions!

manuelbuil · 2022-06-06T13:45:33Z

network/iptables_restore.go

+
+// NewIPTablesRestore build new IPTablesRestore for IPv4
+func NewIPTablesRestore() (IPTablesRestore, error) {
+	return NewIPTablesRestoreWithProtocol(iptables.ProtocolIPv4)


Is there a reason to hardcode IPv4?

IIUC, some users detected a race condition in which these rules: https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L57-L66, are applied in different order. In my opinion, the simplest solution would be adding a lock here https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L30 and then check the lock around here: https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L244 and not allow any other rule to be added until the lock is released. That way the race condition will not exist anymore and the problem would be fixed, or? Using iptables-restore also fixes the problem but it requires adding ~500 lines of code, which makes me wonder if this is an overkill given that the use of iptables in flannel is minimal (if we had some sort of network policy engine using iptables, I would probably think differently).

I'd love to hear more opinions!

You're right in the case of flannel is alone. But I have trouble with some concurrent access to iptables from other programs (in production).
In my case it was kube-router (using for network policy).
I know there are better solutions than using flannel + kube-routeur, but unfortunately not in special my case (some external requirements).
So the main advantage for me with iptables-restore is the "transaction" that allows to preserve the order when you have some concurrents access to iptables.

Is there a reason to hardcode IPv4?

Just for respecting the same convention as implementation based on iptables (New => ipv4 by default and NewWithProtocol => protocol in parameter). For exemple see :
https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L190 and https://github.com/flannel-io/flannel/blob/master/network/iptables.go#L206
I can change it if it's better

I personally would prefer to use the same functions for both ipv4 and ipv6 and change the arg accordingly. Therefore, I'd prefer NewIPTablesRestoreWithProtocol(iptables.ProtocolIPv6) and NewIPTablesRestoreWithProtocol(iptables.ProtocolIPv4) in the code. It feels to me more confusing if we use different functions for each protocol

Change done and push @manuelbuil

network/iptables_restore.go

network/iptables.go

…nd review -> Changes from the review: - refactor protocol to proto - fix check version for wait command available on iptables-restore - apply "Error strings should not be capitalized" - lower case for iptables-restore - refactor function ApplyPartial to ApplyWithoutFlush - remove unused function ApplyFully - comments functions

manuelbuil · 2022-06-23T14:45:30Z

Tested and works

ryarnyah force-pushed the feat/add-iptables-restore-at-startup branch from 0eee12d to 190e2aa Compare March 12, 2020 11:06

luthermonson requested review from Oats87 and luthermonson March 18, 2021 15:58

ryarnyah force-pushed the feat/add-iptables-restore-at-startup branch from 8dfc176 to 09cafb7 Compare April 4, 2021 19:17

Oats87 requested changes Apr 6, 2021

View reviewed changes

louiznk force-pushed the feat/add-iptables-restore-at-startup branch 2 times, most recently from 45222ed to b0f38e7 Compare January 13, 2022 13:17

louiznk reviewed Jan 24, 2022

View reviewed changes

louiznk approved these changes Feb 9, 2022

View reviewed changes

louiznk force-pushed the feat/add-iptables-restore-at-startup branch from 8b689bf to 24062ab Compare May 31, 2022 15:59

louiznk force-pushed the feat/add-iptables-restore-at-startup branch from 24062ab to 04d157d Compare May 31, 2022 17:43

manuelbuil reviewed Jun 6, 2022

View reviewed changes

louiznk force-pushed the feat/add-iptables-restore-at-startup branch from 1f59190 to f8d178c Compare June 8, 2022 13:33

louiznk force-pushed the feat/add-iptables-restore-at-startup branch from f8d178c to 70ed47a Compare June 20, 2022 08:17

louiznk force-pushed the feat/add-iptables-restore-at-startup branch from 70ed47a to 121db21 Compare June 23, 2022 12:45

manuelbuil approved these changes Jun 23, 2022

View reviewed changes

Oats87 approved these changes Jun 23, 2022

View reviewed changes

rbrtbnfgl approved these changes Jun 23, 2022

View reviewed changes

manuelbuil merged commit fb2ee82 into flannel-io:master Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use iptables-restore to preserve order with MASQ & FORWARD rules #1264

Use iptables-restore to preserve order with MASQ & FORWARD rules #1264

ryarnyah commented Mar 10, 2020 •

edited

Loading

rajatchopra commented Mar 13, 2020

ryarnyah commented Mar 14, 2020

ryarnyah commented Apr 24, 2020

luthermonson commented Jan 21, 2021

luthermonson commented Mar 18, 2021

Oats87 commented Apr 1, 2021

ryarnyah commented Apr 3, 2021 •

edited

Loading

ryarnyah commented Apr 4, 2021

Oats87 Apr 6, 2021

louiznk Jan 13, 2022

Oats87 Apr 6, 2021

louiznk Jan 13, 2022

Oats87 Apr 6, 2021

louiznk Jan 13, 2022

rajatchopra commented Oct 14, 2021

louiznk commented Jan 12, 2022

luthermonson commented Jan 12, 2022

luthermonson commented Jan 13, 2022

louiznk commented Jan 13, 2022

louiznk commented Jan 14, 2022

louiznk left a comment

manuelbuil commented May 26, 2022

louiznk commented May 30, 2022 •

edited

Loading

louiznk commented May 31, 2022

manuelbuil left a comment •

edited

Loading

manuelbuil Jun 6, 2022

louiznk Jun 7, 2022 •

edited

Loading

louiznk Jun 7, 2022

manuelbuil Jun 20, 2022

louiznk Jun 20, 2022

manuelbuil commented Jun 23, 2022

Use iptables-restore to preserve order with MASQ & FORWARD rules #1264

Use iptables-restore to preserve order with MASQ & FORWARD rules #1264

Conversation

ryarnyah commented Mar 10, 2020 • edited Loading

Description

Todos

Release Note

rajatchopra commented Mar 13, 2020

ryarnyah commented Mar 14, 2020

ryarnyah commented Apr 24, 2020

luthermonson commented Jan 21, 2021

luthermonson commented Mar 18, 2021

Oats87 commented Apr 1, 2021

ryarnyah commented Apr 3, 2021 • edited Loading

ryarnyah commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatchopra commented Oct 14, 2021

louiznk commented Jan 12, 2022

luthermonson commented Jan 12, 2022

luthermonson commented Jan 13, 2022

louiznk commented Jan 13, 2022

louiznk commented Jan 14, 2022

louiznk left a comment

Choose a reason for hiding this comment

manuelbuil commented May 26, 2022

louiznk commented May 30, 2022 • edited Loading

louiznk commented May 31, 2022

manuelbuil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louiznk Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelbuil commented Jun 23, 2022

ryarnyah commented Mar 10, 2020 •

edited

Loading

ryarnyah commented Apr 3, 2021 •

edited

Loading

louiznk commented May 30, 2022 •

edited

Loading

manuelbuil left a comment •

edited

Loading

louiznk Jun 7, 2022 •

edited

Loading