Support Network Policy for UDN namespaces #4690

pperiyasamy · 2024-09-02T09:28:30Z

What this PR does and why is it needed

This PR has required changes to support Network Policy on User Defined Network.

NetPol Support on L3 UDN network.
NetPol support on on L2 UDN network.

TODOs:

Run NetPol upstream conformance tests over UDN network.
Add hairpin allow ACLs for primary UDNs network policies (https://issues.redhat.com/browse/SDN-5290).

Which issue(s) this PR fixes

Fixes #

Special notes for reviewers

How to verify it

Details to documentation updates

Description for the changelog

Does this PR introduce a user-facing change?

npinaeva · 2024-09-02T18:31:29Z

there are some netpol-related test failures. Also, could you please add a couple of e2es for the new netpol behaviour?

pperiyasamy · 2024-09-05T09:36:03Z

hit this ci flake #4299 in this run:
https://github.com/ovn-org/ovn-kubernetes/actions/runs/10716745664/job/29716177061?pr=4690

go-controller/pkg/util/pod_annotation.go

go-controller/pkg/ovn/base_network_controller.go

go-controller/pkg/ovn/base_network_controller_namespace.go

go-controller/pkg/ovn/base_network_controller_policy.go

jcaamano · 2024-09-06T10:21:42Z

go-controller/pkg/ovn/base_network_controller_policy.go

+	if bnc.NetInfo.IsSecondary() {
+		return nil
+	}
+
+	// add default hairpin allow acl
+	err = bnc.addHairpinAllowACL()


Why don't we need the hairpin allow ACLs for primary networks? Add comment.

added comment about why hairpin allow ACLs disabled now for UDN. will revisit once we have cluster port group exists per network. currently this is not impacting unit tests and e2e's.

Should we have an issue to track?

https://issues.redhat.com/browse/SDN-5290

go-controller/pkg/ovn/base_network_controller_secondary.go

go-controller/pkg/ovn/base_secondary_layer2_network_controller.go

go-controller/pkg/ovn/secondary_layer2_network_controller.go

go-controller/pkg/util/pod_annotation.go

jcaamano · 2024-09-16T12:27:15Z

go-controller/pkg/ovn/base_network_controller.go

+func (bnc *BaseNetworkController) UpdateResourceCommon(objType reflect.Type, oldObj, newObj interface{}) error {
+	switch objType {
+	case factory.PolicyType:
+		oldNp, ok := oldObj.(*knet.NetworkPolicy)
+		if !ok {
+			return fmt.Errorf("could not cast obj of type %T to *knet.NetworkPolicy", oldObj)
+		}
+		if err := bnc.deleteNetworkPolicy(oldNp); err != nil {
+			klog.Infof("NetworkPolicy delete failed for %s/%s, will try again later: %v",
+				oldNp.Namespace, oldNp.Name, err)
+			return err
+		}
+		newNp, ok := newObj.(*knet.NetworkPolicy)
+		if !ok {
+			return fmt.Errorf("could not cast obj of type %T to *knet.NetworkPolicy", newObj)
+		}
+		if err := bnc.addNetworkPolicy(newNp); err != nil {
+			klog.Infof("NetworkPolicy add failed for %s/%s, will try again later: %v",
+				newNp.Namespace, newNp.Name, err)
+			return err
+		}
+	default:
+		return fmt.Errorf("object type %s not supported", objType)
+	}
+	return nil
+}


I don't think update for network policies is going to be called, from
https://github.com/jcaamano/ovn-kubernetes/blob/14afcbf9621264f6d1de32107bfdcf77f655bbbc/go-controller/pkg/ovn/base_event_handler.go#L25-L26

right, removed the method as it's no longer needed.

go-controller/pkg/ovn/base_network_controller.go

jcaamano · 2024-09-17T13:53:36Z

Potential flake
[FAIL] Network Segmentation: services on a user defined primary network should be reachable through their cluster IP and node port [It] L3 dualstack primary UDN, cluster-networked pods, NodePort service
https://github.com/ovn-org/ovn-kubernetes/actions/runs/10900030212/job/30253567040?pr=4690

jcaamano · 2024-09-17T14:36:33Z

Potential flake [FAIL] Network Segmentation: services on a user defined primary network should be reachable through their cluster IP and node port [It] L3 dualstack primary UDN, cluster-networked pods, NodePort service https://github.com/ovn-org/ovn-kubernetes/actions/runs/10900030212/job/30253567040?pr=4690

@pperiyasamy does not look like a flake

trozet

minor comment nits, lgtm

go-controller/pkg/ovn/secondary_layer2_network_controller.go

trozet · 2024-09-17T15:05:36Z

test/e2e/network_segmentation_policy.go

+
+			},
+			ginkgo.Entry(
+				"in L2 dualstack primary UDN",


the title of this PR says netpol for L3, but here are L2 test cases and changes to layer 2 controller. So is L2 also supported and the title just needs to be updated?

yes @trozet , updated title and description accordingly now.

jcaamano · 2024-09-19T11:11:02Z

go-controller/pkg/util/pod_annotation.go

+		if ns != namespace {
+			continue
+		}
+		return []string{nadName}, nil


This is based on two assumptions:

There is a single NAD per namespace for primary networks. However, there is nothing currently that prevents this from happening.

We are aware of the primary network of a pod. However this might be racey.

The first issue should be handled in some way by the NAD controller. It might do that through GetActiveNetworkForNamespace. For the second issue, GetActiveNetworkForNamespace offers a bit more of protection since it will check against the UDN CRDs as well.

I don't want to hold this because of that. But I am entertaining the idea that we could just implement GetActiveNetworkForNamespace in NetInfo that would just end up calling the nad controller GetActiveNetworkForNamespace which is feasible since the nad controller is the one building those NetInfo after all.

I like the idea of having GetActiveNetworkForNamespace inside netInfo.

jcaamano · 2024-09-19T11:19:27Z

test/e2e/network_segmentation_policy.go

+					defer func() {
+						gomega.Expect(cs.CoreV1().Namespaces().Delete(
+							context.Background(),
+							namespace,
+							metav1.DeleteOptions{},
+						)).To(gomega.Succeed())
+					}()


Don't use defer to clean up please, it hinders troubleshooting. Use f.AddNamespacesToDelete; or manually in AfterEach depending on how the framework is configured and if the test case failed or not.

yes @jcaamano , modified the test to use f.AddNamespacesToDelete.

npinaeva · 2024-09-19T14:54:37Z

Is it intended to configure netpol for both default and primary UDN at the same time?

pperiyasamy · 2024-09-19T15:10:08Z

Is it intended to configure netpol for both default and primary UDN at the same time?

yes @npinaeva , The netpol for default and primary UDN networks would coexist with this PR as well. The existing shard-conformance CI lane tests netpol over default network and we had few e2e's to test NetPol over UDN network.
We also ran upstream NetPol tests over UDN L3 network on this PR and it worked fine. We're waiting for #4653 to run upstream tests over L2 network. Does it suffice ? or do you want to cover something more ?

npinaeva · 2024-09-19T15:47:51Z

yes @npinaeva , The netpol for default and primary UDN networks would coexist with this PR as well. The existing shard-conformance CI lane tests netpol over default network and we had few e2e's to test NetPol over UDN network. We also ran upstream NetPol tests over UDN L3 network on this PR and it worked fine. We're waiting for #4653 to run upstream tests over L2 network. Does it suffice ? or do you want to cover something more ?

So when primary UDN exists, we will configure network policy both for UDN and for default network, which means 2 times more port groups, acls, address sets, which will not be used (as default network traffic has its own ACLs that deny almost everything already). So UDN netpol will work, but not sure if it is the most optimal solution.

jcaamano · 2024-09-19T15:56:48Z

So when primary UDN exists, we will configure network policy both for UDN and for default network, which means 2 times more port groups, acls, address sets, which will not be used (as default network traffic has its own ACLs that deny almost everything already). So UDN netpol will work, but not sure if it is the most optimal solution.

@npinaeva has a point.

So when handling a policy we would have to GetActiveNetworkForNamespace(netpolNamespace) and check that it matches our network. Seems straight forward enough.

This commit provides Network Policy support for user defined l3 or l2 networks when it is configured as the primary network for namespace. It's done by subscribing to NetPol events from secondary network controllers and handling it appropriately in base network and policy controllers. Signed-off-by: Periyasamy Palanisamy <[email protected]>

When primary UDN exists for the namespace, the current implementation configured network policy for both UDN and default network. The default network traffic has its own ACLs that deny almost everything already so handling network policy for default is unnecessary and not an optimal solution as it programs another set of port groups, acls and address sets which are never going to be used. Hence this commit skips handling network policy events on the default network controller when namespace contains an active user defined network. Signed-off-by: Periyasamy Palanisamy <[email protected]>

pperiyasamy requested a review from a team as a code owner September 2, 2024 09:28

pperiyasamy requested a review from trozet September 2, 2024 09:28

pperiyasamy marked this pull request as draft September 2, 2024 09:28

pperiyasamy force-pushed the netpol-udn branch from 82c07e0 to be64f5d Compare September 2, 2024 14:29

github-actions bot added the area/unit-testing Issues related to adding/updating unit tests label Sep 2, 2024

pperiyasamy requested review from tssurya and npinaeva and removed request for trozet September 2, 2024 14:30

github-actions bot added the area/e2e-testing label Sep 3, 2024

pperiyasamy force-pushed the netpol-udn branch 3 times, most recently from f55a827 to 259e967 Compare September 5, 2024 08:28

pperiyasamy changed the title ~~Handle NetPol events on UDN namespaces~~ Support Network Policy for UDN namespaces Sep 5, 2024

pperiyasamy marked this pull request as ready for review September 5, 2024 08:30

pperiyasamy force-pushed the netpol-udn branch 2 times, most recently from 13bb7db to 95051a1 Compare September 6, 2024 07:25