Skip to content

Commit

Permalink
policy: Use addKeysWithChanges in AddvisibilityKeys
Browse files Browse the repository at this point in the history 
Use addKeysWithChanges in AddVisibilityKeys instead of directly
manipulating the maps to keep track of changes in a uniform fashion. Also
record changes when adding dependents to existing keys.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
  • Loading branch information
@jrajahalme
jrajahalme committed Jul 12, 2023
1 parent 7877f35 commit cb50a88
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 46 deletions.
2 changes: 1 addition & 1 deletion pkg/policy/distillery_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ func testCaseToMapState(t generatedBPFKey) MapState {
denyL3L4, denyL3L4exists := m[mapKeyDeny_FooL4]
allowL4, allowL4exists := m[mapKeyAllow___L4]
if allowL4exists && !allowL4.IsDeny && denyL3exists && denyL3.IsDeny && denyL3L4exists && denyL3L4.IsDeny {
mapKeyDeny_Foo__.AddDependent(m, mapKeyDeny_FooL4)
m.AddDependent(mapKeyDeny_Foo__, mapKeyDeny_FooL4, nil)
}
return m
}
Expand Down
56 changes: 21 additions & 35 deletions pkg/policy/mapstate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,19 @@ func (e *MapStateEntry) getNets(identities Identities, ident uint32) []*net.IPNe
}

// AddDependent adds 'key' to the set of dependent keys.
func (owner Key) AddDependent(keys MapState, key Key) {
func (keys MapState) AddDependent(owner Key, dependent Key, old MapState) {
if e, exists := keys[owner]; exists {
e.AddDependent(key)
keys.addDependentOnEntry(owner, e, dependent, old)
}
}

// addDependentOnEntry adds 'dependent' to the set of dependent keys of 'e'.
func (keys MapState) addDependentOnEntry(owner Key, e MapStateEntry, dependent Key, old MapState) {
if _, exists := e.dependents[dependent]; !exists {
if old != nil {
old[owner] = e
}
e.AddDependent(dependent)
keys[owner] = e
}
}
Expand Down Expand Up @@ -606,8 +616,7 @@ func (keys MapState) denyPreferredInsertWithChanges(newKey Key, newEntry MapStat
// L3-only entries can be deleted incrementally so we need to track their
// effects on other entries so that those effects can be reverted when the
// identity is removed.
v.AddDependent(denyKeyCpy)
keys[k] = v
keys.addDependentOnEntry(k, v, denyKeyCpy, old)
}
} else if (k.Identity == newKey.Identity ||
entryIdentityIsSupersetOf(k, v, newKey, newEntry, identities)) &&
Expand Down Expand Up @@ -910,13 +919,7 @@ func (keys MapState) AddVisibilityKeys(e PolicyOwner, redirectPort uint16, visMe
logfields.BPFMapValue: entry,
}, "AddVisibilityKeys: Changing L4-only ALLOW key for visibility redirect")

// keep the original value for reverting purposes
oldValues.insertIfNotExists(key, l4Only)

l4Only.ProxyPort = redirectPort
l4Only.DerivedFromRules.MergeSorted(visibilityDerivedFrom)
keys[key] = l4Only
adds[key] = struct{}{}
keys.addKeyWithChanges(key, entry, adds, nil, oldValues)
}
if haveAllowAllKey && !haveL4OnlyKey {
// 2. If allow-all policy exists, add L4-only visibility redirect key if the L4-only
Expand All @@ -926,8 +929,7 @@ func (keys MapState) AddVisibilityKeys(e PolicyOwner, redirectPort uint16, visMe
logfields.BPFMapValue: entry,
}, "AddVisibilityKeys: Adding L4-only ALLOW key for visibilty redirect")
addL4OnlyKey = true
keys[key] = entry
adds[key] = struct{}{}
keys.addKeyWithChanges(key, entry, adds, nil, oldValues)
}
//
// Loop through all L3 keys in the traffic direction of the new key
Expand All @@ -944,17 +946,13 @@ func (keys MapState) AddVisibilityKeys(e PolicyOwner, redirectPort uint16, visMe
// 3. Change all L3/L4 ALLOW keys on matching port that do not
// already redirect to redirect.

// keep the original value for reverting purposes
oldValues.insertIfNotExists(k, v)

v.ProxyPort = redirectPort
v.DerivedFromRules.MergeSorted(visibilityDerivedFrom)
v.DerivedFromRules = visibilityDerivedFrom
e.PolicyDebug(logrus.Fields{
logfields.BPFMapKey: k,
logfields.BPFMapValue: v,
}, "AddVisibilityKeys: Changing L3/L4 ALLOW key for visibility redirect")
keys[k] = v
adds[k] = struct{}{}
keys.addKeyWithChanges(k, v, adds, nil, oldValues)
}
} else if k.DestPort == 0 && k.Nexthdr == 0 {
//
Expand All @@ -976,16 +974,10 @@ func (keys MapState) AddVisibilityKeys(e PolicyOwner, redirectPort uint16, visMe
logfields.BPFMapKey: k2,
logfields.BPFMapValue: v2,
}, "AddVisibilityKeys: Extending L3-only ALLOW key to L3/L4 key for visibilty redirect")
keys[k2] = v2
adds[k2] = struct{}{}

