Repository: kubernetes/kubernetes (language: go)

Issue:
CRI and networking behavior
### What happened?

This is a follow up to [this Slack thread](https://kubernetes.slack.com/archives/C09QYUH5W/p1740368655309169). I can't say that I understand why, but I'm told this behavior is unexpected: the container runtime sometimes gets a `RunPodSandboxRequest` where the host port is zero.

### What did you expect to happen?

I believe @aojea wants to "check if we are sending that info on purpose through the CRI API or is just an oversight".

I think more importantly, the CRI is a bit under-documented here. The [documentation of `host_port`](https://github.com/kubernetes/cri-api/blob/v0.32.2/pkg/apis/runtime/v1/api.proto#L204) is just `Port number on the host. Default: 0 (not specified)`, which reveals basically nothing about how an implementation might handle a zero value for that field.

### How can we reproduce it (as minimally and precisely as possible)?

This can be reproduced with minikube, Istio, and the Gateway API. It can probably be reproduced in other ways, too, but this is the most straightforward I came up with:

```bash
cd minikube  # https://github.com/kubernetes/minikube
git checkout v1.35.0
# Apply this 1-line patch so we can see `PodConfig` in the logs:
git apply patch
make local-kicbase-debug && make

# The problem can be illustrated most easily by booting up Istio and using the Gateway API
# (not sure why), but I was also observing host_port = 0 for my application pods.
./out/minikube start --container-runtime=containerd --memory=16384 --cpus=4
istioctl install --set profile=ambient --skip-confirmation
kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.2.0/standard-install.yaml
```

Where `patch` is this:

```diff
diff --git a/deploy/kicbase/containerd.toml b/deploy/kicbase/containerd.toml
index 873cee549..8bc7bf048 100644
--- a/deploy/kicbase/containerd.toml
+++ b/deploy/kicbase/containerd.toml
@@ -15,7 +15,7 @@ oom_score = 0
   address = ""
   uid = 0
   gid = 0
-  level = ""
+  level = "debug"

 [metrics]
   address = ""
```

Once you run the above, you can check the log by running:

```bash
docker exec -it minikube journalctl -u containerd | grep 'Sandbox config' | grep HostPort
```

You can see that a bunch of pods are run with a port mapping where `HostPort` is zero.

### Anything else we need to know?

_No response_

### Kubernetes version

<details>

```console
Client Version: v1.32.1
Kustomize Version: v5.5.0
Server Version: v1.32.0
```

</details>


### Cloud provider

None


### OS version

<details>

```console
NAME="Manjaro Linux"
PRETTY_NAME="Manjaro Linux"
ID=manjaro
ID_LIKE=arch
BUILD_ID=rolling
ANSI_COLOR="32;1;24;144;200"
HOME_URL="https://manjaro.org/"
DOCUMENTATION_URL="https://wiki.manjaro.org/"
SUPPORT_URL="https://forum.manjaro.org/"
BUG_REPORT_URL="https://docs.manjaro.org/reporting-bugs/"
PRIVACY_POLICY_URL="https://manjaro.org/privacy-policy/"
LOGO=manjarolinux
```

</details>


### Install tools

<details>

</details>


### Container runtime (CRI) and version (if applicable)

<details>

`containerd containerd.io 1.7.25 bcc810d6b9066471b0b6fa75f557a15a1cbf31bb`

</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>

</details>



Discussion / hints:
/sig network

For reference, some detail of the process from one of my [presentations about kubernetes networking](https://docs.google.com/presentation/d/14xNjy9ej8kGLkmSC7RsDO9-039vrwmf6sMofVOkqcD8/edit?usp=sharing)

![Image](https://github.com/user-attachments/assets/fade9f97-56ab-47ae-8e22-ea4907a1da91)

This is the method that calls the runtime from kubelet

https://github.com/kubernetes/kubernetes/blob/a18b4a8d97151a25c4d8088839d77e6ba482cac3/staging/src/k8s.io/cri-client/pkg/remote_runtime.go#L190-L205

That is called from 

https://github.com/kubernetes/kubernetes/blob/a18b4a8d97151a25c4d8088839d77e6ba482cac3/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go#L40-L42

The Pod config is generated here

https://github.com/kubernetes/kubernetes/blob/a18b4a8d97151a25c4d8088839d77e6ba482cac3/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go#L80

and port maps are added here to the config we send to the runtimes

https://github.com/kubernetes/kubernetes/blob/a18b4a8d97151a25c4d8088839d77e6ba482cac3/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go#L117-L137

No place take into consideration that the HostPort is zero

https://github.com/kubernetes/kubernetes/blob/a18b4a8d97151a25c4d8088839d77e6ba482cac3/pkg/kubelet/container/helpers.go#L359-L395

So, this confirms we send to the CRI all the container ports information, but it seems we don't tell them what they are supposed to do if hostPort is zero

At first sight it seems a very impactful to stop sending the information:
- we must understand how CRIO (@mrunalp ) and Containerd (@MikeZappa87 ) are interpreting this information
- is there any other intention for these fields than implementing HostPorts on containers ( @thockin  may remember)
our code in the container runtime for mapping the cri port mappings to cni port mappings:

```
// toCNIPortMappings converts CRI port mappings to CNI.
func toCNIPortMappings(criPortMappings []*runtime.PortMapping) []cni.PortMapping {
	var portMappings []cni.PortMapping
	for _, mapping := range criPortMappings {
		if mapping.HostPort <= 0 {
			continue
		}
		portMappings = append(portMappings, cni.PortMapping{
			HostPort:      mapping.HostPort,
			ContainerPort: mapping.ContainerPort,
			Protocol:      strings.ToLower(mapping.Protocol.String()),
			HostIP:        mapping.HostIp,
		})
	}
	return portMappings
}
```
https://github.com/containerd/containerd/blob/3904a5761fadbb999d3fc1a7f7ab7cc6f6d7d850/internal/cri/server/sandbox_run.go#L443
of course host port zero is invalid.. typically used by client tools to request an unused port number ...


Ok, cri-o also filters hostPort 0

https://github.com/cri-o/cri-o/blob/5991aec3baf922445791cb6dbb9ed4522e251c16/server/sandbox_run.go#L73-L88

I think now is safe to say that container runtimes should not do anything if hostPort <= 0, so we need to update the API docs

Do we feel we should also filter them in the kubelet side?
I think is better to leave it as is today, there is more information related to container ports there and since this was this way for so long, based on [Hyrum's Law](https://www.hyrumslaw.com/) someone may depend on it these days

I think it would be an api decision if they want to support dynamically assigning a port number each time.. and adding that dynamically assigned port to the port mappings.. but that would require some way to get an assignment and have it mapped through.. maybe an NRI plugin.
nod on the backwards compat versioning issues.. This is the way we deal with it today.. we ignore it. I'd bet if you look through you'll see we ignore it to maintain backwards compatibility with old specs..
Agree - 0 must be interpreted as "none" and not "dynamic".  Realisitically, this should never have been sent to CRI if the value is 0 ("not specified") but it was.  We don't have any real plans for randomly-allocated hostports, but if we did it would need to be something other than "hostPort = 0"
So waht is the AI for this issue and who is on it?
Talked about this in the SIG Network meeting:

Originally it should have been an optional field (e.g. pointer in Go), but at this point we can't change that without breaking things. The expectation from hereon is that you have to "ignore 0".

Code Links: [1](https://github.com/kubernetes/kubernetes/blob/9fe95e7586c8778d3b33737a48b7fbf7fa6416f9/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go#L123), [2](https://github.com/containerd/containerd/blob/79309361be72cb679cb46bcb863a2d9b15dfe707/internal/cri/server/sandbox_run.go#L441), [3](https://github.com/kubernetes/cri-api/blob/3dd84b8b2b65ecdf2087863f2e028266cfa46dcb/pkg/apis/runtime/v1/api.proto#L205), [4](https://github.com/containerd/containerd/blob/79309361be72cb679cb46bcb863a2d9b15dfe707/internal/cri/server/sandbox_run.go#L509)

The thing to do here is improve our documentation. We should improve the api.proto documentation for this:

/help
@shaneutt: 
	This request has been marked as needing help from a contributor.

### Guidelines
Please ensure that the issue body includes answers to the following questions:
- Why are we solving this issue?
- To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
- Does this issue have zero to low barrier of entry?
- How can the assignee reach out to you for help?


For more details on the requirements of such an issue, please see [here](https://git.k8s.io/community/contributors/guide/help-wanted.md) and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the `/remove-help` command.


<details>

In response to [this](https://github.com/kubernetes/kubernetes/issues/130460):

>Talked about this in the SIG Network meeting:
>
>Originally it should have been an optional field (e.g. pointer in Go), but at this point we can't change that without breaking things. The expectation from hereon is that you have to "ignore 0".
>
>Code Links: [1](https://github.com/kubernetes/kubernetes/blob/9fe95e7586c8778d3b33737a48b7fbf7fa6416f9/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go#L123), [2](https://github.com/containerd/containerd/blob/79309361be72cb679cb46bcb863a2d9b15dfe707/internal/cri/server/sandbox_run.go#L441), [3](https://github.com/kubernetes/cri-api/blob/3dd84b8b2b65ecdf2087863f2e028266cfa46dcb/pkg/apis/runtime/v1/api.proto#L205), [4](https://github.com/containerd/containerd/blob/79309361be72cb679cb46bcb863a2d9b15dfe707/internal/cri/server/sandbox_run.go#L509)
>
>The thing to do here is improve our documentation. We should improve the api.proto documentation for this:
>
>/help


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
/triage accepted
> We don't have any real plans for randomly-allocated hostports, but if we did it would need to be something other than "hostPort = 0"

Really? AFAIK opening a socket with the port set to 0 is the only reliably way to get a randomly allocated port? This tells the OS you don't care and the OS is the only one that can find an open port without a race.

That ... actually seems like the correct way to ask for a random port.
Doesn't matter whether it makes sense -- using 0 to mean "not specified" dates all the way back. 

Fix patch:
diff --git a/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.go b/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.go
index 22c9c011ca798..0e151bc3e885f 100644
--- a/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.go
+++ b/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.go
@@ -555,7 +555,16 @@ type PortMapping struct {
 	Protocol Protocol `protobuf:"varint,1,opt,name=protocol,proto3,enum=runtime.v1.Protocol" json:"protocol,omitempty"`
 	// Port number within the container. Default: 0 (not specified).
 	ContainerPort int32 `protobuf:"varint,2,opt,name=container_port,json=containerPort,proto3" json:"container_port,omitempty"`
-	// Port number on the host. Default: 0 (not specified).
+	// Port number on the host to map the container port to.
+	//
+	//   - Valid host port range is 1-65535.
+	//   - The value 0 has explicit semantic meaning: it indicates NO host port should be allocated.
+	//   - The value 0 does NOT indicate dynamic port allocation. Future implementations
+	//     of dynamic allocation will use different values/semantics.
+	//   - Implementations MUST handle the case where this field is explicitly set to 0,
+	//     This field SHOULD be omitted when no port is required.
+	//
+	// Default: If omitted, container port will not be exposed on the host.
 	HostPort int32 `protobuf:"varint,3,opt,name=host_port,json=hostPort,proto3" json:"host_port,omitempty"`
 	// Host IP.
 	HostIp               string   `protobuf:"bytes,4,opt,name=host_ip,json=hostIp,proto3" json:"host_ip,omitempty"`
diff --git a/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto b/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto
index a19133001d02f..78398a6163a07 100644
--- a/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto
+++ b/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto
@@ -201,7 +201,16 @@ message PortMapping {
     Protocol protocol = 1;
     // Port number within the container. Default: 0 (not specified).
     int32 container_port = 2;
-    // Port number on the host. Default: 0 (not specified).
+    // Port number on the host to map the container port to.
+    //
+    // * Valid host port range is 1-65535.
+    // * The value 0 has explicit semantic meaning: it indicates NO host port should be allocated.
+    // * The value 0 does NOT indicate dynamic port allocation. Future implementations
+    //   of dynamic allocation will use different values/semantics.
+    // * Implementations MUST handle the case where this field is explicitly set to 0,
+    //   This field SHOULD be omitted when no port is required.
+    //
+    // Default: If omitted, container port will not be exposed on the host.
     int32 host_port = 3;
     // Host IP.
     string host_ip = 4;


Test patch: