Skip to content

[release-4.19] OCPBUGS-77150: Minimize ACLs by combining ipBlocks into single ACL #2996

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

Closed
pperiyasamy wants to merge 1 commit into from
Closed

[release-4.19] OCPBUGS-77150: Minimize ACLs by combining ipBlocks into single ACL #2996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions go-controller/pkg/libovsdb/ops/db_object_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,10 @@ var ACLNetworkPolicyPortIndex = newObjectIDsType(acl, NetworkPolicyPortIndexOwne
// ingress/egress + NetworkPolicy[In/E]gressRule idx - defines given gressPolicy.
// ACLs are created for gp.portPolicies which are grouped by protocol:
// - for empty policy (no selectors and no ip blocks) - empty ACL (see allIPsMatch)
// with idx=emptyIdx (-1)
// OR
// - all selector-based peers ACL
// - for every IPBlock +1 ACL
// - all selector-based peers ACL with idx=emptyIdx (-1)
// - all ipBlocks combined into a single ACL with idx=ipBlockCombinedIdx (-2)
// Therefore unique id for a given gressPolicy is protocol name + IPBlock idx
// (protocol will be "None" if no port policy is defined, and empty policy and all
// selector-based peers ACLs will have idx=-1)
Expand Down
48 changes: 28 additions & 20 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 @@ -22,6 +22,11 @@ import (
const (
// emptyIdx is used to create ACL for gressPolicy that doesn't have ipBlocks
emptyIdx = -1
// ipBlockCombinedIdx is used when creating an ACL for a gressPolicy
// that contains ipBlocks. Previously, one ACL was created per ipBlock.
// This is changed to create a single combined ACL for all ipBlocks,
// and this special index value identifies those new ACLs.
ipBlockCombinedIdx = -2
)

type gressPolicy struct {
Expand Down Expand Up @@ -167,14 +172,14 @@ func (gp *gressPolicy) allIPsMatch() string {
}
}

func (gp *gressPolicy) getMatchFromIPBlock(lportMatch, l4Match string) []string {
func (gp *gressPolicy) getMatchFromIPBlock(lportMatch, l4Match string) string {
var direction string
if gp.policyType == knet.PolicyTypeIngress {
direction = "src"
} else {
direction = "dst"
}
var matchStrings []string
var ipBlockMatches []string
var matchStr, ipVersion string
for _, ipBlock := range gp.ipBlocks {
if utilnet.IsIPv6CIDRString(ipBlock.CIDR) {
Expand All @@ -185,17 +190,22 @@ func (gp *gressPolicy) getMatchFromIPBlock(lportMatch, l4Match string) []string
if len(ipBlock.Except) == 0 {
matchStr = fmt.Sprintf("%s.%s == %s", ipVersion, direction, ipBlock.CIDR)
} else {
matchStr = fmt.Sprintf("%s.%s == %s && %s.%s != {%s}", ipVersion, direction, ipBlock.CIDR,
matchStr = fmt.Sprintf("(%s.%s == %s && %s.%s != {%s})", ipVersion, direction, ipBlock.CIDR,
ipVersion, direction, strings.Join(ipBlock.Except, ", "))
}
if l4Match == libovsdbutil.UnspecifiedL4Match {
matchStr = fmt.Sprintf("%s && %s", matchStr, lportMatch)
} else {
matchStr = fmt.Sprintf("%s && %s && %s", matchStr, l4Match, lportMatch)
}
matchStrings = append(matchStrings, matchStr)
ipBlockMatches = append(ipBlockMatches, matchStr)
}
return matchStrings
var l3Match string
if len(ipBlockMatches) == 1 {
l3Match = ipBlockMatches[0]
} else {
l3Match = fmt.Sprintf("(%s)", strings.Join(ipBlockMatches, " || "))
}

if l4Match == libovsdbutil.UnspecifiedL4Match {
return fmt.Sprintf("%s && %s", l3Match, lportMatch)
}
return fmt.Sprintf("%s && %s && %s", l3Match, l4Match, lportMatch)
}

// addNamespaceAddressSet adds a namespace address set to the gress policy.
Expand Down Expand Up @@ -285,13 +295,11 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *libov
for protocol, l4Match := range libovsdbutil.GetL4MatchesFromNetworkPolicyPorts(gp.portPolicies) {
if len(gp.ipBlocks) > 0 {
// Add ACL allow rule for IPBlock CIDR
ipBlockMatches := gp.getMatchFromIPBlock(lportMatch, l4Match)
for ipBlockIdx, ipBlockMatch := range ipBlockMatches {
aclIDs := gp.getNetpolACLDbIDs(ipBlockIdx, protocol)
acl := libovsdbutil.BuildACLWithDefaultTier(aclIDs, types.DefaultAllowPriority, ipBlockMatch, action,
aclLogging, gp.aclPipeline)
createdACLs = append(createdACLs, acl)
}
ipBlockMatch := gp.getMatchFromIPBlock(lportMatch, l4Match)
aclIDs := gp.getNetpolACLDbIDs(ipBlockCombinedIdx, protocol)
acl := libovsdbutil.BuildACLWithDefaultTier(aclIDs, types.DefaultAllowPriority, ipBlockMatch, action,
aclLogging, gp.aclPipeline)
createdACLs = append(createdACLs, acl)
}
// if there are pod/namespace selector, then allow packets from/to that address_set or
// if the NetworkPolicyPeer is empty, then allow from all sources or to all destinations.
Expand Down Expand Up @@ -334,10 +342,10 @@ func (gp *gressPolicy) getNetpolACLDbIDs(ipBlockIdx int, protocol string) *libov
// gress rule index
libovsdbops.GressIdxKey: strconv.Itoa(gp.idx),
// acls are created for every gp.portPolicies which are grouped by protocol:
// - for empty policy (no selectors and no ip blocks) - empty ACL
// - for empty policy (no selectors and no ip blocks) - empty ACL with idx=emptyIdx (-1)
// OR
// - all selector-based peers ACL
// - for every IPBlock +1 ACL
// - all selector-based peers ACL with idx=emptyIdx (-1)
// - all ipBlocks combined into a single ACL with idx=ipBlockCombinedIdx (-2)
// Therefore unique id for a given gressPolicy is protocol name + IPBlock idx
// (protocol will be "None" if no port policy is defined, and empty policy and all
// selector-based peers ACLs will have idx=-1)
Expand Down
19 changes: 8 additions & 11 deletions go-controller/pkg/ovn/gress_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
ipBlocks []*knet.IPBlock
lportMatch string
l4Match string
expected []string
expected string
}{
{
desc: "IPv4 only no except",
Expand All @@ -27,7 +27,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
},
lportMatch: "fake",
l4Match: "input",
expected: []string{"ip4.src == 0.0.0.0/0 && input && fake"},
expected: "ip4.src == 0.0.0.0/0 && input && fake",
},
{
desc: "multiple IPv4 only no except",
Expand All @@ -41,8 +41,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
},
lportMatch: "fake",
l4Match: "input",
expected: []string{"ip4.src == 0.0.0.0/0 && input && fake",
"ip4.src == 10.1.0.0/16 && input && fake"},
expected: "(ip4.src == 0.0.0.0/0 || ip4.src == 10.1.0.0/16) && input && fake",
},
{
desc: "IPv6 only no except",
Expand All @@ -53,7 +52,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
},
lportMatch: "fake",
l4Match: "input",
expected: []string{"ip6.src == fd00:10:244:3::49/32 && input && fake"},
expected: "ip6.src == fd00:10:244:3::49/32 && input && fake",
},
{
desc: "mixed IPv4 and IPv6 no except",
Expand All @@ -67,8 +66,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
},
lportMatch: "fake",
l4Match: "input",
expected: []string{"ip6.src == ::/0 && input && fake",
"ip4.src == 0.0.0.0/0 && input && fake"},
expected: "(ip6.src == ::/0 || ip4.src == 0.0.0.0/0) && input && fake",
},
{
desc: "IPv4 only with except",
Expand All @@ -80,7 +78,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
},
lportMatch: "fake",
l4Match: "input",
expected: []string{"ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16} && input && fake"},
expected: "(ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16}) && input && fake",
},
{
desc: "multiple IPv4 with except",
Expand All @@ -95,8 +93,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
},
lportMatch: "fake",
l4Match: "input",
expected: []string{"ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16} && input && fake",
"ip4.src == 10.1.0.0/16 && input && fake"},
expected: "((ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16}) || ip4.src == 10.1.0.0/16) && input && fake",
},
{
desc: "IPv4 with IPv4 except",
Expand All @@ -108,7 +105,7 @@ func TestGetMatchFromIPBlock(t *testing.T) {
},
lportMatch: "fake",
l4Match: "input",
expected: []string{"ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16} && input && fake"},
expected: "(ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16}) && input && fake",
},
}

Expand Down
Loading