// keep the original value for reverting purposes
oldValues.insertIfNotExists(k, v)
keys.addKeyWithChanges(k2, v2, adds, nil, oldValues)

// Mark the new entry as a dependent of 'v'
v.AddDependent(k2)
keys[k] = v
adds[k] = struct{}{} // dependent was added
keys.addDependentOnEntry(k, v, k2, oldValues)
}
} else if addL4OnlyKey && v.IsDeny {
// 5. If a new L4-only key was added: For each L3-only DENY
Expand All @@ -997,16 +989,10 @@ func (keys MapState) AddVisibilityKeys(e PolicyOwner, redirectPort uint16, visMe
logfields.BPFMapKey: k2,
logfields.BPFMapValue: v2,
}, "AddVisibilityKeys: Extending L3-only DENY key to L3/L4 key to deny a port with visibility annotation")
keys[k2] = v2
adds[k2] = struct{}{}

// keep the original value for reverting purposes
oldValues.insertIfNotExists(k, v)
keys.addKeyWithChanges(k2, v2, adds, nil, oldValues)

// Mark the new entry as a dependent of 'v'
v.AddDependent(k2)
keys[k] = v
adds[k] = struct{}{} // dependent was added
keys.addDependentOnEntry(k, v, k2, oldValues)
}
}
}
Expand Down
15 changes: 5 additions & 10 deletions pkg/policy/mapstate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1736,7 +1736,7 @@ func (ds *PolicyTestSuite) TestMapState_AddVisibilityKeys(c *check.C) {
visMeta: VisibilityMetadata{Ingress: true, Port: 80, Proto: u8proto.TCP},
},
want: MapState{
HttpIngressKey(0): allowEntryD(12345, labels.LabelArrayList{visibilityDerivedFromLabels, testLabels}),
HttpIngressKey(0): allowEntryD(12345, labels.LabelArrayList{visibilityDerivedFromLabels, testLabels}, nil),
},
},
{
Expand Down Expand Up @@ -1863,15 +1863,12 @@ func (ds *PolicyTestSuite) TestMapState_AddVisibilityKeys(c *check.C) {
for k, v := range tt.keys {
if v2, ok := old[k]; ok {
if equals, _ := checker.DeepEqual(v2, v); !equals {
wantAdds[k] = struct{}{}
wantOld[k] = v2
}
} else {
wantAdds[k] = struct{}{}
}
}
fmt.Printf("Adds: %v\n", adds)
fmt.Printf("wantAdds: %v\n", wantAdds)
c.Assert(adds, checker.DeepEquals, wantAdds, check.Commentf(tt.name))
c.Assert(visOld, checker.DeepEquals, wantOld, check.Commentf(tt.name))
}
Expand Down Expand Up @@ -1917,9 +1914,8 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChangesOnVisibilityKeys(c *
visMeta: VisibilityMetadata{Ingress: true, Port: 80, Proto: u8proto.TCP},
}},
visAdds: Keys{
HttpIngressKey(0): {},
testIngressKey(234, 0, 0): {}, // dependents changed
HttpIngressKey(234): {},
HttpIngressKey(0): {},
HttpIngressKey(234): {},
},
visOld: MapState{
testIngressKey(234, 0, 0): denyEntry(0, csFoo),
Expand Down Expand Up @@ -2110,7 +2106,6 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChangesOnVisibilityKeys(c *
},
visAdds: Keys{
HttpIngressKey(0): {},
HttpEgressKey(0): {},
},
visOld: MapState{
// Old value for the modified entry
Expand All @@ -2126,7 +2121,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChangesOnVisibilityKeys(c *
// Entry added solely due to visibility annotation has a 'nil' owner
HttpIngressKey(0): allowEntryD(12345, visibilityDerivedFrom).WithOwners(nil),
// Entries modified due to visibility annotation keep their existing owners (here none)
HttpEgressKey(0): allowEntryD(12346, visibilityDerivedFrom),
HttpEgressKey(0): allowEntryD(12346, visibilityDerivedFrom, nil),
DNSUDPEgressKey(42): allowEntryD(12347, visibilityDerivedFrom, csBar),
DNSTCPEgressKey(42): allowEntry(0, csBar),
},
Expand Down Expand Up @@ -2162,7 +2157,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChangesOnVisibilityKeys(c *
AnyIngressKey(): allowEntry(0),
HostIngressKey(): allowEntry(0),
HttpIngressKey(0): allowEntryD(12345, visibilityDerivedFrom).WithOwners(nil),
HttpEgressKey(0): allowEntryD(12346, visibilityDerivedFrom),
HttpEgressKey(0): allowEntryD(12346, visibilityDerivedFrom, nil),
DNSUDPEgressKey(42): allowEntryD(12347, visibilityDerivedFrom, csBar),
DNSTCPEgressKey(42): allowEntry(0, csBar),
// Redirect entries are not modified by visibility annotations
Expand Down

0 comments on commit cb50a88

Please sign in to comment.