Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Network policy: use shared pod selector address sets #3329

Merged
merged 5 commits into from
Mar 17, 2023
Merged

Network policy: use shared pod selector address sets #3329

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f6f3ed8
Unlock shared default deny port group early, when no db changes
npinaeva Dec 9, 2022
120140f
Add PodSelectorAddressSet object, that should be used to manage
npinaeva Dec 21, 2022
ed34024
Rework gressPolicy ACLs build: previously all gress policies with at
npinaeva Dec 22, 2022
7f4b409
Add PodSelectorAddressSet tests.
npinaeva Mar 2, 2023
54b67a3
don't create acl if no namespace address sets are selected.
npinaeva Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions go-controller/pkg/libovsdbops/acl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,33 @@ func FindACLsWithPredicate(nbClient libovsdbclient.Client, p aclPredicate) ([]*n
return acls, err
}

func FindACLs(nbClient libovsdbclient.Client, acls []*nbdb.ACL) ([]*nbdb.ACL, error) {
opModels := make([]operationModel, 0, len(acls))
foundACLs := make([]*nbdb.ACL, 0, len(acls))
for i := range acls {
// can't use i in the predicate, for loop replaces it in-memory
acl := acls[i]
found := []*nbdb.ACL{}
opModel := operationModel{
Model: acl,
ModelPredicate: func(item *nbdb.ACL) bool { return isEquivalentACL(item, acl) },
ExistingResult: &found,
ErrNotFound: false,
BulkOp: false,
DoAfter: func() {
if len(found) > 0 {
foundACLs = append(foundACLs, found[0])
}
},
}
opModels = append(opModels, opModel)
}

modelClient := newModelClient(nbClient)
err := modelClient.Lookup(opModels...)
return foundACLs, err
}

