add warning for duplicate containers[*].ports with the same port, protocol, hostPort, and hostIP

pkg/api/pod/warnings.go

@@ -252,16 +252,33 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta
 				}
 			}
 		}
-		// duplicate containers[*].ports
+		return true
-		if len(c.Ports) > 1 {
+	})
-			ports := sets.NewInt64()
+
+	// 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 ports.Has(int64(port.ContainerPort)) {
+			k := fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol)
-					warnings = append(warnings, fmt.Sprintf("%s: duplicate container port %d", p.Child("ports").Index(i).Child("containerPort"), port.ContainerPort))
+			if other, found := allPorts[k]; found {
-				} else {
+				// Someone else has this protcol+port, but it still might not be a conflict.
-					ports.Insert(int64(port.ContainerPort))
+				conflict := false
+
+				// Exact matches are obvious.
+				if port.HostIP == other.port.HostIP && port.HostPort == other.port.HostPort {
+					conflict = true
+				}
+				// Unspecified fields are more subtle.
+				if port.HostPort == 0 || other.port.HostPort == 0 || port.HostIP == "" || other.port.HostIP == "" {
+					conflict = true
+				}
+				if conflict {
+					warnings = append(warnings, fmt.Sprintf("%s: duplicate container port %d/%s, conflicts with %s",
+						fldPath.Child("ports").Index(i), port.ContainerPort, port.Protocol, other.field))
+					continue
 				}
 			}
+			allPorts[k] = portBlock{field: fldPath, port: port}
 		}
 		return true
 	})
@@ -285,6 +302,11 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta
 	return warnings
 }
 
+type portBlock struct {
+	field *field.Path
+	port  api.ContainerPort
+}
+
 func warningsForPodAffinityTerms(terms []api.PodAffinityTerm, fieldPath *field.Path) []string {
 	var warnings []string
 	for i, t := range terms {

pkg/api/pod/warnings_test.go

@@ -24,6 +24,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	utilpointer "k8s.io/utils/pointer"
 )
 
 func BenchmarkNoWarnings(b *testing.B) {
@@ -144,6 +145,7 @@ func TestWarnings(t *testing.T) {
 	testcases := []struct {
 		name        string
 		template    *api.PodTemplateSpec
+		oldTemplate *api.PodTemplateSpec
 		expected    []string
 	}{
 		{
@@ -599,7 +601,17 @@ func TestWarnings(t *testing.T) {
 			},
 		},
 		{
-			name: "no duplicate container ports",
+			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",
@@ -610,52 +622,413 @@ func TestWarnings(t *testing.T) {
 			}},
 			expected: []string{},
 		},
-
 		{
-			name: "duplicate container ports",
+			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, HostPort: 80},
+						{ContainerPort: 80, Protocol: api.ProtocolUDP},
-						{ContainerPort: 80, HostPort: 80},
+						{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{
-				`spec.containers[0].ports[1].containerPort: duplicate container port 80`,
+				`spec.containers[0].ports[1]: duplicate container port 80/TCP, conflicts with spec.containers[0]`,
 			},
 		},
 		{
-			name: "duplicate container ports in two containers",
+			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{
+				`spec.containers[1].ports[0]: duplicate container port 80/TCP, conflicts with spec.containers[0]`,
+			},
+		},
+		{
+			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 container port 80/UDP, conflicts with spec.containers[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 container port 80/UDP, conflicts with spec.containers[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 container port 80/UDP, conflicts with spec.containers[0]`,
+			},
+		},
+		{
+			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]: duplicate container port 80/UDP, conflicts with spec.containers[0]`,
+			},
+		},
+		{
+			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},
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP, HostIP: "10.0.0.1"},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[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"},
+						{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
+					},
+				}},
+			}},
+			expected: []string{
+				`spec.containers[0].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[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 container port 80/UDP, conflicts with spec.containers[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 container port 80/UDP, conflicts with spec.containers[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 container port 80/UDP, conflicts with spec.containers[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]: duplicate container port 80/UDP, conflicts with spec.containers[0]`,
+			},
+		},
+		{
+			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]: duplicate container port 80/UDP, conflicts with spec.containers[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]: duplicate container port 80/UDP, conflicts with spec.containers[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},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
-							{ContainerPort: 180, HostPort: 80},
+							{ContainerPort: 180, HostPort: 80, Protocol: api.ProtocolUDP},
-							{ContainerPort: 80, HostPort: 80},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
 						},
 					},
 					{
 						Name: "foo",
 						Ports: []api.ContainerPort{
-							{ContainerPort: 80, HostPort: 80},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
-							{ContainerPort: 80, HostPort: 80},
+							{ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP},
 						},
 					}},
 			}},
 			expected: []string{
-				`spec.containers[0].ports[2].containerPort: duplicate container port 80`,
+				`spec.containers[0].ports[2]: duplicate container port 80/UDP, conflicts with spec.containers[0]`,
-				`spec.containers[1].ports[1].containerPort: duplicate container port 80`,
+				`spec.containers[1].ports[0]: duplicate container port 80/UDP, conflicts with spec.containers[0]`,
+				`spec.containers[1].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[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 container port 80/TCP, conflicts with spec.containers[0]`,
+				`spec.containers[1].ports[0]: duplicate container port 80/TCP, conflicts with spec.containers[0]`,
+				`spec.containers[1].ports[1]: duplicate container port 80/TCP, conflicts with spec.containers[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)