Skip to content

Commit 99d2b15

Browse files
author
Joseph Chen
committed
Refactor IPTable Rules
1 parent b0ad571 commit 99d2b15

File tree

8 files changed

+268
-179
lines changed

8 files changed

+268
-179
lines changed

‎pkg/ipamd/ipamd.go

+5
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,11 @@ func (c *IPAMContext) nodeInit() error {
395395
if err != nil {
396396
return errors.Wrap(err, "ipamd init: failed to set up host network")
397397
}
398+
err = c.networkClient.CleanUpStaleAWSChains(c.enableIPv4, c.enableIPv6)
399+
if err != nil {
400+
// We should not error if clean up fails since these chains don't affect the rules
401+
log.Debugf("Failed to clean up stale AWS chains: %v", err)
402+
}
398403

399404
metadataResult, err := c.awsClient.DescribeAllENIs()
400405
if err != nil {

‎pkg/ipamd/ipamd_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ func TestNodeInit(t *testing.T) {
150150
m.awsutils.EXPECT().GetVPCIPv4CIDRs().AnyTimes().Return(cidrs, nil)
151151
m.awsutils.EXPECT().GetPrimaryENImac().Return("")
152152
m.network.EXPECT().SetupHostNetwork(cidrs, "", &primaryIP, false, true, false).Return(nil)
153+
m.network.EXPECT().CleanUpStaleAWSChains(true, false).Return(nil)
153154
m.awsutils.EXPECT().GetPrimaryENI().AnyTimes().Return(primaryENIid)
154155
m.awsutils.EXPECT().RefreshSGIDs(gomock.Any()).AnyTimes().Return(nil)
155156

@@ -234,6 +235,7 @@ func TestNodeInitwithPDenabledIPv4Mode(t *testing.T) {
234235
m.awsutils.EXPECT().GetVPCIPv4CIDRs().AnyTimes().Return(cidrs, nil)
235236
m.awsutils.EXPECT().GetPrimaryENImac().Return("")
236237
m.network.EXPECT().SetupHostNetwork(cidrs, "", &primaryIP, false, true, false).Return(nil)
238+
m.network.EXPECT().CleanUpStaleAWSChains(true, false).Return(nil)
237239
m.awsutils.EXPECT().GetPrimaryENI().AnyTimes().Return(primaryENIid)
238240
m.awsutils.EXPECT().RefreshSGIDs(gomock.Any()).AnyTimes().Return(nil)
239241

@@ -308,6 +310,7 @@ func TestNodeInitwithPDenabledIPv6Mode(t *testing.T) {
308310

309311
primaryIP := net.ParseIP(ipaddr01)
310312
m.network.EXPECT().SetupHostNetwork(cidrs, eni1.MAC, &primaryIP, false, false, true).Return(nil)
313+
m.network.EXPECT().CleanUpStaleAWSChains(false, true).Return(nil)
311314
m.awsutils.EXPECT().GetIPv6PrefixesFromEC2(eni1.ENIID).AnyTimes().Return(eni1.IPv6Prefixes, nil)
312315
m.awsutils.EXPECT().GetPrimaryENI().AnyTimes().Return(primaryENIid)
313316
m.awsutils.EXPECT().GetPrimaryENImac().Return(eni1.MAC)

‎pkg/networkutils/mocks/network_mocks.go

+14
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/networkutils/network.go

+95-58
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ type NetworkAPIs interface {
152152
SetupENINetwork(eniIP string, mac string, deviceNumber int, subnetCIDR string) error
153153
// UpdateHostIptablesRules updates the nat table iptables rules on the host
154154
UpdateHostIptablesRules(vpcCIDRs []string, primaryMAC string, primaryAddr *net.IP, v4Enabled bool, v6Enabled bool) error
155+
CleanUpStaleAWSChains(v4Enabled, v6Enabled bool) error
155156
UseExternalSNAT() bool
156157
GetExcludeSNATCIDRs() []string
157158
GetExternalServiceCIDRs() []string
@@ -375,6 +376,44 @@ func (n *linuxNetwork) UpdateHostIptablesRules(vpcCIDRs []string, primaryMAC str
375376
return n.updateHostIptablesRules(vpcCIDRs, primaryMAC, primaryAddr, v4Enabled, v6Enabled)
376377
}
377378

379+
func (n *linuxNetwork) CleanUpStaleAWSChains(v4Enabled, v6Enabled bool) error {
380+
ipProtocol := iptables.ProtocolIPv4
381+
if v6Enabled {
382+
ipProtocol = iptables.ProtocolIPv6
383+
}
384+
385+
ipt, err := n.newIptables(ipProtocol)
386+
if err != nil {
387+
return errors.Wrap(err, "stale chain cleanup: failed to create iptables")
388+
}
389+
390+
existingChains, err := ipt.ListChains("nat")
391+
if err != nil {
392+
return errors.Wrap(err, "stale chain cleanup: failed to list iptables nat chains")
393+
}
394+
395+
for _, chain := range existingChains {
396+
if !strings.HasPrefix(chain, "AWS-CONNMARK-CHAIN") && !strings.HasPrefix(chain, "AWS-SNAT-CHAIN") {
397+
continue
398+
}
399+
parsedChain := strings.Split(chain, "-")
400+
chainNum, err := strconv.Atoi(parsedChain[len(parsedChain)-1])
401+
if err != nil {
402+
return errors.Wrap(err, "stale chain cleanup: failed to convert string to int")
403+
}
404+
// Chains 1 --> x (0 indexed) will be stale
405+
if chainNum > 0 {
406+
// No need to clear the chain since computeStaleIptablesRules cleans up all rules already
407+
log.Infof("Deleting stale chain: %s", chain)
408+
err := ipt.DeleteChain("nat", chain)
409+
if err != nil {
410+
return errors.Wrapf(err, "stale chain cleanup: failed to delete chain %s", chain)
411+
}
412+
}
413+
}
414+
return nil
415+
}
416+
378417
func (n *linuxNetwork) updateHostIptablesRules(vpcCIDRs []string, primaryMAC string, primaryAddr *net.IP, v4Enabled bool,
379418
v6Enabled bool) error {
380419
primaryIntf, err := findPrimaryInterfaceName(primaryMAC)
@@ -434,15 +473,13 @@ func (n *linuxNetwork) buildIptablesSNATRules(vpcCIDRs []string, primaryAddr *ne
434473
log.Debugf("Total CIDRs to program - %d", len(allCIDRs))
435474
// build IPTABLES chain for SNAT of non-VPC outbound traffic and excluded CIDRs
436475
var chains []string
437-
for i := 0; i <= len(allCIDRs); i++ {
438-
chain := fmt.Sprintf("AWS-SNAT-CHAIN-%d", i)
439-
log.Debugf("Setup Host Network: iptables -N %s -t nat", chain)
440-
if err := ipt.NewChain("nat", chain); err != nil && !containChainExistErr(err) {
441-
log.Errorf("ipt.NewChain error for chain [%s]: %v", chain, err)
442-
return []iptablesRule{}, errors.Wrapf(err, "host network setup: failed to add chain")
443-
}
444-
chains = append(chains, chain)
476+
chain := "AWS-SNAT-CHAIN-0"
477+
log.Debugf("Setup Host Network: iptables -N %s -t nat", chain)
478+
if err := ipt.NewChain("nat", chain); err != nil && !containChainExistErr(err) {
479+
log.Errorf("ipt.NewChain error for chain [%s]: %v", chain, err)
480+
return []iptablesRule{}, errors.Wrapf(err, "host network setup: failed to add chain")
445481
}
482+
chains = append(chains, chain)
446483

447484
// build SNAT rules for outbound non-VPC traffic
448485
var iptableRules []iptablesRule
@@ -456,23 +493,20 @@ func (n *linuxNetwork) buildIptablesSNATRules(vpcCIDRs []string, primaryAddr *ne
456493
"-m", "comment", "--comment", "AWS SNAT CHAIN", "-j", "AWS-SNAT-CHAIN-0",
457494
}})
458495

459-
for i, cidr := range allCIDRs {
460-
curChain := chains[i]
461-
curName := fmt.Sprintf("[%d] AWS-SNAT-CHAIN", i)
462-
nextChain := chains[i+1]
496+
for _, cidr := range allCIDRs {
463497
comment := "AWS SNAT CHAIN"
464498
if cidr.isExclusion {
465499
comment += " EXCLUSION"
466500
}
467-
log.Debugf("Setup Host Network: iptables -A %s ! -d %s -t nat -j %s", curChain, cidr, nextChain)
501+
log.Debugf("Setup Host Network: iptables -A %s -d %s -t nat -j %s", chain, cidr, "RETURN")
468502

469503
iptableRules = append(iptableRules, iptablesRule{
470-
name: curName,
504+
name: chain,
471505
shouldExist: !n.useExternalSNAT,
472506
table: "nat",
473-
chain: curChain,
507+
chain: chain,
474508
rule: []string{
475-
"!", "-d", cidr.cidr, "-m", "comment", "--comment", comment, "-j", nextChain,
509+
"-d", cidr.cidr, "-m", "comment", "--comment", comment, "-j", "RETURN",
476510
}})
477511
}
478512

@@ -494,22 +528,21 @@ func (n *linuxNetwork) buildIptablesSNATRules(vpcCIDRs []string, primaryAddr *ne
494528
}
495529
}
496530

497-
lastChain := chains[len(chains)-1]
498-
iptableRules = append(iptableRules, iptablesRule{
499-
name: "last SNAT rule for non-VPC outbound traffic",
500-
shouldExist: !n.useExternalSNAT,
501-
table: "nat",
502-
chain: lastChain,
503-
rule: snatRule,
504-
})
505-
506531
snatStaleRules, err := computeStaleIptablesRules(ipt, "nat", "AWS-SNAT-CHAIN", iptableRules, chains)
507532
if err != nil {
508533
return []iptablesRule{}, err
509534
}
510535

511536
iptableRules = append(iptableRules, snatStaleRules...)
512537

538+
iptableRules = append(iptableRules, iptablesRule{
539+
name: "last SNAT rule for non-VPC outbound traffic",
540+
shouldExist: !n.useExternalSNAT,
541+
table: "nat",
542+
chain: chain,
543+
rule: snatRule,
544+
})
545+
513546
iptableRules = append(iptableRules, iptablesRule{
514547
name: "connmark for primary ENI",
515548
shouldExist: n.nodePortSupportEnabled,
@@ -556,16 +589,15 @@ func (n *linuxNetwork) buildIptablesConnmarkRules(vpcCIDRs []string, ipt iptable
556589
excludeCIDRs := sets.NewString(n.excludeSNATCIDRs...)
557590

558591
log.Debugf("Total CIDRs to exempt from connmark rules - %d", len(allCIDRs))
592+
559593
var chains []string
560-
for i := 0; i <= len(allCIDRs); i++ {
561-
chain := fmt.Sprintf("AWS-CONNMARK-CHAIN-%d", i)
562-
log.Debugf("Setup Host Network: iptables -N %s -t nat", chain)
563-
if err := ipt.NewChain("nat", chain); err != nil && !containChainExistErr(err) {
564-
log.Errorf("ipt.NewChain error for chain [%s]: %v", chain, err)
565-
return []iptablesRule{}, errors.Wrapf(err, "host network setup: failed to add chain")
566-
}
567-
chains = append(chains, chain)
594+
chain := "AWS-CONNMARK-CHAIN-0"
595+
log.Debugf("Setup Host Network: iptables -N %s -t nat", chain)
596+
if err := ipt.NewChain("nat", chain); err != nil && !containChainExistErr(err) {
597+
log.Errorf("ipt.NewChain error for chain [%s]: %v", chain, err)
598+
return []iptablesRule{}, errors.Wrapf(err, "host network setup: failed to add chain")
568599
}
600+
chains = append(chains, chain)
569601

570602
var iptableRules []iptablesRule
571603
log.Debugf("Setup Host Network: iptables -t nat -A PREROUTING -i %s+ -m comment --comment \"AWS, outbound connections\" -j AWS-CONNMARK-CHAIN-0", n.vethPrefix)
@@ -590,37 +622,23 @@ func (n *linuxNetwork) buildIptablesConnmarkRules(vpcCIDRs []string, ipt iptable
590622
"-j", "AWS-CONNMARK-CHAIN-0",
591623
}})
592624

593-
for i, cidr := range allCIDRs {
594-
curChain := chains[i]
595-
curName := fmt.Sprintf("[%d] AWS-SNAT-CHAIN", i)
596-
nextChain := chains[i+1]
625+
for _, cidr := range allCIDRs {
597626
comment := "AWS CONNMARK CHAIN, VPC CIDR"
598627
if excludeCIDRs.Has(cidr) {
599628
comment = "AWS CONNMARK CHAIN, EXCLUDED CIDR"
600629
}
601-
log.Debugf("Setup Host Network: iptables -A %s ! -d %s -t nat -j %s", curChain, cidr, nextChain)
630+
log.Debugf("Setup Host Network: iptables -A %s -d %s -t nat -j %s", chain, cidr, "RETURN")
602631

603632
iptableRules = append(iptableRules, iptablesRule{
604-
name: curName,
633+
name: chain,
605634
shouldExist: !n.useExternalSNAT,
606635
table: "nat",
607-
chain: curChain,
636+
chain: chain,
608637
rule: []string{
609-
"!", "-d", cidr, "-m", "comment", "--comment", comment, "-j", nextChain,
638+
"-d", cidr, "-m", "comment", "--comment", comment, "-j", "RETURN",
610639
}})
611640
}
612641

613-
iptableRules = append(iptableRules, iptablesRule{
614-
name: "connmark rule for external outbound traffic",
615-
shouldExist: !n.useExternalSNAT,
616-
table: "nat",
617-
chain: chains[len(chains)-1],
618-
rule: []string{
619-
"-m", "comment", "--comment", "AWS, CONNMARK", "-j", "CONNMARK",
620-
"--set-xmark", fmt.Sprintf("%#x/%#x", n.mainENIMark, n.mainENIMark),
621-
},
622-
})
623-
624642
// Force delete existing restore mark rule so that the subsequent rule gets added to the end
625643
iptableRules = append(iptableRules, iptablesRule{
626644
name: "connmark to fwmark copy",
@@ -652,14 +670,24 @@ func (n *linuxNetwork) buildIptablesConnmarkRules(vpcCIDRs []string, ipt iptable
652670
}
653671
iptableRules = append(iptableRules, connmarkStaleRules...)
654672

673+
iptableRules = append(iptableRules, iptablesRule{
674+
name: "connmark rule for external outbound traffic",
675+
shouldExist: !n.useExternalSNAT,
676+
table: "nat",
677+
chain: chain,
678+
rule: []string{
679+
"-m", "comment", "--comment", "AWS, CONNMARK", "-j", "CONNMARK",
680+
"--set-xmark", fmt.Sprintf("%#x/%#x", n.mainENIMark, n.mainENIMark),
681+
},
682+
})
683+
655684
log.Debugf("iptableRules: %v", iptableRules)
656685
return iptableRules, nil
657686
}
658687

659688
func (n *linuxNetwork) updateIptablesRules(iptableRules []iptablesRule, ipt iptableswrapper.IPTablesIface) error {
660689
for _, rule := range iptableRules {
661690
log.Debugf("execute iptable rule : %s", rule.name)
662-
663691
exists, err := ipt.Exists(rule.table, rule.chain, rule.rule...)
664692
log.Debugf("rule %v exists %v, err %v", rule, exists, err)
665693
if err != nil {
@@ -668,10 +696,19 @@ func (n *linuxNetwork) updateIptablesRules(iptableRules []iptablesRule, ipt ipta
668696
}
669697

670698
if !exists && rule.shouldExist {
671-
err = ipt.Append(rule.table, rule.chain, rule.rule...)
672-
if err != nil {
673-
log.Errorf("host network setup: failed to add %v, %v", rule, err)
674-
return errors.Wrapf(err, "host network setup: failed to add %v", rule)
699+
if rule.name == "AWS-CONNMARK-CHAIN-0" || rule.name == "AWS-SNAT-CHAIN-0" {
700+
// All CIDR rules must go before the SNAT/Mark rule
701+
err = ipt.Insert(rule.table, rule.chain, 1, rule.rule...)
702+
if err != nil {
703+
log.Errorf("host network setup: failed to insert %v, %v", rule, err)
704+
return errors.Wrapf(err, "host network setup: failed to add %v", rule)
705+
}
706+
} else {
707+
err = ipt.Append(rule.table, rule.chain, rule.rule...)
708+
if err != nil {
709+
log.Errorf("host network setup: failed to add %v, %v", rule, err)
710+
return errors.Wrapf(err, "host network setup: failed to add %v", rule)
711+
}
675712
}
676713
} else if exists && !rule.shouldExist {
677714
err = ipt.Delete(rule.table, rule.chain, rule.rule...)
@@ -726,7 +763,7 @@ func computeStaleIptablesRules(ipt iptableswrapper.IPTablesIface, table, chainPr
726763
return []iptablesRule{}, errors.Wrapf(err, "host network setup: failed to list rules from table %s with chain prefix %s", table, chainPrefix)
727764
}
728765
activeChains := sets.NewString(chains...)
729-
log.Debugf("Setup Host Network: computing stale iptables rules for %s table with chain prefix %s")
766+
log.Debugf("Setup Host Network: computing stale iptables rules for %s table with chain prefix %s", table, chainPrefix)
730767
for _, staleRule := range existingRules {
731768
if len(staleRule.rule) == 0 && activeChains.Has(staleRule.chain) {
732769
log.Debugf("Setup Host Network: active chain found: %s", staleRule.chain)

0 commit comments

Comments
 (0)