multi-network policy support #3382

cathy-zhou · 2023-01-26T18:42:39Z

- What this PR does and why is it needed

- Special notes for reviewers

- How to verify it

- Description for the changelog

coveralls · 2023-01-31T19:34:43Z

Coverage: 53.335% (+0.5%) from 52.869% when pulling 8c10787 on cathy-zhou:multi-network-mr6 into 9e21b3a on ovn-org:master.

maiqueb

Some nits I've found until now (I couldn't finish reviewing the entire PR).

Would it make sense to split the last commit in two ? One adding the "feature", and another adding the ACL logging ? I kept getting distracted by that while reviewing.

maiqueb · 2023-03-15T08:59:33Z

go-controller/pkg/libovsdbops/address_set.go

I don't understand this change.

Does this DeleteAddressSetsWithPredicateOps function have another caller ?

this is used by cleanupPolicyLogicalEntities()

go-controller/pkg/network-controller-manager/network_controller_manager.go

go-controller/pkg/ovn/base_network_controller_acl.go

go-controller/pkg/ovn/base_network_controller_multipolicy.go

go-controller/pkg/ovn/base_network_controller_namespace.go

jcaamano

just reviewed the first few files.
@cathy-zhou is #2740 is pre-requisite to this PR?

go-controller/pkg/libovsdbops/portgroup.go

go-controller/pkg/network-controller-manager/network_controller_manager.go

npinaeva · 2023-03-29T16:53:52Z

Some notes: there is an ACL ExternalIDs change that should be merged before this pr #3334, it will make sure the indexing of the acls is correct between controllers

I think we need to also implement new indices for port groups before this pr, because otherwise syncing port groups for different controllers will be very difficult (and I guess initially we wanted to make sure that all secondary-controller owned objects will have a new set of ids) @jcaamano

I also see multicast is enabled for secondary networks in the same pr, probably it makes sense to state that in the name, and maybe even split it to at least another commit, if not another pr?

A question about controller names: I see all secondary controllers are named netInfo.GetNetworkName() + "-network-controller", is it guaranteed that these names will always be unique?

One concern I have: it is extremely simple in the current version of the code to mix the objects used by the default and secondary network controllers, which may break the functionality for both. A simple example is createMulticastAllowPolicy. If you look at the implementation, it will reference the default network controller port group from
portGroupName := hashedPortGroup(ns) in its acls, and the real port group with another network-scoped name will be created in the end with bnc.buildPortGroup(portGroupName, ns, ports, acls).
I guess there are more examples of that, and maybe if we get the new IDs for all the used objects it will help somewhat, but we need to be extra careful about properly indexing db objects.

jcaamano

We are not synchronizing multi-network policies on NAD events?

Should the peers of a multinet policy be those on the same network or on the same NAD or all? On pod_selector_addres_set.go we are always using the default network for pod peers. For namespace peers we always look in the same network. Something doesn't add up or I am a bit confused.

go-controller/pkg/config/config.go

go-controller/pkg/ovn/base_network_controller.go

go-controller/pkg/ovn/base_network_controller_acl.go

go-controller/pkg/ovn/gress_policy.go

go-controller/pkg/ovn/base_network_controller_policy.go

go-controller/pkg/ovn/network_conntroller_policy_event_handler.go

cathy-zhou · 2023-03-29T17:48:56Z

@npinaeva

Some notes: there is an ACL ExternalIDs change that should be merged before this pr #3334, it will make sure the indexing of the acls is correct between controllers

sure, please let me know when your PR is merged

I think we need to also implement new indices for port groups before this pr, because otherwise syncing port groups for different controllers will be very difficult (and I guess initially we wanted to make sure that all secondary-controller owned objects will have a new set of ids) @jcaamano

why it is needed? we have network external-ids to differenciate them. it is like logical_routers/logical_switches/logical_ports

I also see multicast is enabled for secondary networks in the same pr, probably it makes sense to state that in the name, and maybe even split it to at least another commit, if not another pr?

it is not enabled for secondary networks yet.

A question about controller names: I see all secondary controllers are named netInfo.GetNetworkName() + "-network-controller", is it guaranteed that these names will always be unique?

I would think so. do you have any specific concerns for this?

One concern I have: it is extremely simple in the current version of the code to mix the objects used by the default and secondary network controllers, which may break the functionality for both. A simple example is createMulticastAllowPolicy. If you look at the implementation, it will reference the default network controller port group from
portGroupName := hashedPortGroup(ns) in its acls, and the real port group with another network-scoped name will be created in the end with bnc.buildPortGroup(portGroupName, ns, ports, acls).
I guess there are more examples of that, and maybe if we get the new IDs for all the used objects it will help somewhat, but we need to be extra careful about properly indexing db objects.

for this particular case, note that bnc.buildPortGroup() and bnc.getACLMatch takes the portgroupname without network scope prefix as the input argument, so the final port group name would be corrrect. I probably missed some place in multicast code as multicast for secondary network is not enabled yet. I will fix it to not confuse the reviewer.

as I understand, we need to index the db object only if there is no index for this object type, for example, acls. but for port groups and address-set, name is unique within the ovndb table, which is index. So I am not sure I understand the needs to introduce yet another index for these kind of objects.

jcaamano · 2023-03-30T11:19:38Z

Could we add some basic e2e tests?

npinaeva · 2023-03-30T14:23:37Z

A question about controller names: I see all secondary controllers are named netInfo.GetNetworkName() + "-network-controller", is it guaranteed that these names will always be unique?

I would think so. do you have any specific concerns for this?

Yes, we need to be absolutely sure that the names of controllers are different, because it is a part of objects db index. So let's figure out if/how it is guaranteed.

