Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release-1.0] Allow probes to explicitly set the port to the containerPort (#8288) #12270

Merged
merged 2 commits into from
Nov 12, 2021
Merged

[release-1.0] Allow probes to explicitly set the port to the containerPort (#8288) #12270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
746a489
Allow probes to explicitly set the port to the containerPort (#8288)
Nov 4, 2021
2806e02
Address julz feedback
Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/apis/serving/fieldmask.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ func HTTPGetActionMask(in *corev1.HTTPGetAction) *corev1.HTTPGetAction {
out.Path = in.Path
out.Scheme = in.Scheme
out.HTTPHeaders = in.HTTPHeaders
out.Port = in.Port

return out
}
Expand All @@ -381,6 +382,7 @@ func TCPSocketActionMask(in *corev1.TCPSocketAction) *corev1.TCPSocketAction {

// Allowed fields
out.Host = in.Host
out.Port = in.Port

return out
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/apis/serving/fieldmask_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ func TestHTTPGetActionMask(t *testing.T) {
Path: "/bar",
Scheme: corev1.URISchemeHTTP,
HTTPHeaders: []corev1.HTTPHeader{{}},
Port: intstr.FromInt(8080),
}
in := &corev1.HTTPGetAction{
Host: "foo",
Expand Down Expand Up @@ -338,10 +339,11 @@ func TestHTTPGetActionMask(t *testing.T) {
func TestTCPSocketActionMask(t *testing.T) {
want := &corev1.TCPSocketAction{
Host: "foo",
Port: intstr.FromString("https"),
}
in := &corev1.TCPSocketAction{
Host: "foo",
Port: intstr.FromInt(8080),
Port: intstr.FromString("https"),
}

got := TCPSocketActionMask(in)
Expand Down
79 changes: 48 additions & 31 deletions pkg/apis/serving/k8s_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,14 +324,19 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError {
errs = errs.Also(err.ViaField("volumes"))
}

port, err := validateContainersPorts(ps.Containers)
if err != nil {
errs = errs.Also(err.ViaField("containers[*]"))
}

switch len(ps.Containers) {
case 0:
errs = errs.Also(apis.ErrMissingField("containers"))
case 1:
errs = errs.Also(ValidateContainer(ctx, ps.Containers[0], volumes).
errs = errs.Also(ValidateContainer(ctx, ps.Containers[0], volumes, port).
ViaFieldIndex("containers", 0))
default:
errs = errs.Also(validateContainers(ctx, ps.Containers, volumes))
errs = errs.Also(validateContainers(ctx, ps.Containers, volumes, port))
}
if ps.ServiceAccountName != "" {
for _, err := range validation.IsDNS1123Subdomain(ps.ServiceAccountName) {
Expand All @@ -356,20 +361,20 @@ func validateInitContainers(ctx context.Context, containers []corev1.Container,
return errs
}

func validateContainers(ctx context.Context, containers []corev1.Container, volumes map[string]corev1.Volume) (errs *apis.FieldError) {
func validateContainers(ctx context.Context, containers []corev1.Container, volumes map[string]corev1.Volume, port corev1.ContainerPort) (errs *apis.FieldError) {
features := config.FromContextOrDefaults(ctx).Features
if features.MultiContainer != config.Enabled {
return errs.Also(&apis.FieldError{Message: fmt.Sprintf("multi-container is off, "+
"but found %d containers", len(containers))})
}
errs = errs.Also(validateContainersPorts(containers).ViaField("containers"))
for i := range containers {
// Probes are not allowed on other than serving container,
// ref: http://bit.ly/probes-condition
if len(containers[i].Ports) == 0 {
// Note, if we allow readiness/liveness checks on sidecars, we should pass in an *empty* port here, not the main container's port.
errs = errs.Also(validateSidecarContainer(WithinSidecarContainer(ctx), containers[i], volumes).ViaFieldIndex("containers", i))
} else {
errs = errs.Also(ValidateContainer(WithinUserContainer(ctx), containers[i], volumes).ViaFieldIndex("containers", i))
errs = errs.Also(ValidateContainer(WithinUserContainer(ctx), containers[i], volumes, port).ViaFieldIndex("containers", i))
}
}
return errs
Expand All @@ -386,21 +391,38 @@ func AllMountedVolumes(containers []corev1.Container) sets.String {
return volumeNames
}

// validateContainersPorts validates port when specified multiple containers
func validateContainersPorts(containers []corev1.Container) *apis.FieldError {
// validateContainersPorts validates port when specified multiple containers,
// and returns the single serving port if error is nil
func validateContainersPorts(containers []corev1.Container) (corev1.ContainerPort, *apis.FieldError) {
var count int
var port = corev1.ContainerPort{
Name: "http",
ContainerPort: 8080,
}
for i := range containers {
count += len(containers[i].Ports)
if c := len(containers[i].Ports); c > 0 {
count += c
if containers[i].Ports[0].ContainerPort != 0 {
port.ContainerPort = containers[i].Ports[0].ContainerPort
}
if containers[i].Ports[0].Name != "" {
port.Name = containers[i].Ports[0].Name
}
}
}
// When no container ports are specified.
if count == 0 {
return apis.ErrMissingField("ports")
if count == 0 && len(containers) > 1 {
return port, apis.ErrMissingField("ports")
}
// More than one container sections have ports.
if count > 1 {
return apis.ErrMultipleOneOf("ports")
return port, &apis.FieldError{
Message: "more than one container port is set",
Paths: []string{"ports"},
Details: "Only a single port is allowed across all containers",
}
}
return nil
return port, nil
}

// validateSidecarContainer validate fields for non serving containers
Expand Down Expand Up @@ -447,27 +469,14 @@ func validateInitContainer(ctx context.Context, container corev1.Container, volu
}

// ValidateContainer validate fields for serving containers
func ValidateContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume) (errs *apis.FieldError) {
// Single container cannot have multiple ports
errs = errs.Also(portValidation(container.Ports).ViaField("ports"))
func ValidateContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume, port corev1.ContainerPort) (errs *apis.FieldError) {
// Liveness Probes
errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe"))
errs = errs.Also(validateProbe(container.LivenessProbe, port).ViaField("livenessProbe"))
// Readiness Probes
errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe"))
errs = errs.Also(validateReadinessProbe(container.ReadinessProbe, port).ViaField("readinessProbe"))
return errs.Also(validate(ctx, container, volumes))
}

func portValidation(containerPorts []corev1.ContainerPort) *apis.FieldError {
if len(containerPorts) > 1 {
return &apis.FieldError{
Message: "More than one container port is set",
Paths: []string{apis.CurrentField},
Details: "Only a single port is allowed",
}
}
return nil
}

func validate(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume) *apis.FieldError {
if equality.Semantic.DeepEqual(container, corev1.Container{}) {
return apis.ErrMissingField(apis.CurrentField)
Expand Down Expand Up @@ -653,12 +662,12 @@ func validateContainerPortBasic(port corev1.ContainerPort) *apis.FieldError {
return errs
}

func validateReadinessProbe(p *corev1.Probe) *apis.FieldError {
func validateReadinessProbe(p *corev1.Probe, port corev1.ContainerPort) *apis.FieldError {
if p == nil {
return nil
}

errs := validateProbe(p)
errs := validateProbe(p, port)

if p.PeriodSeconds < 0 {
errs = errs.Also(apis.ErrOutOfBoundsValue(p.PeriodSeconds, 0, math.MaxInt32, "periodSeconds"))
Expand Down Expand Up @@ -700,7 +709,7 @@ func validateReadinessProbe(p *corev1.Probe) *apis.FieldError {
return errs
}

func validateProbe(p *corev1.Probe) *apis.FieldError {
func validateProbe(p *corev1.Probe, port corev1.ContainerPort) *apis.FieldError {
if p == nil {
return nil
}
Expand All @@ -714,10 +723,18 @@ func validateProbe(p *corev1.Probe) *apis.FieldError {
if h.HTTPGet != nil {
handlers = append(handlers, "httpGet")
errs = errs.Also(apis.CheckDisallowedFields(*h.HTTPGet, *HTTPGetActionMask(h.HTTPGet))).ViaField("httpGet")
getPort := h.HTTPGet.Port
if (getPort.StrVal != "" && getPort.StrVal != port.Name) || (getPort.IntVal != 0 && getPort.IntVal != port.ContainerPort) {
errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "May only probe containerPort"))
}
}
if h.TCPSocket != nil {
handlers = append(handlers, "tcpSocket")
errs = errs.Also(apis.CheckDisallowedFields(*h.TCPSocket, *TCPSocketActionMask(h.TCPSocket))).ViaField("tcpSocket")
tcpPort := h.TCPSocket.Port
if (tcpPort.StrVal != "" && tcpPort.StrVal != port.Name) || (tcpPort.IntVal != 0 && tcpPort.IntVal != port.ContainerPort) {
errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "May only probe containerPort"))
}
}
if h.Exec != nil {
handlers = append(handlers, "exec")
Expand Down
100 changes: 78 additions & 22 deletions pkg/apis/serving/k8s_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ func TestPodSpecValidation(t *testing.T) {
ps: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "busybox",
Ports: []corev1.ContainerPort{{}},
}, {
Image: "helloworld",
}},
Expand Down Expand Up @@ -500,7 +501,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
Image: "helloworld",
}},
},
want: apis.ErrMissingField("containers.ports"),
want: apis.ErrMissingField("containers[*].ports"),
}, {
name: "flag enabled: multiple containers with multiple port",
ps: corev1.PodSpec{
Expand All @@ -516,7 +517,11 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
}},
}},
},
want: apis.ErrMultipleOneOf("containers.ports"),
want: &apis.FieldError{
Message: "more than one container port is set",
Paths: []string{"containers[*].ports"},
Details: "Only a single port is allowed across all containers",
},
}, {
name: "flag enabled: multiple containers with multiple ports for each container",
ps: corev1.PodSpec{
Expand All @@ -534,11 +539,11 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
}},
}},
},
want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{
Message: "More than one container port is set",
Paths: []string{"containers[0].ports"},
Details: "Only a single port is allowed",
}),
want: &apis.FieldError{
Message: "more than one container port is set",
Paths: []string{"containers[*].ports"},
Details: "Only a single port is allowed across all containers",
},
}, {
name: "flag enabled: multiple containers with multiple port for a single container",
ps: corev1.PodSpec{
Expand All @@ -553,11 +558,31 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
Image: "helloworld",
}},
},
want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{
Message: "More than one container port is set",
Paths: []string{"containers[0].ports"},
Details: "Only a single port is allowed",
}),
want: &apis.FieldError{
Message: "more than one container port is set",
Paths: []string{"containers[*].ports"},
Details: "Only a single port is allowed across all containers",
},
}, {
name: "flag enabled: multiple containers with readinessProbe targeting different container's port",
ps: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "work",
Ports: []corev1.ContainerPort{{
ContainerPort: 9999,
}},
}, {
Image: "health",
LivenessProbe: &corev1.Probe{
Handler: corev1.Handler{
HTTPGet: &corev1.HTTPGetAction{
Port: intstr.FromInt(9999), // This is the other container's port
},
},
},
}},
},
want: apis.ErrDisallowedFields("containers[1].livenessProbe"),
}, {
name: "flag enabled: multiple containers with illegal env variable defined for side car",
ps: corev1.PodSpec{
Expand Down Expand Up @@ -1111,9 +1136,9 @@ func TestContainerValidation(t *testing.T) {
}},
},
want: &apis.FieldError{
Message: "More than one container port is set",
Message: "more than one container port is set",
Paths: []string{"ports"},
Details: "Only a single port is allowed",
Details: "Only a single port is allowed across all containers",
},
}, {
name: "has an empty port set",
Expand All @@ -1133,9 +1158,9 @@ func TestContainerValidation(t *testing.T) {
}},
},
want: &apis.FieldError{
Message: "More than one container port is set",
Message: "more than one container port is set",
Paths: []string{"ports"},
Details: "Only a single port is allowed",
Details: "Only a single port is allowed across all containers",
},
}, {
name: "has tcp protocol",
Expand Down Expand Up @@ -1262,7 +1287,7 @@ func TestContainerValidation(t *testing.T) {
},
want: apis.ErrMultipleOneOf("readinessProbe.exec", "readinessProbe.tcpSocket", "readinessProbe.httpGet"),
}, {
name: "invalid readiness http probe (has port)",
name: "invalid readiness http probe (has wrong port)",
c: corev1.Container{
Image: "foo",
ReadinessProbe: &corev1.Probe{
Expand All @@ -1273,12 +1298,25 @@ func TestContainerValidation(t *testing.T) {
Handler: corev1.Handler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/",
Port: intstr.FromInt(8080),
Port: intstr.FromInt(5000),
},
},
},
},
want: apis.ErrInvalidValue(5000, "readinessProbe.httpGet.port", "May only probe containerPort"),
}, {
name: "valid readiness http probe with port",
c: corev1.Container{
Image: "foo",
ReadinessProbe: &corev1.Probe{
SuccessThreshold: 1,
Handler: corev1.Handler{
HTTPGet: &corev1.HTTPGetAction{
Port: intstr.FromString("http"), // http is the default
},
},
},
},
want: apis.ErrDisallowedFields("readinessProbe.httpGet.port"),
}, {
name: "invalid readiness probe (has failureThreshold while using special probe)",
c: corev1.Container{
Expand Down Expand Up @@ -1360,12 +1398,29 @@ func TestContainerValidation(t *testing.T) {
LivenessProbe: &corev1.Probe{
Handler: corev1.Handler{
TCPSocket: &corev1.TCPSocketAction{
Port: intstr.FromString("http"),
Port: intstr.FromString("imap"),
},
},
},
},
want: apis.ErrInvalidValue("imap", "livenessProbe.tcpSocket.port", "May only probe containerPort"),
}, {
name: "valid liveness tcp probe with correct port",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{
{
ContainerPort: 8888,
},
},
LivenessProbe: &corev1.Probe{
Handler: corev1.Handler{
TCPSocket: &corev1.TCPSocketAction{
Port: intstr.FromInt(8888),
},
},
},
},
want: apis.ErrDisallowedFields("livenessProbe.tcpSocket.port"),
}, {
name: "disallowed container fields",
c: corev1.Container{
Expand Down Expand Up @@ -1405,8 +1460,9 @@ func TestContainerValidation(t *testing.T) {
}
ctx = config.ToContext(ctx, cfg)
}
port, err := validateContainersPorts([]corev1.Container{test.c})

got := ValidateContainer(ctx, test.c, test.volumes)
got := err.Also(ValidateContainer(ctx, test.c, test.volumes, port))
if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
t.Errorf("ValidateContainer (-want, +got): \n%s", diff)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/serving/v1/revision_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ func TestRevisionSpecValidation(t *testing.T) {
Image: "busybox",
}, {
Image: "helloworld",
Ports: []corev1.ContainerPort{{}},
}},
},
},
Expand Down