Skip to content

Commit

Permalink
Merge pull request #1725 from npinaeva/ocpbugs-15424
Browse files Browse the repository at this point in the history 
OCPBUGS-15424: [release-4.12] Fix network policy to work with long namespace names
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Jun 28, 2023
2 parents 17f9ed5 + 741a619 commit 3aa743c
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 30 deletions.
12 changes: 6 additions & 6 deletions go-controller/pkg/ovn/multicast.go
View file
Expand Up @@ -94,13 +94,13 @@ func (oc *Controller) createMulticastAllowPolicy(ns string, nsInfo *namespaceInf
egressMatch := getACLMatch(portGroupName, getMulticastACLEgrMatch(), aclT)
egressACL := BuildACL(getMcastACLName(ns, "MulticastAllowEgress"),
types.DefaultMcastAllowPriority, egressMatch, nbdb.ACLActionAllow, nil, aclT,
getDefaultDenyPolicyExternalIDs(aclT))
getDirectionPolicyACLExternalIDs(aclT))

aclT = lportIngress
ingressMatch := getACLMatch(portGroupName, getMulticastACLIgrMatch(nsInfo), aclT)
ingressACL := BuildACL(getMcastACLName(ns, "MulticastAllowIngress"),
types.DefaultMcastAllowPriority, ingressMatch, nbdb.ACLActionAllow, nil, aclT,
getDefaultDenyPolicyExternalIDs(aclT))
getDirectionPolicyACLExternalIDs(aclT))

acls := []*nbdb.ACL{egressACL, ingressACL}
ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, acls...)
Expand Down Expand Up @@ -166,13 +166,13 @@ func (oc *Controller) createDefaultDenyMulticastPolicy() error {
aclT := lportEgressAfterLB
egressACL := BuildACL(getMcastACLName(types.ClusterPortGroupName, "DefaultDenyMulticastEgress"),
types.DefaultMcastDenyPriority, match, nbdb.ACLActionDrop, nil,
aclT, getDefaultDenyPolicyExternalIDs(aclT))
aclT, getDirectionPolicyACLExternalIDs(aclT))

