Skip to content

Commit c2b2db1

Browse files
vakalapavakalapa
authored
[NPM] RETURN early on MARK in DROP chains (#881)
* Returning in DROP chains * adding a comment about future cleanup of chains: * Removing duplicate rules in PORT chains of TO/FROM and DROP chain jumps * Adding a image of new chains behavior * Addressing comments * addressing some comments * Correcting some UTs to not have the jump rules * removing jump flag
1 parent 5e534da commit c2b2db1

File tree

6 files changed

+71
-918
lines changed

6 files changed

+71
-918
lines changed

‎docs/images/npmIptableChainEval.png‎

268 KB
Loading

‎npm/iptm/helper.go‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,17 @@ import (
66
"github.com/Azure/azure-container-networking/npm/util"
77
)
88

9-
// GetAllChainsAndRules returns all NPM chains and rules
10-
func getAllChainsAndRules() [][]string {
9+
// getAllDefaultRules returns all NPM chains and rules
10+
func getAllDefaultRules() [][]string {
1111
funcList := []func() [][]string{
1212
getAzureNPMChainRules,
1313
getAzureNPMAcceptChainRules,
1414
getAzureNPMIngressChainRules,
1515
getAzureNPMIngressPortChainRules,
16-
getAzureNPMIngressFromChainRules,
1716
getAzureNPMEgressChainRules,
1817
getAzureNPMEgressPortChainRules,
19-
getAzureNPMEgressToChainRules,
18+
getAzureNPMIngressDropsChainRules,
19+
getAzureNPMEgressDropsChainRules,
2020
}
2121

2222
chainsAndRules := [][]string{}
@@ -168,14 +168,23 @@ func getAzureNPMIngressPortChainRules() [][]string {
168168
util.IptablesCommentFlag,
169169
fmt.Sprintf("RETURN-on-INGRESS-mark-%s", util.IptablesAzureIngressMarkHex),
170170
},
171+
{
172+
util.IptablesAzureIngressPortChain,
173+
util.IptablesJumpFlag,
174+
util.IptablesAzureIngressFromChain,
175+
util.IptablesModuleFlag,
176+
util.IptablesCommentModuleFlag,
177+
util.IptablesCommentFlag,
178+
fmt.Sprintf("ALL-JUMP-TO-%s", util.IptablesAzureIngressFromChain),
179+
},
171180
}
172181
}
173182

174-
// getAzureNPMIngressFromChainRules returns rules for AZURE-NPM-INGRESS-PORT
175-
func getAzureNPMIngressFromChainRules() [][]string {
183+
// getAzureNPMIngressDropsChainRules returns rules for AZURE-NPM-INGRESS-DROPS
184+
func getAzureNPMIngressDropsChainRules() [][]string {
176185
return [][]string{
177186
{
178-
util.IptablesAzureIngressFromChain,
187+
util.IptablesAzureIngressDropsChain,
179188
util.IptablesJumpFlag,
180189
util.IptablesReturn,
181190
util.IptablesModuleFlag,
@@ -232,7 +241,7 @@ func getAzureNPMEgressChainRules() [][]string {
232241
}
233242
}
234243

235-
// getAzureNPMEgressPortChainRules returns rules for AZURE-NPM-INGRESS-PORT
244+
// getAzureNPMEgressPortChainRules returns rules for AZURE-NPM-EGRESS-PORT
236245
func getAzureNPMEgressPortChainRules() [][]string {
237246
return [][]string{
238247
{
@@ -261,14 +270,23 @@ func getAzureNPMEgressPortChainRules() [][]string {
261270
util.IptablesCommentFlag,
262271
fmt.Sprintf("RETURN-on-EGRESS-mark-%s", util.IptablesAzureEgressMarkHex),
263272
},
273+
{
274+
util.IptablesAzureEgressPortChain,
275+
util.IptablesJumpFlag,
276+
util.IptablesAzureEgressToChain,
277+
util.IptablesModuleFlag,
278+
util.IptablesCommentModuleFlag,
279+
util.IptablesCommentFlag,
280+
fmt.Sprintf("ALL-JUMP-TO-%s", util.IptablesAzureEgressToChain),
281+
},
264282
}
265283
}
266284

267-
// getAzureNPMEgressToChainRules returns rules for AZURE-NPM-INGRESS-PORT
268-
func getAzureNPMEgressToChainRules() [][]string {
285+
// getAzureNPMEgressDropsChainRules returns rules for AZURE-NPM-EGRESS-DROPS
286+
func getAzureNPMEgressDropsChainRules() [][]string {
269287
return [][]string{
270288
{
271-
util.IptablesAzureEgressToChain,
289+
util.IptablesAzureEgressDropsChain,
272290
util.IptablesJumpFlag,
273291
util.IptablesReturn,
274292
util.IptablesModuleFlag,
@@ -281,7 +299,7 @@ func getAzureNPMEgressToChainRules() [][]string {
281299
fmt.Sprintf("RETURN-on-EGRESS-and-INGRESS-mark-%s", util.IptablesAzureAcceptMarkHex),
282300
},
283301
{
284-
util.IptablesAzureEgressToChain,
302+
util.IptablesAzureEgressDropsChain,
285303
util.IptablesJumpFlag,
286304
util.IptablesReturn,
287305
util.IptablesModuleFlag,

‎npm/iptm/helper_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import (
77
)
88

99
func TestGetAllChainsAndRules(t *testing.T) {
10-
allChainsandRules := getAllChainsAndRules()
10+
allChainsandRules := getAllDefaultRules()
1111

1212
parentNpmRulesCount := 6
1313

‎npm/iptm/iptm.go‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ type IptEntry struct {
4747
Chain string
4848
Flag string
4949
LockWaitTimeInSeconds string
50-
IsJumpEntry bool
5150
Specs []string
5251
}
5352

@@ -56,6 +55,16 @@ type IptablesManager struct {
5655
OperationFlag string
5756
}
5857

58+
func isDropsChain(chainName string) bool {
59+
60+
// Check if the chain name is one of the two DROP chains
61+
if (chainName == util.IptablesAzureIngressDropsChain) ||
62+
(chainName == util.IptablesAzureEgressDropsChain) {
63+
return true
64+
}
65+
return false
66+
}
67+
5968
// NewIptablesManager creates a new instance for IptablesManager object.
6069
func NewIptablesManager() *IptablesManager {
6170
iptMgr := &IptablesManager{
@@ -213,8 +222,8 @@ func (iptMgr *IptablesManager) UninitNpmChains() error {
213222
// AddAllRulesToChains Checks and adds all the rules in NPM chains
214223
func (iptMgr *IptablesManager) AddAllRulesToChains() error {
215224

216-
allChainsAndRules := getAllChainsAndRules()
217-
for _, rule := range allChainsAndRules {
225+
allDefaultRules := getAllDefaultRules()
226+
for _, rule := range allDefaultRules {
218227
entry := &IptEntry{
219228
Chain: rule[0],
220229
Specs: rule[1:],
@@ -359,7 +368,9 @@ func (iptMgr *IptablesManager) Add(entry *IptEntry) error {
359368

360369
log.Logf("Adding iptables entry: %+v.", entry)
361370

362-
if entry.IsJumpEntry {
371+
// Since there is a RETURN statement added to each DROP chain, we need to make sure
372+
// any new DROP rule added to ingress or egress DROPS chain is added at the BOTTOM
373+
if isDropsChain(entry.Chain) {
363374
iptMgr.OperationFlag = util.IptablesAppendFlag
364375
} else {
365376
iptMgr.OperationFlag = util.IptablesInsertionFlag

‎npm/translatePolicy.go‎

Lines changed: 16 additions & 136 deletions
Original file line numberDiff line numberDiff line change
@@ -162,14 +162,14 @@ func craftPartialIptablesCommentFromSelector(ns string, selector *metav1.LabelSe
162162

163163
func translateIngress(ns string, policyName string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyIngressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {
164164
var (
165-
sets []string // ipsets with type: net:hash
166-
namedPorts []string // ipsets with type: hash:ip,port
167-
lists []string // ipsets with type: list:set
168-
ipCidrs [][]string
169-
entries []*iptm.IptEntry
170-
fromRuleEntries []*iptm.IptEntry
171-
addedCidrEntry bool // all cidr entry will be added in one set per from/to rule
172-
addedIngressFromEntry, addedPortEntry bool // add drop entries at the end of the chain when there are non ALLOW-ALL* rules
165+
sets []string // ipsets with type: net:hash
166+
namedPorts []string // ipsets with type: hash:ip,port
167+
lists []string // ipsets with type: list:set
168+
ipCidrs [][]string
169+
entries []*iptm.IptEntry
170+
fromRuleEntries []*iptm.IptEntry
171+
addedCidrEntry bool // all cidr entry will be added in one set per from/to rule
172+
addedPortEntry bool // add drop entries at the end of the chain when there are non ALLOW-ALL* rules
173173
)
174174

175175
log.Logf("started parsing ingress rule")
@@ -308,7 +308,6 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS
308308
// TODO move IP cidrs rule to allow based only
309309
ipCidrs[i] = append(ipCidrs[i], except+util.IpsetNomatch)
310310
}
311-
addedIngressFromEntry = true
312311
}
313312
if j != 0 && addedCidrEntry {
314313
continue
@@ -408,7 +407,6 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS
408407
"-TO-"+targetSelectorComment,
409408
)
410409
fromRuleEntries = append(fromRuleEntries, entry)
411-
addedIngressFromEntry = true
412410
}
413411
addedCidrEntry = true
414412
}
@@ -524,7 +522,6 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS
524522
"-TO-"+targetSelectorComment,
525523
)
526524
entries = append(entries, entry)
527-
addedIngressFromEntry = true
528525
}
529526
continue
530527
}
@@ -627,7 +624,6 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS
627624
"-TO-"+targetSelectorComment,
628625
)
629626
entries = append(entries, entry)
630-
addedIngressFromEntry = true
631627
}
632628
continue
633629
}
@@ -758,7 +754,6 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS
758754
"-TO-"+targetSelectorComment,
759755
)
760756
entries = append(entries, entry)
761-
addedIngressFromEntry = true
762757
}
763758
}
764759

@@ -790,75 +785,20 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS
790785
entries = append(fromRuleEntries, entries...)
791786
}
792787

793-
if addedPortEntry && !addedIngressFromEntry {
794-
entry := &iptm.IptEntry{
795-
Chain: util.IptablesAzureIngressPortChain,
796-
Specs: append([]string(nil), targetSelectorIptEntrySpec...),
797-
IsJumpEntry: true,
798-
}
799-
entry.Specs = append(
800-
entry.Specs,
801-
util.IptablesJumpFlag,
802-
util.IptablesAzureIngressDropsChain,
803-
util.IptablesModuleFlag,
804-
util.IptablesCommentModuleFlag,
805-
util.IptablesCommentFlag,
806-
"ALLOW-ALL-TO-"+
807-
targetSelectorComment+
808-
"-TO-JUMP-TO-"+util.IptablesAzureIngressDropsChain,
809-
)
810-
entries = append(entries, entry)
811-
} else if addedIngressFromEntry {
812-
portEntry := &iptm.IptEntry{
813-
Chain: util.IptablesAzureIngressPortChain,
814-
Specs: append([]string(nil), targetSelectorIptEntrySpec...),
815-
IsJumpEntry: true,
816-
}
817-
portEntry.Specs = append(
818-
portEntry.Specs,
819-
util.IptablesJumpFlag,
820-
util.IptablesAzureIngressFromChain,
821-
util.IptablesModuleFlag,
822-
util.IptablesCommentModuleFlag,
823-
util.IptablesCommentFlag,
824-
"ALLOW-ALL-TO-"+
825-
targetSelectorComment+
826-
"-TO-JUMP-TO-"+util.IptablesAzureIngressFromChain,
827-
)
828-
entries = append(entries, portEntry)
829-
entry := &iptm.IptEntry{
830-
Chain: util.IptablesAzureIngressFromChain,
831-
Specs: append([]string(nil), targetSelectorIptEntrySpec...),
832-
IsJumpEntry: true,
833-
}
834-
entry.Specs = append(
835-
entry.Specs,
836-
util.IptablesJumpFlag,
837-
util.IptablesAzureIngressDropsChain,
838-
util.IptablesModuleFlag,
839-
util.IptablesCommentModuleFlag,
840-
util.IptablesCommentFlag,
841-
"ALLOW-ALL-TO-"+
842-
targetSelectorComment+
843-
"-TO-JUMP-TO-"+util.IptablesAzureIngressDropsChain,
844-
)
845-
entries = append(entries, entry)
846-
}
847-
848788
log.Logf("finished parsing ingress rule")
849789
return util.DropEmptyFields(sets), util.DropEmptyFields(namedPorts), util.DropEmptyFields(lists), ipCidrs, entries
850790
}
851791

852792
func translateEgress(ns string, policyName string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyEgressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {
853793
var (
854-
sets []string // ipsets with type: net:hash
855-
namedPorts []string // ipsets with type: hash:ip,port
856-
lists []string // ipsets with type: list:set
857-
ipCidrs [][]string
858-
entries []*iptm.IptEntry
859-
toRuleEntries []*iptm.IptEntry
860-
addedCidrEntry bool // all cidr entry will be added in one set per from/to rule
861-
addedEgressToEntry, addedPortEntry bool // add drop entry when there are non ALLOW-ALL* rules
794+
sets []string // ipsets with type: net:hash
795+
namedPorts []string // ipsets with type: hash:ip,port
796+
lists []string // ipsets with type: list:set
797+
ipCidrs [][]string
798+
entries []*iptm.IptEntry
799+
toRuleEntries []*iptm.IptEntry
800+
addedCidrEntry bool // all cidr entry will be added in one set per from/to rule
801+
addedPortEntry bool // add drop entry when there are non ALLOW-ALL* rules
862802
)
863803

864804
log.Logf("started parsing egress rule")
@@ -993,7 +933,6 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe
993933
// TODO move IP cidrs rule to allow based only
994934
ipCidrs[i] = append(ipCidrs[i], except+util.IpsetNomatch)
995935
}
996-
addedEgressToEntry = true
997936
}
998937
if j != 0 && addedCidrEntry {
999938
continue
@@ -1099,7 +1038,6 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe
10991038
"-FROM-"+targetSelectorComment,
11001039
)
11011040
toRuleEntries = append(toRuleEntries, entry)
1102-
addedEgressToEntry = true
11031041
}
11041042
addedCidrEntry = true
11051043
}
@@ -1215,7 +1153,6 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe
12151153
"-TO-"+iptPartialNsComment,
12161154
)
12171155
entries = append(entries, entry)
1218-
addedEgressToEntry = true
12191156
}
12201157
continue
12211158
}
@@ -1318,7 +1255,6 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe
13181255
"-TO-"+iptPartialPodComment,
13191256
)
13201257
entries = append(entries, entry)
1321-
addedEgressToEntry = true
13221258
}
13231259
continue
13241260
}
@@ -1449,7 +1385,6 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe
14491385
"-AND-"+iptPartialPodComment,
14501386
)
14511387
entries = append(entries, entry)
1452-
addedEgressToEntry = true
14531388
}
14541389
}
14551390

@@ -1482,61 +1417,6 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe
14821417
entries = append(toRuleEntries, entries...)
14831418
}
14841419

1485-
if addedPortEntry && !addedEgressToEntry {
1486-
entry := &iptm.IptEntry{
1487-
Chain: util.IptablesAzureEgressPortChain,
1488-
Specs: append([]string(nil), targetSelectorIptEntrySpec...),
1489-
IsJumpEntry: true,
1490-
}
1491-
entry.Specs = append(
1492-
entry.Specs,
1493-
util.IptablesJumpFlag,
1494-
util.IptablesAzureEgressDropsChain,
1495-
util.IptablesModuleFlag,
1496-
util.IptablesCommentModuleFlag,
1497-
util.IptablesCommentFlag,
1498-
"ALLOW-ALL-FROM-"+
1499-
targetSelectorComment+
1500-
"-TO-JUMP-TO-"+util.IptablesAzureEgressDropsChain,
1501-
)
1502-
entries = append(entries, entry)
1503-
} else if addedEgressToEntry {
1504-
portEntry := &iptm.IptEntry{
1505-
Chain: util.IptablesAzureEgressPortChain,
1506-
Specs: append([]string(nil), targetSelectorIptEntrySpec...),
1507-
IsJumpEntry: true,
1508-
}
1509-
portEntry.Specs = append(
1510-
portEntry.Specs,
1511-
util.IptablesJumpFlag,
1512-
util.IptablesAzureEgressToChain,
1513-
util.IptablesModuleFlag,
1514-
util.IptablesCommentModuleFlag,
1515-
util.IptablesCommentFlag,
1516-
"ALLOW-ALL-FROM-"+
1517-
targetSelectorComment+
1518-
"-TO-JUMP-TO-"+util.IptablesAzureEgressToChain,
1519-
)
1520-
entries = append(entries, portEntry)
1521-
entry := &iptm.IptEntry{
1522-
Chain: util.IptablesAzureEgressToChain,
1523-
Specs: append([]string(nil), targetSelectorIptEntrySpec...),
1524-
IsJumpEntry: true,
1525-
}
1526-
entry.Specs = append(
1527-
entry.Specs,
1528-
util.IptablesJumpFlag,
1529-
util.IptablesAzureEgressDropsChain,
1530-
util.IptablesModuleFlag,
1531-
util.IptablesCommentModuleFlag,
1532-
util.IptablesCommentFlag,
1533-
"ALLOW-ALL-FROM-"+
1534-
targetSelectorComment+
1535-
"-TO-JUMP-TO-"+util.IptablesAzureEgressDropsChain,
1536-
)
1537-
entries = append(entries, entry)
1538-
}
1539-
15401420
log.Logf("finished parsing egress rule")
15411421
return util.DropEmptyFields(sets), util.DropEmptyFields(namedPorts), util.DropEmptyFields(lists), ipCidrs, entries
15421422
}

0 commit comments

Comments
 (0)