User defined primary network canary impl #4459

maiqueb · 2024-06-20T11:17:19Z

What this PR does and why is it needed

This PR tries to highlight all the changes that are needed e2e to plumb a user defined primary network into the pod.

It also provides an e2e test to assert communication over this network.

What the PR essentially does is:

set the annotations for the user defined primary
consume those annotations to create the LS/LSP for the user defined primary
consume those annotations in the CNI side to configure both interfaces (default cluster net + user defined primary net)
provides an e2e test for east/west over the user defined attachment.

Which issue(s) this PR fixes

Fixes #

Special notes for reviewers

It currently only works iif the name of the NAD matches the name of the attachment. That's pretty much because we don't currently have a way to pass around the name of the network to the CNI side. Or I fail to see it.

How to verify it

Details to documentation updates

Description for the changelog

Does this PR introduce a user-facing change?

The namespace active-network annotation is handled at the NAD controller, since namespaces are common to the cluster they need to be updated at a common places, in the case of IC this is control-manager. This change add the namespace informer to it so nad controller can check if the annotation is already set. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

At IC the controller manager will be responsible of of patching namespace with active-network, so it needs permissions to do so. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

Signed-off-by: Enrique Llorente <ellorent@redhat.com>

Add a net attach def manager system on react on creation/deletion of NAds, this will be used to attach at network segmentation namespace manager that reacts on NAD events. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

Signed-off-by: Enrique Llorente <ellorent@redhat.com>

This change ensure that there are no multiple primary network at the same namespace, that there are no pods before activate the primary network and update the namesapce with active-network if everything is fine else it will annotate with "unknown", Also namespace is created with active-network="default". Signed-off-by: Enrique Llorente <ellorent@redhat.com>

This change add the following scenarios: - Delete a duplicate primary network nad - Delete primary network nad with no pods - Delete primary network nad with pods Signed-off-by: Enrique Llorente <ellorent@redhat.com>

This commit adds the primary network boolean field to a pod's annotation info so that this information can be used to tell if the pod is managed by the default network OR secondary network. This will be useful in future commits and also for other feature controller's to leverage in the future. k8s.ovn.org/pod-networks: '{ "default":{ "ip_addresses":["10.244.1.4/24"], "mac_address":"0a:58:0a:f4:01:04", "routes":[{"dest":"10.244.0.0/16","nextHop":"10.244.1.1"},{"dest":"100.64.0.0/16","nextHop":"10.244.1.1"}], "ip_address":"10.244.1.4/24", "primary":false }, "ns2/l2-network":{ "ip_addresses":["10.100.200.9/24"], "mac_address":"0a:58:0a:64:c8:09", "ip_address":"10.100.200.9/24", "tunnel_id":2, "primary":true } }' Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>

This commit ensures that for L3 and L2 by default all traffic should leave via the default gatewayIP of the podsubnet that is part of the user defined secondary network which is the delegated primary network of the pod. L3: surya@fedora:~/user-defined-networks$ oc rsh -n ns1 tinypod / # ip r default via 10.128.0.1 dev net1 10.96.0.0/16 via 10.128.0.1 dev net1 10.128.0.0/24 dev net1 proto kernel scope link src 10.128.0.4 10.128.0.0/16 via 10.128.0.1 dev net1 10.244.0.0/16 via 10.244.1.1 dev eth0 10.244.1.0/24 dev eth0 proto kernel scope link src 10.244.1.3 100.64.0.0/16 via 10.244.1.1 dev eth0 L2: / # ip r 10.96.0.0/16 via 10.100.200.1 dev net1 10.100.200.0/24 dev net1 proto kernel scope link src 10.100.200.9 10.244.0.0/16 via 10.244.1.1 dev eth0 10.244.1.0/24 dev eth0 proto kernel scope link src 10.244.1.4 100.64.0.0/16 via 10.244.1.1 dev eth0 Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

maiqueb · 2024-06-20T11:18:30Z

go-controller/pkg/clustermanager/network_cluster_controller.go

+
+	// TODO: this should be a watch grabbed from the watch factory
+	nadClient networkattchmentdefclientset.Interface


this is a leftover; but we'll need the NAD watcher to plumb into the allocator, so we can figure out what's the namespace's primary network

i did this already in my PR there is NAD watcher

maiqueb · 2024-06-20T11:19:13Z

