Improve scalabiltiy of bridge network isolation rules - #1534
Conversation
| actionMsg = "remove" | ||
| } | ||
|
|
||
| for i := 0; i < 2; i++ { |
There was a problem hiding this comment.
Refactor this to something like this:
for i, chain := range chains {
iptables.ProgramRule(..., chain, action, rules[i])
....
}
There was a problem hiding this comment.
Ok, I will change it, thanks.
| IsolationChain = "DOCKER-ISOLATION" | ||
| DockerChain = "DOCKER" | ||
| IsolationChain = "DOCKER-ISOLATION" | ||
| IsolationChain2 = "DOCKER-ISOLATION-REV" |
There was a problem hiding this comment.
Please rename DOCKER-ISOLATION-REV chain name and IsolationChain2 variable to something more meaningful. Right now no one will understand what they are for.
There was a problem hiding this comment.
I thought REV for reverse as it goes had in hand with DOCKER-ISOLATION chain.
One filter by source, the other one by destination.
I chose a short suffix because the chain name is already long.
Would expand it to DOCKER-ISOLATION-REVERSE make it better ?
Or should I change also the existing one and have two different suffixes, like FORWARD and REVERSE or SRC and DST ?
In the meantime I will replace the variable names to something more descriptive. Or I will add a comment.
| if err := iptables.ProgramRule(iptables.Filter, chains[i], action, rules[i]); err != nil { | ||
| msg := fmt.Sprintf("unable to %s inter-network communication rule: %v", actionMsg, err) | ||
| if enable { | ||
| return fmt.Errorf(msg) |
There was a problem hiding this comment.
Shouldn't you have to rollback the rule #1 if there is a failure in plumbing rule#2?
There was a problem hiding this comment.
Yes we should. Thanks.
|
Thanks @mrjana for the comments. I think I addressed them. PTAL. |
- This reduces complexity from O(N^2) to O(2N) Signed-off-by: Alessandro Boch <aboch@docker.com>
|
@aboch It has been detected that this issue has not received any activity in over 6 months. Can you please let us know if it is still relevant:
Thank you! |
Yes
No |
|
@fcrisciani @ctelfer @thaJeztah Any chance to get this merged? 🙏 |
|
@AkihiroSuda there is conflicts so we will need someone to take this patch over, will try to see if there is someone able to follow up on this |
|
@AkihiroSuda are you interested in carrying this? |
|
carried as #2117 |
|
closing because carried in another PR |
Related to moby/moby#26435
Reported as example the time measurement for creating 50 bridge networks and for pruning them.
Also because of the removal of the loop in
isolateNetwork(), the timing is drastically improved: