Skip to content

Commit

Permalink
Merge pull request ovn-org#1558 from npinaeva/ocpbugs-ocpbugs-8471
Browse files Browse the repository at this point in the history 
OCPBUGS-8504,OCPBUGS-8505,OCPBUGS-8471: [release-4.13] Fix EFW's name truncation logic & make EFW ACLs unique using extIDs
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Mar 8, 2023
2 parents 17e923a + 7c153b3 commit 5c534b9
Show file tree
Hide file tree
Showing 4 changed files with 205 additions and 30 deletions.
5 changes: 5 additions & 0 deletions go-controller/pkg/libovsdbops/acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ func CreateOrUpdateACLsOps(nbClient libovsdbclient.Client, ops []libovsdb.Operat
for i := range acls {
// can't use i in the predicate, for loop replaces it in-memory
acl := acls[i]
// ensure names are truncated (let's cover our bases from snippets that don't call BuildACL and call this directly)
if acl.Name != nil {
// node ACLs won't have names set
*acl.Name = fmt.Sprintf("%.63s", *acl.Name)
}
opModel := operationModel{
Model: acl,
ModelPredicate: func(item *nbdb.ACL) bool { return isEquivalentACL(item, acl) },
Expand Down
8 changes: 6 additions & 2 deletions go-controller/pkg/ovn/egressfirewall.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ const (
egressFirewallAppliedCorrectly = "EgressFirewall Rules applied"
egressFirewallAddError = "EgressFirewall Rules not correctly added"
// egressFirewallACLExtIdKey external ID key for egress firewall ACLs
egressFirewallACLExtIdKey = "egressFirewall"
egressFirewallACLExtIdKey = "egressFirewall"
egressFirewallACLPriorityKey = "priority"
)

type egressFirewall struct {
Expand Down Expand Up @@ -403,7 +404,10 @@ func (oc *DefaultNetworkController) createEgressFirewallRules(priority int, matc
aclLogging,
// since egressFirewall has direction to-lport, set type to ingress
lportIngress,
map[string]string{egressFirewallACLExtIdKey: externalID},
map[string]string{
egressFirewallACLExtIdKey: externalID,
egressFirewallACLPriorityKey: fmt.Sprintf("%d", priority),
},
)
ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, egressFirewallACL)
if err != nil {
Expand Down
177 changes: 161 additions & 16 deletions go-controller/pkg/ovn/egressfirewall_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,19 +202,33 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
// Both ACLs will be removed from the node switch
nodeSwitch.ACLs = nil

// keepACL will be re-created with the new external ID
asHash, _ := getNsAddrSetHashNames(namespace1.Name)
newKeepACL := libovsdbops.BuildACL(
buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority),
nbdb.ACLDirectionToLport,
t.EgressFirewallStartPriority,
"(ip4.dst == 1.2.3.4/23) && ip4.src == $"+asHash,
nbdb.ACLActionAllow,
t.OvnACLLoggingMeter,
"",
false,
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
newKeepACL.UUID = "newKeepACL-UUID"
// keepACL will be added to the clusterPortGroup
clusterPortGroup.ACLs = []string{keepACL.UUID}
clusterPortGroup.ACLs = []string{newKeepACL.UUID}

// Direction of both ACLs will be converted to
keepACL.Direction = nbdb.ACLDirectionToLport
newName := buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority)
keepACL.Name = &newName
// check severity was reset from default to nil
keepACL.Severity = nil
// subnet exclusion will be deleted
asHash, _ := getNsAddrSetHashNames(namespace1.Name)
keepACL.Match = "(ip4.dst == 1.2.3.4/23) && ip4.src == $" + asHash

// purgeACL ACL will be deleted when test server starts deleting dereferenced ACLs
// for now we need to update its fields, since it is present in the db
purgeACL.Direction = nbdb.ACLDirectionToLport
Expand All @@ -225,6 +239,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
expectedDatabaseState := []libovsdb.TestData{
otherACL,
purgeACL,
newKeepACL,
keepACL,
nodeSwitch,
joinSwitch,
Expand Down Expand Up @@ -290,7 +305,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -358,7 +376,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv6ACL.UUID = "ipv6ACL-UUID"
Expand Down Expand Up @@ -435,7 +456,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
udpACL.UUID = "udpACL-UUID"
Expand Down Expand Up @@ -510,7 +534,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -593,7 +620,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -819,7 +849,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{"egressFirewall": "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -898,7 +931,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -1006,7 +1042,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -1109,7 +1148,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -1197,7 +1239,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{"egressFirewall": namespace1.Name},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL.UUID = "ipv4ACL-UUID"
Expand Down Expand Up @@ -1267,7 +1312,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
t.OvnACLLoggingMeter,
"",
false,
map[string]string{egressFirewallACLExtIdKey: "namespace1"},
map[string]string{
egressFirewallACLExtIdKey: "namespace1",
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
acl.UUID = "acl-UUID"
Expand All @@ -1282,6 +1330,103 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
err := app.Run([]string{app.Name})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
})
ginkgo.It(fmt.Sprintf("correctly creates an egressfirewall for namespace name > 43 symbols, gateway mode %s", gwMode), func() {
app.Action = func(ctx *cli.Context) error {
// 52 characters namespace
namespace1 := *newNamespace("abcdefghigklmnopqrstuvwxyzabcdefghigklmnopqrstuvwxyz")
egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{
{
Type: "Allow",
To: egressfirewallapi.EgressFirewallDestination{
CIDRSelector: "1.2.3.5/23",
},
},
{
Type: "Allow",
To: egressfirewallapi.EgressFirewallDestination{
CIDRSelector: "2.2.3.5/23",
},
},
})

fakeOVN.startWithDBSetup(dbSetup,
&egressfirewallapi.EgressFirewallList{
Items: []egressfirewallapi.EgressFirewall{
*egressFirewall,
},
},
&v1.NamespaceList{
Items: []v1.Namespace{
namespace1,
},
})

err := fakeOVN.controller.WatchNamespaces()
gomega.Expect(err).NotTo(gomega.HaveOccurred())
err = fakeOVN.controller.WatchEgressFirewall()
gomega.Expect(err).NotTo(gomega.HaveOccurred())

asHash, _ := getNsAddrSetHashNames(namespace1.Name)
ipv4ACL1 := libovsdbops.BuildACL(
buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority),
nbdb.ACLDirectionToLport,
t.EgressFirewallStartPriority,
"(ip4.dst == 1.2.3.5/23) && ip4.src == $"+asHash,
nbdb.ACLActionAllow,
t.OvnACLLoggingMeter,
"",
false,
map[string]string{
egressFirewallACLExtIdKey: namespace1.Name,
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
},
nil,
)
ipv4ACL1.UUID = "ipv4ACL1-UUID"
ipv4ACL2 := libovsdbops.BuildACL(
buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority-1),
nbdb.ACLDirectionToLport,
t.EgressFirewallStartPriority-1,
"(ip4.dst == 2.2.3.5/23) && ip4.src == $"+asHash,
nbdb.ACLActionAllow,
t.OvnACLLoggingMeter,
"",
false,
map[string]string{
egressFirewallACLExtIdKey: namespace1.Name,
egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority-1),
},
nil,
)
ipv4ACL2.UUID = "ipv4ACL2-UUID"

// new ACL will be added to the port group
clusterPortGroup.ACLs = []string{ipv4ACL1.UUID, ipv4ACL2.UUID}
expectedDatabaseState := append(initialData, ipv4ACL1, ipv4ACL2)

gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))

// NOTE: syncEgressFirewall is not calling libovsdbops.BuildACL and directly calls CreateOrUpdateACLs
// This part of test ensures syncEgressFirewall code path is tested well and that we truncate the ACL names correctly
err = fakeOVN.controller.syncEgressFirewall([]interface{}{*egressFirewall})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))

err = fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Delete(context.TODO(), egressFirewall.Name, *metav1.NewDeleteOptions(0))
gomega.Expect(err).NotTo(gomega.HaveOccurred())

// ACL should be removed from the port group egfw is deleted
clusterPortGroup.ACLs = []string{}
// this ACL will be deleted when test server starts deleting dereferenced ACLs
expectedDatabaseState = append(initialData, ipv4ACL1, ipv4ACL2)
gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))

return nil
}

err := app.Run([]string{app.Name})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
})
}
})
})
Expand Down
45 changes: 33 additions & 12 deletions go-controller/pkg/ovn/master.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"k8s.io/klog/v2"
utilnet "k8s.io/utils/net"

libovsdbclient "github.com/ovn-org/libovsdb/client"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/kube"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdbops"
Expand Down Expand Up @@ -118,23 +119,43 @@ func (oc *DefaultNetworkController) SetupMaster(existingNodeNames []string) erro
}
oc.defaultCOPPUUID = *(logicalRouter.Copp)

// Create a cluster-wide port group that all logical switch ports are part of
pg := libovsdbops.BuildPortGroup(types.ClusterPortGroupName, types.ClusterPortGroupName, nil, nil)
err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
pg := &nbdb.PortGroup{
Name: types.ClusterPortGroupName,
}
pg, err = libovsdbops.GetPortGroup(oc.nbClient, pg)
if err != nil && err != libovsdbclient.ErrNotFound {
return err
}
if pg == nil {
// we didn't find an existing clusterPG, let's create a new empty PG (fresh cluster install)
// Create a cluster-wide port group that all logical switch ports are part of
pg := libovsdbops.BuildPortGroup(types.ClusterPortGroupName, types.ClusterPortGroupName, nil, nil)
err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
return err
}
}

// Create a cluster-wide port group with all node-to-cluster router
// logical switch ports. Currently the only user is multicast but it might
// be used for other features in the future.
pg = libovsdbops.BuildPortGroup(types.ClusterRtrPortGroupName, types.ClusterRtrPortGroupName, nil, nil)
err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
pg = &nbdb.PortGroup{
Name: types.ClusterRtrPortGroupName,
}
pg, err = libovsdbops.GetPortGroup(oc.nbClient, pg)
if err != nil && err != libovsdbclient.ErrNotFound {
return err
}
if pg == nil {
// we didn't find an existing clusterRtrPG, let's create a new empty PG (fresh cluster install)
// Create a cluster-wide port group with all node-to-cluster router
// logical switch ports. Currently the only user is multicast but it might
// be used for other features in the future.
pg = libovsdbops.BuildPortGroup(types.ClusterRtrPortGroupName, types.ClusterRtrPortGroupName, nil, nil)
err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
return err
}
}

// If supported, enable IGMP relay on the router to forward multicast
// traffic between nodes.
Expand Down

0 comments on commit 5c534b9

Please sign in to comment.