// BuildACL builds an ACL with empty optional properties unset
func BuildACL(name string, direction nbdb.ACLDirection, priority int, match string, action nbdb.ACLAction, meter string,
severity nbdb.ACLSeverity, log bool, externalIds map[string]string, options map[string]string) *nbdb.ACL {
Expand Down
26 changes: 18 additions & 8 deletions go-controller/pkg/ovn/gress_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ func (gp *gressPolicy) addIPBlock(ipblockJSON *knet.IPBlock) {
gp.ipBlock = append(gp.ipBlock, ipblockJSON)
}

// getL3MatchFromAddressSet may return empty string, which means that there are no address sets selected for giver
// gressPolicy at the time, and acl should not be created.
func (gp *gressPolicy) getL3MatchFromAddressSet() string {
v4AddressSets := syncMapToSortedList(gp.peerV4AddressSets)
v6AddressSets := syncMapToSortedList(gp.peerV6AddressSets)
Expand All @@ -141,7 +143,7 @@ func (gp *gressPolicy) getL3MatchFromAddressSet() string {
// At this point there will be address sets in one or both of them.
// Contents in both address sets mean dual stack, else one will be empty because we will only populate
// entries for enabled stacks
if config.IPv4Mode {
if config.IPv4Mode && len(v4AddressSets) > 0 {
v4AddressSetStr := strings.Join(v4AddressSets, ", ")
v4Match = fmt.Sprintf("%s.%s == {%s}", "ip4", direction, v4AddressSetStr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Previously l3Match for gress with namespace selector that doesn't select
anything looked like "ip4.src == {}", now it will be
"ip4.src == {}"."

Do we really want to program ACLs that wont match on anything? In the previous behavior it made sense because address sets are populated without touching ACL again. But ACL will have to be updated to have the right address set once the selector finally matches something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the previous behaviour for namespace selector was

  1. create gress address set that will always be empty, use it in match
  2. add namespace address sets to that list on update

We can delete this empty-match acl, but that will complicate the code, since we need to delete the acl on update when no namespaces are matched, and we will need to track which acls got deleted. Now we just update all existing acls, and on namespace delete acl will be updated to empty match

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the behavior here of ovn-controller when the match is an empty brace? Does it throw any error or warning, does it assume it does not match anything? @dceara ?

Copy link
Contributor

@dceara dceara Mar 15, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ovn-controller complains, e.g.:

2023-03-15T11:06:40.219Z|00030|lflow|WARN|error parsing match "ip4.src == {}": Syntax error at `}' expecting constant.

So the flow will not be installed (and will obviously never match).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i will use the update instead of mutate for port group acl updates then

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of extra clarification: the "ip4.src == {}" match can be replaced by "0" with the exact same effect - nothing will match.

if gp.policyType == knet.PolicyTypeIngress {
Expand All @@ -150,7 +152,7 @@ func (gp *gressPolicy) getL3MatchFromAddressSet() string {
}
match = v4Match
}
if config.IPv6Mode {
if config.IPv6Mode && len(v6AddressSets) > 0 {
v6AddressSetStr := strings.Join(v6AddressSets, ", ")
v6Match = fmt.Sprintf("%s.%s == {%s}", "ip6", direction, v6AddressSetStr)
if gp.policyType == knet.PolicyTypeIngress {
Expand All @@ -159,7 +161,8 @@ func (gp *gressPolicy) getL3MatchFromAddressSet() string {
}
match = v6Match
}
if config.IPv4Mode && config.IPv6Mode {
// if at least one match is empty in dualstack mode, the non-empty one will be assigned to match
if config.IPv4Mode && config.IPv6Mode && v4Match != "" && v6Match != "" {
match = fmt.Sprintf("(%s || %s)", v4Match, v6Match)
}
return match
Expand Down Expand Up @@ -251,7 +254,8 @@ func (gp *gressPolicy) isEmpty() bool {
// by the parent NetworkPolicy)
// buildLocalPodACLs is safe for concurrent use, since it only uses gressPolicy fields that don't change
// since creation, or are safe for concurrent use like peerVXAddressSets
func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLoggingLevels) []*nbdb.ACL {
func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLoggingLevels) (createdACLs []*nbdb.ACL,
skippedACLs []*nbdb.ACL) {
var lportMatch, match, l3Match string
if gp.policyType == knet.PolicyTypeIngress {
lportMatch = fmt.Sprintf("outport == @%s", portGroupName)
Expand All @@ -272,14 +276,13 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLo
l4Matches = append(l4Matches, l4Match)
}
}
acls := []*nbdb.ACL{}
for _, l4Match := range l4Matches {
if len(gp.ipBlock) > 0 {
// Add ACL allow rule for IPBlock CIDR
cidrMatches := gp.getMatchFromIPBlock(lportMatch, l4Match)
for i, cidrMatch := range cidrMatches {
acl := gp.buildACLAllow(cidrMatch, l4Match, i+1, aclLogging)
acls = append(acls, acl)
createdACLs = append(createdACLs, acl)
}
}
// if there are pod/namespace selector, then allow packets from/to that address_set or
Expand All @@ -297,10 +300,17 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLo
match = fmt.Sprintf("%s && %s && %s", l3Match, l4Match, lportMatch)
}
acl := gp.buildACLAllow(match, l4Match, 0, aclLogging)
acls = append(acls, acl)
if l3Match == "" {
// if l3Match is empty, then no address sets are selected for a given gressPolicy.
// fortunately l3 match is not a part of externalIDs, that means that we can find
// the deleted acl without knowing the previous match.
skippedACLs = append(skippedACLs, acl)
} else {
createdACLs = append(createdACLs, acl)
}
}
}
return acls
return
}

func getGressPolicyACLName(ns, policyName string, idx int) string {
Expand Down
17 changes: 14 additions & 3 deletions go-controller/pkg/ovn/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,11 +1327,11 @@ func (oc *DefaultNetworkController) addNetworkPolicy(policy *knet.NetworkPolicy)
func (oc *DefaultNetworkController) buildNetworkPolicyACLs(np *networkPolicy, aclLogging *ACLLoggingLevels) []*nbdb.ACL {
acls := []*nbdb.ACL{}
for _, gp := range np.ingressPolicies {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: in the future we should make network policy actually have an update instead of delete+add

acl := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acl, _ := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acls = append(acls, acl...)
}
for _, gp := range np.egressPolicies {
acl := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acl, _ := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acls = append(acls, acl...)
}

Expand Down Expand Up @@ -1525,7 +1525,7 @@ func (oc *DefaultNetworkController) peerNamespaceUpdate(np *networkPolicy, gp *g
return nil
}
// buildLocalPodACLs is safe for concurrent use, see function comment for details
acls := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acls, deletedACLs := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, acls...)
if err != nil {
return err
Expand All @@ -1534,6 +1534,17 @@ func (oc *DefaultNetworkController) peerNamespaceUpdate(np *networkPolicy, gp *g
if err != nil {
return err
}
if len(deletedACLs) > 0 {
deletedACLsWithUUID, err := libovsdbops.FindACLs(oc.nbClient, deletedACLs)
if err != nil {
return fmt.Errorf("failed to find deleted acls: %w", err)
}

ops, err = libovsdbops.DeleteACLsFromPortGroupOps(oc.nbClient, ops, np.portGroupName, deletedACLsWithUUID...)
if err != nil {
return err
}
}
_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
return err
}
Expand Down
44 changes: 23 additions & 21 deletions go-controller/pkg/ovn/policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,22 +220,18 @@ func getGressACLs(i int, namespace, policyName string, peerNamespaces []string,
hashedASName, _ := getNsAddrSetHashNames(nsName)
hashedASNames = append(hashedASNames, hashedASName)
}
hasNsSelector := false
ipBlock := ""
for _, peer := range peers {
if peer.PodSelector != nil && len(peerNamespaces) == 0 {
peerIndex := getPodSelectorAddrSetDbIDs(getPodSelectorKey(peer.PodSelector, peer.NamespaceSelector, namespace), DefaultNetworkControllerName)
asv4, _ := addressset.GetHashNamesForAS(peerIndex)
hashedASNames = append(hashedASNames, asv4)
}
if peer.NamespaceSelector != nil {
hasNsSelector = true
}
if peer.IPBlock != nil {
ipBlock = peer.IPBlock.CIDR
}
}
if len(hashedASNames) > 0 || hasNsSelector {
if len(hashedASNames) > 0 {
gressAsMatch := asMatch(hashedASNames)
match := fmt.Sprintf("ip4.%s == {%s}", ipDir, gressAsMatch)
if policyType == knet.PolicyTypeIngress {
Expand Down Expand Up @@ -1592,8 +1588,12 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
Delete(context.TODO(), namespace2.Name, *metav1.NewDeleteOptions(0))
gomega.Expect(err).NotTo(gomega.HaveOccurred())
expectedData = []libovsdb.TestData{}
gressPolicyExpectedData = getPolicyData(networkPolicy, []string{nPodTest.portUUID}, []string{},
nil, "", false)
// acls will be deleted, since no namespaces are selected at this point.
// since test server doesn't garbage-collect de-referenced acls, they will stay in the db
// gressPolicyExpectedData[2] is the policy port group
gressPolicyExpectedData[2].(*nbdb.PortGroup).ACLs = nil
//gressPolicyExpectedData = getPolicyData(networkPolicy, []string{nPodTest.portUUID}, []string{},
// nil, "", false)
expectedData = append(expectedData, gressPolicyExpectedData...)
expectedData = append(expectedData, defaultDenyExpectedData...)
expectedData = append(expectedData, getExpectedDataPodsAndSwitches([]testPod{nPodTest}, []string{nodeName})...)
Expand Down Expand Up @@ -1632,8 +1632,10 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
gomega.Expect(err).NotTo(gomega.HaveOccurred())

expectedData = []libovsdb.TestData{}
gressPolicyExpectedData = getPolicyData(networkPolicy, nil, []string{},
nil, "", false)
// acls will be deleted, since no namespaces are selected at this point.
// since test server doesn't garbage-collect de-referenced acls, they will stay in the db
// gressPolicyExpectedData[2] is the policy port group
gressPolicyExpectedData[2].(*nbdb.PortGroup).ACLs = nil
expectedData = append(expectedData, gressPolicyExpectedData...)
expectedData = append(expectedData, defaultDenyExpectedData...)
expectedData = append(expectedData, initialDB.NBData...)
Expand Down Expand Up @@ -2211,20 +2213,20 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(one.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected := buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one}, &defaultAclLogging)
actual := gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ := gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.addNamespaceAddressSet(two.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one, two}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// address sets should be alphabetized
gomega.Expect(gp.addNamespaceAddressSet(three.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one, two, three}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// re-adding an existing set is a no-op
Expand All @@ -2233,14 +2235,14 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(four.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one, two, three, four}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// now delete a set
gomega.Expect(gp.delNamespaceAddressSet(one.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, three, four}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// deleting again is a no-op
Expand All @@ -2250,13 +2252,13 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(five.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, three, four, five}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(three.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, four, five}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// deleting again is no-op
Expand All @@ -2265,31 +2267,31 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(six.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, four, five, six}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(two.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, four, five, six}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(five.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, four, six}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(six.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, four}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(four.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// deleting again is no-op
Expand Down