NPL(node Port Local) doesn't works for Multi-Port Services objects

[tamilhce]

Describe the bug
For the service object with multiple ports, the NPL pod annotations are created for one of the port. Not for the all the ports

To Reproduce
step1 : created SVC object with Multiple Ports

apiVersion: v1
kind: Service
metadata:
  annotations:
    nodeportlocal.antrea.io/enabled: "true"
  labels:
    app: nginx
  name: svc-nginx
spec:
  ports:
  - name: web
    port: 80
    protocol: TCP
    targetPort: 8080
  - name: web-sec
    port: 443
    protocol: TCP
    targetPort: 8081
  selector:
    app: nginx

root@tvk-k8s119-ant-master-1:~# kubectl get svc -o wide
NAME         TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)          AGE    SELECTOR
svc-nginx    ClusterIP   10.96.219.65   <none>        80/TCP,443/TCP   101m   app=nginx

step2 : observed that NPL annotations are created only for the targetPort 8080 and it doesn't got created for the targetPorts/PodPorts 8081. As per the Expectation the NPL annotations should be added all the targetPorts/PodPorts.

root@tvk-k8s119-ant-master-1:~# kubectl get pods -o=jsonpath="{range .items[*]}{'\n'}{.metadata.name}{':\t'}{.metadata.annotations}{end}"

nginx-deployment-b68c6d696-frrfj:	{"nodeportlocal.antrea.io":"[{\"podPort\":8080,\"nodeIP\":\"192.168.11.14\",\"nodePort\":50001}]"}
nginx-deployment-b68c6d696-qb2j4:	{"nodeportlocal.antrea.io":"[{\"podPort\":8080,\"nodeIP\":\"192.168.11.14\",\"nodePort\":50000}]"}
nginx-deployment-b68c6d696-qtzm7:	{"nodeportlocal.antrea.io":"[{\"podPort\":8080,\"nodeIP\":\"192.168.11.14\",\"nodePort\":50002}]"}
nginx-deployment-b68c6d696-xtg5t:	{"nodeportlocal.antrea.io":"[{\"podPort\":8080,\"nodeIP\":\"192.168.11.15\",\"nodePort\":50000}]"}root@tvk-k8s119-ant-master-1:~#

Expected
As per the Expectation the NPL annotations should be added for the all the targetPorts/Pod Ports.

Versions:
Please provide the following information:

• Antrea version (v0.13.0).
• Kubernetes version (1.19)

[antoninbas]

Do the Pods selected by your Service have 2 container ports (8080 and 8081)? Could you share the yaml for your Deployment?

I am not able to reproduce with the following yaml:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: my-nginx
spec:
  selector:
    matchLabels:
      app: nginx
  replicas: 3
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: my-nginx
        image: nginx
        ports:
        - containerPort: 8080
        - containerPort: 8081
---
apiVersion: v1
kind: Service
metadata:
  annotations:
    nodeportlocal.antrea.io/enabled: "true"
  labels:
    app: nginx
  name: svc-nginx
spec:
  ports:
  - name: web
    port: 80
    protocol: TCP
    targetPort: 8080
  - name: web-sec
    port: 443
    protocol: TCP
    targetPort: 8081
  selector:
    app: nginx

$ kubectl get pods -o=jsonpath="{range .items[*]}{'\n'}{.metadata.name}{':\t'}{.metadata.annotations}{end}"

my-nginx-9494bb7df-m4txm:	{"nodeportlocal.antrea.io":"[{\"podPort\":8080,\"nodeIP\":\"192.168.77.101\",\"nodePort\":40004},{\"podPort\":8081,\"nodeIP\":\"192.168.77.101\",\"nodePort\":40005}]"}
my-nginx-9494bb7df-pxftm:	{"nodeportlocal.antrea.io":"[{\"podPort\":8080,\"nodeIP\":\"192.168.77.101\",\"nodePort\":40000},{\"podPort\":8081,\"nodeIP\":\"192.168.77.101\",\"nodePort\":40001}]"}
my-nginx-9494bb7df-xscwc:	{"nodeportlocal.antrea.io":"[{\"podPort\":8080,\"nodeIP\":\"192.168.77.101\",\"nodePort\":40002},{\"podPort\":8081,\"nodeIP\":\"192.168.77.101\",\"nodePort\":40003}]"}

