Add label values into traffic in agent side and retrieve the values from traffic in transit side #478

Hong-Chang · 2021-05-05T23:34:51Z

What does this PR do?

Add necessary geneve option structs for label values
Add label values intro traffic in agent side
Retrieve label values from traffic in transit side

How was this tested?
I did manual e2e test and label values can be retrieved correctly in transit side from packet.

Are there any user facing / API changes?
No.

…om traffic in transit side

vinaykul · 2021-05-07T19:43:10Z

Can you see the labels on the wire? (e.g. tcpdump capture on the egress NIC of the node)
[Hong] The data is on the wire because it's correctly retrieved in transit side. I checked tcpdump data but it's difficult to recognize because there are many other data in the packet.

src/xdp/trn_agent_xdp.c

src/xdp/trn_transit_xdp.c

vinaykul

We should handle the cases where there are no labels, and cases where just one of the labels (pod / ns) are present as well.

[Hong] label value = 0 covers the cases.

vinaykul · 2021-05-07T20:29:49Z

Please add unit tests.

[Hong] There is no unit test for this part (src/xdp/*). And this part was written in a way of non unit testable. Let's consider whether we should put some efforts to enforce unit test for this part.

src/xdp/trn_transit_xdp.c

vinaykul · 2021-05-07T20:57:37Z

Please add unit tests.

[Hong] There is no unit test for this part (src/xdp/*). And this part was written in a way of non unit testable. Let's consider whether we should put some efforts to enforce unit test for this part.

What do we have under mizar/src/tests and mizar/tests and mizar/testse2e ?
[Hong] I don't see mizar/src/tests. For mizar/tests, it's for python code. I don't think mizar/testse2e applies for this pr.

vinaykul · 2021-05-07T21:30:05Z

Please add unit tests.
[Hong] There is no unit test for this part (src/xdp/*). And this part was written in a way of non unit testable. Let's consider whether we should put some efforts to enforce unit test for this part.

What do we have under mizar/src/tests and mizar/tests and mizar/testse2e ?
[Hong] I don't see mizar/src/tests. For mizar/tests, it's for python code. I don't think mizar/testse2e applies for this pr.

In the description, can you please add tcpdump trace of packets captured on the wire for each of the 4 cases for simple ping test? Scenarios: No labels, both ns and pod labels, only pod label, only ns label.

You can create two pods so that they are scheduled on different worker nodes, and ping one pod from the other using this example yaml:

apiVersion: v1
kind: Pod
metadata:
  name: netpod1
  labels:
    podkey: netpodkey1
spec:
  affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: kubernetes.io/hostname
            operator: In
            values:
            - kind-worker
  restartPolicy: OnFailure
  terminationGracePeriodSeconds: 2
  containers:
  - name: netctr
    image: yuvalif/fedora-tcpdump
    command: ["tail", "-f", "/dev/null"]
---
apiVersion: v1
kind: Pod
metadata:
  name: netpod2
spec:
  affinity:
    podAntiAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
      - labelSelector:
          matchExpressions:
          - key: podkey
            operator: In
            values:
            - netpodkey1
        topologyKey: kubernetes.io/hostname
  restartPolicy: OnFailure
  terminationGracePeriodSeconds: 2
  containers:
  - name: netctr
    image: yuvalif/fedora-tcpdump
    command: ["tail", "-f", "/dev/null"]

Then look for ICMP packets doing tcpdump on the veth of the kind-node with the ping source pod. e.g

ubuntu on ip-192-168-1-59 in ~ 
❯ docker exec -ti $(docker ps | grep kind-worker$ | awk '{print $1}') ip a s | grep ": eth0"                   
1698: eth0@if1699: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9000 xdpgeneric/id:1877 qdisc noqueue state UP group default 

ubuntu on ip-192-168-1-59 in ~ 
❯ ip a s | grep "1699:"                                                                                                           
1699: veth53c269b@if1698: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9000 qdisc noqueue master br-fea389efa3d5 state UP group default 

ubuntu on ip-192-168-1-59 in ~ 
❯ kubectl get no -owide
NAME                 STATUS   ROLES    AGE   VERSION   INTERNAL-IP   EXTERNAL-IP   OS-IMAGE           KERNEL-VERSION   CONTAINER-RUNTIME
kind-control-plane   Ready    master   13m   v1.18.2   172.18.0.2    <none>        Ubuntu 20.04 LTS   5.6.0-rc2        containerd://1.3.3-14-g449e9269
kind-worker          Ready    <none>   13m   v1.18.2   172.18.0.3    <none>        Ubuntu 20.04 LTS   5.6.0-rc2        containerd://1.3.3-14-g449e9269
kind-worker2         Ready    <none>   13m   v1.18.2   172.18.0.4    <none>        Ubuntu 20.04 LTS   5.6.0-rc2        containerd://1.3.3-14-g449e9269

ubuntu on ip-192-168-1-59 in ~ 
> sudo tcpdump -nvXi veth53c269b ip dst 172.18.0.4

vinaykul · 2021-05-07T22:32:28Z

Please add unit tests.
[Hong] There is no unit test for this part (src/xdp/*). And this part was written in a way of non unit testable. Let's consider whether we should put some efforts to enforce unit test for this part.

What do we have under mizar/src/tests and mizar/tests and mizar/testse2e ?
[Hong] I don't see mizar/src/tests. For mizar/tests, it's for python code. I don't think mizar/testse2e applies for this pr.

Also, if you want to respond to my comment, please reply below or quote-reply. If you edit my comment, I don't get notification.

I don't think we should even have that permission to edit someone else's comments. Let me investigate that ..

phudtran

Looks ok to me

phudtran · 2021-05-08T02:15:54Z

src/xdp/trn_transit_xdp.c

+	}	
+
+	pkt->namespace_label_value_opt = (void *)pkt->pod_label_value_opt + sizeof(*pkt->pod_label_value_opt);
+	if (pkt->namespace_label_value_opt + 1 > pkt->data_end) {


Also check the option class like above

Also I see option type is not checked or used anywhere in transit xdp. Should we check here as well?

Added check for both class and type.

phudtran · 2021-05-08T02:16:06Z

src/xdp/trn_transit_xdp.c

@@ -729,6 +729,22 @@ static __inline int trn_process_geneve(struct transit_packet *pkt)
 		return XDP_ABORTED;
 	}

+	pkt->pod_label_value_opt = (void *)pkt->scaled_ep_opt + sizeof(*pkt->scaled_ep_opt);
+	if (pkt->pod_label_value_opt + 1 > pkt->data_end) {


Also check the option class like above

Same as above for option type comment.

Added check for both class and type.

Hong-Chang · 2021-05-10T23:42:27Z

Please add unit tests.
[Hong] There is no unit test for this part (src/xdp/*). And this part was written in a way of non unit testable. Let's consider whether we should put some efforts to enforce unit test for this part.

What do we have under mizar/src/tests and mizar/tests and mizar/testse2e ?
[Hong] I don't see mizar/src/tests. For mizar/tests, it's for python code. I don't think mizar/testse2e applies for this pr.

In the description, can you please add tcpdump trace of packets captured on the wire for each of the 4 cases for simple ping test? Scenarios: No labels, both ns and pod labels, only pod label, only ns label.

You can create two pods so that they are scheduled on different worker nodes, and ping one pod from the other using this example yaml:

apiVersion: v1
kind: Pod
metadata:
  name: netpod1
  labels:
    podkey: netpodkey1
spec:
  affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: kubernetes.io/hostname
            operator: In
            values:
            - kind-worker
  restartPolicy: OnFailure
  terminationGracePeriodSeconds: 2
  containers:
  - name: netctr
    image: yuvalif/fedora-tcpdump
    command: ["tail", "-f", "/dev/null"]
---
apiVersion: v1
kind: Pod
metadata:
  name: netpod2
spec:
  affinity:
    podAntiAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
      - labelSelector:
          matchExpressions:
          - key: podkey
            operator: In
            values:
            - netpodkey1
        topologyKey: kubernetes.io/hostname
  restartPolicy: OnFailure
  terminationGracePeriodSeconds: 2
  containers:
  - name: netctr
    image: yuvalif/fedora-tcpdump
    command: ["tail", "-f", "/dev/null"]

Then look for ICMP packets doing tcpdump on the veth of the kind-node with the ping source pod. e.g

ubuntu on ip-192-168-1-59 in ~ 
❯ docker exec -ti $(docker ps | grep kind-worker$ | awk '{print $1}') ip a s | grep ": eth0"                   
1698: eth0@if1699: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9000 xdpgeneric/id:1877 qdisc noqueue state UP group default 

ubuntu on ip-192-168-1-59 in ~ 
❯ ip a s | grep "1699:"                                                                                                           
1699: veth53c269b@if1698: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9000 qdisc noqueue master br-fea389efa3d5 state UP group default 

ubuntu on ip-192-168-1-59 in ~ 
❯ kubectl get no -owide
NAME                 STATUS   ROLES    AGE   VERSION   INTERNAL-IP   EXTERNAL-IP   OS-IMAGE           KERNEL-VERSION   CONTAINER-RUNTIME
kind-control-plane   Ready    master   13m   v1.18.2   172.18.0.2    <none>        Ubuntu 20.04 LTS   5.6.0-rc2        containerd://1.3.3-14-g449e9269
kind-worker          Ready    <none>   13m   v1.18.2   172.18.0.3    <none>        Ubuntu 20.04 LTS   5.6.0-rc2        containerd://1.3.3-14-g449e9269
kind-worker2         Ready    <none>   13m   v1.18.2   172.18.0.4    <none>        Ubuntu 20.04 LTS   5.6.0-rc2        containerd://1.3.3-14-g449e9269

ubuntu on ip-192-168-1-59 in ~ 
> sudo tcpdump -nvXi veth53c269b ip dst 172.18.0.4

I added a new issue and let's work on this later.
#480 Optimize Geneve option to be dynamic spaced

Hong-Chang

Addressed comments

Hong-Chang · 2021-05-11T00:09:31Z

src/xdp/trn_transit_xdp.c

+	}	
+
+	pkt->namespace_label_value_opt = (void *)pkt->pod_label_value_opt + sizeof(*pkt->pod_label_value_opt);
+	if (pkt->namespace_label_value_opt + 1 > pkt->data_end) {


Added check for both class and type.

Hong-Chang · 2021-05-11T00:11:35Z

src/xdp/trn_transit_xdp.c

@@ -729,6 +729,22 @@ static __inline int trn_process_geneve(struct transit_packet *pkt)
 		return XDP_ABORTED;
 	}

+	pkt->pod_label_value_opt = (void *)pkt->scaled_ep_opt + sizeof(*pkt->scaled_ep_opt);
+	if (pkt->pod_label_value_opt + 1 > pkt->data_end) {


Added check for both class and type.

vinaykul · 2021-05-12T01:38:56Z

/approve

vinaykul · 2021-05-12T01:40:20Z

@phudtran Does it look good to you now?

Add label values into traffic in agent side and retrive the values fr…

3677299

…om traffic in transit side

Hong-Chang added the area/networkPolicy label May 5, 2021

Hong-Chang added this to the Release 0.8 milestone May 5, 2021

Hong-Chang requested review from vinaykul, phudtran and clu2xlu May 5, 2021 23:34

Hong-Chang self-assigned this May 5, 2021

This was linked to issues May 5, 2021

Implement Label Based Network Policies - Add label values into traffic #452

Closed

Implement Label Based Network Policies - Get label values from traffic #453

Closed

vinaykul reviewed May 7, 2021

View reviewed changes

src/xdp/trn_agent_xdp.c Show resolved Hide resolved

vinaykul reviewed May 7, 2021

View reviewed changes

src/xdp/trn_agent_xdp.c Show resolved Hide resolved

vinaykul reviewed May 7, 2021

View reviewed changes

src/xdp/trn_transit_xdp.c Show resolved Hide resolved

vinaykul requested changes May 7, 2021

View reviewed changes

vinaykul reviewed May 7, 2021

View reviewed changes

src/xdp/trn_transit_xdp.c Show resolved Hide resolved

vinaykul requested a review from w-yue May 7, 2021 23:59

phudtran approved these changes May 10, 2021

View reviewed changes

Hong-Chang added 2 commits May 10, 2021 23:56

Address comments

643b7b5

Add geneve option check for type

ee6625e

Hong-Chang commented May 11, 2021

View reviewed changes

w-yue approved these changes May 11, 2021

View reviewed changes

vinaykul approved these changes May 12, 2021

View reviewed changes

vinaykul merged commit 6c290df into CentaurusInfra:dev-next May 12, 2021

Hong-Chang deleted the hc-p-2 branch May 14, 2021 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add label values into traffic in agent side and retrieve the values from traffic in transit side #478

Add label values into traffic in agent side and retrieve the values from traffic in transit side #478

Hong-Chang commented May 5, 2021

vinaykul commented May 7, 2021 •

edited by Hong-Chang

Loading

vinaykul left a comment •

edited by Hong-Chang

Loading

vinaykul commented May 7, 2021 •

edited by Hong-Chang

Loading

vinaykul commented May 7, 2021 •

edited by Hong-Chang

Loading

vinaykul commented May 7, 2021

vinaykul commented May 7, 2021

phudtran left a comment

phudtran May 8, 2021

w-yue May 10, 2021 •

edited

Loading

Hong-Chang May 11, 2021

phudtran May 8, 2021

w-yue May 10, 2021 •

edited

Loading

Hong-Chang May 11, 2021

Hong-Chang commented May 10, 2021

Hong-Chang left a comment

Hong-Chang May 11, 2021

Hong-Chang May 11, 2021

vinaykul commented May 12, 2021

vinaykul commented May 12, 2021

Add label values into traffic in agent side and retrieve the values from traffic in transit side #478

Add label values into traffic in agent side and retrieve the values from traffic in transit side #478

Conversation

Hong-Chang commented May 5, 2021

vinaykul commented May 7, 2021 • edited by Hong-Chang Loading

vinaykul left a comment • edited by Hong-Chang Loading

Choose a reason for hiding this comment

vinaykul commented May 7, 2021 • edited by Hong-Chang Loading

vinaykul commented May 7, 2021 • edited by Hong-Chang Loading

vinaykul commented May 7, 2021

vinaykul commented May 7, 2021

phudtran left a comment

Choose a reason for hiding this comment

phudtran May 8, 2021

Choose a reason for hiding this comment

w-yue May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Hong-Chang May 11, 2021

Choose a reason for hiding this comment

phudtran May 8, 2021

Choose a reason for hiding this comment

w-yue May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Hong-Chang May 11, 2021

Choose a reason for hiding this comment

Hong-Chang commented May 10, 2021

Hong-Chang left a comment

Choose a reason for hiding this comment

Hong-Chang May 11, 2021

Choose a reason for hiding this comment

Hong-Chang May 11, 2021

Choose a reason for hiding this comment

vinaykul commented May 12, 2021

vinaykul commented May 12, 2021

vinaykul commented May 7, 2021 •

edited by Hong-Chang

Loading

vinaykul left a comment •

edited by Hong-Chang

Loading

vinaykul commented May 7, 2021 •

edited by Hong-Chang

Loading

vinaykul commented May 7, 2021 •

edited by Hong-Chang

Loading

w-yue May 10, 2021 •

edited

Loading

w-yue May 10, 2021 •

edited

Loading