// By default deny any ingress multicast traffic to any pod.
aclT = lportIngress
ingressACL := BuildACL(getMcastACLName(types.ClusterPortGroupName, "DefaultDenyMulticastIngress"),
types.DefaultMcastDenyPriority, match, nbdb.ACLActionDrop, nil,
aclT, getDefaultDenyPolicyExternalIDs(aclT))
aclT, getDirectionPolicyACLExternalIDs(aclT))

ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, egressACL, ingressACL)
if err != nil {
Expand Down Expand Up @@ -210,13 +210,13 @@ func (oc *Controller) createDefaultAllowMulticastPolicy() error {
egressMatch := getACLMatch(types.ClusterRtrPortGroupName, mcastMatch, aclT)
egressACL := BuildACL(getMcastACLName(types.ClusterRtrPortGroupName, "DefaultAllowMulticastEgress"),
types.DefaultMcastAllowPriority, egressMatch, nbdb.ACLActionAllow, nil,
aclT, getDefaultDenyPolicyExternalIDs(aclT))
aclT, getDirectionPolicyACLExternalIDs(aclT))

aclT = lportIngress
ingressMatch := getACLMatch(types.ClusterRtrPortGroupName, mcastMatch, aclT)
ingressACL := BuildACL(getMcastACLName(types.ClusterRtrPortGroupName, "DefaultAllowMulticastIngress"),
types.DefaultMcastAllowPriority, ingressMatch, nbdb.ACLActionAllow, nil,
aclT, getDefaultDenyPolicyExternalIDs(aclT))
aclT, getDirectionPolicyACLExternalIDs(aclT))

ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, egressACL, ingressACL)
if err != nil {
Expand Down
52 changes: 46 additions & 6 deletions go-controller/pkg/ovn/policy.go
View file
Expand Up @@ -28,9 +28,12 @@ import (
utilnet "k8s.io/utils/net"
)

type netpolDefaultDenyACLAction string

const (
// defaultDenyPolicyTypeACLExtIdKey external ID key for default deny policy type
defaultDenyPolicyTypeACLExtIdKey = "default-deny-policy-type"
defaultDenyPolicyTypeACLExtIdKey = "default-deny-policy-type"
defaultDenyPolicyActionACLExtIdKey = "default-deny-policy-action"
// l4MatchACLExtIdKey external ID key for L4 Match on 'gress policy ACLs
l4MatchACLExtIdKey = "l4Match"
// ipBlockCIDRACLExtIdKey external ID key for IP block CIDR on 'gress policy ACLs
Expand All @@ -54,7 +57,9 @@ const (
// staleArpAllowPolicyMatch "was" the old match used when creating default allow ARP ACLs for a namespace
// NOTE: This is succeed by arpAllowPolicyMatch to allow support for IPV6. This is currently only
// used when removing stale ACLs from the syncNetworkPolicy function and should NOT be used in any main logic.
staleArpAllowPolicyMatch = "arp"
staleArpAllowPolicyMatch = "arp"
denyAction netpolDefaultDenyACLAction = "deny"
allowAction netpolDefaultDenyACLAction = "allow"
)

// defaultDenyPortGroups is a shared object and should be used by only 1 thread at a time
Expand Down Expand Up @@ -288,6 +293,28 @@ func (oc *Controller) updateStaleDefaultDenyACLNames(npType knet.PolicyType, gre
return nil
}

func (oc *Controller) updateStaleDefaultDenyACLExternalIDs() error {
p := func(item *nbdb.ACL) bool {
return item.ExternalIDs[defaultDenyPolicyTypeACLExtIdKey] != "" && // default deny + multicast acls
(item.Priority == types.DefaultDenyPriority || item.Priority == types.DefaultAllowPriority) && // default deny acls only
item.ExternalIDs[defaultDenyPolicyActionACLExtIdKey] == "" // non-updated default deny acls only
}
staleACLs, err := libovsdbops.FindACLsWithPredicate(oc.nbClient, p)
if err != nil {
return fmt.Errorf("cannot find NetworkPolicy default deny ACLs: %v", err)
}
for _, acl := range staleACLs {
var aclAction netpolDefaultDenyACLAction
if acl.Priority == types.DefaultDenyPriority {
aclAction = denyAction
} else {
aclAction = allowAction
}
acl.ExternalIDs[defaultDenyPolicyActionACLExtIdKey] = string(aclAction)
}
return libovsdbops.CreateOrUpdateACLs(oc.nbClient, staleACLs...)
}

func (oc *Controller) syncNetworkPolicies(networkPolicies []interface{}) error {
expectedPolicies := make(map[string]map[string]bool)
for _, npInterface := range networkPolicies {
Expand Down Expand Up @@ -420,6 +447,10 @@ func (oc *Controller) syncNetworkPolicies(networkPolicies []interface{}) error {
return fmt.Errorf("cannot clean up ingress default deny ACL name: %v", err)
}

if err := oc.updateStaleDefaultDenyACLExternalIDs(); err != nil {
return fmt.Errorf("failed to update default deny ACL ExternalIDs: %w", err)
}

return nil
}

Expand Down Expand Up @@ -468,8 +499,17 @@ func getDefaultDenyPolicyACLName(ns string, aclT aclType) string {
return joinACLName(ns, defaultDenySuffix)
}

func getDefaultDenyPolicyExternalIDs(aclT aclType) map[string]string {
return map[string]string{defaultDenyPolicyTypeACLExtIdKey: string(aclTypeToPolicyType(aclT))}
func getDenyACLExternalIDs(aclT aclType, aclAction netpolDefaultDenyACLAction) map[string]string {
return map[string]string{
defaultDenyPolicyTypeACLExtIdKey: string(aclTypeToPolicyType(aclT)),
defaultDenyPolicyActionACLExtIdKey: string(aclAction),
}
}

func getDirectionPolicyACLExternalIDs(aclT aclType) map[string]string {
return map[string]string{
defaultDenyPolicyTypeACLExtIdKey: string(aclTypeToPolicyType(aclT)),
}
}

func getARPAllowACLName(ns string) string {
Expand All @@ -484,9 +524,9 @@ func buildDenyACLs(namespace, pg string, aclLogging *ACLLoggingLevels, aclT aclT
denyMatch := getACLMatch(pg, "", aclT)
allowMatch := getACLMatch(pg, arpAllowPolicyMatch, aclT)
denyACL = BuildACL(getDefaultDenyPolicyACLName(namespace, aclT), types.DefaultDenyPriority, denyMatch,
nbdb.ACLActionDrop, aclLogging, aclT, getDefaultDenyPolicyExternalIDs(aclT))
nbdb.ACLActionDrop, aclLogging, aclT, getDenyACLExternalIDs(aclT, denyAction))
allowACL = BuildACL(getARPAllowACLName(namespace), types.DefaultAllowPriority, allowMatch,
nbdb.ACLActionAllow, nil, aclT, getDefaultDenyPolicyExternalIDs(aclT))
nbdb.ACLActionAllow, nil, aclT, getDenyACLExternalIDs(aclT, allowAction))
return
}

Expand Down
79 changes: 61 additions & 18 deletions go-controller/pkg/ovn/policy_test.go
View file
Expand Up @@ -82,9 +82,7 @@ func getDefaultDenyData(networkPolicy *knet.NetworkPolicy, ports []string,
types.OvnACLLoggingMeter,
denyLogSeverity,
shouldBeLogged,
map[string]string{
defaultDenyPolicyTypeACLExtIdKey: string(knet.PolicyTypeEgress),
},
getDenyACLExternalIDs(lportEgressAfterLB, denyAction),
map[string]string{
"apply-after-lb": "true",
},
Expand All @@ -101,9 +99,7 @@ func getDefaultDenyData(networkPolicy *knet.NetworkPolicy, ports []string,
types.OvnACLLoggingMeter,
"",
false,
map[string]string{
defaultDenyPolicyTypeACLExtIdKey: string(knet.PolicyTypeEgress),
},
getDenyACLExternalIDs(lportEgressAfterLB, allowAction),
map[string]string{
"apply-after-lb": "true",
},
Expand All @@ -121,9 +117,7 @@ func getDefaultDenyData(networkPolicy *knet.NetworkPolicy, ports []string,
types.OvnACLLoggingMeter,
denyLogSeverity,
shouldBeLogged,
map[string]string{
defaultDenyPolicyTypeACLExtIdKey: string(knet.PolicyTypeIngress),
},
getDenyACLExternalIDs(lportIngress, denyAction),
nil,
)
ingressDenyACL.UUID = aclName + "-ingressDenyACL-UUID"
Expand All @@ -138,9 +132,7 @@ func getDefaultDenyData(networkPolicy *knet.NetworkPolicy, ports []string,
types.OvnACLLoggingMeter,
"",
false,
map[string]string{
defaultDenyPolicyTypeACLExtIdKey: string(knet.PolicyTypeIngress),
},
getDenyACLExternalIDs(lportIngress, allowAction),
nil,
)
ingressAllowACL.UUID = aclName + "-ingressAllowACL-UUID"
Expand Down Expand Up @@ -193,6 +185,7 @@ func getStaleDefaultACL(acls []*nbdb.ACL) []*nbdb.ACL {
for _, acl := range acls {
acl.Options = nil
acl.Direction = nbdb.ACLDirectionToLport
delete(acl.ExternalIDs, defaultDenyPolicyActionACLExtIdKey)
}
return acls
}
Expand Down Expand Up @@ -724,6 +717,44 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
err := app.Run([]string{app.Name})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
})

ginkgo.It("reconciles an existing networkPolicy with long name updating stale ACLs", func() {
app.Action = func(ctx *cli.Context) error {
namespace1 := *newNamespace("abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijk")
namespace2 := *newNamespace(namespaceName2)
networkPolicy := getMatchLabelsNetworkPolicy(netPolicyName1, namespace1.Name,
namespace2.Name, "", true, true)
// start with stale ACLs
// this function will cut off the namespace name to have no suffix
gressPolicyInitialData := getPolicyData(networkPolicy, nil, []string{namespace2.Name},
nil, "", true)
defaultDenyInitialData := getDefaultDenyData(networkPolicy, nil, "", true)
initialData := initialDB.NBData
initialData = append(initialData, gressPolicyInitialData...)
initialData = append(initialData, defaultDenyInitialData...)
startOvn(libovsdb.TestSetup{NBData: initialData}, []v1.Namespace{namespace1, namespace2},
[]knet.NetworkPolicy{*networkPolicy}, nil, nil)

fakeOvn.asf.ExpectEmptyAddressSet(namespace1.Name)
fakeOvn.asf.ExpectEmptyAddressSet(namespaceName2)

_, err := fakeOvn.fakeClient.KubeClient.NetworkingV1().NetworkPolicies(networkPolicy.Namespace).
Get(context.TODO(), networkPolicy.Name, metav1.GetOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
// make sure stale ACLs were updated
expectedData := getPolicyData(networkPolicy, nil, []string{namespace2.Name}, nil,
"", false)
defaultDenyExpectedData := getDefaultDenyData(networkPolicy, nil, "", false)
expectedData = append(expectedData, defaultDenyExpectedData...)
expectedData = append(expectedData, initialDB.NBData...)
gomega.Eventually(fakeOvn.nbClient).Should(libovsdb.HaveData(expectedData...))

return nil
}
err := app.Run([]string{app.Name})
gomega.Expect(err).NotTo(gomega.HaveOccurred())

})
})

ginkgo.Context("during execution", func() {
Expand Down Expand Up @@ -991,7 +1022,9 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
// ensure the default PGs and ACLs were removed via rollback from add failure
expectedData := []libovsdb.TestData{}
expectedData = append(expectedData, getExpectedDataPodsAndSwitches([]testPod{nPodTest}, []string{nodeName})...)
// note stale leftovers from previous upgrades won't be cleanedup
// note stale leftovers from previous upgrades won't be cleanedup, but will be updated
leftOverACLFromUpgrade1.ExternalIDs = getDenyACLExternalIDs(lportEgressAfterLB, allowAction)
leftOverACLFromUpgrade2.ExternalIDs = getDenyACLExternalIDs(lportEgressAfterLB, allowAction)
expectedData = append(expectedData, leftOverACLFromUpgrade1, leftOverACLFromUpgrade2)
gomega.Eventually(fakeOvn.nbClient).Should(libovsdb.HaveData(expectedData...))

Expand Down Expand Up @@ -1136,6 +1169,9 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
newDefaultDenyIngressACLName := getDefaultDenyPolicyACLName("shortName", lportIngress)
leftOverACL3FromUpgrade.Name = &newDefaultDenyEgressACLName
leftOverACL4FromUpgrade.Name = &newDefaultDenyIngressACLName
leftOverACL3FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportEgressAfterLB, denyAction)
leftOverACL4FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportIngress, denyAction)

expectedData = append(expectedData, leftOverACL3FromUpgrade)
expectedData = append(expectedData, leftOverACL4FromUpgrade)
testOnlyIngressDenyPG.ACLs = nil // Sync Function should remove stale ACL from PGs
Expand All @@ -1152,6 +1188,9 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
leftOverACL2FromUpgrade.Name = &newDefaultDenyLeftoverIngressACLName
leftOverACL1FromUpgrade.Name = &newDefaultDenyLeftoverEgressACLName
leftOverACL1FromUpgrade.Options = egressOptions
leftOverACL1FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportEgressAfterLB, allowAction)
leftOverACL2FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportIngress, allowAction)

