Add logic to handle multiple VPC CIDRs#234
Conversation
mogren
left a comment
mogren
left a comment
There was a problem hiding this comment.
Will take another look in the morning, thanks for adding this!
| // setting will be disabled. | ||
| func (n *linuxNetwork) UseExternalSNAT() bool { | ||
| return getBoolEnvVar(envExternalSNAT, false) | ||
| } |
There was a problem hiding this comment.
Nit. Could it just be return useExternalSNAT() instead, since they do the same thing? Or should the env variable be saved as a property on the linuxNetwork?
| log.Errorf("Failed to add fromContainer rule for %s err: %v", addr.String(), err) | ||
| return errors.Wrap(err, "add NS network: failed to add fromContainer rule") | ||
| if useExternalSNAT { | ||
| // add rule: 1536: from <podIP> use table <table> |
There was a problem hiding this comment.
That's the priority of the ip rule.
| } | ||
| log.Infof("Added rule priority %d from %s table %d", fromContainerRulePriority, addr.String(), table) | ||
| } else { | ||
| // add rule: 1536: list of from <podIP> to <vpcCIDR> use table <table> |
| return nil | ||
| } | ||
|
|
||
| if toFlag { |
There was a problem hiding this comment.
You pass in !c.networkClient.UseExternalSNAT() to set this boolean. I think it needs a more descriptive name, maybe something as straight forward as useExternalSNAT (and then flip the if-statements of course.)
There was a problem hiding this comment.
I am trying to keep useExternalSNAT on level above UpdateRuleListBySrc() call.
| { | ||
| name: fmt.Sprintf("rule for primary address %s", primaryAddr), | ||
| // build IPTABLES chain for SNAT of non-VPC outbound traffic | ||
| for i, _ := range vpcCIDRs { |
There was a problem hiding this comment.
If this for-loop was from 0 to len(vpcCIDRs), there would be no need for the duplicate code on lines 256-262:
for i := 0; i <= len(vpcCIDRs); i++ {
liwenwu-amazon
force-pushed
the
nat-rule
branch
from
2a67f7d to
2dd374e
Compare
mogren
left a comment
mogren
left a comment
There was a problem hiding this comment.
Still some refactors that would improve the readability, but they can wait a bit.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
|
Awesome, I believe we ran into this... how do we get/run the latest version with this fix on EKS? Right now we're disabling SNAT with the environment variable. Edit: Oh, looks like this the fix that uses the env var? |
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
|
Was losing my mind trying to understand why my EKS networking was totally unreliable and this was my issue. Thank you so much @liwenwu-amazon! |
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
Two small bugfixes in the error handling when deleting rules, otherwise no functional changes.
5 participants
Issue #35 , if available:
Description of Changes:
modify IP rules for the Pod who is using non-primary
ENIAWS_VPC_K8S_CNI_EXTERNALSNATis set tofalseand packet's IP-DA is not in any VPC CIDRs range, use main routing tableENI's route tablemodify IPTable rules
AWS_VPC_K8S_CNI_EXTERNALSNATis set tofalse, SNAT the packet with primary interface IP address if packet's IP-DA is not in any VPC CIDR rangesTests Performed
Testing Topology
Verify ping works and also verify expected ping traffic is going through expected
ENI(usingtcpdump) for following cases:AWS_VPC_K8S_CNI_EXTERNALSNATis set tofalseAWS_VPC_K8S_CNI_EXTERNALSNATis set totrueBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.