From df0d51d3b35cc60ab4733e274a81cb2e44fba087 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Wed, 12 Jul 2023 16:39:03 +0800 Subject: [PATCH] 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]`, }, }, }