go-controller/pkg/clustermanager/pod/allocator.go

+	ns, err := a.namespaceLister.Get(pod.Namespace)
+	if err != nil {
+		return err
+	}
+	activeNetworkName, hasAnActiveNetwork := ns.Annotations["k8s.ovn.org/active-network"]


waiting on @tssurya 's helper to identify the namespace's user defined primary net.

Meaning, this code should be replaced by @tssurya 's helper.

maiqueb · 2024-06-20T11:20:41Z

go-controller/pkg/clustermanager/pod/allocator.go

+	)
+	if hasAnActiveNetwork && activeNetworkName != "default" {
+		network := &nettypes.NetworkSelectionElement{
+			// TODO: below we need to have the NAD name, *not* the network name


@tssurya we need your helper to give us the NAD name since that's what the ovnkube-controller uses to match on the ovn-k annotations.

Plus, keep in mind the network is cluster scoped, while the NAD is namespace scoped. Trying to say that <ns>/<network name> doesn't make sense.

maiqueb · 2024-06-20T11:21:50Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+			// TODO: we need to get the NAD name here, not the network name.
+			Name:      bsnc.GetNetworkName(),


@tssurya I assume your helper will be available here.

We also need to figure out what's the NAD name here, in order to build the network selection element.

maiqueb · 2024-06-20T11:23:46Z

go-controller/pkg/ovn/base_network_controller_secondary.go

@@ -235,6 +235,15 @@ func (bsnc *BaseSecondaryNetworkController) ensurePodForSecondaryNetwork(pod *ka
 		return err
 	}

+	if bsnc.IsPrimaryNetwork() && (bsnc.TopologyType() == "layer2" || bsnc.TopologyType() == "layer3") {
+		network := &nadapi.NetworkSelectionElement{


@qinqon we'll need to come up w/ a way to figure out the name of the IPAMClaim for a persistent IPs user defined network.

Maybe we can just hard-code it ?... Like ... <vm name>.user-defined-primary-net

... or something like that. The kubevirt controller would create them w/ that name if there is a NAD on the namespace of the VM w/ the primary knob enabled.

Let's just annotate the virt-launcher pods with ipamclaim name at kubevirt-ipam-controller k8s.ovn.org/ipamclaim-refrence or the like.

why would we do that ? aren't we actually saying everywhere else that annotation / labeling based solutions are bad, and should not be used ?

NetworkSelectionElement is an annotation too.

This annotation is reconciled by kubevirt-ipam-controller if someone change it it will be restored

an alternative is ovn-k reading the kubevirt vm label to compose the IPAMCLaim name.

NetworkSelectionElement is an annotation too.

It's part of a standard. I wouldn't put it in the same level.

Can you first please explain to me why you're against just hard-coding this name ? It's by far the simplest solution from my perspective.

Then we can evaluate alternative, sure.

Let's just annotate the virt-launcher pods with ipamclaim name at kubevirt-ipam-controller k8s.ovn.org/ipamclaim-refrence or the like.

I don't like this because it must happen in the webhook and the controller, if we want to put it back in case someone tampers with it.

an alternative is ovn-k reading the kubevirt vm label to compose the IPAMCLaim name.

I don't follow. Which label ?

Thinking a bit more, I just realized that we need the annotation or label or whatever; otherwise, we'd do this always, depending on the network - and we don't want to do this for pods.

maiqueb · 2024-06-20T11:59:34Z

go-controller/pkg/clustermanager/pod/allocator.go

+			Name:      activeNetworkName,
+			Namespace: pod.Namespace,
+		}
+		nadName := fmt.Sprintf("%s/%s", pod.Namespace, activeNetworkName)


it wouldn't be activeNetworkName, but the NAD.metadata.Name.

maiqueb · 2024-06-20T15:38:41Z

test/e2e/network_segmentation.go

+					name:           userDefinedNetworkName,
+					networkName:    userDefinedNetworkName,


ensure these values are different, once we're able to access both the network name + NAD name from both ovnkube-controller / ovnkube-control-plane.

maiqueb · 2024-06-20T16:06:20Z

test/e2e/network_segmentation.go

+	if len(netStatus.IPs) > 1 {
+		return "", fmt.Errorf("attachment for network %q with more than one IP", attachmentName)
+	}
+	return netStatus.IPs[0].IP.String(), nil


for dualstack, we'll need to parametrize this. Check what we're doing on secondary nets, and do that. Should just work.

maiqueb · 2024-06-20T16:19:16Z

go-controller/pkg/cni/cni.go

+			"",
+			userDefinedNetworkName, // TODO: we need to find a way to have here access to the network name
+			userDefinedNetworkName, // TODO: we can have the NAD name from the annotations. But the network name is missing. maybe we can add that to the annotations
+			pr.CNIConf.MTU,


we need a way to grab the MTU from the right network.

I think we need to actually read the NAD on ovnkube-controller.

right now I think the best way for @tssurya 's helper function to just spit out the NAD object.

Then anyone that needs it, can access the metadata.name info / unmarshall the NAD.Spec.Config data into an OVN-Kubernetes configuration object and do whatever they need.

Food for thought.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

In this test, we start a container running an HTTP service outside the Kubernetes cluster. We then start a client pod in the Kubernetes cluster, with a user defined primary network attachment, and try to access the service located outside the cluster from it. Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

maiqueb · 2024-06-21T11:42:54Z

go-controller/pkg/ovn/base_network_controller_pods.go

@@ -538,8 +538,10 @@ func (bnc *BaseNetworkController) addLogicalPortToNetwork(pod *kapi.Pod, nadName
 	// it for the default network as well. If at all possible, keep them
 	// functionally equivalent going forward.
 	var annotationUpdated bool
-	if bnc.IsSecondary() {
+	if bnc.IsSecondary() || bnc.IsPrimaryNetwork() {


might want to revisit the definition of a primary net; right now, a user defined primary network is a secondary net. So this added or condition is not required.

maiqueb · 2024-06-21T11:45:44Z

go-controller/pkg/ovn/base_network_controller_secondary.go

+		}
+		return bsnc.ensurePodForUserDefinedPrimaryNet(pod, network)
+	}
+
 	on, networkMap, err := util.GetPodNADToNetworkMapping(pod, bsnc.NetInfo)


an alternative would be to set the dummy NSE created above in this map. Then it would just munch it and spit out the LS+LSP for the user-defined primary network.

maiqueb · 2024-06-21T11:49:47Z

go-controller/pkg/cni/cni.go

+		pr.nadName, func(annotations map[string]string, nadName string) (*util.PodAnnotation, bool) {
+			// TODO: we need to simplify this function.
+			annotation, isReady := annotCondFn(annotations, nadName)
+			if nadName == "default" {


this thing below should only happen IIF the feature flag allows it.

Since this runs on ovnkube, I think we should be able to have access to that.

maiqueb · 2024-06-21T11:56:21Z

go-controller/pkg/cni/cni.go

+		if hasUserDefinedPrimaryNetwork && userDefinedNetPodInfo != nil {
+			klog.Infof("creating the user defined net request for %q on pod %q", userDefinedNetworkName, podName)
+			userDefinedRequest := newUserDefinedPrimaryNetworkPodRequest(
+				pr.Command,
+				pod,
+				pr.SandboxID,
+				pr.Netns,
+				"sdn1", // TODO: need to find a name here. This one is OK I guess
+				userDefinedNetworkName,
+				userDefinedNetPodInfo.NADName,
+			)
+			if _, err := userDefinedRequest.getCNIResult(clientset, userDefinedNetPodInfo); err != nil {
+				return nil, err
+			}
+		}


if we're on unpriviledged more, this code right here must happen in the CNI shim.

This means that probably the CNI server side (running in ovnkube-controller) should reply to the client (shim) w/ whatever extra info it needs, thus avoiding reading more stuff on the shim.

maiqueb · 2024-06-21T12:04:47Z

go-controller/pkg/clustermanager/pod/allocator.go

maybe this is not exactly required / something else is missing, but I can say that if this commit is not present, you'd miss the default route below:

default via 10.128.0.1 dev sdn1 10.96.0.0/16 via 10.128.0.1 dev sdn1 10.128.0.0/16 dev sdn1 proto kernel scope link src 10.128.0.120 10.244.0.0/16 via 10.244.2.1 dev eth0 10.244.2.0/24 dev eth0 proto kernel scope link src 10.244.2.28 100.64.0.0/16 via 10.244.2.1 dev eth0

tssurya · 2024-07-01T09:58:02Z

#4473 will superceed this PR

tssurya · 2024-07-08T10:50:36Z

closing in favor of #4473 where @maiqueb and @qinqon are working together to get it prod ready

qinqon and others added 11 commits June 18, 2024 14:48

rbac: Allow controller manager to patch ns

56aaf25

At IC the controller manager will be responsible of of patching namespace with active-network, so it needs permissions to do so. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

util: Add namespace active-network updater

8ccc653

Signed-off-by: Enrique Llorente <ellorent@redhat.com>

nad, controller: Add manager system

11f542e

Add a net attach def manager system on react on creation/deletion of NAds, this will be used to attach at network segmentation namespace manager that reacts on NAD events. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

gh: Add network-segmentation lane

cb57eb8

Signed-off-by: Enrique Llorente <ellorent@redhat.com>

nad: Handler primary network deletion

81fc424

This change add the following scenarios: - Delete a duplicate primary network nad - Delete primary network nad with no pods - Delete primary network nad with pods Signed-off-by: Enrique Llorente <ellorent@redhat.com>

user-def-nets: patch OVN annotations

a6d6703

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

e2e: remove unused function

9d40816

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

github-actions bot added area/unit-testing Issues related to adding/updating unit tests area/e2e-testing labels Jun 20, 2024

maiqueb commented Jun 20, 2024

View reviewed changes

maiqueb added 5 commits June 21, 2024 12:27

e2e, user-defined-nets: add east/west test

21c6486

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

user-def-nets, node: create LSP for primary net

c0d1609

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

user-def-nets, cni: configure the additional net iface

b2ad906

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

user-def-nets, control-plane: set default GW on the user defined net

707485d

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

maiqueb force-pushed the user-defined-primary-network-canary-impl branch from 74245dc to cb92150 Compare June 21, 2024 10:28

tssurya added feature/user-defined-network-segmentation All PRs related to User defined network segmentation area/core-kubernetes Issues related to core kubernetes like pods, services, endpoints, endpointslices labels Jun 21, 2024

tssurya added the feature/pods All issues related to the PodAPI label Jun 21, 2024

maiqueb commented Jun 21, 2024

View reviewed changes

tssurya mentioned this pull request Jul 5, 2024

UDN: Use synthetic network selection element #4473

Merged

tssurya closed this Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User defined primary network canary impl #4459

User defined primary network canary impl #4459

maiqueb commented Jun 20, 2024 •

edited

Loading

maiqueb Jun 20, 2024

tssurya Jun 21, 2024

maiqueb Jun 20, 2024 •

edited

Loading

maiqueb Jun 20, 2024 •

edited

Loading

maiqueb Jun 20, 2024

maiqueb Jun 20, 2024

qinqon Jun 20, 2024 •

edited

Loading

maiqueb Jun 20, 2024

qinqon Jun 20, 2024

qinqon Jun 20, 2024

qinqon Jun 20, 2024

maiqueb Jun 20, 2024

maiqueb Jun 20, 2024

maiqueb Jun 20, 2024

maiqueb Jun 20, 2024

maiqueb Jun 20, 2024

maiqueb Jun 20, 2024

maiqueb Jun 21, 2024

maiqueb Jun 21, 2024

maiqueb Jun 21, 2024

maiqueb Jun 21, 2024

maiqueb Jun 21, 2024

maiqueb Jun 21, 2024

tssurya commented Jul 1, 2024

tssurya commented Jul 8, 2024


		// TODO: this should be a watch grabbed from the watch factory
		nadClient networkattchmentdefclientset.Interface

		// TODO: we need to get the NAD name here, not the network name.
		Name: bsnc.GetNetworkName(),

		name: userDefinedNetworkName,
		networkName: userDefinedNetworkName,

User defined primary network canary impl #4459

User defined primary network canary impl #4459

Conversation

maiqueb commented Jun 20, 2024 • edited Loading

What this PR does and why is it needed

Which issue(s) this PR 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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

maiqueb Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinqon Jun 20, 2024 • edited Loading

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

tssurya commented Jul 1, 2024

tssurya commented Jul 8, 2024

maiqueb commented Jun 20, 2024 •

edited

Loading

maiqueb Jun 20, 2024 •

edited

Loading

maiqueb Jun 20, 2024 •

edited

Loading

qinqon Jun 20, 2024 •

edited

Loading