Skip to content

Add validation and flag to support multi container #7167

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

Merged
knative-prow-robot merged 7 commits into from
Mar 13, 2020
Merged

Add validation and flag to support multi container #7167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fa171a9
Add validation and flag to support multi container
savitaashture Mar 6, 2020
35c9fb7
fix review comments
savitaashture Mar 7, 2020
c6efde9
fix review comment
savitaashture Mar 9, 2020
50f1397
fix nit and changed error message for multiple conatiners when flag d…
savitaashture Mar 10, 2020
04f589b
add logic to generate unique names for containers
savitaashture Mar 11, 2020
ee44718
fix review comments
savitaashture Mar 12, 2020
47e0ee4
fix review comments and removed duplicate testcases
savitaashture Mar 13, 2020
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
3 changes: 3 additions & 0 deletions config/core/configmaps/defaults.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,6 @@ data:
# must be at or below this value.
# Must be greater than 1.
container-concurrency-max-limit: "1000"

# feature flag indicates whether to enable multi container support or not
enable-multi-container: "false"
7 changes: 7 additions & 0 deletions pkg/apis/config/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"io/ioutil"
"math"
"strconv"
"strings"
"text/template"

corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -68,6 +69,9 @@ func defaultConfig() *Defaults {
func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) {
nc := defaultConfig()

// Process bool field.
nc.EnableMultiContainer = strings.EqualFold(data["enable-multi-container"], "true")
Copy link
Copy Markdown
Member

@dprotaso dprotaso Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If we ever change the default for this property to true this will override it to false if the property is not set in a defaults config map

not important now

Copy link
Copy Markdown
Member

@mattmoor mattmoor Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's WAI? Alpha=Opt-In, Beta=Opt-Out, GA=remove the option.

Copy link
Copy Markdown
Member

@dprotaso dprotaso Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alpha=Opt-In, Beta=Opt-Out, GA=remove the option.

Yeah but data["enable-multi-container"] will return an empty string - so there's an explicit change that'll be needed when we swap from alpha to beta

not something that's necessary right now

Copy link
Copy Markdown
Contributor

@vagababov vagababov Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for beta this will require explicit code change, but I think that's by design, no? :)

Copy link
Copy Markdown
Member

@dprotaso dprotaso Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - that's why I just marked it a nit


// Process int64 fields
for _, i64 := range []struct {
key string
Expand Down Expand Up @@ -158,6 +162,9 @@ func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error)

// Defaults includes the default values to be populated by the webhook.
type Defaults struct {
// Feature flag to enable multi container support
EnableMultiContainer bool
Comment thread
savitaashture marked this conversation as resolved.

RevisionTimeoutSeconds int64
// This is the timeout set for cluster ingress.
// RevisionTimeoutSeconds must be less than this value.
Expand Down
15 changes: 15 additions & 0 deletions pkg/apis/config/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,19 +60,34 @@ func TestDefaultsConfiguration(t *testing.T) {
name: "specified values",
wantErr: false,
wantDefaults: &Defaults{
EnableMultiContainer: true,
RevisionTimeoutSeconds: 123,
MaxRevisionTimeoutSeconds: 456,
ContainerConcurrencyMaxLimit: 1984,
RevisionCPURequest: &oneTwoThree,
UserContainerNameTemplate: "{{.Name}}",
},
data: map[string]string{
"enable-multi-container": "true",
"revision-timeout-seconds": "123",
"max-revision-timeout-seconds": "456",
"revision-cpu-request": "123m",
"container-concurrency-max-limit": "1984",
"container-name-template": "{{.Name}}",
},
}, {
name: "invalid multi container flag value",
wantErr: false,
wantDefaults: &Defaults{
EnableMultiContainer: false,
RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds,
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
UserContainerNameTemplate: DefaultUserContainerName,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
},
data: map[string]string{
"enable-multi-container": "invalid",
},
}, {
name: "bad revision timeout",
wantErr: true,
Expand Down
94 changes: 80 additions & 14 deletions pkg/apis/serving/k8s_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package serving

import (
"context"
"fmt"
"math"
"path/filepath"
Expand All @@ -29,6 +30,7 @@ import (
"k8s.io/apimachinery/pkg/util/validation"
"knative.dev/pkg/apis"
"knative.dev/pkg/profiling"
"knative.dev/serving/pkg/apis/config"
"knative.dev/serving/pkg/apis/networking"
)

Expand Down Expand Up @@ -236,7 +238,8 @@ func validateEnvFrom(envFromList []corev1.EnvFromSource) *apis.FieldError {
return errs
}

func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError {
// ValidatePodSpec validates the pod spec
func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError {
Comment thread
savitaashture marked this conversation as resolved.
// This is inlined, and so it makes for a less meaningful
// error message.
// if equality.Semantic.DeepEqual(ps, corev1.PodSpec{}) {
Expand All @@ -257,7 +260,7 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError {
errs = errs.Also(ValidateContainer(ps.Containers[0], volumes).
ViaFieldIndex("containers", 0))
default:
errs = errs.Also(apis.ErrMultipleOneOf("containers"))
errs = errs.Also(validateContainers(ctx, ps.Containers, volumes))
}
if ps.ServiceAccountName != "" {
for range validation.IsDNS1123Subdomain(ps.ServiceAccountName) {
Expand All @@ -267,7 +270,82 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError {
return errs
}

func validateContainers(ctx context.Context, containers []corev1.Container, volumes sets.String) *apis.FieldError {
var errs *apis.FieldError
cfg := config.FromContextOrDefaults(ctx).Defaults
if !cfg.EnableMultiContainer {
errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("enable-multi-container is off, "+
"but found %d containers", len(containers))})
} else {
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 {
errs = errs.Also(validateSidecarContainer(containers[i], volumes).ViaFieldIndex("containers", i))
} else {
errs = errs.Also(ValidateContainer(containers[i], volumes).ViaFieldIndex("containers", i))
}
}
}
return errs
}

// validateContainersPorts validates port when specified multiple containers
func validateContainersPorts(containers []corev1.Container) *apis.FieldError {
var count int
for i := range containers {
count += len(containers[i].Ports)
}
// When no container ports are specified.
if count == 0 {
return apis.ErrMissingField("ports")
}
// More than one container sections have ports.
if count > 1 {
Comment thread
savitaashture marked this conversation as resolved.
return apis.ErrMultipleOneOf("ports")
}
return nil
}

// validateSidecarContainer validate fields for non serving containers
func validateSidecarContainer(container corev1.Container, volumes sets.String) *apis.FieldError {
var errs *apis.FieldError
if container.LivenessProbe != nil {
errs = errs.Also(apis.CheckDisallowedFields(*container.LivenessProbe,
*ProbeMask(&corev1.Probe{})).ViaField("livenessProbe"))
}
if container.ReadinessProbe != nil {
errs = errs.Also(apis.CheckDisallowedFields(*container.ReadinessProbe,
*ProbeMask(&corev1.Probe{})).ViaField("readinessProbe"))
}
return errs.Also(validate(container, volumes))
}

// ValidateContainer validate fields for serving containers
func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError {
var errs *apis.FieldError
// Single container cannot have multiple ports
errs = errs.Also(portValidation(container.Ports).ViaField("ports"))
// Liveness Probes
errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe"))
// Readiness Probes
errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe"))
return errs.Also(validate(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(container corev1.Container, volumes sets.String) *apis.FieldError {
if equality.Semantic.DeepEqual(container, corev1.Container{}) {
return apis.ErrMissingField(apis.CurrentField)
}
Expand Down Expand Up @@ -296,12 +374,8 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi
}
errs = errs.Also(fe)
}
// Liveness Probes
errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe"))
// Ports
errs = errs.Also(validateContainerPorts(container.Ports).ViaField("ports"))
// Readiness Probes
errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe"))
// Resources
errs = errs.Also(validateResources(&container.Resources).ViaField("resources"))
// SecurityContext
Expand Down Expand Up @@ -397,14 +471,6 @@ func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError {
// user can set container port which names "user-port" to define application's port.
// Queue-proxy will use it to send requests to application
// if user didn't set any port, it will set default port user-port=8080.
if len(ports) > 1 {
errs = errs.Also(&apis.FieldError{
Message: "More than one container port is set",
Paths: []string{apis.CurrentField},
Details: "Only a single port is allowed",
})
}

userPort := ports[0]

errs = errs.Also(apis.CheckDisallowedFields(userPort, *ContainerPortMask(&userPort)))
Expand Down
Loading