diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index c3360538035..2d472dcf7d7 100644 --- a/pkg/api/pod/warnings.go +++ b/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"))) diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 8a2ba9e5482..72241ce4ade 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -143,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", @@ -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)