From 7a80d7c839c777249bc765ba4e63cdf65723a003 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Fri, 21 Oct 2022 16:34:35 +0800 Subject: [PATCH] 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)