expectedData = append(expectedData, leftOverACL2FromUpgrade)
expectedData = append(expectedData, leftOverACL1FromUpgrade)
// end of db hack
Expand Down Expand Up @@ -1343,6 +1382,11 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
leftOverACL4FromUpgrade.Name = utilpointer.StringPtr(longLeftOverNameSpaceName2 + "_egressDefa") // trims it according to RFC1123
leftOverACL5FromUpgrade.Name = utilpointer.StringPtr(longLeftOverNameSpaceName2 + "_ingressDef") // trims it according to RFC1123
leftOverACL6FromUpgrade.Name = utilpointer.StringPtr(longLeftOverNameSpaceName62 + "_") // name stays the same here since its no-op
leftOverACL3FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportIngress, allowAction)
leftOverACL4FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportEgressAfterLB, denyAction)
leftOverACL5FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportIngress, denyAction)
leftOverACL6FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportIngress, denyAction)

expectedData = append(expectedData, leftOverACL3FromUpgrade)
expectedData = append(expectedData, leftOverACL4FromUpgrade)
expectedData = append(expectedData, leftOverACL5FromUpgrade)
Expand All @@ -1361,6 +1405,9 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
leftOverACL2FromUpgrade.Name = &longLeftOverIngressName
leftOverACL1FromUpgrade.Name = &longLeftOverEgressName
leftOverACL1FromUpgrade.Options = egressOptions
leftOverACL1FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportEgressAfterLB, allowAction)
leftOverACL2FromUpgrade.ExternalIDs = getDenyACLExternalIDs(lportIngress, allowAction)

expectedData = append(expectedData, leftOverACL2FromUpgrade)
expectedData = append(expectedData, leftOverACL1FromUpgrade)
// end of db hack
Expand All @@ -1377,11 +1424,7 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
fakeOvn.controller.networkPolicies.Delete(getPolicyKey(networkPolicy1))
fakeOvn.controller.sharedNetpolPortGroups.Delete(networkPolicy1.Namespace) // reset cache so that we simulate the add that happens during upgrades
err = fakeOvn.controller.addNetworkPolicy(networkPolicy1)
// TODO: FIX ME
gomega.Expect(err).To(gomega.HaveOccurred())
gomega.Expect(err.Error()).To(gomega.ContainSubstring("failed to create Network Policy " +
"abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijk/networkpolicy1: " +
"failed to create default deny port groups: unexpectedly found multiple results for provided predicate"))
gomega.Expect(err).ToNot(gomega.HaveOccurred())

return nil
}
Expand Down

0 comments on commit 3aa743c

Please sign in to comment.