[tamilhce]

My Mistake, Actually My container spec has only port 8080 exposed, once i added the port 8081, the NPL annotations are added as expected

apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx-deployment
  labels:
    app: nginx
spec:
  replicas: 4
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx
        image: 10.79.204.3:5001/service-chain-apps
        ports:
        - containerPort: 8080

Just one doubt here, even though the container port 8081 is not mentioned in the POD container spec(container port in the pod spec is optional, there are chances that apps can be deployed without mentioning the container port in the POD), Nothing stops us from creating SVC object with the targetPort 8081, By default containers ports are exposed and it works fine. Can we create the NPL annotations for the POD based on the svc object Target Ports.

[antoninbas]

@tamilhce I tend to agree with you. I am not sure why the current implementation is using the (optional) containerPorts from the Pod spec, as opposed to the targetPorts from the Service spec. It may be because of the initial NPL implementation in Antrea: NPL had to be enabled for each Pod using an annotation, as opposed to via an annotation on the Service selecting the Pods.

I would be open to making this change, but I think we need to hear from the folks who designed & implemented the NPL feature: @sudswasavi @monotosh-avi @hemantavi @chauhanshubham

[monotosh-avi]

@antoninbas @tamilhce
We had explored few options while designing NPL (https://docs.google.com/document/d/1SoAKBvbbt2QMnGmt53E8ja0ZxCv_T4r1zLVnMzpykTg), one option was creating a new CRD - NPLEndpointSlice which would follow the default Endpoint slice implementation and use target port for service. The other option - which we implemented, was annotating Pod with Container Ports of the Pod. In case of Endpoint or EndpointSlice, the objects are created per Service, so the target port fits perfectly. But the Pod annotation is tied to a Pod, if a Pod is selected by multiple Services, it can get complicated.

If we stick with current implementation, the user have to use ContainerPorts in the Pod Spec, which should be possible as the app developer should know about the Ports used by the application.

With the target port based approach, one option is to ignore Container Ports and select a union of all target ports from all Services selecting a Pod, and then adding that in Pod Annotation. But NPLEndpointSlice is probably more suited for target port based rule provisioning.

[antoninbas]

With the target port based approach, one option is to ignore Container Ports and select a union of all target ports from all Services selecting a Pod, and then adding that in Pod Annotation. But NPLEndpointSlice is probably more suited for target port based rule provisioning.

This seems like the most sensible solution to me. I'm not sure why you think this solution is more complicated. In the current implementation, whenever we process a Pod, we already iterate through all Services:
https://github.com/vmware-tanzu/antrea/blob/347de8266882f92dea4654be980b27dedda63881/pkg/agent/nodeportlocal/k8s/npl_controller.go#L263-L281

It would be easy to have this function return the list of NPL-enabled Services selecting the Pod, which can then be used to compute the union of all target ports, as you suggested. Why do you think things would be more complicated with this solution? Would it be more complicated for users trying to use NPL, from an implementation perspective, or for external LBs trying to consume the mapping information?

I think the Service target port approach is better from a usability perspective. The discussion on Pod annotation vs NPLEndpointSlice is not as much about usability, and more about integrating with LBs. If necessary, we could include more information in the NPL Pod annotation, e.g. for each mapped port we could include the list of Services consuming the port.

[monotosh-avi]

@antoninbas I was concerned about the implementation in case we need to keep track of a target port being used by multiple services, but looks like it won't involve extensive changes. In any case, we should be able to implement this.

About the usability improvement, I think we don't need the service name in the Pod annotation, because in that case the annotation can get big, if a Pod is being referred by a large number of services. Because of this issue and as Annotation is unstructured, I thought NPLEndpointSlice becomes useful, where we could define our own spec.

For now, we can include the target Port in Pod annotation without the service name, the external LB should be able to derive the mapping by looking at the Service and Pod Annotation.