I also see multicast is enabled for secondary networks in the same pr, probably it makes sense to state that in the name, and maybe even split it to at least another commit, if not another pr?

it is not enabled for secondary networks yet.

I mean it may not be enabled as a flag, but the code for is there, e.g. this pr adds updateNamespaceForSecondaryNetwork functino, that calls multicastUpdateNamespace. Also the fact that configureNamespaceCommon calls multicastUpdateNamespace is also confusing

About referencing port groups, looks like it is correct now, I will suggest some changes to make the code more clear about that.

go-controller/pkg/ovn/base_network_controller_acl.go

npinaeva · 2023-03-30T11:26:17Z

go-controller/pkg/ovn/base_network_controller_acl.go

@@ -106,8 +116,8 @@ func getACLMatch(portGroupName, match string, aclT aclType) string {

 // GetNamespaceACLLogging retrieves ACLLoggingLevels for the Namespace.
 // nsInfo will be locked (and unlocked at the end) for given namespace if it exists.
-func (oc *DefaultNetworkController) GetNamespaceACLLogging(ns string) *ACLLoggingLevels {
-	nsInfo, nsUnlock := oc.getNamespaceLocked(ns, true)
+func (bnc *BaseNetworkController) GetNamespaceACLLogging(ns string) *ACLLoggingLevels {


looks like we will be able to leave this file just named acl without base_network_controller prefix, if we move this function to base_network_controller-namespace, which I guess makes sense?

we now have a getACLMatch() which is also receiver function of BaseNetworkController

but getACLMatch doesn't use bnc in any way, I expected it to be reverted

so can we now do what is suggested in first comment?

npinaeva · 2023-03-30T11:30:21Z

go-controller/pkg/ovn/base_network_controller_multicast.go

@@ -87,60 +87,63 @@ func getMcastACLName(nsORpg, mcastSuffix string) string {
 //   - one "to-lport" ACL allowing ingress multicast traffic to pods in 'ns'.
 //     This matches only traffic originated by pods in 'ns' (based on the
 //     namespace address set).
-func (oc *DefaultNetworkController) createMulticastAllowPolicy(ns string, nsInfo *namespaceInfo) error {
+func (bnc *BaseNetworkController) createMulticastAllowPolicy(ns string, nsInfo *namespaceInfo) error {
 	portGroupName := hashedPortGroup(ns)


since the port group name is not that simple now, let's move building the port group here, and then just use pg.Name to build acl?

I am not sure I understand. The portgroup name would follow the same convention as the default network port group name, why it is no longer simple now?

npinaeva · 2023-03-30T11:43:33Z

go-controller/pkg/ovn/base_network_controller_policy.go

@@ -477,32 +488,44 @@ func getARPAllowACLName(ns string) string {
 	return joinACLName(ns, arpAllowPolicySuffix)
 }

+// Note that the hashName argument is the portGroup's Name without network scope prefix
+func (ncni *NetworkControllerNetInfo) buildPortGroup(hashName, name string, ports []*nbdb.LogicalSwitchPort, acls []*nbdb.ACL) *nbdb.PortGroup {


this is a very important part, let's make sure we have a unified set of functions that all controllers will use.
So I suggest to use ncni.GetNetworkScopedName not on the hash, but before the hash, so it has a similar format with already exisiting port groups

func (ncni *NetworkControllerNetInfo) getPortGroupName(name string) string { networkScopedName := ncni.GetNetworkScopedName(name) return hashedPortGroup(networkScopedName) } // Note that the hashName argument is the portGroup's Name without network scope prefix func (ncni *NetworkControllerNetInfo) buildPortGroup(name string, ports []*nbdb.LogicalSwitchPort, acls []*nbdb.ACL) *nbdb.PortGroup { var externalIds map[string]string pgName := ncni.getPortGroupName(name) externalIds = map[string]string{"name": name} if ncni.IsSecondary() { externalIds[types.NetworkExternalID] = ncni.GetNetworkName() } return libovsdbops.BuildPortGroup(pgName, name, ports, acls, externalIds) }

then controller should never hash or GetNetworkScopedName themselves, they should call getPortGroupName to get the Name.

I am not sure I understand: this is afunction that all controller use. I don't see issue with prefix after hash, but I don't have strong opinions one way or another.

The problem for us is that our downstream zone would have issues when upgrading to the latest upstream code if we change the port group name.

exactly, spreading the details about how the port group name is setup may be error-prone, so I am suggesting to replace GetNetworkScopedName calls with getPortGroupName calls
about the hash name - I see Jaime had a similar concern #3382 (comment), we can discuss what is the easiest solution for this situation

this is done the same way when we determine the logical router name/port name etc. for secondary networks. Note that I am getting rid of NetworkControllerNetInfo structure now and in both gressPolicy and BaseNetworkController's receiver functions needs to call the same function to get port group name.

What I am saying is that calling GetNetworkScopedName every time someone needs a real port group name, and adding comments like Note that the hashName argument is the portGroup's Name without network scope prefix is error-prone
So I suggest to always build network-prefixed port group name as early as possible by using getPortGroupName function, that would be the only function that knows how a port group name is built, and free other functions from that logic

so we can remove all hashedPortGroup and GetNetworkScopedName for port groups, and replace them with getPortGroupName and buildPortGroup functions that only take name as an argument, without any extra hashes or prefixes
then you can also remove all comments about if portGroup name is prefixed or not, because it will always be prefixed, e.g. getACLMatch won't need to prefix port group name, it will get the final port group name version

npinaeva · 2023-03-30T13:59:05Z

go-controller/pkg/ovn/gress_policy.go

-		peerV6AddressSets: &sync.Map{},
-		portPolicies:      make([]*portPolicy, 0),
+		NetworkControllerNetInfo: NetworkControllerNetInfo{NetInfo: netInfo},
+		controllerName:           controllerName,


controllerName should be enough here with the new acl indexes

only when acls are also indexed with controllerName. This will be changed after your acl index PR merged.

npinaeva · 2023-03-30T13:59:34Z

go-controller/pkg/ovn/gress_policy.go

@@ -246,9 +249,11 @@ 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
+// Note that portGroupName is portGroup's name without network scope prefix


let's make sure portGroupName is the pg.Name

let's wait for the decision if we are going to use which form of portgroup name, then we decide what needs to be changed. if it is still in the form of networkPrefix+hash, I'll keep it as is.

npinaeva · 2023-03-30T14:17:52Z

go-controller/pkg/ovn/network_conntroller_policy_event_handler.go

+	"k8s.io/klog/v2"
+)
+
+func (bnc *BaseNetworkController) initRetryFramework() {


this should be 2 functions for default and secondary controller

same logic I explained above: this function is directly called from the default and secondary controllers, so you can just call the right function from there

npinaeva · 2023-03-30T14:18:53Z

go-controller/pkg/ovn/network_conntroller_policy_event_handler.go

+}
+
+// event handlers handles policy related events
+type networkControllerPolicyEventHandler struct {


baseNetworkPolicyEventHandler?

npinaeva · 2023-03-30T14:20:05Z

go-controller/pkg/ovn/network_conntroller_policy_event_handler.go

+// newRetryFrameworkWithParameters is instead called directly by the watchers that are
+// dynamically created when a network policy is added: AddressSetNamespaceAndPodSelectorType,
+// PeerNamespaceSelectorType, AddressSetPodSelectorType.
+func (bnc *BaseNetworkController) newRetryFrameworkWithParameters(


newNetpolRetryFramework? also comments to this struct need to be updated, let's say which events are handled by this framework

npinaeva · 2023-04-03T18:27:16Z

go-controller/pkg/ovn/network_conntroller_policy_event_handler.go

+
+// event handlers handles policy related events
+type networkControllerPolicyEventHandler struct {
+	baseHandler     baseNetworkControllerEventHandler


I guess baseHandler is not really required here, because networkControllerPolicyEventHandler handles all netpol-related events, so you can just move these parts frof baseNetworkControllerEventHandler?

maiqueb

some more questions.

go-controller/pkg/ovn/base_network_controller_multipolicy.go

maiqueb · 2023-04-05T11:15:17Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+		klog.Infof("[%s] adding namespace took %v for network %s", ns.Name, time.Since(start), bsnc.GetNetworkName())
+	}()
+
+	_, nsUnlock, err := bsnc.ensureNamespaceLockedForSecondaryNetwork(ns.Name, false, ns)


IMO this ensureNamespaceLockedForSecondaryNetwork method is poorly named (I would expect it hint it creates the address set for this particular namespace), and I also think it does too much.

A method should do one thing this one, at least according to its docs does:

locks namespacesMutex

gets/creates an entry for ns

configures OVN nsInfo (which actually means creating the address set encapsulating the ns info), and returns it with its mutex locked.

Would it be possible to break this method down into smaller blocks ?

I'm afraid it'll be hard to unit test as is.

this is a variants of ensureNamespaceLocked() function for default networks. It does the same thing except the things that does not exist in secondary networks.

maiqueb · 2023-04-05T11:16:37Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+		}
+		// we are creating nsInfo and going to set it in namespaces map
+		// so safe to hold the lock while we create and add it
+		defer bsnc.namespacesMutex.Unlock()


shouldn't we just move this immediately below the bsnc.namespacesMutex.Lock() ?

Why have it in both the if and else clauses ?

because depends on if nsInfoExisted, the unlock happens in different place. This is the same as default network ensureNamespaceLocked().

maiqueb · 2023-04-05T11:21:30Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+	case factory.NamespaceType:
+		ns, ok := obj.(*kapi.Namespace)
+		if !ok {
+			return fmt.Errorf("could not cast %T object to *kapi.Namespace", obj)
+		}
+		return bsnc.AddNamespaceForSecondaryNetwork(ns)
+


question: in the future, for when I introduce the IPAMless ipBlock policies, we won't have to do any of this right ? i.e. there won't be any IPs to keep track of in these address sets.

I could just wrap this in an if !bsnc.doesNetworkRequireIPAM() right ?

... or I could introduce an interface, and provide a separate implementation for the ipamless scenario.

didn't think this through yet. But I don't think you need namespace related handling if policy is not based on namespace/pod selector. If so, you don't need to watch for namespace at all. you could simply direct return in WatchNamespaces() without calling WatchResource()

as discussed yesterday, I haven't considered the non-ipam case in thsi PR yet. I am adding it now.

@cathy-zhou feel free to keep working exclusively the IPAM case in this PR.

I can tackle the non-IPAM case as a follow-up.

I think this would help keep this PR "small", otherwise, it'll bloat even more.

maiqueb · 2023-04-05T11:21:59Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+	case factory.NamespaceType:
+		oldNs, newNs := oldObj.(*kapi.Namespace), newObj.(*kapi.Namespace)
+		return bsnc.updateNamespaceForSecondaryNetwork(oldNs, newNs)


ditto: this will not be needed for IPAMless scenario, right?

maiqueb · 2023-04-05T11:22:10Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+	case factory.NamespaceType:
+		ns := obj.(*kapi.Namespace)
+		return bsnc.deleteNamespace4SecondaryNetwork(ns)


maiqueb · 2023-04-05T11:22:38Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+	// Ensure the namespace/nsInfo exists
+	addOps, err := bsnc.addPodToNamespaceForSecondaryNetwork(pod.Namespace, podAnnotation.IPs)
+	if err != nil {
+		return err
+	}
+	ops = append(ops, addOps...)
+


ditto, this is not needed for the ipamless scenario.

go-controller/pkg/ovn/base_network_controller_secondary.go

npinaeva · 2023-04-06T12:42:47Z

go-controller/pkg/ovn/external_ids_syncer/acl/acl_sync_test.go

@@ -69,6 +69,11 @@ func joinACLName(substrings ...string) string {
 	return strings.Join(substrings, "_")
 }

+func buildPortGroup(hashName, name string, ports []*nbdb.LogicalSwitchPort, acls []*nbdb.ACL) *nbdb.PortGroup {


the sync should never be updated, because the code only works with the previous version of ids, so I think you can just revert changes of this file and it should work

I made this change because it does not compile

npinaeva

another round :)

npinaeva · 2023-04-13T13:44:54Z

go-controller/pkg/ovn/base_event_handler.go

@@ -45,7 +45,7 @@ func hasResourceAnUpdateFunc(objType reflect.Type) bool {
 func (h *baseNetworkControllerEventHandler) areResourcesEqual(objType reflect.Type, obj1, obj2 interface{}) (bool, error) {
 	// switch based on type
 	switch objType {
-	case factory.PolicyType:
+	case factory.PolicyType: //


npinaeva · 2023-04-13T13:45:08Z

go-controller/pkg/ovn/base_event_handler.go

@@ -123,6 +115,17 @@ func (h *baseNetworkControllerEventHandler) areResourcesEqual(objType reflect.Ty
 	case factory.NamespaceType:
 		// force update path for Namespace resource.
 		return false, nil
+
+	case factory.MultiNetworkPolicyType: //


remove //?

npinaeva · 2023-04-13T13:46:58Z

go-controller/pkg/ovn/base_network_controller_acl.go

@@ -106,8 +116,8 @@ func getACLMatch(portGroupName, match string, aclT aclType) string {

 // GetNamespaceACLLogging retrieves ACLLoggingLevels for the Namespace.
 // nsInfo will be locked (and unlocked at the end) for given namespace if it exists.
-func (oc *DefaultNetworkController) GetNamespaceACLLogging(ns string) *ACLLoggingLevels {
-	nsInfo, nsUnlock := oc.getNamespaceLocked(ns, true)
+func (bnc *BaseNetworkController) GetNamespaceACLLogging(ns string) *ACLLoggingLevels {


but getACLMatch doesn't use bnc in any way, I expected it to be reverted

npinaeva · 2023-04-14T10:18:45Z

go-controller/pkg/ovn/base_network_controller_multicast.go

 	if err != nil {
 		klog.Warningf("Failed to get pods for namespace %q: %v", ns, err)
 	}
 	for _, pod := range pods {
 		if util.PodCompleted(pod) {
 			continue
 		}
-		if portInfo, err := oc.logicalPortCache.get(pod, types.DefaultNetworkName); err != nil {
+		portInfoMap, err := bnc.logicalPortCache.getAll(pod)


we should only add logical ports for a specific nad, not all?

it should for all NADs belong to this network controller

npinaeva · 2023-04-14T10:31:40Z

go-controller/pkg/ovn/base_network_controller_multicast.go

 	if err != nil {
 		return err
 	}

-	ops, err = libovsdbops.AddACLsToPortGroupOps(oc.nbClient, ops, types.ClusterPortGroupName, acls...)
+	ops, err = libovsdbops.AddACLsToPortGroupOps(bnc.nbClient, ops, bnc.getPortGroupName(types.ClusterPortGroupName), acls...)


getPortGroupName(...PortGroupName) doesn't read well, because it means that the argument is not a PortGroupName. So I suggest to update variable names ClusterPortGroupName and ClusterRtrPortGroupName to something like ClusterPortGroupNameBase and ClusterRtrPortGroupNameBase

npinaeva · 2023-04-14T11:10:32Z

go-controller/pkg/ovn/default_network_controller.go

+		case factory.PolicyType:
+			syncFunc = h.oc.syncNetworkPolicies
+


npinaeva · 2023-04-14T11:12:21Z

go-controller/pkg/ovn/gress_policy.go

 	return &gressPolicy{
+		NetInfo:           netInfo,


NetInfo is not used anymore?

npinaeva · 2023-04-14T11:13:49Z

go-controller/pkg/ovn/namespace_test.go

@@ -295,7 +295,6 @@ var _ = ginkgo.Describe("OVN Namespace Operations", func() {

 			err = fakeOvn.controller.WatchNamespaces()
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			fakeOvn.asf.EventuallyExpectEmptyAddressSetExist(hostNetworkNamespace)


can you please comment on what changed here

npinaeva · 2023-04-14T11:16:23Z

go-controller/pkg/ovn/ovn.go

@@ -494,3 +429,73 @@ func newEgressServiceController(client clientset.Interface, nbClient libovsdbcli
 		svcFactory.Discovery().V1().EndpointSlices(),
 		svcFactory.Core().V1().Nodes())
 }
+
+func (oc *DefaultNetworkController) syncNetworkPolicies(networkPolicies []interface{}) error {


I am not sure what is our common strategy, but I think it may be better to leave these netpol-specific function under base_network_controller_policy, otherwise all the default controller specific functions will flood this file, and then it is difficult to find them
wdyt @cathy-zhou @jcaamano ?

Yeah, I am all for moving stuff out of ovn.go :P

probably to default_network_controller.go? this is not for base_network_controller, so it should not belong to base_network_controller_policy.go

if we had proper packaging, every file would be split into bnc/<feature_name>, default_network_controller/<feature_name> and where needed same for the secondary controllers. Now when we put the default network controller parts to the same file (ovn or default_network_controller) it may difficult to split them back when/if we change the packages structure.
I agree that it doesn't make a lot of sense to put these function under bnc_ files, so we can make default_network_controller_policy file for that purpose, wdyt?

create a file only for one function? I am not sure.

I guess it should be based on if we are planning to have the same separate file in the future under default network controller package, or if we want to leave all functions in one big file . For now e.g. I am not sure what is the difference between master.go ovn.go and default_network_controller.go, so what is the right place for default network controller netpol functions (that also includes addHairpinAllowACL)
@jcaamano any thoughts?

We can make default_network_controller_policy with WatchNetwoerkPolicies, addHairpinAllowACL and syncNetworkPolicies.

npinaeva · 2023-04-14T11:22:09Z

go-controller/pkg/ovn/pod_selector_address_set.go

+			ipv4Mode:          ipv4Mode,
+			ipv6Mode:          ipv6Mode,


it is different now since we have netInfo also. Are we going to be able to get ipMode from it as @jcaamano suggested?

go-controller/pkg/ovn/base_network_controller_policy.go

go-controller/pkg/ovn/multipolicy_test.go

cathy-zhou · 2023-04-14T19:47:28Z

@npinaeva @jcaamano again, please take a look.

npinaeva · 2023-04-17T09:15:57Z

go-controller/pkg/ovn/base_network_controller_policy.go

-			continue
+		nadNames := bnc.getPodNADNames(pod)
+		for _, nadName := range nadNames {
+			portInfo, err := bnc.getNewLocalPolicyPortsForNAD(np, pod, nadName)


I guess at this point it makes sense to inline getNewLocalPolicyPortsForNAD back to this function, since it is not called from anywhere else?

npinaeva · 2023-04-17T09:17:20Z

go-controller/pkg/ovn/base_network_controller_policy.go

 	for _, obj := range objs {
 		pod := obj.(*kapi.Pod)

-		logicalPortName := util.GetLogicalPortName(pod.Namespace, pod.Name)
+		nadNames := bnc.getPodNADNames(pod)
+		if !bnc.IsSecondary() {


can't we just do

for _, nadName := range bnc.getPodNADNames(pod) { logicalPortName := bnc.GetLogicalPortName(pod, nadName)

then the rest of this function can stay as it was

npinaeva · 2023-04-17T16:13:27Z

go-controller/pkg/ovn/base_network_controller_multicast.go

 	if err != nil {
 		return err
 	}

 	// Remove old multicastDefaultDeny port group now that all ports
 	// have been added to the clusterPortGroup by WatchPods()
-	ops, err = libovsdbops.DeletePortGroupsOps(oc.nbClient, ops, legacyMulticastDefaultDenyPortGroup)
+	ops, err = libovsdbops.DeletePortGroupsOps(bnc.nbClient, ops, bnc.getPortGroupName(legacyMulticastDefaultDenyPortGroup))


we don't need to prefix stale port group right? since it was deleted before the networkPrefix is defined

in that case, it is no-op. Otherwise, secondary network controllers would be trying to delete the legacy port groups of default network, which I don't want to happen. Another way is to do it only when if !bnc.IsSecondary() , but seems something we want to avoid as much as possible?

I personally is fine with either way.

npinaeva · 2023-04-17T16:14:15Z

go-controller/pkg/ovn/base_network_controller_policy.go

@@ -58,6 +58,10 @@ type defaultDenyPortGroups struct {
 	policies map[string]bool
 }

+func (bnc *BaseNetworkController) getPortGroupName(readableGroupName string) string {


I guess at this point this function is not needed, we will have a separate function for every type of port group to get its name?

cathy-zhou · 2023-04-17T20:22:10Z

@npinaeva I made the change, please take a look.

npinaeva · 2023-04-19T07:19:35Z

go-controller/pkg/ovn/base_network_controller_multicast.go

@@ -30,10 +29,17 @@ const (
 	legacyMulticastDefaultDenyPortGroup = "mcastPortGroupDeny"
 )

-func getACLMatchAF(ipv4Match, ipv6Match string) string {
-	if config.IPv4Mode && config.IPv6Mode {
+func (bnc *BaseNetworkController) GetMulticastPortGroupName(base string) string {


I guess that should be getClusterPortGroupName? Because cluster port groups are not only used by multicast, but e.g. by egress firewall
If so, I would suggest to move it to base_network_controller, update the name and add a comment for which base names it should be used

npinaeva

Great progress, we are getting there :)
Added some new comments, and here are some comments I don't see addressed:
#3382 (comment)
#3382 (comment)

npinaeva · 2023-04-19T07:49:27Z

go-controller/pkg/ovn/namespace.go

 	}
+	defer oc.namespacesMutex.Unlock()


I think releasing the lock that is acquired in another function may be error-prone, so maybe we can add configureNamespace func(nsInfo *namespaceInfo, ns *kapi.Namespace) as an argument to ensureNamespaceLockedCommon, and leave the internal logic the same?

how, note that in the two functions for default network controllers, configureNamespace() is receiver function of DefaultNetworkController. We need to somehow pass that to ensureNamespaceLockedCommon(), which is strange to me.

I am not sure I understand, being a method doesn't change function signature, so you still can do

func (bnc *BaseNetworkController) ensureNamespaceLockedCommon(ns string, readOnly bool, namespace *kapi.Namespace, ips []net.IP, configureNamespace func(nsInfo *namespaceInfo, ns *kapi.Namespace) error) (*kapi.Namespace, *namespaceInfo, func(), error) {...} namespace, nsInfo, unlockFunc, err := oc.ensureNamespaceLockedCommon(ns, readOnly, namespace, ips, oc.configureNamespace) namespace, nsInfo, unlockFunc, err := bsnc.ensureNamespaceLockedCommon(ns, readOnly, namespace, ips, bsnc.configureNamespaceCommon)

you are right. fixed. Please take a look

npinaeva · 2023-04-19T07:57:13Z

go-controller/pkg/ovn/namespace.go

@@ -476,57 +284,36 @@ func (oc *DefaultNetworkController) ensureNamespaceLocked(ns string, readOnly bo
 	return nsInfo, unlockFunc, nil
 }

-func (oc *DefaultNetworkController) createNamespaceAddrSetAllPods(ns string) (addressset.AddressSet, error) {
+//nolint:unused


is this lint command required?

npinaeva · 2023-04-19T08:06:04Z

go-controller/pkg/ovn/base_network_controller_policy.go

-		}
+		nadNames := bnc.getPodNADNames(pod)
+		for _, nadName := range nadNames {
+			var logicalPortName string


logicalPortName := bnc.GetLogicalPortName(pod, nadName) ?

are you suggesting the change the function signature of GetSecondaryNetworkLogicalPortName(), or you meant to have one single function for both default and secondary network?

for latter, the problem is that we'd still need to check if it is default network, to decide weather or not prefix the logical port name. I believe that is the reason why we agreed to have two different functions.

I mean this function already exists

func (bnc *BaseNetworkController) GetLogicalPortName(pod *kapi.Pod, nadName string) string { if !bnc.IsSecondary() { return util.GetLogicalPortName(pod.Namespace, pod.Name) } else { return util.GetSecondaryNetworkLogicalPortName(pod.Namespace, pod.Name, nadName) } }

and seems to do exactly the same thing, so I just suggest using it

Ah, I see. I didn't even remember we have this function. thanks!

npinaeva · 2023-04-19T08:08:36Z

go-controller/pkg/ovn/base_network_controller_policy.go

-// if there are problems with fetching port info from logicalPortCache, pod will be added to errObjs.
-func (oc *DefaultNetworkController) getExistingLocalPolicyPorts(np *networkPolicy,
-	objs ...interface{}) (policyPortsToUUIDs map[string]string, policyPortUUIDs []string, errObjs []interface{}) {
+// if there are problems with fetching port info from logicalPortCache, error will be added to return error array.


looks like errors are not returned anymore, probably we can delete that comment line

npinaeva · 2023-04-19T08:09:01Z

go-controller/pkg/ovn/base_network_controller_policy.go

+			var logicalPortName string
+			if !bnc.IsSecondary() {
+				logicalPortName = util.GetLogicalPortName(pod.Namespace, pod.Name)
+			} else {
+				logicalPortName = util.GetSecondaryNetworkLogicalPortName(pod.Namespace, pod.Name, nadName)
+			}


logicalPortName := bnc.GetLogicalPortName(pod, nadName) ?

same as above

npinaeva · 2023-04-19T08:26:26Z

go-controller/pkg/ovn/multicast_test.go

@@ -52,7 +52,7 @@ func setIpMode(m ipMode) {
 	config.IPv6Mode = m.IPv6Mode
 }

-func getMulticastDefaultExpectedData(clusterPortGroup, clusterRtrPortGroup *nbdb.PortGroup) []libovsdb.TestData {
+func getMulticastDefaultExpectedData(fakeController *DefaultNetworkController, clusterPortGroup, clusterRtrPortGroup *nbdb.PortGroup) []libovsdb.TestData {


fakeController unused?

npinaeva · 2023-04-19T08:27:15Z

go-controller/pkg/ovn/multicast_test.go

@@ -135,25 +135,25 @@ func getMulticastDefaultExpectedData(clusterPortGroup, clusterRtrPortGroup *nbdb
 	}
 }

-func getMulticastDefaultStaleData(clusterPortGroup, clusterRtrPortGroup *nbdb.PortGroup) []libovsdb.TestData {
-	testData := getMulticastDefaultExpectedData(clusterPortGroup, clusterRtrPortGroup)
+func getMulticastDefaultStaleData(fakeController *DefaultNetworkController, clusterPortGroup, clusterRtrPortGroup *nbdb.PortGroup) []libovsdb.TestData {


fakeController will be unused, if getMulticastDefaultExpectedData is updated?

npinaeva · 2023-04-19T08:29:27Z

go-controller/pkg/ovn/multicast_test.go

-	pg_hash := getMulticastPortGroupName(ns)
-	egressMatch := getACLMatch(pg_hash, getMulticastACLEgrMatch(), aclEgress)
+func getMulticastPolicyExpectedData(fakeController *DefaultNetworkController, ns string, ports []string) []libovsdb.TestData {
+	pg_hash := fakeController.getMulticastPortGroupName(ns)


we usually use
fakeController = getFakeController(DefaultNetworkControllerName) right here in the function to avoid adding arguments (of course in case the controller type is predefined)

npinaeva

All the previous comments look addressed, just a couple more for the unit test commit.
It's great to have a unit test with both multi and regular netpols, thanks for implementing that @cathy-zhou !

npinaeva · 2023-04-21T09:53:22Z

go-controller/pkg/ovn/policy_test.go

@@ -40,17 +40,6 @@ const (
 	arpAllowPolicySuffix = "ARPallowPolicy"
 )

-func getFakeController(controllerName string) *DefaultNetworkController {


I would say we can leave both getFakeController and getFakeBaseController, so that function that used DefaultNetworkController can keep using it, and multinet-tests can use getFakeBaseController?

npinaeva · 2023-04-21T10:02:27Z

go-controller/pkg/ovn/policy_test.go

@@ -101,7 +98,7 @@ func getDefaultDenyData(networkPolicy *knet.NetworkPolicy, ports []string,
 			"apply-after-lb": "true",
 		},
 	)
-	egressDenyACL.UUID = *egressDenyACL.Name + "-egressDenyACL-UUID"
+	egressDenyACL.UUID = netInfo.GetNetworkScopedName(*egressDenyACL.Name + "-egressDenyACL-UUID")


I think it may be a good time to switch to
UUID = aclIDs.String()
it should be unique, so hopefully we won't need to change it again

npinaeva · 2023-04-21T10:06:20Z

go-controller/pkg/ovn/namespace_test.go

@@ -67,8 +67,8 @@ func newNamespace(namespace string) *v1.Namespace {
 	}
 }

-func getNsAddrSetHashNames(ns string) (string, string) {
-	return addressset.GetHashNamesForAS(getNamespaceAddrSetDbIDs(ns, DefaultNetworkControllerName))
+func getNsAddrSetHashNames(ns, controllerName string) (string, string) {


I would say let's leave this function as is, and make a new one like getMultinetNsAddrSetHashNames(ns, controllerName string), and it will have only 1 usage from getGressACLs, and will reduce the number of required changes

npinaeva · 2023-04-21T10:06:55Z

go-controller/pkg/ovn/multicast_test.go

@@ -186,11 +186,11 @@ func getDefaultPortGroups() (clusterPortGroup, clusterRtrPortGroup *nbdb.PortGro
 }

 func getMulticastPolicyExpectedData(ns string, ports []string) []libovsdb.TestData {
-	fakeController := getFakeController(DefaultNetworkControllerName)
+	fakeController := getFakeBaseController(&util.DefaultNetInfo{}, &util.DefaultNetConfInfo{})


I guess if we get getFakeController back, this may be reverted

npinaeva · 2023-04-21T10:33:53Z

go-controller/pkg/ovn/multipolicy_test.go

+				startOvn(initialDB, []v1.Namespace{namespace1, namespace2}, nil, nil,
+					[]nettypes.NetworkAttachmentDefinition{*nad}, nil, nil)
+
+				ocInfo, ok := fakeOvn.secondaryControllers["network1"]


let's make "network1" constant and use in nad definition too?

npinaeva · 2023-04-21T10:37:28Z

go-controller/pkg/ovn/multipolicy_test.go

+				netconf, err := util.ParseNetConf(nad)
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+				netInfo := util.NewNetInfo(netconf)


we can make netconf and netInfo variables, since they only depend on nad and used in all tests?

npinaeva · 2023-04-21T10:38:55Z

go-controller/pkg/ovn/multipolicy_test.go

+				ocInfo, ok := fakeOvn.secondaryControllers["network1"]
+				gomega.Expect(ok).To(gomega.Equal(true))
+				asf := ocInfo.asf
+				gomega.Expect(asf).NotTo(gomega.Equal(nil))
+				gomega.Expect(asf.ControllerName).To(gomega.Equal("network1-network-controller"))


maybe we can make it a part of startOvn, so it checks expected secondary controllers are created based on given nads?

npinaeva · 2023-04-21T10:46:42Z

go-controller/pkg/ovn/policy_test.go

+		NetConfInfo:    netConfInfo,
+	}
+}
+
 func getDefaultDenyData(networkPolicy *knet.NetworkPolicy, ports []string,


If we merge #3540 soon, we will be able to change the signature of this function, then pass default network args for existing wrappers, and create new functions like getDefaultDenyDataMultinet and getPolicyDataMultinet.
In that case calls in this file won't need to be changed

npinaeva · 2023-04-21T10:49:34Z

go-controller/pkg/ovn/base_network_controller_multicast.go

 	if err != nil {
 		return err
 	}

 	// Remove old multicastDefaultDeny port group now that all ports
 	// have been added to the clusterPortGroup by WatchPods()
-	ops, err = libovsdbops.DeletePortGroupsOps(oc.nbClient, ops, legacyMulticastDefaultDenyPortGroup)
+	ops, err = libovsdbops.DeletePortGroupsOps(bnc.nbClient, ops, bnc.getClusterPortGroupName(legacyMulticastDefaultDenyPortGroup))


I think maybe in that case it makes sense to do

if !bnc.IsSecondary() { ops, err = libovsdbops.DeletePortGroupsOps(bnc.nbClient, ops, legacyMulticastDefaultDenyPortGroup) if err != nil { return err } }

to avoid confusion with prefixing

cathy-zhou · 2023-04-24T23:55:24Z

/retest

…oning Also updated the OVNK Kube client to feature a multi-policy client. Signed-off-by: Yun Zhou <yunz@nvidia.com>

Signed-off-by: Yun Zhou <yunz@nvidia.com>

npinaeva

one last nit, otherwise
/lgtm
thanks @cathy-zhou!

npinaeva · 2023-04-25T07:20:37Z

go-controller/pkg/ovn/egressfirewall_test.go

@@ -130,7 +130,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 				config.Gateway.Mode = gwMode
 				app.Action = func(ctx *cli.Context) error {
 					// owned by non-existing namespace
-					fakeController := getFakeController(DefaultNetworkControllerName)
+					fakeController := &DefaultNetworkController{BaseNetworkController: *getFakeBaseController(&util.DefaultNetInfo{}, &util.DefaultNetConfInfo{})}


fixed. thanks!

Signed-off-by: Yun Zhou <yunz@nvidia.com>

cathy-zhou force-pushed the multi-network-mr6 branch from 0f0f8ec to 980052f Compare January 31, 2023 19:12

cathy-zhou force-pushed the multi-network-mr6 branch 3 times, most recently from baa390f to 9c8db39 Compare February 7, 2023 01:04

martinkennelly self-assigned this Feb 13, 2023

cathy-zhou force-pushed the multi-network-mr6 branch 2 times, most recently from 8ded4d4 to c022ab6 Compare March 15, 2023 16:45

cathy-zhou requested review from npinaeva, maiqueb, trozet and jcaamano March 15, 2023 18:32

cathy-zhou force-pushed the multi-network-mr6 branch from c022ab6 to 4ab7b69 Compare March 16, 2023 01:14

maiqueb reviewed Mar 16, 2023

View reviewed changes

cathy-zhou force-pushed the multi-network-mr6 branch from 4ab7b69 to 5465ebd Compare March 18, 2023 03:00

jcaamano reviewed Mar 28, 2023

View reviewed changes

go-controller/pkg/libovsdbops/portgroup.go Outdated Show resolved Hide resolved

go-controller/pkg/network-controller-manager/network_controller_manager.go Show resolved Hide resolved

go-controller/pkg/network-controller-manager/network_controller_manager.go Outdated Show resolved Hide resolved

cathy-zhou force-pushed the multi-network-mr6 branch from 5465ebd to 963df9c Compare March 29, 2023 02:38

jcaamano requested changes Mar 29, 2023

View reviewed changes

npinaeva reviewed Mar 30, 2023

View reviewed changes

npinaeva reviewed Apr 3, 2023

View reviewed changes

maiqueb reviewed Apr 5, 2023

View reviewed changes

cathy-zhou force-pushed the multi-network-mr6 branch 3 times, most recently from 3ea669b to 7e82300 Compare April 6, 2023 04:03

npinaeva reviewed Apr 6, 2023

View reviewed changes

cathy-zhou force-pushed the multi-network-mr6 branch from 7e82300 to 7b4ca5d Compare April 6, 2023 21:51

cathy-zhou force-pushed the multi-network-mr6 branch from 22332f4 to 8047b7f Compare April 14, 2023 00:34

npinaeva reviewed Apr 14, 2023

View reviewed changes

jcaamano reviewed Apr 14, 2023

View reviewed changes

go-controller/pkg/ovn/base_network_controller_policy.go Outdated Show resolved Hide resolved

go-controller/pkg/ovn/multipolicy_test.go Show resolved Hide resolved

cathy-zhou force-pushed the multi-network-mr6 branch from 8047b7f to 6162d1c Compare April 14, 2023 19:15

cathy-zhou force-pushed the multi-network-mr6 branch from 6162d1c to 4bf7bb3 Compare April 14, 2023 19:48

npinaeva reviewed Apr 17, 2023

View reviewed changes

cathy-zhou force-pushed the multi-network-mr6 branch from 4bf7bb3 to 9b8e4d7 Compare April 17, 2023 20:21

npinaeva reviewed Apr 19, 2023

View reviewed changes

cathy-zhou force-pushed the multi-network-mr6 branch 2 times, most recently from 47a46d9 to 0697af0 Compare April 20, 2023 17:38

npinaeva reviewed Apr 21, 2023

View reviewed changes

cathy-zhou force-pushed the multi-network-mr6 branch from 0697af0 to 474f6b8 Compare April 21, 2023 20:20

cathy-zhou added 2 commits April 24, 2023 17:28

MultiNetworkPolicy, multi-policy handlers: watch multi-policy provisi…

04201ab

…oning Also updated the OVNK Kube client to feature a multi-policy client. Signed-off-by: Yun Zhou <yunz@nvidia.com>

add multi-policy support to secondary network controller

c973d61

Signed-off-by: Yun Zhou <yunz@nvidia.com>

cathy-zhou force-pushed the multi-network-mr6 branch from 474f6b8 to a09dbca Compare April 25, 2023 00:29

npinaeva reviewed Apr 25, 2023

View reviewed changes

add multi-network-policy unit testing cases

9161599

Signed-off-by: Yun Zhou <yunz@nvidia.com>

cathy-zhou force-pushed the multi-network-mr6 branch from a09dbca to 9161599 Compare April 25, 2023 15:53

jcaamano approved these changes Apr 26, 2023

View reviewed changes

jcaamano merged commit b24e304 into ovn-org:master Apr 26, 2023
22 checks passed

tssurya added this to the v1.0.0 milestone Mar 11, 2024

tssurya added network-policy Issues related to network policies (primary and secondary networks) multi-networks Issues related to secondary networks, L3, L2, localnet kind/feature All issues/PRs that are new features labels Mar 12, 2024

multi-network policy support #3382

multi-network policy support #3382

Conversation

cathy-zhou commented Jan 26, 2023

coveralls commented Jan 31, 2023 • edited

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcaamano left a comment

Choose a reason for hiding this comment

npinaeva commented Mar 29, 2023

jcaamano left a comment

Choose a reason for hiding this comment

cathy-zhou commented Mar 29, 2023 • edited

jcaamano commented Mar 30, 2023

npinaeva commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathy-zhou Apr 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npinaeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathy-zhou commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 31, 2023 •

edited

cathy-zhou commented Mar 29, 2023 •

edited

cathy-zhou Apr 5, 2023 •

edited