Validate "ownership" of hostPort service being deleted

[yasz24]

This addresses a bug where deleting a completed pod would
delete the service map entry for the corresponding running pod on
the same node. This is due to the hostPort mapping keys being the
same for both old and new pods.

Ideally we should validate whether the (completed) pod being deleted
"owns" the host port service -- in order to prevent breaking host port
connectivity for any running pods with the same service as frontend.

This PR adds such a validation.

Testing-
Automated (Control Plane):
This fix is captured by a control plane test case that does the
following:

1. Create a hostport pod, and terminate it's running containers to mark
   it as "Completed".
2. Create another hostport pod using the same port as the "Completed" pod.
3. Delete the "Completed" pod, and verify that the hostport service has
   not been deleted in the Datapath.

Manual Testing -

1. Add the GracefulNodeShutdown in the kubelet config on all nodes by
   modifying the configuration in /var/lib/kubelet/config.yaml

    featureGates:
      GracefulNodeShutdown: true
    shutdownGracePeriod: 30s
    shutdownGracePeriodCriticalPods: 10s
   

2. Run sudo systemctl restart kubelet on each node to apply the kubelet
   config change
3. Deploy an nginx web server with hostPort set, as well as a
   nodeSelector, so pods get scheduled on the same node after node
   restarts.

   apiVersion: apps/v1
   kind: Deployment
   metadata:
     name: nginx-deployment
   spec:
     selector:
       matchLabels:
         app: nginx
    replicas: 1
    template:
      metadata:
        labels:
          app: nginx
      spec:
        nodeSelector:
      kubernetes.io/hostname: <node-name>
    containers:
    - name: nginx
          image: nginx:1.14.2
          ports:
          - containerPort: 80
            hostPort: 8081
   

4. Run systemctl reboot on the worker node to restart the machine.
5. After reboot spot the old pod in Completed state, while the new pod
   is Running.

   $ kubectl get pods
   NAME                                READY   STATUS      RESTARTS    AGE
   nginx-deployment-645797c867-8p2hp   0/1     Completed   0           13m
   nginx-deployment-645797c867-dx2m8   1/1     Running     0           4m2s
   

6. curl nodeIP:hostPort successfully get the result.
7. Manually deleted the old pod which is in Completed state.

   $ kubectl delete pod/nginx-deployment-645797c867-8p2hp
   

8. Redo the curl nodeIP:hostPort, and successfully get the result again. // hostPort service has been preserved.

Fixes: #22460

Signed-off-by: Yash Shetty yashshetty@google.com

[maintainer-s-little-helper]

Commit 704e71af6c06ac5edc5dae6f0babf71d4e012466 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[maintainer-s-little-helper]

Commits 704e71af6c06ac5edc5dae6f0babf71d4e012466, 16b827c2c54f8ff20a20658a61f685d2bbc6bd67 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[aanm]

Thank you for the PR. What happens in case there is only one pod running on the node and that pod is removed? IF we are skipping the generation of service mappings will the entries in the bpf maps be deleted?

[aditighag]

👋 The fix doesn't look right to me, see the inline comment.

[yasz24]

Thank you for the PR. What happens in case there is only one pod running on the node and that pod is removed? IF we are skipping the generation of service mappings will the entries in the bpf maps be deleted?

If the single pod on the node is in running status, we would delete the map entries. But, as pointed out, we might still want to delete service map entries for completed status pods that were not hostPort (pods associated with completed jobs, etc.), so we'll need to find another way to fix the hostPort connectivity breaking i guess. Any suggestions for a possible approach would be really appreciated!

[squeed]

One possible option - though it's a pretty significant refactor - is to manage HostPort mappings via CNI capability arguments rather than via pod objects. This would then serialize hostport mappings via the kubelet.

[squeed]

I also wonder if we can elide the pod deletion entirely if the pod is newly "Completed" when we first see it?

[yasz24]

One possible option - though it's a pretty significant refactor - is to manage HostPort mappings via CNI capability arguments rather than via pod objects. This would then serialize hostport mappings via the kubelet.

@squeed IIUC, the issue isn't really caused by a race condition, so serializing the hostport mappings wouldn't help us I believe. I think what we really need is a way to know if the hostPort mappings "belong" to the "completed" pod being deleted or not. One way to do that would be to inspect the hostPort mappings for the service in question, and verifying that the IP for the "completed" pod is one of the backends. But I wasn't able to find any APIs to do a simple lookup for a service's hostPort mappings to verify this (GetService or similar on the svcManager could help accomplish this). Wondering if it would be okay to add such a method on the interface?

[maintainer-s-little-helper]

Commit b5e248597f70f568cc41b78a9bcbd79bbf61a9a0 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[maintainer-s-little-helper]

Commit b5e248597f70f568cc41b78a9bcbd79bbf61a9a0 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[aojea]

lgtm, two nits only about the the empty line in the end of two files

[squeed]

nice work!

[nebril]

Thanks for the PR! LGTM overall, please add the missing unit test, otherwise it's good to go!

[aanm]

/test

[borkmann]

@yasz24 looks like the PR broke Go code precheck test on master, could you take a look and fix? Thanks

https://github.com/cilium/cilium/actions/runs/5483316043/jobs/9989522587

[...]
contrib/scripts/lock-check.sh
contrib/scripts/check-viper.sh
contrib/scripts/custom-vet-check.sh
-: # github.com/cilium/cilium/test/controlplane/pod/hostport
Error: test/controlplane/pod/hostport/hostport.go:39:2: not enough arguments in call to test.UpdateObjectsFromFile(abs("init.yaml")).SetupEnvironment(modConfig).StartAgent
	have ()
	want (func(*"github.com/cilium/cilium/pkg/option".DaemonConfig), ...cell.Cell)
Error: test/controlplane/pod/hostport/hostport.go:41:20: too many arguments in call to test.UpdateObjectsFromFile(abs("init.yaml")).SetupEnvironment
	have (func(daemonCfg *"github.com/cilium/cilium/pkg/option".DaemonConfig, _ *"github.com/cilium/cilium/operator/option".OperatorConfig))
	want ()
Error: /home/runner/work/cilium/cilium/src/github.com/cilium/cilium/test/controlplane/pod/hostport/hostport.go:41:20: too many arguments in call to test.UpdateObjectsFromFile(abs("init.yaml")).SetupEnvironment
	have (func(daemonCfg *"github.com/cilium/cilium/pkg/option".DaemonConfig, _ *"github.com/cilium/cilium/operator/option".OperatorConfig))
	want ()
Error: /home/runner/work/cilium/cilium/src/github.com/cilium/cilium/test/controlplane/pod/hostport/hostport.go:42:[14](https://github.com/cilium/cilium/actions/runs/5483316043/jobs/9989522587#step:4:15): not enough arguments in call to test.UpdateObjectsFromFile(abs("init.yaml")).SetupEnvironment(modConfig).StartAgent
	have ()
	want (func(*"github.com/cilium/cilium/pkg/option".DaemonConfig), ...cell.Cell)
customvet: 3 errors during loading
exit status 1
make: *** [Makefile:653: precheck] Error 1
Error: Process completed with exit code 2.

[pippolo84]

@yasz24 looks like the PR broke Go code precheck test on master, could you take a look and fix? Thanks

Fix is very simple, here the PR.