Rolling back the port configs if failed to programIngress() #2069

fanjiyun · 2018-02-05T06:23:10Z

Description for the changelog
Rolling back the port configs if failed to programIngress()

Signed-off-by: fanjiyun fan.jiyun@zte.com.cn

codecov-io · 2018-02-05T06:42:50Z

Codecov Report

❗ No coverage uploaded for pull request base (master@20dd462). Click here to learn what that means.
The diff coverage is 0%.

@@           Coverage Diff            @@
##             master   #2069   +/-   ##
========================================
  Coverage          ?   40.5%           
========================================
  Files             ?     138           
  Lines             ?   22174           
  Branches          ?       0           
========================================
  Hits              ?    8982           
  Misses            ?   11875           
  Partials          ?    1317

Impacted Files	Coverage Δ
service_linux.go	`3.25% <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 20dd462...f51df90. Read the comment docs.

fcrisciani · 2018-03-08T23:30:39Z

service_linux.go

 		if sb.ingress {
 			filteredPorts = filterPortConfigs(ingressPorts, false)
 			if err := programIngress(gwIP, filteredPorts, false); err != nil {
+				filterPortConfigs(ingressPorts, true)


I believe that potentially that is not enough, it will depend on where the failure happened... if that fails in the middle there can be some half configured stuff left behind

I concur. The programIngress() function does not fail gracefully or leave the system in a consistent state if it fails, unfortunately. These changes are necessary for a successful rollback, but insufficient.

Reviewing programIngress() a bit I see there are several different stages of changes:

first ingress programming after startup

basic iptables setup required for per-port configuration

per-ingress port rule add/deletion consisting of:
a. DNAT rule programming
b. SNAT rule programming
c. filter bypass for the given port
d. setting up a listening proxy (for reasons I don't quite get yet)

Failures in 1. never abort processing: they only flag the error. So no problem there. Failures in 2. do abort processing, but every time programIngress(...true) /* add */ is invoked, 2 checks each step to avoid clobbering existing work. So if failure happens here, we are probably ok to just undo the filterPortConfigs().

Failures in part 3. are the problem in several ways. We could have gotten through some, but not all of the ports. In that case we should either undo the operation we just did on the port configurations that were successful, or return a list of ports that we were not successful in programming. Those could then be passed to filterPortConfigs() to undo their tracking. The latter solution seems simpler and cleaner to me.

Unfortunately, that's not quite sufficient either, because note that programming each port is a multi-stage process. If the programming fails in the middle of one of the stages and we want to make things clean, we have to undo the parts that did succeed. (e.g. if 3c fails, we need to undo parts 3a and 3b for that port to leave things in a consistent state.)

But even that isn't quite sufficient, because it depends on whether this was for add or delete and the reason for the failure. If the operation was an "add", then removing the partial add makes sense. But if the operation was a remove and it failed because for some reason the entry wasn't there, then adding it back would be an error. Finally, there is the question of what to do if for some bizarre reason rolling back the partial port configuration fails. Ugh. :(

I think the goal of this patch is definitely worthwhile, but making such a solution robust is going to be tricky to say the least.

ctelfer

Unfortunately, I think that the patch is insufficient. The changes provided as-is will only work for a limited set of cases and will leave the system in an even more inconsistent state in others. This patch would need to include a bit of refactoring to programIngress() in order to be correct/effective.

ctelfer · 2018-03-09T15:57:46Z

service_linux.go

 		if sb.ingress {
 			filteredPorts = filterPortConfigs(ingressPorts, false)
 			if err := programIngress(gwIP, filteredPorts, false); err != nil {
+				filterPortConfigs(ingressPorts, true)


I concur. The programIngress() function does not fail gracefully or leave the system in a consistent state if it fails, unfortunately. These changes are necessary for a successful rollback, but insufficient.

Reviewing programIngress() a bit I see there are several different stages of changes:

first ingress programming after startup

basic iptables setup required for per-port configuration

per-ingress port rule add/deletion consisting of:
a. DNAT rule programming
b. SNAT rule programming
c. filter bypass for the given port
d. setting up a listening proxy (for reasons I don't quite get yet)

Failures in 1. never abort processing: they only flag the error. So no problem there. Failures in 2. do abort processing, but every time programIngress(...true) /* add */ is invoked, 2 checks each step to avoid clobbering existing work. So if failure happens here, we are probably ok to just undo the filterPortConfigs().

Failures in part 3. are the problem in several ways. We could have gotten through some, but not all of the ports. In that case we should either undo the operation we just did on the port configurations that were successful, or return a list of ports that we were not successful in programming. Those could then be passed to filterPortConfigs() to undo their tracking. The latter solution seems simpler and cleaner to me.

Unfortunately, that's not quite sufficient either, because note that programming each port is a multi-stage process. If the programming fails in the middle of one of the stages and we want to make things clean, we have to undo the parts that did succeed. (e.g. if 3c fails, we need to undo parts 3a and 3b for that port to leave things in a consistent state.)

But even that isn't quite sufficient, because it depends on whether this was for add or delete and the reason for the failure. If the operation was an "add", then removing the partial add makes sense. But if the operation was a remove and it failed because for some reason the entry wasn't there, then adding it back would be an error. Finally, there is the question of what to do if for some bizarre reason rolling back the partial port configuration fails. Ugh. :(

I think the goal of this patch is definitely worthwhile, but making such a solution robust is going to be tricky to say the least.

fanjiyun · 2018-03-12T10:16:59Z

@fcrisciani @ctelfer , thanks to both of you very much. Yes, it is tricky to solve all abnormal situations completely. This patch can only handle part of the situation. I need further analysis and thinking to solve the problem completely.

fanjiyun · 2018-08-09T08:23:55Z

@ctelfer, @fcrisciani, Recently, I have further modified the function programIngress(), based on our previous discussion.

As described by @ctelfer，programIngress() contains three stages：

first ingress programming after startup
basic iptables setup required for per-port configuration
per-ingress port rule add/deletion

Stage.1 does not need to be dealt with.
If failures in stage.2, we should undo the filterPortConfigs().
For stage.3, it depends on whether add or deletion. For deletion, there is no rollback because the function will not be abort. On the contrary, we will rollback the changes for the add port rules to ensure data consistency.

Could you review the PR，thanks to both of you.

fanjiyun · 2018-08-21T01:56:49Z

@ctelfer , @fcrisciani , could you review this PR again，thanks.

ctelfer

First, thanks for continuing to carry this! I think this could benefit from two improvements:

move the filterPortConfigs() back out of programIngress() to the caller as before, but for add only.
create a slice of rollback rules for each successful iptables programming and issue them upon failure.

More details below. Thanks again!

ctelfer · 2018-08-22T14:20:45Z

service_linux.go

+			filterPortConfigs(ingressPorts, !isDelete)
+		}
+	}()
+


My reading of this is that if we get past the setup phase (the next block) then we remove the ingress ports that we were trying to add. In the first place, this seems wrong since it does this whether rest of the function succeeds or fails. But more importantly ... I think that the filterPortConfigs() aspect of your original patch was the more correct way to handle this part of the rollback. i.e. if programIngress() returns an error during an add operation, then run filterPortConfigs(lb.service.ingressPorts, false) in the error block. The only thing wrong with that part of the original was that it was incomplete. The rest of your patch sees to undoing the iptables programming.

ctelfer · 2018-08-22T14:21:42Z

service_linux.go

 	for _, iPort := range ingressPorts {
+
+		var portErr error
+


Suggest moving this outside the block for reasons below.

ctelfer · 2018-08-22T14:24:20Z

service_linux.go

+				filterPortConfigs(rollbackPort, !isDelete)
+			}
+		}()
+


I don't think that this block was ever needed because you were already running filterPortConfigs(ingressPorts, !isDelete) above. But that's kind of moot if we move the filterPortConfigs() "undo" out to the caller.

ctelfer · 2018-08-22T14:32:37Z

service_linux.go

+		defer func() {
+			if portErr != nil && !isDelete {
+				rule := strings.Fields(fmt.Sprintf("-t nat %s %s -p %s --dport %d -j DNAT --to-destination %s:%d", rollbackAddDelOpt,
+					ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort, gwIP, iPort.PublishedPort))


Hrm... Rather than stacking up a defer function per programmed port, I'd consider having a list of "rollback rules". Then, there could just be one defer function defined before the loop that looks something like this:

var portErr error rollbackRules := make([][]string, 0, len(ingressPorts)*3) defer func() { if portErr != nil && !isDelete { for _, rule := range rollbackRules { if err := iptables.RawCombinedOutput(rule...); err != nil { logrus.Warnf("setting up rule failed while rolling back:, %v: %v", rule, err) } } } }

Then at this point in the code, you don't need this to be in a defer block, you just do:

rollBackRule := strings.Fields(...) rollbackRules = append(rollbackRules, rollbackRule)

Incidentally, moving portErr outside of the loop has the added benefit that it can help recover from proxy errors: see below.

ctelfer · 2018-08-22T14:33:39Z

service_linux.go

+			if portErr != nil && !isDelete {
+				rule = strings.Fields(fmt.Sprintf("%s %s -m state -p %s --sport %d --state ESTABLISHED,RELATED -j ACCEPT", rollbackAddDelOpt,
+					ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort))
+				if err := iptables.RawCombinedOutput(rule...); err != nil {


Same idea here re: adding a rollback rule to a slice of them.

ctelfer · 2018-08-22T14:35:24Z

service_linux.go

-			errStr := fmt.Sprintf("setting up rule failed, %v: %v", rule, err)
+		if portErr = iptables.RawCombinedOutput(rule...); portErr != nil {
+			errStr := fmt.Sprintf("setting up rule failed, %v: %v", rule, portErr)
 			if !isDelete {


We need a rollback rule here as well because if there is more than one port, the first one could be successfully programmed, but a subsequent one could fail.

ctelfer · 2018-08-22T14:37:18Z

service_linux.go

+			errStr := fmt.Sprintf("setting up rule failed, %v: %v", rule, portErr)
 			if !isDelete {
 				return fmt.Errorf("%s", errStr)
 			}


A few lines below here, if we catch the return of plumbProxy in portErr then we'll successfully be able to roll back from that error as well. This is contingent on moving the portErr definition outside the loop as mentioned above.

fanjiyun · 2018-08-23T09:13:39Z

@ctelfer ,thank you very much for analyzing this patch so deeply. I think your suggestion is better that add a slice of rollback rules. I have made the corresponding improvement.

However, I think if programIngress() failures in stage.2 (basic iptables setup required for per-port configuration), we should undo filterPortConfigs (), both add and deletion. Because the the port rule add/deletion is not executed actually.

In the patch, I add a parameter "handleIngressPorts" to determine whether programIngress() starts to add/delete port rule, and the initial value of handleIngressPorts is false.
When programIngress() failures in stage.2 (basic iptables setup required for per-port configuration), "!handleIngressPorts == true" and filterPortConfigs() could be executed.
Conversely, if stage.2 is successful and stage.3(port rule add/deletion) begins to be executed, handleIngressPorts will be assigned true. "!handleIngressPorts == false" and whether or not to execute filterPortConfigs() will be determined by other parameters (such as portErr, isDelete).

That's why I put filterPortConfigs() inside the programIngress(). It depend on where the failure of programIngress() happened.

The above is my thought. Thank you again for your time and look forward to further discussions with you.

fanjiyun · 2018-08-29T09:24:48Z

@ctelfer I have a new idea. Maybe we can move filterPortConfigs() into programIngress() when programIngress() is called. And start to filter the ingress ports until stage.3(port rule add/deletion) begins to be executed.

ctelfer · 2018-08-29T15:05:47Z

I like it! That seems like an even cleaner solution. Just one small nit. You mentioned:

However, I think if programIngress() failures in stage.2 (basic iptables setup required for per-port configuration), we should undo filterPortConfigs (), both add and deletion. Because the the port rule add/deletion is not executed actually.

and I think you are right. However, I also noticed that the way your patch is currently arranged, it only undoes filterPortConfigs() if the operation is not delete. So I think that this:

		if portErr != nil && !isDelete {
			filterPortConfigs(filteredPorts, !isDelete)
                        ...

would probably need to change to something like this:

               if portErr != nil {
                       filterPortConfigs(filteredPorts, !isDelete)
                       if isDelete {
                               return
                       }
                       ...

(there are other possible approaches. I'm sure you get the idea.)

fanjiyun · 2018-08-29T16:26:15Z

In today's latest version, By filtering ports until stage.3(port rule add/deletion) begins to be executed, we can avoid the effect of the failures in stage.2 (basic iptables setup) on port configuration.
And in the stage.3(port rule add/deletion), you mean if there has an error for the port rule deletion, we also should undo filterPortConfigs() ?
In the code, if there has an error for the deletion, it jsut print a warning, and not interrupt follow-up operation, as shown:

		if ..., portErr != nil {
 			...
 			if !isDelete {
 				return fmt.Errorf("%s", errStr)
 			}
 			logrus.Warnf("%s", errStr)
 		}

if we undo filterPortConfigs() and not roll back rules for all ports, is this appropriate? Maybe a warning is enough if an error has occurred for deletion? Do you think so?

ctelfer · 2018-08-29T18:52:49Z

Ok, I clearly misunderstood your intent w/ the message I quoted. The question boils down to whether a port should still be tracked as a "filtered port" if dockerd encountered some issue when trying to delete the corresponding rules. I suppose it would depend on the nature of the failure. If it failed because the rule already vanished for some stupid reason, then ignoring the error is fine. OTOH, if, say, there's some issue w/ firewalld and the rules programming didn't go through, and the rules are still left then the port is going to seem open from docker's perspective, but effectively be blocked. The fact that there are 3 such rules that could fail for any port is also troublesome here.

Ok. For purpose of this patch, it isn't making the situation any worse by only issuing a warning during a delete failure. So it's probably ok as is. Will give it one more pass and update my review. Thanks!

ctelfer

LGTM

(one minor nit on wording below if you wouldn't mind changing... not a blocker)

ctelfer · 2018-08-29T19:58:55Z

service_linux.go

+			filterPortConfigs(filteredPorts, !isDelete)
+			for _, rule := range rollbackRules {
+				if err := iptables.RawCombinedOutput(rule...); err != nil {
+					logrus.Warnf("setting up rule failed while rolling back, %v: %v", rule, err)


minor nit: we know this is only going to happen when rolling back an "add" so instead of "setting up", the message should probably say "removing" or "rolling back" or some such.

yes, and I have changed. Thanks

fanjiyun · 2018-08-30T04:43:17Z

Yes, I also think that keeping the status quo may be a better way for a delete failure by print a warning. Thank you very much for your time.

fanjiyun · 2018-09-02T15:39:59Z

@ctelfer , excuse me, does this need someone else to review? Can I trouble you to help ping someone? Thanks very much.

fanjiyun · 2018-09-08T02:44:20Z

@fcrisciani @thaJeztah , I don't know if this PR need more than one person to review, can you take a look? @ctelfer, is there anything I need to do to make this PR merge? Thanks.

ctelfer · 2018-09-10T15:01:08Z

@fanjiyun Just need another review. Otherwise looks good to go from my standpoint.

Hopefully @fcrisciani can take a look, although I believe he is travelling today . @euanh or @selansen Could either of you take a look?

selansen · 2018-09-10T16:05:54Z

I was taking a look at on weekend but I realized @fcrisciani <https://github.com/fcrisciani> had already reviewed it so he might have better context. Let me take a look at it now.

…

On Mon, Sep 10, 2018 at 11:01 AM Christopher Adam Telfer < ***@***.***> wrote: @fanjiyun <https://github.com/fanjiyun> Just need another review. Otherwise looks good to go from my standpoint. Hopefully @fcrisciani <https://github.com/fcrisciani> can take a look, although I believe he is travelling today . @euanh <https://github.com/euanh> or @selansen <https://github.com/selansen> Could either of you take a look? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2069 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKn5oEkfNUANYDuIjCzYjLO3lvA9gcks5uZn7GgaJpZM4R4-JD> .

selansen · 2018-09-10T16:36:12Z

service_linux.go

+			filterPortConfigs(filteredPorts, !isDelete)
+			for _, rule := range rollbackRules {
+				if err := iptables.RawCombinedOutput(rule...); err != nil {
+					logrus.Warnf("rolling back rule failed, %v: %v", rule, err)


what will happen if few rules were successfully rolled back and few of them failed. I think we will be in wired state. Hope that never happens.

selansen · 2018-09-10T17:02:23Z

one general nit . I see everywhere "rolling back " "setting up" words. it would be good to keep it "roll back rule " "set up rule".

selansen · 2018-09-10T17:02:29Z

LGTM

Signed-off-by: fanjiyun <fan.jiyun@zte.com.cn>

fanjiyun · 2018-09-11T11:16:54Z

I have updated these words. Besides, there are some functions that also contain "setting up", and I updated together. Thank you all.

selansen · 2018-09-11T14:34:07Z

Thanks. LGTM

ctelfer · 2018-09-11T18:03:24Z

Thanks for sticking with this and seeing it through @fanjiyun . Change is merged.

fanjiyun · 2018-09-12T07:07:44Z

@ctelfer, @fcrisciani, @selansen, thanks for your time and review.

selansen · 2018-09-12T17:16:03Z

Thank you for your contribution.

Signed-off-by: Dani Louca <dani.louca@docker.com>

Backporting PR #2069 to bump_17.06

Backporting PR #2069 to bump_18.09

Signed-off-by: Dani Louca <dani.louca@docker.com> (cherry picked from commit 2ce4242) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

fcrisciani requested a review from ctelfer March 8, 2018 23:24

fcrisciani reviewed Mar 8, 2018

View reviewed changes

ctelfer suggested changes Mar 9, 2018

View reviewed changes

fanjiyun force-pushed the 1-Rolling-back-the-port-configs branch 2 times, most recently from b92bddd to 9a00354 Compare August 9, 2018 08:01

ctelfer suggested changes Aug 22, 2018

View reviewed changes

fanjiyun force-pushed the 1-Rolling-back-the-port-configs branch from 9a00354 to 9220c35 Compare August 23, 2018 09:08

fanjiyun force-pushed the 1-Rolling-back-the-port-configs branch 2 times, most recently from acbea82 to b8d6826 Compare August 29, 2018 09:23

fanjiyun force-pushed the 1-Rolling-back-the-port-configs branch from b8d6826 to efcdf4a Compare August 29, 2018 09:56

ctelfer approved these changes Aug 29, 2018

View reviewed changes

fanjiyun force-pushed the 1-Rolling-back-the-port-configs branch from efcdf4a to 78104ee Compare August 30, 2018 04:35

fanjiyun changed the title ~~Rolling back the port configs if failed to add/delete ingress~~ Rolling back the port configs if failed to programIngress() Aug 30, 2018

selansen reviewed Sep 10, 2018

View reviewed changes

fanjiyun force-pushed the 1-Rolling-back-the-port-configs branch from 78104ee to 8c39369 Compare September 11, 2018 11:02

Rolling back the port configs if failed to programIngress()

db50782

Signed-off-by: fanjiyun <fan.jiyun@zte.com.cn>

fanjiyun force-pushed the 1-Rolling-back-the-port-configs branch from 8c39369 to db50782 Compare September 11, 2018 11:12

ctelfer merged commit a5a6ca3 into moby:master Sep 11, 2018

lowenna mentioned this pull request Sep 14, 2018

Revendor libnetwork @ 36d3bed0e9 moby/moby#37843

Merged

dani-docker pushed a commit to dani-docker/libnetwork that referenced this pull request May 22, 2019

Backporting PR moby#2069

2ce4242

Signed-off-by: Dani Louca <dani.louca@docker.com>

This was referenced May 22, 2019

Backporting PR #2069 to bump_17.06 #2381

Merged

Backporting PR #2069 to bump_18.09 #2382

Merged

euanh added a commit that referenced this pull request Jun 24, 2019

Merge pull request #2381 from dani-docker/bump_17.06_1550

8f33909

Backporting PR #2069 to bump_17.06

euanh added a commit that referenced this pull request Jun 24, 2019

Merge pull request #2382 from dani-docker/bump_18.09_1550

36ff5f8

Backporting PR #2069 to bump_18.09

kolyshkin pushed a commit to kolyshkin/libnetwork that referenced this pull request Jul 25, 2019

Backporting PR moby#2069

7e9536d

Signed-off-by: Dani Louca <dani.louca@docker.com> (cherry picked from commit 2ce4242) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin mentioned this pull request Jul 25, 2019

[18.03 backport] Rolling back the port configs if failed to programIngress() (ENGCORE-914) #2426

Merged

Rolling back the port configs if failed to programIngress() #2069

Rolling back the port configs if failed to programIngress() #2069

Uh oh!

Conversation

fanjiyun commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 5, 2018

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctelfer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanjiyun commented Mar 12, 2018

Uh oh!

fanjiyun commented Aug 9, 2018

Uh oh!

fanjiyun commented Aug 21, 2018

Uh oh!

ctelfer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanjiyun commented Aug 23, 2018

Uh oh!

fanjiyun commented Aug 29, 2018

Uh oh!

ctelfer commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanjiyun commented Aug 29, 2018

Uh oh!

ctelfer commented Aug 29, 2018

Uh oh!

ctelfer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanjiyun commented Aug 30, 2018

Uh oh!

fanjiyun commented Sep 2, 2018

Uh oh!

fanjiyun commented Sep 8, 2018

Uh oh!

ctelfer commented Sep 10, 2018

Uh oh!

selansen commented Sep 10, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen commented Sep 10, 2018

Uh oh!

selansen commented Sep 10, 2018

Uh oh!

fanjiyun commented Sep 11, 2018

Uh oh!

selansen commented Sep 11, 2018

Uh oh!

fanjiyun commented Feb 5, 2018 •

edited

Loading

ctelfer commented Aug 29, 2018 •

edited

Loading