From 009ed661f8c3253e8a7dca03068f73de12448d1d Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Thu, 22 Feb 2018 14:34:14 -0800 Subject: [PATCH 1/7] Add validation for ContainerSpec of Configurations. Even though ContainerSpec accepts all fields of core/v1/Container, the controller will silently overwrite some fields, like Name, Resources, Ports, and VolumeMounts. This will cause confusions if the users expect their settings to be effective. This change disallows specifying those fields to avoid the confusions. --- pkg/webhook/BUILD.bazel | 1 + pkg/webhook/configuration.go | 44 ++++++++++++++++++++++-- pkg/webhook/configuration_test.go | 57 +++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 3 deletions(-) diff --git a/pkg/webhook/BUILD.bazel b/pkg/webhook/BUILD.bazel index cb404dfd41b5..7d60b6cce051 100644 --- a/pkg/webhook/BUILD.bazel +++ b/pkg/webhook/BUILD.bazel @@ -44,6 +44,7 @@ go_test( "//vendor/github.com/mattbaird/jsonpatch:go_default_library", "//vendor/k8s.io/api/admission/v1beta1:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/client-go/kubernetes/fake:go_default_library", ], diff --git a/pkg/webhook/configuration.go b/pkg/webhook/configuration.go index 0847eb538603..c05df23c6ee6 100644 --- a/pkg/webhook/configuration.go +++ b/pkg/webhook/configuration.go @@ -17,11 +17,14 @@ package webhook import ( "errors" + "fmt" "reflect" + "strings" "github.com/golang/glog" "github.com/google/elafros/pkg/apis/ela/v1alpha1" "github.com/mattbaird/jsonpatch" + corev1 "k8s.io/api/core/v1" ) var ( @@ -57,10 +60,45 @@ func validateConfiguration(configuration *v1alpha1.Configuration) error { if reflect.DeepEqual(configuration.Spec, v1alpha1.ConfigurationSpec{}) { return errEmptySpecInConfiguration } - // TODO: add validation for configuration.Spec.Template, after we add a - // validation for Revision. - if reflect.DeepEqual(configuration.Spec.Template, v1alpha1.Revision{}) { + if err := validateTemplate(&configuration.Spec.Template); err != nil { + return err + } + return nil +} + +func validateTemplate(template *v1alpha1.Revision) error { + if reflect.DeepEqual(*template, v1alpha1.Revision{}) { return errEmptyTemplateInSpec } + if err := validateContainerSpec(template.Spec.ContainerSpec); err != nil { + return err + } + return nil +} + +func validateContainerSpec(container *corev1.Container) error { + if container == nil { + return nil + } + // Some corev1.Container fields are set by Elafros controller. We disallow them + // here to avoid silently overwriting these fields and causing confusions for + // the users. + var ignoredFields []string + if container.Name != "" { + ignoredFields = append(ignoredFields, "template.spec.containerSpec.name") + } + if !reflect.DeepEqual(container.Resources, corev1.ResourceRequirements{}) { + ignoredFields = append(ignoredFields, "template.spec.containerSpec.resources") + } + if len(container.Ports) > 0 { + ignoredFields = append(ignoredFields, "template.spec.containerSpec.ports") + } + if len(container.VolumeMounts) > 0 { + ignoredFields = append(ignoredFields, "template.spec.containerSpec.volumeMounts") + } + if len(ignoredFields) > 0 { + // Complain about all ignored fields so that user can remove them all at once. + return fmt.Errorf("The configuration spec must not set the field(s) %s", strings.Join(ignoredFields, ", ")) + } return nil } diff --git a/pkg/webhook/configuration_test.go b/pkg/webhook/configuration_test.go index 235081e956b6..ea3643bfb57d 100644 --- a/pkg/webhook/configuration_test.go +++ b/pkg/webhook/configuration_test.go @@ -16,9 +16,13 @@ limitations under the License. package webhook import ( + "fmt" + "strings" "testing" "github.com/google/elafros/pkg/apis/ela/v1alpha1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -66,3 +70,56 @@ func TestEmptyTemplateInSpecNotAllowed(t *testing.T) { t.Fatalf("Expected: %s. Failed with %s", errEmptyTemplateInSpec, err) } } + +func TestUnwantedFieldInContainerSpecNotAllowed(t *testing.T) { + container := corev1.Container{ + Name: "Not Allowed", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceName("cpu"): resource.MustParse("25m"), + }, + }, + Ports: []corev1.ContainerPort{{ + Name: "http", + ContainerPort: 8080, + }}, + VolumeMounts: []corev1.VolumeMount{{ + MountPath: "mount/path", + Name: "name", + }}, + } + configuration := v1alpha1.Configuration{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: testConfigurationName, + }, + Spec: v1alpha1.ConfigurationSpec{ + Generation: testGeneration, + Template: v1alpha1.Revision{ + Spec: v1alpha1.RevisionSpec{ + ContainerSpec: &container, + }, + }, + }, + } + unwanted := []string{ + "template.spec.containerSpec.name", + "template.spec.containerSpec.resources", + "template.spec.containerSpec.ports", + "template.spec.containerSpec.volumeMounts", + } + expected := fmt.Sprintf("The configuration spec must not set the field(s) %s", strings.Join(unwanted, ", ")) + if err := ValidateConfiguration(nil, &configuration, &configuration); err == nil || err.Error() != expected { + t.Fatalf("Expected: %s. Failed with %s", expected, err) + } + container.Name = "" + expected = fmt.Sprintf("The configuration spec must not set the field(s) %s", strings.Join(unwanted[1:], ", ")) + if err := ValidateConfiguration(nil, &configuration, &configuration); err == nil || err.Error() != expected { + t.Fatalf("Expected: %s. Failed with %s", expected, err) + } + container.Resources = corev1.ResourceRequirements{} + expected = fmt.Sprintf("The configuration spec must not set the field(s) %s", strings.Join(unwanted[2:], ", ")) + if err := ValidateConfiguration(nil, &configuration, &configuration); err == nil || err.Error() != expected { + t.Fatalf("Expected: %s. Failed with %s", expected, err) + } +} From 584674452d0551a9b164acc4204d703a7e452a8e Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Thu, 22 Feb 2018 16:11:39 -0800 Subject: [PATCH 2/7] Disallow empty ContainerSpec in a Configurations' Template. --- pkg/webhook/configuration.go | 11 ++++++----- pkg/webhook/configuration_test.go | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/pkg/webhook/configuration.go b/pkg/webhook/configuration.go index c05df23c6ee6..f84252e95605 100644 --- a/pkg/webhook/configuration.go +++ b/pkg/webhook/configuration.go @@ -28,9 +28,10 @@ import ( ) var ( - errEmptySpecInConfiguration = errors.New("The configuration must have configuration spec") - errEmptyTemplateInSpec = errors.New("The configuration spec must have configuration") - errInvalidConfigurationInput = errors.New("Failed to convert input into configuration") + errEmptyContainerSpecInTemplate = errors.New("The configuration template must have a container spec") + errEmptySpecInConfiguration = errors.New("The configuration must have configuration spec") + errEmptyTemplateInSpec = errors.New("The configuration spec must have configuration") + errInvalidConfigurationInput = errors.New("Failed to convert input into configuration") ) // ValidateConfiguration is Configuration resource specific validation and mutation handler @@ -77,8 +78,8 @@ func validateTemplate(template *v1alpha1.Revision) error { } func validateContainerSpec(container *corev1.Container) error { - if container == nil { - return nil + if container == nil || reflect.DeepEqual(*container, corev1.Container{}) { + return errEmptyContainerSpecInTemplate } // Some corev1.Container fields are set by Elafros controller. We disallow them // here to avoid silently overwriting these fields and causing confusions for diff --git a/pkg/webhook/configuration_test.go b/pkg/webhook/configuration_test.go index ea3643bfb57d..44b7de017ac3 100644 --- a/pkg/webhook/configuration_test.go +++ b/pkg/webhook/configuration_test.go @@ -71,6 +71,27 @@ func TestEmptyTemplateInSpecNotAllowed(t *testing.T) { } } +func TestEmptyContainerSpecNotAllowed(t *testing.T) { + configuration := v1alpha1.Configuration{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: testConfigurationName, + }, + Spec: v1alpha1.ConfigurationSpec{ + Generation: testGeneration, + Template: v1alpha1.Revision{ + Spec: v1alpha1.RevisionSpec{ + ContainerSpec: &corev1.Container{}, + }, + }, + }, + } + + if err := ValidateConfiguration(nil, &configuration, &configuration); err != errEmptyContainerSpecInTemplate { + t.Fatalf("Expected: %s. Failed with %s", errEmptyTemplateInSpec, err) + } +} + func TestUnwantedFieldInContainerSpecNotAllowed(t *testing.T) { container := corev1.Container{ Name: "Not Allowed", From d7ef70fc994e8bc558dd5175a9ee77147f4d103e Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Thu, 22 Feb 2018 16:20:57 -0800 Subject: [PATCH 3/7] Validation reminders when overwritting fields in corev1.Container. --- pkg/controller/revision/ela_pod.go | 2 ++ pkg/webhook/configuration.go | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/controller/revision/ela_pod.go b/pkg/controller/revision/ela_pod.go index 5f3f6cc28701..91549d080a85 100644 --- a/pkg/controller/revision/ela_pod.go +++ b/pkg/controller/revision/ela_pod.go @@ -33,6 +33,8 @@ func MakeElaPodSpec(u *v1alpha1.Revision) *corev1.PodSpec { nginxConfigMapName := name + "-" + serviceID + "-proxy-configmap" elaContainer := u.Spec.ContainerSpec.DeepCopy() + // Adding or removing an overwritten corev1.Container field here? Don't forget to + // update the validations in pkg/webhook.validateContainerSpec. elaContainer.Name = elaContainerName elaContainer.Resources = corev1.ResourceRequirements{ Requests: corev1.ResourceList{ diff --git a/pkg/webhook/configuration.go b/pkg/webhook/configuration.go index f84252e95605..d5e2646207cc 100644 --- a/pkg/webhook/configuration.go +++ b/pkg/webhook/configuration.go @@ -83,7 +83,8 @@ func validateContainerSpec(container *corev1.Container) error { } // Some corev1.Container fields are set by Elafros controller. We disallow them // here to avoid silently overwriting these fields and causing confusions for - // the users. + // the users. See pkg/controller/revision.MakeElaPodSpec for the list of fields + // overridden. var ignoredFields []string if container.Name != "" { ignoredFields = append(ignoredFields, "template.spec.containerSpec.name") From 1ce9623b8554abbe82eacfd7259e4eebc58b6caf Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Mon, 26 Feb 2018 14:23:43 -0800 Subject: [PATCH 4/7] Tests to make sure we pass through fields of corev1.Container. --- .../configuration/controller_test.go | 24 +++++++++++++++ pkg/controller/revision/controller_test.go | 30 ++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/pkg/controller/configuration/controller_test.go b/pkg/controller/configuration/controller_test.go index b2f4060d1d63..d27a72966731 100644 --- a/pkg/controller/configuration/controller_test.go +++ b/pkg/controller/configuration/controller_test.go @@ -66,6 +66,26 @@ func getTestConfiguration() *v1alpha1.Configuration { Template: v1alpha1.Revision{ Spec: v1alpha1.RevisionSpec{ Service: "test-service", + // corev1.Container has a lot of setting. We try to pass many + // of them here to verify that we pass through the settings to + // the derived Revisions. + ContainerSpec: &corev1.Container{ + Image: "gcr.io/repo/image", + Command: []string{"echo"}, + Args: []string{"hello", "world"}, + WorkingDir: "/tmp", + Env: []corev1.EnvVar{{ + Name: "EDITOR", + Value: "emacs", + }}, + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 42, + }, + ReadinessProbe: &corev1.Probe{ + TimeoutSeconds: 43, + }, + TerminationMessagePath: "/dev/null", + }, }, }, }, @@ -147,6 +167,10 @@ func TestCreateConfigurationsCreatesRevision(t *testing.T) { t.Errorf("rev service was not %s", config.Spec.Template.Spec.Service) } + if !reflect.DeepEqual(rev.Spec, config.Spec.Template.Spec) { + t.Error("rev spec does not match config template spec") + } + if rev.Labels[ConfigurationLabelKey] != config.Name { t.Errorf("rev does not have label <%s:%s>", ConfigurationLabelKey, config.Name) } diff --git a/pkg/controller/revision/controller_test.go b/pkg/controller/revision/controller_test.go index 70a1b6d5f070..2c514aa7053b 100644 --- a/pkg/controller/revision/controller_test.go +++ b/pkg/controller/revision/controller_test.go @@ -29,6 +29,7 @@ package revision */ import ( "fmt" + "reflect" "regexp" "testing" "time" @@ -61,8 +62,25 @@ func getTestRevision() *v1alpha1.Revision { }, Spec: v1alpha1.RevisionSpec{ Service: "test-service", + // corev1.Container has a lot of setting. We try to pass many + // of them here to verify that we pass through the settings to + // the derived Revisions. ContainerSpec: &corev1.Container{ - Image: "test-image", + Image: "gcr.io/repo/image", + Command: []string{"echo"}, + Args: []string{"hello", "world"}, + WorkingDir: "/tmp", + Env: []corev1.EnvVar{{ + Name: "EDITOR", + Value: "emacs", + }}, + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 42, + }, + ReadinessProbe: &corev1.Probe{ + TimeoutSeconds: 43, + }, + TerminationMessagePath: "/dev/null", }, }, } @@ -158,6 +176,16 @@ func TestCreateRevCreatesStuff(t *testing.T) { func(t *testing.T) { for _, c := range p.Spec.Containers { if c.Image == rev.Spec.ContainerSpec.Image { + // Make a copy and removed fields set by the controller. + container := c.DeepCopy() + container.Name = "" + container.Resources = corev1.ResourceRequirements{} + container.Ports = nil + container.VolumeMounts = nil + // Verify that all other fields match revision container spec. + if !reflect.DeepEqual(container, rev.Spec.ContainerSpec) { + t.Error("pod container spec does not match revision") + } return } } From 01c0af0899ba3bdaf76f9025019032611c698f3c Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Mon, 26 Feb 2018 14:30:23 -0800 Subject: [PATCH 5/7] Update comment. --- pkg/controller/revision/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/revision/controller_test.go b/pkg/controller/revision/controller_test.go index 2c514aa7053b..7dd10b1b511a 100644 --- a/pkg/controller/revision/controller_test.go +++ b/pkg/controller/revision/controller_test.go @@ -64,7 +64,7 @@ func getTestRevision() *v1alpha1.Revision { Service: "test-service", // corev1.Container has a lot of setting. We try to pass many // of them here to verify that we pass through the settings to - // the derived Revisions. + // derived objects. ContainerSpec: &corev1.Container{ Image: "gcr.io/repo/image", Command: []string{"echo"}, From 457967a6c0a8c7aa0c1601f0727e45708c4bbddb Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Wed, 28 Feb 2018 10:46:03 -0800 Subject: [PATCH 6/7] Update per review feedback. Remove stale TODOs. --- pkg/controller/configuration/BUILD.bazel | 1 + .../configuration/controller_test.go | 5 ++-- pkg/controller/revision/BUILD.bazel | 2 ++ pkg/controller/revision/controller_test.go | 25 ++++++------------- 4 files changed, 14 insertions(+), 19 deletions(-) diff --git a/pkg/controller/configuration/BUILD.bazel b/pkg/controller/configuration/BUILD.bazel index f52495ea2805..ccda8fce3c20 100644 --- a/pkg/controller/configuration/BUILD.bazel +++ b/pkg/controller/configuration/BUILD.bazel @@ -44,6 +44,7 @@ go_test( "//pkg/client/informers/externalversions:go_default_library", "//pkg/controller/testing:go_default_library", "//vendor/github.com/golang/glog:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", diff --git a/pkg/controller/configuration/controller_test.go b/pkg/controller/configuration/controller_test.go index d27a72966731..2e8005aabb95 100644 --- a/pkg/controller/configuration/controller_test.go +++ b/pkg/controller/configuration/controller_test.go @@ -32,6 +32,7 @@ import ( "time" "github.com/golang/glog" + "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -167,8 +168,8 @@ func TestCreateConfigurationsCreatesRevision(t *testing.T) { t.Errorf("rev service was not %s", config.Spec.Template.Spec.Service) } - if !reflect.DeepEqual(rev.Spec, config.Spec.Template.Spec) { - t.Error("rev spec does not match config template spec") + if diff := cmp.Diff(rev.Spec, config.Spec.Template.Spec); diff != "" { + t.Errorf("rev spec != config template spec (-want +got): %v", diff) } if rev.Labels[ConfigurationLabelKey] != config.Name { diff --git a/pkg/controller/revision/BUILD.bazel b/pkg/controller/revision/BUILD.bazel index 8e28ad13fb35..53fdf2dd3521 100644 --- a/pkg/controller/revision/BUILD.bazel +++ b/pkg/controller/revision/BUILD.bazel @@ -56,6 +56,8 @@ go_test( "//pkg/client/informers/externalversions:go_default_library", "//pkg/controller/testing:go_default_library", "//vendor/github.com/golang/glog:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", + "//vendor/github.com/google/go-cmp/cmp/cmpopts:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/api/extensions/v1beta1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/pkg/controller/revision/controller_test.go b/pkg/controller/revision/controller_test.go index 7dd10b1b511a..48113127faac 100644 --- a/pkg/controller/revision/controller_test.go +++ b/pkg/controller/revision/controller_test.go @@ -17,23 +17,18 @@ limitations under the License. package revision /* TODO tests: -- When a Revision is created: - - a namespace is created - - a deployment is created - - an autoscaler is created - - an nginx configmap is created - - Revision status is updated - - When a Revision is updated TODO - When a Revision is deleted TODO */ import ( "fmt" - "reflect" "regexp" "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/golang/glog" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -176,15 +171,11 @@ func TestCreateRevCreatesStuff(t *testing.T) { func(t *testing.T) { for _, c := range p.Spec.Containers { if c.Image == rev.Spec.ContainerSpec.Image { - // Make a copy and removed fields set by the controller. - container := c.DeepCopy() - container.Name = "" - container.Resources = corev1.ResourceRequirements{} - container.Ports = nil - container.VolumeMounts = nil - // Verify that all other fields match revision container spec. - if !reflect.DeepEqual(container, rev.Spec.ContainerSpec) { - t.Error("pod container spec does not match revision") + // Ignoring fields set by Elafros controller. + ignored := cmpopts.IgnoreFields(corev1.Container{}, "Name", "Ports", "Resources", "VolumeMounts") + // All other fields must match. + if diff := cmp.Diff(rev.Spec.ContainerSpec, &c, ignored); diff != "" { + t.Errorf("Pod container spec != revision container spec (-want +got): %v", diff) } return } From f4dfbf163752babdb767190fad8080497b3526cd Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Wed, 28 Feb 2018 10:53:30 -0800 Subject: [PATCH 7/7] Fix cmp statement so that expected value is left. --- pkg/controller/configuration/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/configuration/controller_test.go b/pkg/controller/configuration/controller_test.go index 2e8005aabb95..2255a67ac0a0 100644 --- a/pkg/controller/configuration/controller_test.go +++ b/pkg/controller/configuration/controller_test.go @@ -168,7 +168,7 @@ func TestCreateConfigurationsCreatesRevision(t *testing.T) { t.Errorf("rev service was not %s", config.Spec.Template.Spec.Service) } - if diff := cmp.Diff(rev.Spec, config.Spec.Template.Spec); diff != "" { + if diff := cmp.Diff(config.Spec.Template.Spec, rev.Spec); diff != "" { t.Errorf("rev spec != config template spec (-want +got): %v", diff) }