From fa171a9285c6ba96a61587ed2eb7a01a646224d5 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 6 Mar 2020 19:42:41 +0530 Subject: [PATCH 1/7] Add validation and flag to support multi container --- config/core/configmaps/defaults.yaml | 3 + pkg/apis/config/defaults.go | 21 ++ pkg/apis/config/defaults_test.go | 19 ++ pkg/apis/serving/k8s_validation.go | 84 ++++++-- pkg/apis/serving/k8s_validation_test.go | 151 +++++++++++++- pkg/apis/serving/v1/revision_defaults.go | 96 +++++---- pkg/apis/serving/v1/revision_defaults_test.go | 35 +++- pkg/apis/serving/v1/revision_validation.go | 2 +- .../serving/v1/revision_validation_test.go | 156 +++++++++++++++ .../v1alpha1/revision_defaults_test.go | 73 +++++++ .../v1alpha1/revision_validation_test.go | 168 ++++++++++++++++ .../serving/v1beta1/revision_defaults_test.go | 37 +++- .../v1beta1/revision_validation_test.go | 185 ++++++++++++++++++ 13 files changed, 970 insertions(+), 60 deletions(-) diff --git a/config/core/configmaps/defaults.yaml b/config/core/configmaps/defaults.yaml index 7327dd187efd..85a1943db254 100644 --- a/config/core/configmaps/defaults.yaml +++ b/config/core/configmaps/defaults.yaml @@ -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" diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index ea4d47e13011..f0df9de8d747 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -23,6 +23,7 @@ import ( "io/ioutil" "math" "strconv" + "strings" "text/template" corev1 "k8s.io/api/core/v1" @@ -68,6 +69,24 @@ func defaultConfig() *Defaults { func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) { nc := defaultConfig() + // Process bool fields. + for _, b := range []struct { + key string + field *bool + defaultValue bool + }{ + { + key: "enable-multi-container", + field: &nc.EnableMultiContainer, + defaultValue: false, + }} { + if raw, ok := data[b.key]; !ok { + *b.field = b.defaultValue + } else { + *b.field = strings.EqualFold(raw, "true") + } + } + // Process int64 fields for _, i64 := range []struct { key string @@ -158,6 +177,8 @@ func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) // Defaults includes the default values to be populated by the webhook. type Defaults struct { + EnableMultiContainer bool + RevisionTimeoutSeconds int64 // This is the timeout set for cluster ingress. // RevisionTimeoutSeconds must be less than this value. diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index f8a3ffdf7c3f..c58c92f881e3 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -73,6 +73,25 @@ func TestDefaultsConfiguration(t *testing.T) { "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, + }, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "enable-multi-container": "invalid", + }, + }, }, { name: "bad revision timeout", wantErr: true, diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 09be210985cc..a92efe8b7b06 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -17,6 +17,7 @@ limitations under the License. package serving import ( + "context" "fmt" "math" "path/filepath" @@ -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" ) @@ -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 { // This is inlined, and so it makes for a less meaningful // error message. // if equality.Semantic.DeepEqual(ps, corev1.PodSpec{}) { @@ -257,7 +260,21 @@ 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")) + cfg := config.FromContextOrDefaults(ctx).Defaults + if !cfg.EnableMultiContainer { + errs = errs.Also(apis.ErrMultipleOneOf("containers")) + } else { + errs = errs.Also(ValidateMultiContainerPorts(ps.Containers)).ViaField("containers") + for i := range ps.Containers { + // probes are not allowed other than serving container + // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZU + if len(ps.Containers[i].Ports) == 0 { + errs = errs.Also(ValidateSidecarContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) + } else { + errs = errs.Also(ValidateContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) + } + } + } } if ps.ServiceAccountName != "" { for range validation.IsDNS1123Subdomain(ps.ServiceAccountName) { @@ -267,7 +284,58 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { return errs } +// ValidateMultiContainerPorts validates port when specified multiple containers +func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError { + var ( + count int + errs *apis.FieldError + ) + for i := range containers { + count += len(containers[i].Ports) + } + if count == 0 { + errs = errs.Also(apis.ErrMissingField(apis.CurrentField)) + } + return errs.Also(portValidation(count)).ViaField("ports") +} + +// 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 + errs = errs.Also(portValidation(len(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(count int) *apis.FieldError { + if count > 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) } @@ -296,12 +364,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 @@ -397,14 +461,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))) diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index dd3150d7528d..b92eecd01253 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -17,6 +17,7 @@ limitations under the License. package serving import ( + "context" "fmt" "math" "testing" @@ -24,12 +25,32 @@ import ( "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" "knative.dev/pkg/apis" + logtesting "knative.dev/pkg/logging/testing" "knative.dev/pkg/ptr" + "knative.dev/serving/pkg/apis/config" + autoscalerconfig "knative.dev/serving/pkg/autoscaler/config" ) +func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { + logger := logtesting.TestLogger(t) + s := config.NewStore(logger) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-multi-container": "true", + }, + }) + + return s.ToContext(ctx) +} + func TestPodSpecValidation(t *testing.T) { tests := []struct { name string @@ -183,7 +204,135 @@ func TestPodSpecValidation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := ValidatePodSpec(test.ps) + got := ValidatePodSpec(context.Background(), test.ps) + if !cmp.Equal(test.want.Error(), got.Error()) { + t.Errorf("ValidatePodSpec (-want, +got) = %v", + cmp.Diff(test.want.Error(), got.Error())) + } + }) + } +} + +func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { + tests := []struct { + name string + ps corev1.PodSpec + wc context.Context + want *apis.FieldError + }{{ + name: "flag disabled: more than one container", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + want: apis.ErrMultipleOneOf("containers"), + }, { + name: "flag enabled: more than one container with one container port", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + wc: enableMultiContainer(context.Background(), t), + want: nil, + }, { + name: "flag enabled: probes are not allowed for non serving containers", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + ReadinessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + }}, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, + }, + }, { + name: "flag enabled: too many containers with no port", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + }}, + }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMissingField("containers.ports"), + }, { + name: "flag enabled: too many containers with too many port", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 9999, + }}, + }}, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports"}, + Details: "Only a single port is allowed", + }, + }, { + name: "flag enabled: too many containers with too many port for a single container", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }}, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports, containers[0].ports"}, + Details: "Only a single port is allowed", + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + if test.wc != nil { + ctx = test.wc + } + got := ValidatePodSpec(ctx, test.ps) if !cmp.Equal(test.want.Error(), got.Error()) { t.Errorf("ValidatePodSpec (-want, +got) = %v", cmp.Diff(test.want.Error(), got.Error())) diff --git a/pkg/apis/serving/v1/revision_defaults.go b/pkg/apis/serving/v1/revision_defaults.go index 01df5b626204..043981c7a4ed 100644 --- a/pkg/apis/serving/v1/revision_defaults.go +++ b/pkg/apis/serving/v1/revision_defaults.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "strconv" corev1 "k8s.io/api/core/v1" "knative.dev/pkg/apis" @@ -51,52 +52,73 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) { for idx := range rs.PodSpec.Containers { if rs.PodSpec.Containers[idx].Name == "" { - rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) - } - - if rs.PodSpec.Containers[idx].Resources.Requests == nil { - rs.PodSpec.Containers[idx].Resources.Requests = corev1.ResourceList{} - } - if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU]; !ok { - if rsrc := cfg.Defaults.RevisionCPURequest; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU] = *rsrc - } - } - if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory]; !ok { - if rsrc := cfg.Defaults.RevisionMemoryRequest; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory] = *rsrc + if len(rs.PodSpec.Containers) > 1 { + rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) + "-" + strconv.Itoa(idx) + } else { + rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) } } - if rs.PodSpec.Containers[idx].Resources.Limits == nil { - rs.PodSpec.Containers[idx].Resources.Limits = corev1.ResourceList{} - } - if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU]; !ok { - if rsrc := cfg.Defaults.RevisionCPULimit; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU] = *rsrc - } + rs.applyDefault(&rs.PodSpec.Containers[idx], cfg) + } +} +func (rs *RevisionSpec) applyDefault(container *corev1.Container, cfg *config.Config) { + if container.Resources.Requests == nil { + container.Resources.Requests = corev1.ResourceList{} + } + if _, ok := container.Resources.Requests[corev1.ResourceCPU]; !ok { + if rc := cfg.Defaults.RevisionCPURequest; rc != nil { + container.Resources.Requests[corev1.ResourceCPU] = *rc } - if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory]; !ok { - if rsrc := cfg.Defaults.RevisionMemoryLimit; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory] = *rsrc - } + } + if _, ok := container.Resources.Requests[corev1.ResourceMemory]; !ok { + if rm := cfg.Defaults.RevisionMemoryRequest; rm != nil { + container.Resources.Requests[corev1.ResourceMemory] = *rm } - if rs.PodSpec.Containers[idx].ReadinessProbe == nil { - rs.PodSpec.Containers[idx].ReadinessProbe = &corev1.Probe{} + } + + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} + } + if _, ok := container.Resources.Limits[corev1.ResourceCPU]; !ok { + if rc := cfg.Defaults.RevisionCPULimit; rc != nil { + container.Resources.Limits[corev1.ResourceCPU] = *rc } - if rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket == nil && - rs.PodSpec.Containers[idx].ReadinessProbe.HTTPGet == nil && - rs.PodSpec.Containers[idx].ReadinessProbe.Exec == nil { - rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{} + } + if _, ok := container.Resources.Limits[corev1.ResourceMemory]; !ok { + if rm := cfg.Defaults.RevisionMemoryLimit; rm != nil { + container.Resources.Limits[corev1.ResourceMemory] = *rm } + } - if rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold == 0 { - rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold = 1 + if len(rs.PodSpec.Containers) == 1 { + rs.applyProbes(container) + } else { + // If there are multiple containers then default probes will be applied to the container where user specified PORT + // default probes will not be applied for non serving containers + if len(container.Ports) != 0 { + rs.applyProbes(container) } + } - vms := rs.PodSpec.Containers[idx].VolumeMounts - for i := range vms { - vms[i].ReadOnly = true - } + vms := container.VolumeMounts + for i := range vms { + vms[i].ReadOnly = true } } + +func (*RevisionSpec) applyProbes(container *corev1.Container) { + if container.ReadinessProbe == nil { + container.ReadinessProbe = &corev1.Probe{} + } + if container.ReadinessProbe.TCPSocket == nil && + container.ReadinessProbe.HTTPGet == nil && + container.ReadinessProbe.Exec == nil { + container.ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{} + } + + if container.ReadinessProbe.SuccessThreshold == 0 { + container.ReadinessProbe.SuccessThreshold = 1 + } + +} diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 162cd8451d5d..e159f1d36ddd 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -304,6 +304,9 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, }, { Name: "helloworld", }}, @@ -319,10 +322,36 @@ func TestRevisionDefaulting(t *testing.T) { Name: "busybox", Resources: defaultResources, ReadinessProbe: defaultProbe, + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, }, { - Name: "helloworld", - Resources: defaultResources, - ReadinessProbe: defaultProbe, + Name: "helloworld", + Resources: defaultResources, + }}, + }, + }, + }, + }, { + name: "multiple containers without container name", + in: &Revision{ + Spec: RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + }, + }, + want: &Revision{ + Spec: RevisionSpec{ + TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), + ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "user-container-0", + Resources: defaultResources, + }, { + Name: "user-container-1", + Resources: defaultResources, }}, }, }, diff --git a/pkg/apis/serving/v1/revision_validation.go b/pkg/apis/serving/v1/revision_validation.go index d83b3760cb05..ac584c702020 100644 --- a/pkg/apis/serving/v1/revision_validation.go +++ b/pkg/apis/serving/v1/revision_validation.go @@ -96,7 +96,7 @@ func (current *RevisionTemplateSpec) VerifyNameChange(ctx context.Context, og Re // Validate implements apis.Validatable func (rs *RevisionSpec) Validate(ctx context.Context) *apis.FieldError { - errs := serving.ValidatePodSpec(rs.PodSpec) + errs := serving.ValidatePodSpec(ctx, rs.PodSpec) if rs.TimeoutSeconds != nil { errs = errs.Also(serving.ValidateTimeoutSeconds(ctx, *rs.TimeoutSeconds)) diff --git a/pkg/apis/serving/v1/revision_validation_test.go b/pkg/apis/serving/v1/revision_validation_test.go index 4cdd94864ad5..ba71813803b6 100644 --- a/pkg/apis/serving/v1/revision_validation_test.go +++ b/pkg/apis/serving/v1/revision_validation_test.go @@ -907,3 +907,159 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { }) } } + +func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { + logger := logtesting.TestLogger(t) + s := config.NewStore(logger) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-multi-container": "true", + }, + }) + + return s.ToContext(ctx) +} + +func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { + tests := []struct { + name string + rs *RevisionSpec + wc context.Context + want *apis.FieldError + }{{ + name: "flag disabled: more than one container", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + want: apis.ErrMultipleOneOf("containers"), + }, { + name: "flag enabled: more than one container with one container port", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: nil, + }, { + name: "flag enabled: probes are not allowed for non serving containers", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + ReadinessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + }}, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, + }, + }, { + name: "flag enabled: too many containers with no port", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + }}, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMissingField("containers.ports"), + }, { + name: "flag enabled: too many containers with too many port", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 9999, + }}, + }}, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports"}, + Details: "Only a single port is allowed", + }, + }, { + name: "flag enabled: too many containers with too many port for a single container", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }}, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports, containers[0].ports"}, + Details: "Only a single port is allowed", + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + if test.wc != nil { + ctx = test.wc + } + + got := test.rs.Validate(ctx) + if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { + t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + } + }) + } +} diff --git a/pkg/apis/serving/v1alpha1/revision_defaults_test.go b/pkg/apis/serving/v1alpha1/revision_defaults_test.go index 8de06c504b23..39a6d4f89009 100644 --- a/pkg/apis/serving/v1alpha1/revision_defaults_test.go +++ b/pkg/apis/serving/v1alpha1/revision_defaults_test.go @@ -296,6 +296,79 @@ func TestRevisionDefaulting(t *testing.T) { }, }, }, + }, { + name: "multiple containers", + wc: v1.WithUpgradeViaDefaulting, + in: &Revision{ + Spec: RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Name: "helloworld", + }}, + }, + ContainerConcurrency: ptr.Int64(1), + TimeoutSeconds: ptr.Int64(99), + }, + }, + }, + want: &Revision{ + Spec: RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + Resources: defaultResources, + ReadinessProbe: defaultProbe, + }, { + Name: "helloworld", + Resources: defaultResources, + }}, + }, + ContainerConcurrency: ptr.Int64(1), + TimeoutSeconds: ptr.Int64(99), + }, + }, + }, + }, { + name: "multiple containers without container name", + wc: v1.WithUpgradeViaDefaulting, + in: &Revision{ + Spec: RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + ContainerConcurrency: ptr.Int64(1), + TimeoutSeconds: ptr.Int64(99), + }, + }, + }, + want: &Revision{ + Spec: RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "user-container-0", + Resources: defaultResources, + }, { + Name: "user-container-1", + Resources: defaultResources, + }}, + }, + ContainerConcurrency: ptr.Int64(1), + TimeoutSeconds: ptr.Int64(99), + }, + }, + }, }} for _, test := range tests { diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index b8e9efb88c40..9f76cc53652a 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -888,3 +888,171 @@ func TestRevisionProtocolType(t *testing.T) { } } } + +func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { + logger := logtesting.TestLogger(t) + s := config.NewStore(logger) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-multi-container": "true", + }, + }) + + return s.ToContext(ctx) +} + +func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { + tests := []struct { + name string + rs *RevisionSpec + wc context.Context + want *apis.FieldError + }{{ + name: "flag disabled: more than one container", + rs: &RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + }, + want: apis.ErrMultipleOneOf("containers"), + }, { + name: "flag enabled: more than one container with one container port", + rs: &RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: nil, + }, { + name: "flag enabled: probes are not allowed for non serving containers", + rs: &RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + ReadinessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, + }, + }, { + name: "flag enabled: too many containers with no port", + rs: &RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMissingField("containers.ports"), + }, { + name: "flag enabled: too many containers with too many port", + rs: &RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 9999, + }}, + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports"}, + Details: "Only a single port is allowed", + }, + }, { + name: "flag enabled: too many containers with too many port for a single container", + rs: &RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports, containers[0].ports"}, + Details: "Only a single port is allowed", + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + if test.wc != nil { + ctx = test.wc + } + + got := test.rs.Validate(ctx) + if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { + t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + } + }) + } +} diff --git a/pkg/apis/serving/v1beta1/revision_defaults_test.go b/pkg/apis/serving/v1beta1/revision_defaults_test.go index 7666f37b2d18..b3e4441020ff 100644 --- a/pkg/apis/serving/v1beta1/revision_defaults_test.go +++ b/pkg/apis/serving/v1beta1/revision_defaults_test.go @@ -259,6 +259,9 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, }, { Name: "helloworld", }}, @@ -271,13 +274,39 @@ func TestRevisionDefaulting(t *testing.T) { ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: "busybox", + Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, Resources: defaultResources, ReadinessProbe: defaultProbe, }, { - Name: "helloworld", - Resources: defaultResources, - ReadinessProbe: defaultProbe, + Name: "helloworld", + Resources: defaultResources, + }}, + }, + }, + }, + }, { + name: "multiple containers without container name", + in: &Revision{ + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + }, + }, + want: &Revision{ + Spec: v1.RevisionSpec{ + TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), + ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "user-container-0", + Resources: defaultResources, + }, { + Name: "user-container-1", + Resources: defaultResources, }}, }, }, diff --git a/pkg/apis/serving/v1beta1/revision_validation_test.go b/pkg/apis/serving/v1beta1/revision_validation_test.go index f8971547e5b4..c707485ea561 100644 --- a/pkg/apis/serving/v1beta1/revision_validation_test.go +++ b/pkg/apis/serving/v1beta1/revision_validation_test.go @@ -859,3 +859,188 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { }) } } + +func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { + logger := logtesting.TestLogger(t) + s := config.NewStore(logger) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-multi-container": "true", + }, + }) + + return s.ToContext(ctx) +} + +func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { + tests := []struct { + name string + r *Revision + wc context.Context + want *apis.FieldError + }{{ + name: "flag disabled: more than one container", + r: &Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid", + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + }, + want: apis.ErrMultipleOneOf("spec.containers"), + }, { + name: "flag enabled: more than one container with one container port", + r: &Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid", + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: nil, + }, { + name: "flag enabled: probes are not allowed for non serving containers", + r: &Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid", + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + ReadinessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"spec.containers[1].livenessProbe.timeoutSeconds", "spec.containers[1].readinessProbe.timeoutSeconds"}, + }, + }, { + name: "flag enabled: too many containers with no port", + r: &Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid", + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMissingField("spec.containers.ports"), + }, { + name: "flag enabled: too many containers with too many port", + r: &Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid", + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 9999, + }}, + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"spec.containers.ports"}, + Details: "Only a single port is allowed", + }, + }, { + name: "flag enabled: too many containers with too many port for a single container", + r: &Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid", + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }}, + }, + }, + }, + wc: enableMultiContainer(context.Background(), t), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"spec.containers.ports, spec.containers[0].ports"}, + Details: "Only a single port is allowed", + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + if test.wc != nil { + ctx = test.wc + } + got := test.r.Validate(ctx) + if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { + t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + } + }) + } +} From 35c9fb7dfddd7099ed48991c91590995df9d6b1f Mon Sep 17 00:00:00 2001 From: savitaashture Date: Sat, 7 Mar 2020 16:14:18 +0530 Subject: [PATCH 2/7] fix review comments --- pkg/apis/config/defaults.go | 17 ++---- pkg/apis/config/defaults_test.go | 12 ++-- pkg/apis/serving/k8s_validation.go | 54 ++++++++++------- pkg/apis/serving/k8s_validation_test.go | 54 ++++++++++------- pkg/apis/serving/v1/revision_defaults.go | 10 +--- .../serving/v1/revision_validation_test.go | 55 +++++++++++------ .../v1alpha1/revision_validation_test.go | 54 +++++++++++------ .../v1beta1/revision_validation_test.go | 60 +++++++++++++------ 8 files changed, 194 insertions(+), 122 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index f0df9de8d747..915560d668d0 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -70,22 +70,16 @@ func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) { nc := defaultConfig() // Process bool fields. - for _, b := range []struct { + b := struct { key string field *bool defaultValue bool }{ - { - key: "enable-multi-container", - field: &nc.EnableMultiContainer, - defaultValue: false, - }} { - if raw, ok := data[b.key]; !ok { - *b.field = b.defaultValue - } else { - *b.field = strings.EqualFold(raw, "true") - } + key: "enable-multi-container", + field: &nc.EnableMultiContainer, + defaultValue: false, } + nc.EnableMultiContainer = strings.EqualFold(data[b.key], "true") // Process int64 fields for _, i64 := range []struct { @@ -177,6 +171,7 @@ 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 RevisionTimeoutSeconds int64 diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index c58c92f881e3..e26ba829b95e 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -60,6 +60,7 @@ func TestDefaultsConfiguration(t *testing.T) { name: "specified values", wantErr: false, wantDefaults: &Defaults{ + EnableMultiContainer: true, RevisionTimeoutSeconds: 123, MaxRevisionTimeoutSeconds: 456, ContainerConcurrencyMaxLimit: 1984, @@ -67,6 +68,7 @@ func TestDefaultsConfiguration(t *testing.T) { UserContainerNameTemplate: "{{.Name}}", }, data: map[string]string{ + "enable-multi-container": "true", "revision-timeout-seconds": "123", "max-revision-timeout-seconds": "456", "revision-cpu-request": "123m", @@ -83,14 +85,8 @@ func TestDefaultsConfiguration(t *testing.T) { UserContainerNameTemplate: DefaultUserContainerName, ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency, }, - config: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace(), - Name: DefaultsConfigName, - }, - Data: map[string]string{ - "enable-multi-container": "invalid", - }, + data: map[string]string{ + "enable-multi-container": "invalid", }, }, { name: "bad revision timeout", diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index a92efe8b7b06..630977d7a3a2 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -260,21 +260,7 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError { errs = errs.Also(ValidateContainer(ps.Containers[0], volumes). ViaFieldIndex("containers", 0)) default: - cfg := config.FromContextOrDefaults(ctx).Defaults - if !cfg.EnableMultiContainer { - errs = errs.Also(apis.ErrMultipleOneOf("containers")) - } else { - errs = errs.Also(ValidateMultiContainerPorts(ps.Containers)).ViaField("containers") - for i := range ps.Containers { - // probes are not allowed other than serving container - // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZU - if len(ps.Containers[i].Ports) == 0 { - errs = errs.Also(ValidateSidecarContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) - } else { - errs = errs.Also(ValidateContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) - } - } - } + errs = errs.Also(validateContainers(ctx, ps.Containers, volumes)) } if ps.ServiceAccountName != "" { for range validation.IsDNS1123Subdomain(ps.ServiceAccountName) { @@ -284,8 +270,28 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError { return errs } -// ValidateMultiContainerPorts validates port when specified multiple containers -func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError { +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.ErrMultipleOneOf("containers")) + } else { + errs = errs.Also(validateContainersPorts(containers).ViaField("containers")) + for i := range containers { + // Probes are not allowed 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 errs *apis.FieldError @@ -293,14 +299,19 @@ func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError for i := range containers { count += len(containers[i].Ports) } + // When no container ports are specified if count == 0 { - errs = errs.Also(apis.ErrMissingField(apis.CurrentField)) + errs = errs.Also(apis.ErrMissingField("ports")) } - return errs.Also(portValidation(count)).ViaField("ports") + // Each container section have ports + if count > 1 { + errs = errs.Also(apis.ErrMultipleOneOf("ports")) + } + return errs } -// ValidateSidecarContainer validate fields for non serving containers -func ValidateSidecarContainer(container corev1.Container, volumes sets.String) *apis.FieldError { +// 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, @@ -316,6 +327,7 @@ func ValidateSidecarContainer(container corev1.Container, volumes sets.String) * // ValidateContainer validate fields for serving containers func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError { var errs *apis.FieldError + // Single container have multiple ports errs = errs.Also(portValidation(len(container.Ports))).ViaField("ports") // Liveness Probes errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe")) diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index b92eecd01253..9d74b003478a 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -205,9 +205,8 @@ func TestPodSpecValidation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { got := ValidatePodSpec(context.Background(), test.ps) - if !cmp.Equal(test.want.Error(), got.Error()) { - t.Errorf("ValidatePodSpec (-want, +got) = %v", - cmp.Diff(test.want.Error(), got.Error())) + if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { + t.Errorf("ValidatePodSpec (-want, +got): \n%s", diff) } }) } @@ -270,7 +269,7 @@ func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, }, }, { - name: "flag enabled: too many containers with no port", + name: "flag enabled: multiple containers with no port", ps: corev1.PodSpec{ Containers: []corev1.Container{{ Image: "busybox", @@ -281,7 +280,7 @@ func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { wc: enableMultiContainer(context.Background(), t), want: apis.ErrMissingField("containers.ports"), }, { - name: "flag enabled: too many containers with too many port", + name: "flag enabled: multiple containers with multiple port", ps: corev1.PodSpec{ Containers: []corev1.Container{{ Image: "busybox", @@ -295,14 +294,10 @@ func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }}, }}, }, - wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"containers.ports"}, - Details: "Only a single port is allowed", - }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMultipleOneOf("containers.ports"), }, { - name: "flag enabled: too many containers with too many port for a single container", + name: "flag enabled: multiple containers with multiple ports for each container", ps: corev1.PodSpec{ Containers: []corev1.Container{{ Image: "busybox", @@ -319,11 +314,31 @@ func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }}, }, wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ + want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ Message: "More than one container port is set", - Paths: []string{"containers.ports, containers[0].ports"}, + Paths: []string{"containers[0].ports"}, Details: "Only a single port is allowed", + }), + }, { + name: "flag enabled: multiple containers with multiple port for a single container", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }, { + Image: "helloworld", + }}, }, + wc: enableMultiContainer(context.Background(), 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", + }), }} for _, test := range tests { @@ -333,9 +348,8 @@ func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { ctx = test.wc } got := ValidatePodSpec(ctx, test.ps) - if !cmp.Equal(test.want.Error(), got.Error()) { - t.Errorf("ValidatePodSpec (-want, +got) = %v", - cmp.Diff(test.want.Error(), got.Error())) + if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { + t.Errorf("ValidatePodSpec (-want, +got): \n%s", diff) } }) } @@ -969,7 +983,7 @@ func TestContainerValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := ValidateContainer(test.c, test.volumes) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("ValidateContainer (-want, +got) = %v", diff) + t.Errorf("ValidateContainer (-want, +got): \n%s", diff) } }) } @@ -1201,7 +1215,7 @@ func TestVolumeValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := validateVolume(test.v) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("validateVolume (-want, +got) = %v", diff) + t.Errorf("validateVolume (-want, +got): \n%s", diff) } }) } @@ -1295,7 +1309,7 @@ func TestObjectReferenceValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := ValidateNamespacedObjectReference(test.r) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("ValidateNamespacedObjectReference (-want, +got) = %v", diff) + t.Errorf("ValidateNamespacedObjectReference (-want, +got): \n%s", diff) } }) } diff --git a/pkg/apis/serving/v1/revision_defaults.go b/pkg/apis/serving/v1/revision_defaults.go index 043981c7a4ed..67844f29c5b7 100644 --- a/pkg/apis/serving/v1/revision_defaults.go +++ b/pkg/apis/serving/v1/revision_defaults.go @@ -91,14 +91,10 @@ func (rs *RevisionSpec) applyDefault(container *corev1.Container, cfg *config.Co } } - if len(rs.PodSpec.Containers) == 1 { + // If there are multiple containers then default probes will be applied to the container where user specified PORT + // default probes will not be applied for non serving containers + if len(rs.PodSpec.Containers) == 1 || len(container.Ports) != 0 { rs.applyProbes(container) - } else { - // If there are multiple containers then default probes will be applied to the container where user specified PORT - // default probes will not be applied for non serving containers - if len(container.Ports) != 0 { - rs.applyProbes(container) - } } vms := container.VolumeMounts diff --git a/pkg/apis/serving/v1/revision_validation_test.go b/pkg/apis/serving/v1/revision_validation_test.go index ba71813803b6..1650563ac975 100644 --- a/pkg/apis/serving/v1/revision_validation_test.go +++ b/pkg/apis/serving/v1/revision_validation_test.go @@ -81,9 +81,8 @@ func TestRevisionValidation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { got := test.r.Validate(context.Background()) - if !cmp.Equal(test.want.Error(), got.Error()) { - t.Errorf("Validate (-want, +got) = %v", - cmp.Diff(test.want.Error(), got.Error())) + if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -237,7 +236,7 @@ func TestRevisionLabelAnnotationValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := test.r.Validate(context.Background()) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %s", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -276,7 +275,7 @@ func TestContainerConcurrencyValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := serving.ValidateContainerConcurrency(&test.cc) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -444,7 +443,7 @@ func TestRevisionSpecValidation(t *testing.T) { } got := test.rs.Validate(ctx) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -902,7 +901,7 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { got := test.rts.Validate(ctx) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -987,7 +986,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, }, }, { - name: "flag enabled: too many containers with no port", + name: "flag enabled: multiple containers with no port", rs: &RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ @@ -1000,7 +999,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { wc: enableMultiContainer(context.Background(), t), want: apis.ErrMissingField("containers.ports"), }, { - name: "flag enabled: too many containers with too many port", + name: "flag enabled: multiple containers with multiple port", rs: &RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ @@ -1016,14 +1015,10 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }}, }, }, - wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"containers.ports"}, - Details: "Only a single port is allowed", - }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMultipleOneOf("containers.ports"), }, { - name: "flag enabled: too many containers with too many port for a single container", + name: "flag enabled: multiple containers with multiple port for each container", rs: &RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ @@ -1042,11 +1037,33 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }, }, wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ + want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ Message: "More than one container port is set", - Paths: []string{"containers.ports, containers[0].ports"}, + Paths: []string{"containers[0].ports"}, Details: "Only a single port is allowed", + }), + }, { + name: "flag enabled: multiple containers with multiple port for a single container", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }}, + }, }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers[1].ports"}, + Details: "Only a single port is allowed", + }), }} for _, test := range tests { @@ -1058,7 +1075,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { got := test.rs.Validate(ctx) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index 9f76cc53652a..1998db2a112a 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -69,7 +69,7 @@ func TestConcurrencyModelValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := test.cm.Validate(context.Background()) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("Validate (-want, +got) = %v", diff) + t.Errorf("Validate (-want, +got) = %s", diff) } }) } @@ -110,7 +110,7 @@ func TestServingStateType(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := test.cm.Validate(context.Background()) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("Validate (-want, +got) = %v", diff) + t.Errorf("Validate (-want, +got): \n%s", diff) } }) } @@ -346,7 +346,7 @@ func TestRevisionSpecValidation(t *testing.T) { } got := test.rs.Validate(ctx) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("Validate (-want, +got) = %v", diff) + t.Errorf("Validate (-want, +got): \n%s", diff) } }) } @@ -506,7 +506,7 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { got := test.rts.Validate(ctx) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("Validate (-want, +got) = %v", diff) + t.Errorf("Validate (-want, +got): \n%s", diff) } }) } @@ -862,7 +862,7 @@ func TestImmutableFields(t *testing.T) { } got := test.new.Validate(ctx) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("Validate (-want, +got) = %v", diff) + t.Errorf("Validate (-want, +got): \n%s", diff) } }) } @@ -974,7 +974,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, }, }, { - name: "flag enabled: too many containers with no port", + name: "flag enabled: multiple containers with no port", rs: &RevisionSpec{ RevisionSpec: v1.RevisionSpec{ PodSpec: corev1.PodSpec{ @@ -989,7 +989,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { wc: enableMultiContainer(context.Background(), t), want: apis.ErrMissingField("containers.ports"), }, { - name: "flag enabled: too many containers with too many port", + name: "flag enabled: multiple containers with multiple port", rs: &RevisionSpec{ RevisionSpec: v1.RevisionSpec{ PodSpec: corev1.PodSpec{ @@ -1007,14 +1007,10 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }, }, }, - wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"containers.ports"}, - Details: "Only a single port is allowed", - }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMultipleOneOf("containers.ports"), }, { - name: "flag enabled: too many containers with too many port for a single container", + name: "flag enabled: multiple containers with multiple port for each container", rs: &RevisionSpec{ RevisionSpec: v1.RevisionSpec{ PodSpec: corev1.PodSpec{ @@ -1035,11 +1031,35 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }, }, wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ + want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ Message: "More than one container port is set", - Paths: []string{"containers.ports, containers[0].ports"}, + Paths: []string{"containers[0].ports"}, Details: "Only a single port is allowed", + }), + }, { + name: "flag enabled: multiple containers with multiple port for a single container", + rs: &RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }}, + }, + }, }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers[1].ports"}, + Details: "Only a single port is allowed", + }), }} for _, test := range tests { @@ -1051,7 +1071,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { got := test.rs.Validate(ctx) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } diff --git a/pkg/apis/serving/v1beta1/revision_validation_test.go b/pkg/apis/serving/v1beta1/revision_validation_test.go index c707485ea561..52168be81e43 100644 --- a/pkg/apis/serving/v1beta1/revision_validation_test.go +++ b/pkg/apis/serving/v1beta1/revision_validation_test.go @@ -83,9 +83,8 @@ func TestRevisionValidation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { got := test.r.Validate(context.Background()) - if !cmp.Equal(test.want.Error(), got.Error()) { - t.Errorf("Validate (-want, +got) = %v", - cmp.Diff(test.want.Error(), got.Error())) + if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -239,7 +238,7 @@ func TestRevisionLabelAnnotationValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := test.r.Validate(context.Background()) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %s", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -278,7 +277,7 @@ func TestContainerConcurrencyValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := serving.ValidateContainerConcurrency(&test.cc) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -446,7 +445,7 @@ func TestRevisionSpecValidation(t *testing.T) { } got := test.rs.Validate(ctx) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -854,7 +853,7 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { got := test.rts.Validate(ctx) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } @@ -954,7 +953,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { Paths: []string{"spec.containers[1].livenessProbe.timeoutSeconds", "spec.containers[1].readinessProbe.timeoutSeconds"}, }, }, { - name: "flag enabled: too many containers with no port", + name: "flag enabled: multiple containers with no port", r: &Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "valid", @@ -972,7 +971,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { wc: enableMultiContainer(context.Background(), t), want: apis.ErrMissingField("spec.containers.ports"), }, { - name: "flag enabled: too many containers with too many port", + name: "flag enabled: multiple containers with multiple port", r: &Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "valid", @@ -993,14 +992,10 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }, }, }, - wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"spec.containers.ports"}, - Details: "Only a single port is allowed", - }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMultipleOneOf("spec.containers.ports"), }, { - name: "flag enabled: too many containers with too many port for a single container", + name: "flag enabled: multiple containers with multiple port for each container", r: &Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "valid", @@ -1024,11 +1019,38 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }, }, wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ + want: apis.ErrMultipleOneOf("spec.containers.ports").Also(&apis.FieldError{ Message: "More than one container port is set", - Paths: []string{"spec.containers.ports, spec.containers[0].ports"}, + Paths: []string{"spec.containers[0].ports"}, Details: "Only a single port is allowed", + }), + }, { + name: "flag enabled: multiple containers with multiple port for a single container", + r: &Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid", + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }, { + ContainerPort: 9999, + }}, + }}, + }, + }, }, + wc: enableMultiContainer(context.Background(), t), + want: apis.ErrMultipleOneOf("spec.containers.ports").Also(&apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"spec.containers[1].ports"}, + Details: "Only a single port is allowed", + }), }} for _, test := range tests { @@ -1039,7 +1061,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { } got := test.r.Validate(ctx) if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got) = %v", cmp.Diff(want, got)) + t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) } }) } From c6efde9aeddd3aee61e7c51e3207a95f7e8c9b30 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Mon, 9 Mar 2020 16:16:49 +0530 Subject: [PATCH 3/7] fix review comment --- pkg/apis/config/defaults.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 915560d668d0..50a54488e06d 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -69,17 +69,8 @@ func defaultConfig() *Defaults { func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) { nc := defaultConfig() - // Process bool fields. - b := struct { - key string - field *bool - defaultValue bool - }{ - key: "enable-multi-container", - field: &nc.EnableMultiContainer, - defaultValue: false, - } - nc.EnableMultiContainer = strings.EqualFold(data[b.key], "true") + // Process bool field. + nc.EnableMultiContainer = strings.EqualFold(data["enable-multi-container"], "true") // Process int64 fields for _, i64 := range []struct { From 50f139769a9cb1f4052f6d5dd5237a1af51c2047 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Tue, 10 Mar 2020 12:18:56 +0530 Subject: [PATCH 4/7] fix nit and changed error message for multiple conatiners when flag disabled --- pkg/apis/serving/k8s_validation.go | 9 +++++---- pkg/apis/serving/k8s_validation_test.go | 4 ++-- pkg/apis/serving/v1/revision_validation_test.go | 4 ++-- pkg/apis/serving/v1alpha1/revision_validation_test.go | 2 +- pkg/apis/serving/v1beta1/revision_validation_test.go | 4 ++-- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 630977d7a3a2..3fcc4732a2b7 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -274,11 +274,12 @@ func validateContainers(ctx context.Context, containers []corev1.Container, volu var errs *apis.FieldError cfg := config.FromContextOrDefaults(ctx).Defaults if !cfg.EnableMultiContainer { - errs = errs.Also(apis.ErrMultipleOneOf("containers")) + 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 other than serving container, + // 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)) @@ -299,11 +300,11 @@ func validateContainersPorts(containers []corev1.Container) *apis.FieldError { for i := range containers { count += len(containers[i].Ports) } - // When no container ports are specified + // When no container ports are specified. if count == 0 { errs = errs.Also(apis.ErrMissingField("ports")) } - // Each container section have ports + // More than one container sections have ports. if count > 1 { errs = errs.Also(apis.ErrMultipleOneOf("ports")) } diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index 9d74b003478a..3d90561e1bad 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -179,7 +179,7 @@ func TestPodSpecValidation(t *testing.T) { Image: "helloworld", }}, }, - want: apis.ErrMultipleOneOf("containers"), + want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, }, { name: "extra field", ps: corev1.PodSpec{ @@ -230,7 +230,7 @@ func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { Image: "helloworld", }}, }, - want: apis.ErrMultipleOneOf("containers"), + want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, }, { name: "flag enabled: more than one container with one container port", ps: corev1.PodSpec{ diff --git a/pkg/apis/serving/v1/revision_validation_test.go b/pkg/apis/serving/v1/revision_validation_test.go index 1650563ac975..9ca3c57f409b 100644 --- a/pkg/apis/serving/v1/revision_validation_test.go +++ b/pkg/apis/serving/v1/revision_validation_test.go @@ -382,7 +382,7 @@ func TestRevisionSpecValidation(t *testing.T) { }}, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, }, { name: "exceed max timeout", rs: &RevisionSpec{ @@ -943,7 +943,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }}, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, }, { name: "flag enabled: more than one container with one container port", rs: &RevisionSpec{ diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index 1998db2a112a..5b88086e7b0f 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -927,7 +927,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, }, { name: "flag enabled: more than one container with one container port", rs: &RevisionSpec{ diff --git a/pkg/apis/serving/v1beta1/revision_validation_test.go b/pkg/apis/serving/v1beta1/revision_validation_test.go index 52168be81e43..365cfcf206b4 100644 --- a/pkg/apis/serving/v1beta1/revision_validation_test.go +++ b/pkg/apis/serving/v1beta1/revision_validation_test.go @@ -384,7 +384,7 @@ func TestRevisionSpecValidation(t *testing.T) { }}, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, }, { name: "exceed max timeout", rs: &v1.RevisionSpec{ @@ -900,7 +900,7 @@ func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { }, }, }, - want: apis.ErrMultipleOneOf("spec.containers"), + want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, }, { name: "flag enabled: more than one container with one container port", r: &Revision{ From 04f589b9408ed68a56981577675794bb7589fec2 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Thu, 12 Mar 2020 00:01:52 +0530 Subject: [PATCH 5/7] add logic to generate unique names for containers --- pkg/apis/serving/v1/revision_defaults.go | 7 ++- pkg/apis/serving/v1/revision_defaults_test.go | 38 ++++++-------- .../v1alpha1/revision_defaults_test.go | 49 +++++++------------ .../serving/v1beta1/revision_defaults_test.go | 38 ++++++-------- 4 files changed, 51 insertions(+), 81 deletions(-) diff --git a/pkg/apis/serving/v1/revision_defaults.go b/pkg/apis/serving/v1/revision_defaults.go index 67844f29c5b7..0849af3742db 100644 --- a/pkg/apis/serving/v1/revision_defaults.go +++ b/pkg/apis/serving/v1/revision_defaults.go @@ -18,10 +18,12 @@ package v1 import ( "context" - "strconv" + + "github.com/google/uuid" corev1 "k8s.io/api/core/v1" "knative.dev/pkg/apis" + "knative.dev/pkg/kmeta" "knative.dev/pkg/ptr" "knative.dev/serving/pkg/apis/config" ) @@ -53,7 +55,7 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) { for idx := range rs.PodSpec.Containers { if rs.PodSpec.Containers[idx].Name == "" { if len(rs.PodSpec.Containers) > 1 { - rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) + "-" + strconv.Itoa(idx) + rs.PodSpec.Containers[idx].Name = kmeta.ChildName(cfg.Defaults.UserContainerName(ctx), "-"+uuid.New().String()) } else { rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) } @@ -62,6 +64,7 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) { rs.applyDefault(&rs.PodSpec.Containers[idx], cfg) } } + func (rs *RevisionSpec) applyDefault(container *corev1.Container, cfg *config.Config) { if container.Resources.Requests == nil { container.Resources.Requests = corev1.ResourceList{} diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index e159f1d36ddd..59583b8cb2e4 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -332,30 +332,6 @@ func TestRevisionDefaulting(t *testing.T) { }, }, }, - }, { - name: "multiple containers without container name", - in: &Revision{ - Spec: RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{}, {}}, - }, - }, - }, - want: &Revision{ - Spec: RevisionSpec{ - TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), - ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Name: "user-container-0", - Resources: defaultResources, - }, { - Name: "user-container-1", - Resources: defaultResources, - }}, - }, - }, - }, }} for _, test := range tests { @@ -373,3 +349,17 @@ func TestRevisionDefaulting(t *testing.T) { }) } } + +func TestRevisionDefaultingContainerName(t *testing.T) { + got := &Revision{ + Spec: RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + }, + } + got.SetDefaults(context.Background()) + if got.Spec.Containers[0].Name == "" && got.Spec.Containers[1].Name == "" { + t.Errorf("Failed to set default values for container name") + } +} diff --git a/pkg/apis/serving/v1alpha1/revision_defaults_test.go b/pkg/apis/serving/v1alpha1/revision_defaults_test.go index 39a6d4f89009..436e71a1c50f 100644 --- a/pkg/apis/serving/v1alpha1/revision_defaults_test.go +++ b/pkg/apis/serving/v1alpha1/revision_defaults_test.go @@ -338,37 +338,6 @@ func TestRevisionDefaulting(t *testing.T) { }, }, }, - }, { - name: "multiple containers without container name", - wc: v1.WithUpgradeViaDefaulting, - in: &Revision{ - Spec: RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{}, {}}, - }, - ContainerConcurrency: ptr.Int64(1), - TimeoutSeconds: ptr.Int64(99), - }, - }, - }, - want: &Revision{ - Spec: RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Name: "user-container-0", - Resources: defaultResources, - }, { - Name: "user-container-1", - Resources: defaultResources, - }}, - }, - ContainerConcurrency: ptr.Int64(1), - TimeoutSeconds: ptr.Int64(99), - }, - }, - }, }} for _, test := range tests { @@ -385,3 +354,21 @@ func TestRevisionDefaulting(t *testing.T) { }) } } + +func TestRevisionDefaultingContainerName(t *testing.T) { + got := &Revision{ + Spec: RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + ContainerConcurrency: ptr.Int64(1), + TimeoutSeconds: ptr.Int64(99), + }, + }, + } + got.SetDefaults(context.Background()) + if got.Spec.Containers[0].Name == "" && got.Spec.Containers[1].Name == "" { + t.Errorf("Failed to set default values for container name") + } +} diff --git a/pkg/apis/serving/v1beta1/revision_defaults_test.go b/pkg/apis/serving/v1beta1/revision_defaults_test.go index b3e4441020ff..2b131d917e13 100644 --- a/pkg/apis/serving/v1beta1/revision_defaults_test.go +++ b/pkg/apis/serving/v1beta1/revision_defaults_test.go @@ -287,30 +287,6 @@ func TestRevisionDefaulting(t *testing.T) { }, }, }, - }, { - name: "multiple containers without container name", - in: &Revision{ - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{}, {}}, - }, - }, - }, - want: &Revision{ - Spec: v1.RevisionSpec{ - TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), - ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Name: "user-container-0", - Resources: defaultResources, - }, { - Name: "user-container-1", - Resources: defaultResources, - }}, - }, - }, - }, }} for _, test := range tests { @@ -328,3 +304,17 @@ func TestRevisionDefaulting(t *testing.T) { }) } } + +func TestRevisionDefaultingContainerName(t *testing.T) { + got := &Revision{ + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + }, + } + got.SetDefaults(context.Background()) + if got.Spec.Containers[0].Name == "" && got.Spec.Containers[1].Name == "" { + t.Errorf("Failed to set default values for container name") + } +} From ee447181b8b0c4a29cd0d5521b2be74a5be0ae34 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Thu, 12 Mar 2020 17:28:44 +0530 Subject: [PATCH 6/7] fix review comments --- pkg/apis/serving/k8s_validation.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 3fcc4732a2b7..4edc3fe3645e 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -293,22 +293,19 @@ func validateContainers(ctx context.Context, containers []corev1.Container, volu // validateContainersPorts validates port when specified multiple containers func validateContainersPorts(containers []corev1.Container) *apis.FieldError { - var ( - count int - errs *apis.FieldError - ) + var count int for i := range containers { count += len(containers[i].Ports) } // When no container ports are specified. if count == 0 { - errs = errs.Also(apis.ErrMissingField("ports")) + return apis.ErrMissingField("ports") } // More than one container sections have ports. if count > 1 { - errs = errs.Also(apis.ErrMultipleOneOf("ports")) + return apis.ErrMultipleOneOf("ports") } - return errs + return nil } // validateSidecarContainer validate fields for non serving containers @@ -329,7 +326,7 @@ func validateSidecarContainer(container corev1.Container, volumes sets.String) * func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError { var errs *apis.FieldError // Single container have multiple ports - errs = errs.Also(portValidation(len(container.Ports))).ViaField("ports") + errs = errs.Also(portValidation(container.Ports).ViaField("ports")) // Liveness Probes errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe")) // Readiness Probes @@ -337,8 +334,8 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi return errs.Also(validate(container, volumes)) } -func portValidation(count int) *apis.FieldError { - if count > 1 { +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}, From 47e0ee4c2794e48af9e2f50a9eb2a86fde858317 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 13 Mar 2020 11:17:33 +0530 Subject: [PATCH 7/7] fix review comments and removed duplicate testcases --- pkg/apis/serving/k8s_validation.go | 2 +- pkg/apis/serving/k8s_validation_test.go | 19 +- .../serving/v1/revision_validation_test.go | 174 --------------- .../v1alpha1/revision_validation_test.go | 190 +--------------- .../v1beta1/revision_validation_test.go | 208 ------------------ 5 files changed, 6 insertions(+), 587 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 4edc3fe3645e..8de357613836 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -325,7 +325,7 @@ func validateSidecarContainer(container corev1.Container, volumes sets.String) * // ValidateContainer validate fields for serving containers func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError { var errs *apis.FieldError - // Single container have multiple ports + // Single container cannot have multiple ports errs = errs.Also(portValidation(container.Ports).ViaField("ports")) // Liveness Probes errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe")) diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index 3d90561e1bad..081e3bfcce26 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -25,30 +25,19 @@ import ( "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" "knative.dev/pkg/apis" - logtesting "knative.dev/pkg/logging/testing" "knative.dev/pkg/ptr" "knative.dev/serving/pkg/apis/config" - autoscalerconfig "knative.dev/serving/pkg/autoscaler/config" ) func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { - logger := logtesting.TestLogger(t) - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-multi-container": "true", + return config.ToContext(ctx, &config.Config{ + Defaults: &config.Defaults{ + EnableMultiContainer: true, }, }) - - return s.ToContext(ctx) } func TestPodSpecValidation(t *testing.T) { @@ -212,7 +201,7 @@ func TestPodSpecValidation(t *testing.T) { } } -func TestPodSpecValidationOnUpdateDefaultConfigMap(t *testing.T) { +func TestPodSpecMultiContainerValidation(t *testing.T) { tests := []struct { name string ps corev1.PodSpec diff --git a/pkg/apis/serving/v1/revision_validation_test.go b/pkg/apis/serving/v1/revision_validation_test.go index 9ca3c57f409b..906cba18f4cf 100644 --- a/pkg/apis/serving/v1/revision_validation_test.go +++ b/pkg/apis/serving/v1/revision_validation_test.go @@ -906,177 +906,3 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { }) } } - -func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { - logger := logtesting.TestLogger(t) - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-multi-container": "true", - }, - }) - - return s.ToContext(ctx) -} - -func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { - tests := []struct { - name string - rs *RevisionSpec - wc context.Context - want *apis.FieldError - }{{ - name: "flag disabled: more than one container", - rs: &RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - }}, - }, - }, - want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, - }, { - name: "flag enabled: more than one container with one container port", - rs: &RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - }}, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: nil, - }, { - name: "flag enabled: probes are not allowed for non serving containers", - rs: &RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - LivenessProbe: &corev1.Probe{ - TimeoutSeconds: 1, - }, - ReadinessProbe: &corev1.Probe{ - TimeoutSeconds: 1, - }, - }}, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ - Message: "must not set the field(s)", - Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, - }, - }, { - name: "flag enabled: multiple containers with no port", - rs: &RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - }, { - Image: "helloworld", - }}, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMissingField("containers.ports"), - }, { - name: "flag enabled: multiple containers with multiple port", - rs: &RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 9999, - }}, - }}, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMultipleOneOf("containers.ports"), - }, { - name: "flag enabled: multiple containers with multiple port for each container", - rs: &RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }, { - ContainerPort: 9999, - }}, - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 80, - }}, - }}, - }, - }, - wc: enableMultiContainer(context.Background(), 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", - }), - }, { - name: "flag enabled: multiple containers with multiple port for a single container", - rs: &RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }, { - ContainerPort: 9999, - }}, - }}, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"containers[1].ports"}, - Details: "Only a single port is allowed", - }), - }} - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - ctx := context.Background() - if test.wc != nil { - ctx = test.wc - } - - got := test.rs.Validate(ctx) - if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) - } - }) - } -} diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index 5b88086e7b0f..46cf1bb04a8a 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -69,7 +69,7 @@ func TestConcurrencyModelValidation(t *testing.T) { t.Run(test.name, func(t *testing.T) { got := test.cm.Validate(context.Background()) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("Validate (-want, +got) = %s", diff) + t.Errorf("Validate (-want, +got): \n%s", diff) } }) } @@ -888,191 +888,3 @@ func TestRevisionProtocolType(t *testing.T) { } } } - -func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { - logger := logtesting.TestLogger(t) - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-multi-container": "true", - }, - }) - - return s.ToContext(ctx) -} - -func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { - tests := []struct { - name string - rs *RevisionSpec - wc context.Context - want *apis.FieldError - }{{ - name: "flag disabled: more than one container", - rs: &RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - }}, - }, - }, - }, - want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, - }, { - name: "flag enabled: more than one container with one container port", - rs: &RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: nil, - }, { - name: "flag enabled: probes are not allowed for non serving containers", - rs: &RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - LivenessProbe: &corev1.Probe{ - TimeoutSeconds: 1, - }, - ReadinessProbe: &corev1.Probe{ - TimeoutSeconds: 1, - }, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ - Message: "must not set the field(s)", - Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, - }, - }, { - name: "flag enabled: multiple containers with no port", - rs: &RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - }, { - Image: "helloworld", - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMissingField("containers.ports"), - }, { - name: "flag enabled: multiple containers with multiple port", - rs: &RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 9999, - }}, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMultipleOneOf("containers.ports"), - }, { - name: "flag enabled: multiple containers with multiple port for each container", - rs: &RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }, { - ContainerPort: 9999, - }}, - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 80, - }}, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), 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", - }), - }, { - name: "flag enabled: multiple containers with multiple port for a single container", - rs: &RevisionSpec{ - RevisionSpec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }, { - ContainerPort: 9999, - }}, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"containers[1].ports"}, - Details: "Only a single port is allowed", - }), - }} - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - ctx := context.Background() - if test.wc != nil { - ctx = test.wc - } - - got := test.rs.Validate(ctx) - if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) - } - }) - } -} diff --git a/pkg/apis/serving/v1beta1/revision_validation_test.go b/pkg/apis/serving/v1beta1/revision_validation_test.go index 365cfcf206b4..39977302e689 100644 --- a/pkg/apis/serving/v1beta1/revision_validation_test.go +++ b/pkg/apis/serving/v1beta1/revision_validation_test.go @@ -858,211 +858,3 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { }) } } - -func enableMultiContainer(ctx context.Context, t *testing.T) context.Context { - logger := logtesting.TestLogger(t) - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-multi-container": "true", - }, - }) - - return s.ToContext(ctx) -} - -func TestRevpecValidationOnUpdateDefaultConfigMap(t *testing.T) { - tests := []struct { - name string - r *Revision - wc context.Context - want *apis.FieldError - }{{ - name: "flag disabled: more than one container", - r: &Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - }}, - }, - }, - }, - want: &apis.FieldError{Message: "enable-multi-container is off, but found 2 containers"}, - }, { - name: "flag enabled: more than one container with one container port", - r: &Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: nil, - }, { - name: "flag enabled: probes are not allowed for non serving containers", - r: &Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - LivenessProbe: &corev1.Probe{ - TimeoutSeconds: 1, - }, - ReadinessProbe: &corev1.Probe{ - TimeoutSeconds: 1, - }, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: &apis.FieldError{ - Message: "must not set the field(s)", - Paths: []string{"spec.containers[1].livenessProbe.timeoutSeconds", "spec.containers[1].readinessProbe.timeoutSeconds"}, - }, - }, { - name: "flag enabled: multiple containers with no port", - r: &Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - }, { - Image: "helloworld", - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMissingField("spec.containers.ports"), - }, { - name: "flag enabled: multiple containers with multiple port", - r: &Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }}, - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 9999, - }}, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMultipleOneOf("spec.containers.ports"), - }, { - name: "flag enabled: multiple containers with multiple port for each container", - r: &Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }, { - ContainerPort: 9999, - }}, - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 80, - }}, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMultipleOneOf("spec.containers.ports").Also(&apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"spec.containers[0].ports"}, - Details: "Only a single port is allowed", - }), - }, { - name: "flag enabled: multiple containers with multiple port for a single container", - r: &Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "busybox", - }, { - Image: "helloworld", - Ports: []corev1.ContainerPort{{ - ContainerPort: 8888, - }, { - ContainerPort: 9999, - }}, - }}, - }, - }, - }, - wc: enableMultiContainer(context.Background(), t), - want: apis.ErrMultipleOneOf("spec.containers.ports").Also(&apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{"spec.containers[1].ports"}, - Details: "Only a single port is allowed", - }), - }} - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - ctx := context.Background() - if test.wc != nil { - ctx = test.wc - } - got := test.r.Validate(ctx) - if got, want := got.Error(), test.want.Error(); !cmp.Equal(got, want) { - t.Errorf("Validate (-want, +got): \n%s", cmp.Diff(want, got)) - } - }) - } -}