From 64c9070f8d07f50577a81de28cf2e1676eaec5de Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Fri, 21 Oct 2022 16:06:40 +0800 Subject: [PATCH 1/3] add pod containers[*].port duplicate warning --- pkg/api/pod/warnings.go | 11 ++++++++ pkg/api/pod/warnings_test.go | 54 +++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 31a66b116be..d062373a2df 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -252,6 +252,17 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } } + // duplicate containers[*].ports + if len(c.Ports) > 1 { + ports := sets.NewInt64() + for i, port := range c.Ports { + if ports.Has(int64(port.ContainerPort)) { + warnings = append(warnings, fmt.Sprintf("%s: duplicate container port %d", p.Child("ports").Index(i).Child("containerPort"), port.ContainerPort)) + } else { + ports.Insert(int64(port.ContainerPort)) + } + } + } return true }) diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index e849fb37bef..ee7e507577e 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -24,7 +24,6 @@ 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) { @@ -599,6 +598,59 @@ func TestWarnings(t *testing.T) { `spec.affinity.podAntiAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: a null labelSelector results in matching no pod`, }, }, + { + name: "no duplicate container ports", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Containers: []api.Container{{ + Name: "foo", + Ports: []api.ContainerPort{ + {ContainerPort: 80, HostPort: 80}, + }, + }}, + }}, + expected: []string{}, + }, + + { + name: "duplicate container ports", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Containers: []api.Container{{ + Name: "foo", + Ports: []api.ContainerPort{ + {ContainerPort: 80, HostPort: 80}, + {ContainerPort: 80, HostPort: 80}, + }, + }}, + }}, + expected: []string{ + `spec.containers[0].ports[1].containerPort: duplicate container port 80`, + }, + }, + { + name: "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: 180, HostPort: 80}, + {ContainerPort: 80, HostPort: 80}, + }, + }, + { + Name: "foo", + Ports: []api.ContainerPort{ + {ContainerPort: 80, HostPort: 80}, + {ContainerPort: 80, HostPort: 80}, + }, + }}, + }}, + expected: []string{ + `spec.containers[0].ports[2].containerPort: duplicate container port 80`, + `spec.containers[1].ports[1].containerPort: duplicate container port 80`, + }, + }, } for _, tc := range testcases { From 7a80d7c839c777249bc765ba4e63cdf65723a003 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Fri, 21 Oct 2022 16:34:35 +0800 Subject: [PATCH 2/3] add warning for duplicate containers[*].ports with the same port, protocol, hostPort, and hostIP --- pkg/api/pod/warnings.go | 38 +++- pkg/api/pod/warnings_test.go | 409 +++++++++++++++++++++++++++++++++-- 2 files changed, 421 insertions(+), 26 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index d062373a2df..dcebd74dfeb 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -252,16 +252,33 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } } - // duplicate containers[*].ports - if len(c.Ports) > 1 { - ports := sets.NewInt64() - for i, port := range c.Ports { - if ports.Has(int64(port.ContainerPort)) { - warnings = append(warnings, fmt.Sprintf("%s: duplicate container port %d", p.Child("ports").Index(i).Child("containerPort"), port.ContainerPort)) - } else { - ports.Insert(int64(port.ContainerPort)) + return true + }) + + // 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 { + k := fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol) + if other, found := allPorts[k]; found { + // Someone else has this protcol+port, but it still might not be a conflict. + 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 { diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index ee7e507577e..194ba2d43d0 100644 --- a/pkg/api/pod/warnings_test.go +++ b/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) { @@ -142,9 +143,10 @@ func TestWarnings(t *testing.T) { api.ResourceEphemeralStorage: resource.MustParse("4m"), } testcases := []struct { - name string - template *api.PodTemplateSpec - expected []string + name string + template *api.PodTemplateSpec + oldTemplate *api.PodTemplateSpec + expected []string }{ { name: "null", @@ -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, HostPort: 80}, + {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{ - `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: 180, HostPort: 80}, - {ContainerPort: 80, HostPort: 80}, + {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}, - {ContainerPort: 80, HostPort: 80}, + {ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP}, + {ContainerPort: 80, HostPort: 80, Protocol: api.ProtocolUDP}, }, }}, }}, expected: []string{ - `spec.containers[0].ports[2].containerPort: duplicate container port 80`, - `spec.containers[1].ports[1].containerPort: duplicate container port 80`, + `spec.containers[0].ports[2]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `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) From df0d51d3b35cc60ab4733e274a81cb2e44fba087 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Wed, 12 Jul 2023 16:39:03 +0800 Subject: [PATCH 3/3] add some detailed message for dup container ports(steal from thockin) --- pkg/api/pod/warnings.go | 53 +++++++++++++------------- pkg/api/pod/warnings_test.go | 72 +++++++++++++++++++++++------------- 2 files changed, 74 insertions(+), 51 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index dcebd74dfeb..d9b0d01a86d 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -255,30 +255,38 @@ 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{} + allPorts := map[string][]portBlock{} pods.VisitContainersWithPath(podSpec, fieldPath.Child("spec"), func(c *api.Container, fldPath *field.Path) bool { for i, port := range c.Ports { - k := fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol) - if other, found := allPorts[k]; found { - // Someone else has this protcol+port, but it still might not be a conflict. - 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 - } + 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}} } - allPorts[k] = portBlock{field: fldPath, port: port} } return true }) @@ -302,11 +310,6 @@ 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 { diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 194ba2d43d0..5404c414516 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -659,9 +659,7 @@ func TestWarnings(t *testing.T) { }, }}, }}, - expected: []string{ - `spec.containers[0].ports[1]: duplicate container port 80/TCP, conflicts with spec.containers[0]`, - }, + expected: []string{}, }, { name: "one container, two ports, same protocol, port and hostPort, different hostIP", @@ -725,9 +723,7 @@ func TestWarnings(t *testing.T) { }, }}, }}, - expected: []string{ - `spec.containers[1].ports[0]: duplicate container port 80/TCP, conflicts with spec.containers[0]`, - }, + expected: []string{}, }, { name: "two containers, one port each, same protocol, port and hostPort, different hostIP", @@ -758,7 +754,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[0].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[0].ports[1]: duplicate port definition with spec.containers[0].ports[0]`, }, }, { @@ -773,7 +769,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[0].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[0].ports[1]: duplicate port definition with spec.containers[0].ports[0]`, }, }, { @@ -788,7 +784,21 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[0].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `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}`, }, }, { @@ -803,22 +813,25 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[0].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `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 one with", + 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}, {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]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `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]`, }, }, { @@ -833,7 +846,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[0].ports[1]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[0].ports[1]: dangerously ambiguous port definition with spec.containers[0].ports[0]`, }, }, { @@ -852,7 +865,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[1].ports[0]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`, }, }, { @@ -871,7 +884,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[1].ports[0]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`, }, }, { @@ -890,7 +903,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[1].ports[0]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[1].ports[0]: duplicate port definition with spec.containers[0].ports[0]`, }, }, { @@ -909,7 +922,8 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[1].ports[0]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `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}`, }, }, { @@ -928,7 +942,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[1].ports[0]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[1].ports[0]: dangerously ambiguous port definition with spec.containers[0].ports[0]`, }, }, { @@ -947,7 +961,7 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[1].ports[0]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, + `spec.containers[1].ports[0]: dangerously ambiguous port definition with spec.containers[0].ports[0]`, }, }, { @@ -971,9 +985,12 @@ func TestWarnings(t *testing.T) { }}, }}, expected: []string{ - `spec.containers[0].ports[2]: duplicate container port 80/UDP, conflicts with spec.containers[0]`, - `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]`, + `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]`, }, }, { @@ -1015,9 +1032,12 @@ func TestWarnings(t *testing.T) { }}, }}, 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]`, + `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]`, }, }, }