Merge pull request #113245 from pacoxu/dup-ports-warning

add warning for dup ports in containers[*].ports and service.ports

pkg/api/pod/warnings.go

@@ -267,6 +267,42 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta
 		return true
 	})
 
+	type portBlock struct {
+		field *field.Path
+		port  api.ContainerPort
+	}
+
+	// Accumulate ports across all containers
+	allPorts := map[string][]portBlock{}
+	pods.VisitContainersWithPath(podSpec, fieldPath.Child("spec"), func(c *api.Container, fldPath *field.Path) bool {
+		for i, port := range c.Ports {
+			if port.HostIP != "" && port.HostPort == 0 {
+				warnings = append(warnings, fmt.Sprintf("%s: hostIP set without hostPort: %+v",
+					fldPath.Child("ports").Index(i), port))
+			}
+			k := fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol)
+			if others, found := allPorts[k]; found {
+				// Someone else has this protcol+port, but it still might not be a conflict.
+				for _, other := range others {
+					if port.HostIP == other.port.HostIP && port.HostPort == other.port.HostPort {
+						// Exactly-equal is obvious. Validation should already filter for this except when these are unspecified.
+						warnings = append(warnings, fmt.Sprintf("%s: duplicate port definition with %s", fldPath.Child("ports").Index(i), other.field))
+					} else if port.HostPort == 0 || other.port.HostPort == 0 {
+						// HostPort = 0 is redundant with any other value, which is odd but not really dangerous.  HostIP doesn't matter here.
+						warnings = append(warnings, fmt.Sprintf("%s: overlapping port definition with %s", fldPath.Child("ports").Index(i), other.field))
+					} else if a, b := port.HostIP == "", other.port.HostIP == ""; port.HostPort == other.port.HostPort && ((a || b) && !(a && b)) {
+						// If the HostPorts are the same and either HostIP is not specified while the other is not, the behavior is undefined.
+						warnings = append(warnings, fmt.Sprintf("%s: dangerously ambiguous port definition with %s", fldPath.Child("ports").Index(i), other.field))
+					}
+				}
+				allPorts[k] = append(allPorts[k], portBlock{field: fldPath.Child("ports").Index(i), port: port})
+			} else {
+				allPorts[k] = []portBlock{{field: fldPath.Child("ports").Index(i), port: port}}
+			}
+		}
+		return true
+	})
+
 	// warn if the terminationGracePeriodSeconds is negative.
 	if podSpec.TerminationGracePeriodSeconds != nil && *podSpec.TerminationGracePeriodSeconds < 0 {
 		warnings = append(warnings, fmt.Sprintf("%s: must be >= 0; negative values are invalid and will be treated as 1", fieldPath.Child("spec", "terminationGracePeriodSeconds")))

pkg/api/pod/warnings_test.go

@@ -145,6 +145,7 @@ func TestWarnings(t *testing.T) {
 	testcases := []struct {
 		name        string
 		template    *api.PodTemplateSpec
+		oldTemplate *api.PodTemplateSpec
 		expected    []string
 	}{
 		{
@@ -617,11 +618,455 @@ func TestWarnings(t *testing.T) {
 				`spec.affinity.podAntiAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: a null labelSelector results in matching no pod`,
 			},
 		},
+		{
+			name: "container no ports",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name:  "foo",
+					Ports: []api.ContainerPort{},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "one container, one port",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "one container, two ports, same protocol, different ports",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+						{ContainerPort: 81, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "one container, two ports, different protocols, same port",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+						{ContainerPort: 80, Protocol: api.ProtocolTCP},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "one container, two ports, same protocol, same port, different hostport",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 80},
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 81},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "one container, two ports, same protocol, port and hostPort, different hostIP",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 80, HostIP: "10.0.0.1"},
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 80, HostIP: "10.0.0.2"},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "two containers, one port each, same protocol, different ports",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 81, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "two containers, one port each, different protocols, same port",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolTCP},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "two containers, one port each, same protocol, same port, different hostport",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 80},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 81},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "two containers, one port each, same protocol, port and hostPort, different hostIP",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 80, HostIP: "10.0.0.1"},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolTCP, HostPort: 80, HostIP: "10.0.0.2"},
+					},
+				}},
+			}},
+			expected: []string{},
+		},
+		{
+			name: "duplicate container ports with same port and protocol",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: duplicate port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "duplicate container ports with same port, hostPort and protocol",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: duplicate port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "duplicate container ports with same port, host port, host IP and protocol",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: duplicate port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "one container port hostIP set without host port set",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[0]: hostIP set without hostPort: {Name: HostPort:0 ContainerPort:80 Protocol:UDP HostIP:10.0.0.1}`,
+			},
+		},
+		{
+			name: "duplicate container ports with one host port set and one without",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+						{ContainerPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: overlapping port definition with spec.containers[0].ports[0]`,
+				`spec.containers[0].ports[1]: hostIP set without hostPort: {Name: HostPort:0 ContainerPort:80 Protocol:UDP HostIP:10.0.0.1}`,
+			},
+		},
+		{
+			name: "duplicate container ports without one host IP set and two with",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.2"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: dangerously ambiguous port definition with spec.containers[0].ports[0]`,
+				`spec.containers[0].ports[2]: dangerously ambiguous port definition with spec.containers[0].ports[1]`,
+			},
+		},
+		{
+			name: "duplicate container ports with one host IP set and one without",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: dangerously ambiguous port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "duplicate containers with same port and protocol",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "duplicate containers with same port, hostPort and protocol",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "duplicate containers with same port, host port, host IP and protocol",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "duplicate containers with one host port set and one without",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[1].ports[0]: overlapping port definition with spec.containers[0].ports[0]`,
+				`spec.containers[1].ports[0]: hostIP set without hostPort: {Name: HostPort:0 ContainerPort:80 Protocol:UDP HostIP:10.0.0.1}`,
+			},
+		},
+		{
+			name: "duplicate container ports without one host IP set and one with",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[1].ports[0]: dangerously ambiguous port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "duplicate container ports with one host IP set and one without",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name: "foo",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}, {
+					Name: "bar",
+					Ports: []api.ContainerPort{
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[1].ports[0]: dangerously ambiguous port definition with spec.containers[0].ports[0]`,
+			},
+		},
+		{
+			name: "create duplicate container ports in two containers",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{
+					{
+						Name: "foo1",
+						Ports: []api.ContainerPort{
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+							{ContainerPort: 180, HostPort: 80, Protocol: api.ProtocolUDP},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+						},
+					},
+					{
+						Name: "foo",
+						Ports: []api.ContainerPort{
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+						},
+					}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[2]: duplicate port definition with spec.containers[0].ports[0]`,
+				`spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`,
+				`spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[2]`,
+				`spec.containers[1].ports[1]: duplicate port definition with spec.containers[0].ports[0]`,
+				`spec.containers[1].ports[1]: duplicate port definition with spec.containers[0].ports[2]`,
+				`spec.containers[1].ports[1]: duplicate port definition with spec.containers[1].ports[0]`,
+			},
+		},
+		{
+			name: "update duplicate container ports in two containers",
+			template: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{
+					{
+						Name: "foo1",
+						Ports: []api.ContainerPort{
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolTCP},
+							{ContainerPort: 180, HostPort: 80, Protocol: api.ProtocolTCP},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolTCP},
+						},
+					},
+					{
+						Name: "foo",
+						Ports: []api.ContainerPort{
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolTCP},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolTCP},
+						},
+					}},
+			}},
+			oldTemplate: &api.PodTemplateSpec{Spec: api.PodSpec{
+				Containers: []api.Container{
+					{
+						Name: "foo1",
+						Ports: []api.ContainerPort{
+							{ContainerPort: 80, HostPort: 180, Protocol: api.ProtocolTCP},
+							{ContainerPort: 180, HostPort: 80, Protocol: api.ProtocolTCP},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolTCP},
+						},
+					},
+					{
+						Name: "foo",
+						Ports: []api.ContainerPort{
+							{ContainerPort: 80, HostPort: 180, Protocol: api.ProtocolTCP},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolTCP},
+						},
+					}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[2]: duplicate port definition with spec.containers[0].ports[0]`,
+				`spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`,
+				`spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[2]`,
+				`spec.containers[1].ports[1]: duplicate port definition with spec.containers[0].ports[0]`,
+				`spec.containers[1].ports[1]: duplicate port definition with spec.containers[0].ports[2]`,
+				`spec.containers[1].ports[1]: duplicate port definition with spec.containers[1].ports[0]`,
+			},
+		},
 	}
 
 	for _, tc := range testcases {
 		t.Run("podspec_"+tc.name, func(t *testing.T) {
-			actual := sets.NewString(GetWarningsForPodTemplate(context.TODO(), nil, tc.template, &api.PodTemplateSpec{})...)
+			var oldTemplate *api.PodTemplateSpec
+			if tc.oldTemplate != nil {
+				oldTemplate = tc.oldTemplate
+			}
+			actual := sets.NewString(GetWarningsForPodTemplate(context.TODO(), nil, tc.template, oldTemplate)...)
 			expected := sets.NewString(tc.expected...)
 			for _, missing := range expected.Difference(actual).List() {
 				t.Errorf("missing: %s", missing)