From 695fd25502db3d040ae4c690d7c53a7b842500d3 Mon Sep 17 00:00:00 2001 From: Prithvi Ramesh Date: Tue, 8 Jan 2019 08:01:42 -0800 Subject: [PATCH] fix (build_validation) Incorrect field paths returned (#495) * fixed field-paths returned for setting the Template field and also including Steps * switched to correct FieldError: ErrMultipleOneOf instead of disallowing Steps * changed errors: multiple sources and missing both template/steps * fixed field paths in build_validation.go, added comments around some extraneous code/potential bugs * fixed field-path for bad-kind in template, tested with unit test * added ViaField() to Template validation caller, chained a bunch of validation calls with .Also(), made the reciever variable for validateTimeOut bs in line with the rest of build_validation.go * addressed most of review comments, targetpath->targetPath * removed ViaField(source) from validateSources errors * fixed comment formatting * updated error sent back for target path validation * corrected fieldpaths for duplicate step names and multiple sources w/ subpaths * standardized volume validation error field path * Redid build_validation_test.go: check errors recieved against expected values, fixed bugs in Build Timeout tests * fixed targetPath validation tests to reflect the FieldError change * Added ErrOutOfBoundsValue for build timeout validation * including vendor updates for unit tests * rebased with upstream/master, fixed merge conflicts * ran update-codegen.sh --- Gopkg.lock | 4 +- Gopkg.toml | 4 +- .../v1alpha1/build_template_validation.go | 4 +- pkg/apis/build/v1alpha1/build_validation.go | 65 +++----- .../build/v1alpha1/build_validation_test.go | 101 +++++++----- .../build/v1alpha1/target_path_validation.go | 5 +- .../v1alpha1/target_path_validation_test.go | 40 ++++- .../apis/duck/v1alpha1/addressable_types.go | 1 - .../pkg/apis/duck/v1alpha1/condition_set.go | 86 +++++++--- .../apis/duck/v1alpha1/conditions_types.go | 21 ++- .../apis/duck/v1alpha1/generational_types.go | 1 - .../duck/v1alpha1/legacy_targetable_types.go | 1 - .../duck/v1alpha1/retired_targetable_types.go | 1 - .../knative/pkg/apis/field_error.go | 48 +++--- .../knative/pkg/controller/controller.go | 150 +++++++++--------- .../knative/pkg/controller/stats_reporter.go | 2 +- .../knative/pkg/test/spoof/spoof.go | 21 ++- .../knative/pkg/test/zipkin/util.go | 6 +- .../github.com/knative/pkg/webhook/webhook.go | 12 +- 19 files changed, 343 insertions(+), 230 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 88a088da..09a952df 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -225,7 +225,7 @@ revision = "06e9787157505a649b07677e77d26202ac91431c" [[projects]] - digest = "1:30457e774f9117d31aac140bed69fa62594f5c99af9932fb4f9fb1c427ffc1df" + digest = "1:e7222d1d533f87f6115cb15eb64faac9501226803d1bd7fca8f8640d71d02eb5" name = "github.com/knative/pkg" packages = [ "apis", @@ -245,7 +245,7 @@ "webhook", ] pruneopts = "NUT" - revision = "088e3f7faf9d7ed0122937f66289cc050d0a78b0" + revision = "585076ba67e86ccbe9c2c146d49502d7433da7c0" [[projects]] branch = "master" diff --git a/Gopkg.toml b/Gopkg.toml index 01072822..da05bd97 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -47,8 +47,8 @@ required = [ [[override]] name = "github.com/knative/pkg" - # HEAD as of 2018-11-06 - revision = "088e3f7faf9d7ed0122937f66289cc050d0a78b0" + # HEAD as of 2018-12-09 + revision = "585076ba67e86ccbe9c2c146d49502d7433da7c0" [prune] go-tests = true diff --git a/pkg/apis/build/v1alpha1/build_template_validation.go b/pkg/apis/build/v1alpha1/build_template_validation.go index cd3464c1..383e46f5 100644 --- a/pkg/apis/build/v1alpha1/build_template_validation.go +++ b/pkg/apis/build/v1alpha1/build_template_validation.go @@ -47,7 +47,7 @@ func ValidateVolumes(volumes []corev1.Volume) *apis.FieldError { vols := map[string]struct{}{} for _, v := range volumes { if _, ok := vols[v.Name]; ok { - return apis.ErrMultipleOneOf("volumeName") + return apis.ErrMultipleOneOf("name") } vols[v.Name] = struct{}{} } @@ -66,7 +66,7 @@ func validateSteps(steps []corev1.Container) *apis.FieldError { continue } if _, ok := names[s.Name]; ok { - return apis.ErrMultipleOneOf("stepName") + return apis.ErrMultipleOneOf("name") } names[s.Name] = struct{}{} } diff --git a/pkg/apis/build/v1alpha1/build_validation.go b/pkg/apis/build/v1alpha1/build_validation.go index 950d0cb2..d15c4544 100644 --- a/pkg/apis/build/v1alpha1/build_validation.go +++ b/pkg/apis/build/v1alpha1/build_validation.go @@ -17,7 +17,6 @@ limitations under the License. package v1alpha1 import ( - "fmt" "time" "github.com/knative/pkg/apis" @@ -31,44 +30,33 @@ func (b *Build) Validate() *apis.FieldError { // Validate for build spec func (bs *BuildSpec) Validate() *apis.FieldError { if bs.Template == nil && len(bs.Steps) == 0 { - return apis.ErrMissingField("b.spec.template").Also(apis.ErrMissingField("b.spec.steps")) + return apis.ErrMissingOneOf("template", "steps") } if bs.Template != nil && len(bs.Steps) > 0 { - return apis.ErrMissingField("b.spec.template").Also(apis.ErrMissingField("b.spec.steps")) + return apis.ErrMultipleOneOf("template", "steps") } - if bs.Template != nil && bs.Template.Name == "" { - apis.ErrMissingField("build.spec.template.name") - } - - if err := bs.validateSources(); err != nil { - return err - } // If a build specifies a template, all the template's parameters without // defaults must be satisfied by the build's parameters. if bs.Template != nil { - return bs.Template.Validate() - } - if err := ValidateVolumes(bs.Volumes); err != nil { - return err - } - if err := bs.validateTimeout(); err != nil { - return err + return bs.Template.Validate().ViaField("template") } - if err := validateSteps(bs.Steps); err != nil { - return err - } - return nil + // Below method potentially has a bug: + // It does not Validate if only a "Source" has been set, it only validates if multiple sources have been set + return bs.validateSources(). + Also(ValidateVolumes(bs.Volumes).ViaField("volumes")). + Also(bs.validateTimeout()). + Also(validateSteps(bs.Steps).ViaField("steps")) } -// Validate templateKind +// Validate template func (b *TemplateInstantiationSpec) Validate() *apis.FieldError { if b == nil { return nil } if b.Name == "" { - return apis.ErrMissingField("build.spec.template.name") + return apis.ErrMissingField("name") } if b.Kind != "" { switch b.Kind { @@ -76,23 +64,21 @@ func (b *TemplateInstantiationSpec) Validate() *apis.FieldError { BuildTemplateKind: return nil default: - return apis.ErrInvalidValue(string(b.Kind), apis.CurrentField) + return apis.ErrInvalidValue(string(b.Kind), "kind") } } return nil } // Validate build timeout -func (bt *BuildSpec) validateTimeout() *apis.FieldError { - if bt.Timeout == nil { +func (bs *BuildSpec) validateTimeout() *apis.FieldError { + if bs.Timeout == nil { return nil } maxTimeout := time.Duration(24 * time.Hour) - if bt.Timeout.Duration > maxTimeout { - return apis.ErrInvalidValue(fmt.Sprintf("%s should be < 24h", bt.Timeout), "b.spec.timeout") - } else if bt.Timeout.Duration < 0 { - return apis.ErrInvalidValue(fmt.Sprintf("%s should be > 0", bt.Timeout), "b.spec.timeout") + if bs.Timeout.Duration > maxTimeout || bs.Timeout.Duration < 0 { + return apis.ErrOutOfBoundsValue(bs.Timeout.Duration.String(), "0", "24", "timeout") } return nil } @@ -106,20 +92,19 @@ func (bs BuildSpec) validateSources() *apis.FieldError { nodeMap: map[string]map[string]string{}, } - // both source and sources cannot be defined in build + // Both source and sources cannot be defined in build if len(bs.Sources) > 0 && bs.Source != nil { - return apis.ErrMultipleOneOf("b.spec.source", "b.spec.sources") + return apis.ErrMultipleOneOf("source", "sources") } - for _, source := range bs.Sources { - // check all source have unique names + // Check all source have unique names if _, ok := names[source.Name]; ok { - return apis.ErrMultipleOneOf("b.spec.sources.names") + return apis.ErrMultipleOneOf("name").ViaField("sources") } - // multiple sources cannot have subpath defined + // Multiple sources cannot have subpath defined if source.SubPath != "" { if subPathExists { - return apis.ErrInvalidValue("b.spec.sources.subpath", source.SubPath) + return apis.ErrMultipleOneOf("subpath").ViaField("sources") } subPathExists = true } @@ -130,14 +115,14 @@ func (bs BuildSpec) validateSources() *apis.FieldError { continue } if emptyTargetPath { - return apis.ErrInvalidValue("empty target path", "b.spec.sources.targetPath") + return apis.ErrInvalidValue("Empty Target Path", "targetPath").ViaField("sources") } emptyTargetPath = true } else { if source.Custom != nil { - return apis.ErrInvalidValue(source.TargetPath, "b.spec.sources.targetPath") + return apis.ErrInvalidValue(source.TargetPath, "targetPath").ViaField("sources") } - if err := insertNode(source.TargetPath, pathtree); err != nil { + if err := insertNode(source.TargetPath, pathtree).ViaField("sources"); err != nil { return err } } diff --git a/pkg/apis/build/v1alpha1/build_validation_test.go b/pkg/apis/build/v1alpha1/build_validation_test.go index ee436aa5..413774a0 100644 --- a/pkg/apis/build/v1alpha1/build_validation_test.go +++ b/pkg/apis/build/v1alpha1/build_validation_test.go @@ -20,16 +20,19 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/knative/pkg/apis" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestValidateBuild(t *testing.T) { for _, c := range []struct { - desc string - build *Build - reason string // if "", expect success. + name string + build *Build + want *apis.FieldError }{{ + name: "Valid Container", build: &Build{ Spec: BuildSpec{ Steps: []corev1.Container{{ @@ -38,8 +41,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: nil, }, { - desc: "source and sources presence", + name: "Both source and sources present", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -61,9 +65,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "source and sources cannot be declared in same build", + want: apis.ErrMultipleOneOf("spec.source", "spec.sources"), }, { - desc: "source without name", + name: "Source defined without a name", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -78,8 +82,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: nil, }, { - desc: "source with targetPath", + name: "source with targetPath", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -95,8 +100,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: nil, }, { - desc: "sources with empty targetPaths", + name: "Multiple sources with empty targetPaths", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -125,9 +131,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "multiple sources with empty target paths", + want: apis.ErrInvalidValue("Empty Target Path", "targetPath").ViaField("sources").ViaField("spec"), }, { - desc: "custom sources with targetPaths", + name: "Defining a targetPath while using a custom source", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -143,9 +149,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "custom sources with targetPaths", + want: apis.ErrInvalidValue("a/b", "targetPath").ViaField("sources").ViaField("spec"), }, { - desc: "multiple custom sources without targetPaths", + name: "Multiple custom sources without a targetPath", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -166,7 +172,7 @@ func TestValidateBuild(t *testing.T) { }, }, }, { - desc: "sources with combination of different targetPath with common parent dir", + name: "Sources with targetPaths that overlap with a common parent directory", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -190,9 +196,12 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "multiple sources with overlap of target paths", + want: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"spec.sources.targetPath"}, + }, }, { - desc: "sources with combination of individual targetpath", + name: "Sources with combination of individual targetpath", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -216,8 +225,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: nil, }, { - desc: "Mix of sources with and without target path", + name: "Mix of sources with and without target path", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -247,8 +257,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: nil, }, { - desc: "source with duplicate names", + name: "Multiple sources with duplicate names", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -270,9 +281,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "sources with duplicate names", + want: apis.ErrMultipleOneOf("spec.sources.name"), }, { - desc: "a source with subpath", + name: "Source with a subpath", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -289,8 +300,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: nil, }, { - desc: "sources with subpath", + name: "Multiple sources with subpaths", build: &Build{ Spec: BuildSpec{ Sources: []SourceSpec{{ @@ -314,9 +326,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "sources without subpaths", + want: apis.ErrMultipleOneOf("spec.sources.subpath"), }, { - reason: "negative build timeout", + name: "Negative build timeout", build: &Build{ Spec: BuildSpec{ Timeout: &metav1.Duration{Duration: -48 * time.Hour}, @@ -326,15 +338,15 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: apis.ErrOutOfBoundsValue("-48h0m0s", "0", "24", "spec.timeout"), }, { - desc: "No template and steps", - reason: "no template & steps", + name: "No template and steps", build: &Build{ Spec: BuildSpec{}, }, + want: apis.ErrMissingOneOf("spec.template", "spec.steps"), }, { - desc: "Bad template kind", - reason: "invalid template", + name: "Invalid template Kind", build: &Build{ Spec: BuildSpec{ Template: &TemplateInstantiationSpec{ @@ -343,8 +355,9 @@ func TestValidateBuild(t *testing.T) { }, }, }, + want: apis.ErrInvalidValue("bad-kind", "spec.template.kind"), }, { - desc: "good template kind", + name: "Valid template Kind", build: &Build{ Spec: BuildSpec{ Template: &TemplateInstantiationSpec{ @@ -353,8 +366,9 @@ func TestValidateBuild(t *testing.T) { }, }, }, + want: nil, }, { - reason: "maximum timeout", + name: "Greater than maximum build timeout", build: &Build{ Spec: BuildSpec{ Timeout: &metav1.Duration{Duration: 48 * time.Hour}, @@ -364,7 +378,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: apis.ErrOutOfBoundsValue("48h0m0s", "0", "24", "spec.timeout"), }, { + name: "5 minute build timeout", build: &Build{ Spec: BuildSpec{ Timeout: &metav1.Duration{Duration: 5 * time.Minute}, @@ -374,8 +390,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, + want: nil, }, { - desc: "Multiple unnamed steps", + name: "Multiple unnamed steps", build: &Build{ Spec: BuildSpec{ Steps: []corev1.Container{{ @@ -388,6 +405,7 @@ func TestValidateBuild(t *testing.T) { }, }, }, { + name: "Multiple steps with the same name", build: &Build{ Spec: BuildSpec{ Steps: []corev1.Container{{ @@ -399,15 +417,17 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "DuplicateStepName", + want: apis.ErrMultipleOneOf("spec.steps.name"), }, { + name: "Missing step image", build: &Build{ Spec: BuildSpec{ Steps: []corev1.Container{{Name: "foo"}}, }, }, - reason: "StepMissingImage", + want: apis.ErrMissingField("spec.steps.Image"), }, { + name: "Multiple volumes with the same name", build: &Build{ Spec: BuildSpec{ Steps: []corev1.Container{{ @@ -427,8 +447,9 @@ func TestValidateBuild(t *testing.T) { }}, }, }, - reason: "DuplicateVolumeName", + want: apis.ErrMultipleOneOf("spec.volumes.name"), }, { + name: "Template and Steps both defined", build: &Build{ Spec: BuildSpec{ Steps: []corev1.Container{{ @@ -440,23 +461,21 @@ func TestValidateBuild(t *testing.T) { }, }, }, - reason: "TemplateAndSteps", + want: apis.ErrMultipleOneOf("spec.template", "spec.steps"), }, { + name: "No template name defined", build: &Build{ Spec: BuildSpec{ Template: &TemplateInstantiationSpec{}, }, }, - reason: "MissingTemplateName", + want: apis.ErrMissingField("spec.template.name"), }} { - name := c.desc - if c.reason != "" { - name = "invalid-" + c.reason - } + name := c.name t.Run(name, func(t *testing.T) { - verr := c.build.Validate() - if gotErr, wantErr := verr != nil, c.reason != ""; gotErr != wantErr { - t.Errorf("validateBuild(%s); got %#v, want %q", name, verr, c.reason) + got := c.build.Validate() + if diff := cmp.Diff(c.want.Error(), got.Error()); diff != "" { + t.Errorf("validateBuild(%s) (-want, +got) = %v", name, diff) } }) } diff --git a/pkg/apis/build/v1alpha1/target_path_validation.go b/pkg/apis/build/v1alpha1/target_path_validation.go index 22ee4838..3439a87e 100644 --- a/pkg/apis/build/v1alpha1/target_path_validation.go +++ b/pkg/apis/build/v1alpha1/target_path_validation.go @@ -28,7 +28,10 @@ type pathTree struct { // insertNode functions checks the path does not have overlap with existing // paths in path.nodeMap. If not it creates a key for path and adds func insertNode(path string, pathtree pathTree) *apis.FieldError { - err := apis.ErrMultipleOneOf("b.spec.sources.targetPath") + err := &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + } path = strings.Trim(path, "/") parts := strings.Split(path, "/") diff --git a/pkg/apis/build/v1alpha1/target_path_validation_test.go b/pkg/apis/build/v1alpha1/target_path_validation_test.go index f526a1c9..572009b1 100644 --- a/pkg/apis/build/v1alpha1/target_path_validation_test.go +++ b/pkg/apis/build/v1alpha1/target_path_validation_test.go @@ -23,14 +23,20 @@ func TestValidateTargetPaths(t *testing.T) { "a/b/c/d", "a/b", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths with overlap in different order", paths: []string{ "a/b", "a/b/c", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths with leaf node overlap", paths: []string{ @@ -56,7 +62,10 @@ func TestValidateTargetPaths(t *testing.T) { "a/b/c", "a/b/c", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths with same length and different leaf node", paths: []string{ @@ -69,14 +78,20 @@ func TestValidateTargetPaths(t *testing.T) { "github.com/foo/bar", "github.com/foo/", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths with overlap in different order", paths: []string{ "github.com/foo", "github.com/foo/bar", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths with different leaf", paths: []string{ @@ -97,7 +112,10 @@ func TestValidateTargetPaths(t *testing.T) { "/dir/a", "/dir/a/b", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths with different parent node", paths: []string{ @@ -112,7 +130,10 @@ func TestValidateTargetPaths(t *testing.T) { "a/d", "a/d", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths starts with combination of / and no /", paths: []string{ @@ -125,7 +146,10 @@ func TestValidateTargetPaths(t *testing.T) { "/a/a/b/d", "a/a", }, - wantErr: apis.ErrMultipleOneOf("b.spec.sources.targetPath"), + wantErr: &apis.FieldError{ + Message: "Overlapping Target Paths", + Paths: []string{"targetPath"}, + }, }, { desc: "paths with repeating nodes", paths: []string{ diff --git a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/addressable_types.go b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/addressable_types.go index 0bf1f986..bc791797 100644 --- a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/addressable_types.go +++ b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/addressable_types.go @@ -37,7 +37,6 @@ type Addressable struct { Hostname string `json:"hostname,omitempty"` } - // Addressable is an Implementable "duck type". var _ duck.Implementable = (*Addressable)(nil) diff --git a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/condition_set.go b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/condition_set.go index 30b76a5f..e330970b 100644 --- a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/condition_set.go +++ b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/condition_set.go @@ -62,7 +62,7 @@ type ConditionManager interface { SetCondition(new Condition) // MarkTrue sets the status of t to true, and then marks the happy condition to - // true if all other dependents are also true. + // true if all dependents are true. MarkTrue(t ConditionType) // MarkUnknown sets the status of t to Unknown and also sets the happy condition @@ -82,12 +82,14 @@ type ConditionManager interface { // NewLivingConditionSet returns a ConditionSet to hold the conditions for the // living resource. ConditionReady is used as the happy condition. +// The set of condition types provided are those of the terminal subconditions. func NewLivingConditionSet(d ...ConditionType) ConditionSet { return newConditionSet(ConditionReady, d...) } // NewBatchConditionSet returns a ConditionSet to hold the conditions for the // batch resource. ConditionSucceeded is used as the happy condition. +// The set of condition types provided are those of the terminal subconditions. func NewBatchConditionSet(d ...ConditionType) ConditionSet { return newConditionSet(ConditionSucceeded, d...) } @@ -209,13 +211,30 @@ func (r conditionsImpl) SetCondition(new Condition) { r.accessor.SetConditions(conditions) } +func (r conditionsImpl) isTerminal(t ConditionType) bool { + for _, cond := range append(r.dependents, r.happy) { + if cond == t { + return true + } + } + return false +} + +func (r conditionsImpl) severity(t ConditionType) ConditionSeverity { + if r.isTerminal(t) { + return ConditionSeverityError + } + return ConditionSeverityInfo +} + // MarkTrue sets the status of t to true, and then marks the happy condition to // true if all other dependents are also true. func (r conditionsImpl) MarkTrue(t ConditionType) { // set the specified condition r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionTrue, + Type: t, + Status: corev1.ConditionTrue, + Severity: r.severity(t), }) // check the dependents. @@ -229,8 +248,9 @@ func (r conditionsImpl) MarkTrue(t ConditionType) { // set the happy condition r.SetCondition(Condition{ - Type: r.happy, - Status: corev1.ConditionTrue, + Type: r.happy, + Status: corev1.ConditionTrue, + Severity: r.severity(r.happy), }) } @@ -239,13 +259,15 @@ func (r conditionsImpl) MarkTrue(t ConditionType) { func (r conditionsImpl) MarkUnknown(t ConditionType, reason, messageFormat string, messageA ...interface{}) { // set the specified condition r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionUnknown, - Reason: reason, - Message: fmt.Sprintf(messageFormat, messageA...), + Type: t, + Status: corev1.ConditionUnknown, + Reason: reason, + Message: fmt.Sprintf(messageFormat, messageA...), + Severity: r.severity(t), }) // check the dependents. + isDependent := false for _, cond := range r.dependents { c := r.GetCondition(cond) // Failed conditions trump Unknown conditions @@ -257,28 +279,39 @@ func (r conditionsImpl) MarkUnknown(t ConditionType, reason, messageFormat strin } return } + if cond == t { + isDependent = true + } } - // set the happy condition - r.SetCondition(Condition{ - Type: r.happy, - Status: corev1.ConditionUnknown, - Reason: reason, - Message: fmt.Sprintf(messageFormat, messageA...), - }) + if isDependent { + // set the happy condition, if it is one of our dependent subconditions. + r.SetCondition(Condition{ + Type: r.happy, + Status: corev1.ConditionUnknown, + Reason: reason, + Message: fmt.Sprintf(messageFormat, messageA...), + Severity: r.severity(r.happy), + }) + } } // MarkFalse sets the status of t and the happy condition to False. func (r conditionsImpl) MarkFalse(t ConditionType, reason, messageFormat string, messageA ...interface{}) { - for _, t := range []ConditionType{ - t, - r.happy, - } { + types := []ConditionType{t} + for _, cond := range r.dependents { + if cond == t { + types = append(types, r.happy) + } + } + + for _, t := range types { r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionFalse, - Reason: reason, - Message: fmt.Sprintf(messageFormat, messageA...), + Type: t, + Status: corev1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf(messageFormat, messageA...), + Severity: r.severity(t), }) } } @@ -295,8 +328,9 @@ func (r conditionsImpl) InitializeConditions() { func (r conditionsImpl) InitializeCondition(t ConditionType) { if c := r.GetCondition(t); c == nil { r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionUnknown, + Type: t, + Status: corev1.ConditionUnknown, + Severity: r.severity(t), }) } } diff --git a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/conditions_types.go b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/conditions_types.go index 44b99921..0dbad339 100644 --- a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/conditions_types.go +++ b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/conditions_types.go @@ -42,6 +42,21 @@ const ( ConditionSucceeded ConditionType = "Succeeded" ) +// ConditionSeverity expresses the severity of a Condition Type failing. +type ConditionSeverity string + +const ( + // ConditionSeverityError specifies that a failure of a condition type + // should be viewed as an error. + ConditionSeverityError ConditionSeverity = "Error" + // ConditionSeverityWarning specifies that a failure of a condition type + // should be viewed as a warning, but that things could still work. + ConditionSeverityWarning ConditionSeverity = "Warning" + // ConditionSeverityInfo specifies that a failure of a condition type + // should be viewed as purely informational, and that things could still work. + ConditionSeverityInfo ConditionSeverity = "Info" +) + // Conditions defines a readiness condition for a Knative resource. // See: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties // +k8s:deepcopy-gen=true @@ -54,6 +69,11 @@ type Condition struct { // +required Status corev1.ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"` + // Severity with which to treat failures of this type of condition. + // When this is not specified, it defaults to Error. + // +optional + Severity ConditionSeverity `json:"severity,omitempty" description:"how to interpret failures of this condition, one of Error, Warning, Info"` + // LastTransitionTime is the last time the condition transitioned from one status to another. // We use VolatileTime in place of metav1.Time to exclude this from creating equality.Semantic // differences (all other things held constant). @@ -93,7 +113,6 @@ func (c *Condition) IsUnknown() bool { return c.Status == corev1.ConditionUnknown } - // Conditions is an Implementable "duck type". var _ duck.Implementable = (*Conditions)(nil) diff --git a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/generational_types.go b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/generational_types.go index c8197f67..13ef0180 100644 --- a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/generational_types.go +++ b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/generational_types.go @@ -27,7 +27,6 @@ import ( // Generation is the schema for the generational portion of the payload type Generation int64 - // Generation is an Implementable "duck type". var _ duck.Implementable = (*Generation)(nil) diff --git a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/legacy_targetable_types.go b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/legacy_targetable_types.go index ee50201e..3e13d9ea 100644 --- a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/legacy_targetable_types.go +++ b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/legacy_targetable_types.go @@ -41,7 +41,6 @@ type LegacyTargetable struct { DomainInternal string `json:"domainInternal,omitempty"` } - // LegacyTargetable is an Implementable "duck type". var _ duck.Implementable = (*LegacyTargetable)(nil) diff --git a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/retired_targetable_types.go b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/retired_targetable_types.go index 695e11c3..ab578e52 100644 --- a/vendor/github.com/knative/pkg/apis/duck/v1alpha1/retired_targetable_types.go +++ b/vendor/github.com/knative/pkg/apis/duck/v1alpha1/retired_targetable_types.go @@ -37,7 +37,6 @@ type Targetable struct { DomainInternal string `json:"domainInternal,omitempty"` } - // Targetable is an Implementable "duck type". var _ duck.Implementable = (*Targetable)(nil) diff --git a/vendor/github.com/knative/pkg/apis/field_error.go b/vendor/github.com/knative/pkg/apis/field_error.go index 023db789..4ae02bc1 100644 --- a/vendor/github.com/knative/pkg/apis/field_error.go +++ b/vendor/github.com/knative/pkg/apis/field_error.go @@ -133,34 +133,36 @@ func (fe *FieldError) isEmpty() bool { return fe.Message == "" && fe.Details == "" && len(fe.errors) == 0 && len(fe.Paths) == 0 } -func (fe *FieldError) getNormalizedErrors() []FieldError { - // in case we call getNormalizedErrors on a nil object, return just an empty +// normalized returns a flattened copy of all the errors. +func (fe *FieldError) normalized() []*FieldError { + // In case we call normalized on a nil object, return just an empty // list. This can happen when .Error() is called on a nil object. if fe == nil { - return []FieldError(nil) + return []*FieldError(nil) } - var errors []FieldError - // if this FieldError is a leaf, + + // Allocate errors with at least as many objects as we'll get on the first pass. + errors := make([]*FieldError, 0, len(fe.errors)+1) + // If this FieldError is a leaf, add it. if fe.Message != "" { - err := FieldError{ + errors = append(errors, &FieldError{ Message: fe.Message, Paths: fe.Paths, Details: fe.Details, - } - errors = append(errors, err) + }) } - // and then collect all other errors recursively. + // And then collect all other errors recursively. for _, e := range fe.errors { - errors = append(errors, e.getNormalizedErrors()...) + errors = append(errors, e.normalized()...) } return errors } // Error implements error func (fe *FieldError) Error() string { - var errs []string // Get the list of errors as a flat merged list. - normedErrors := merge(fe.getNormalizedErrors()) + normedErrors := merge(fe.normalized()) + errs := make([]string, 0, len(normedErrors)) for _, e := range normedErrors { if e.Details == "" { errs = append(errs, fmt.Sprintf("%v: %v", e.Message, strings.Join(e.Paths, ", "))) @@ -198,7 +200,7 @@ func flatten(path []string) string { if p == CurrentField { continue } else if len(newPath) > 0 && isIndex(p) { - newPath[len(newPath)-1] = fmt.Sprintf("%s%s", newPath[len(newPath)-1], p) + newPath[len(newPath)-1] += p } else { newPath = append(newPath, p) } @@ -232,22 +234,21 @@ func containsString(slice []string, s string) bool { } // merge takes in a flat list of FieldErrors and returns back a merged list of -// FiledErrors. FieldErrors have their Paths combined (and de-duped) if their +// FieldErrors. FieldErrors have their Paths combined (and de-duped) if their // Message and Details are the same. Merge will not inspect FieldError.errors. // Merge will also sort the .Path slice, and the errors slice before returning. -func merge(errs []FieldError) []FieldError { +func merge(errs []*FieldError) []*FieldError { // make a map big enough for all the errors. - m := make(map[string]FieldError, len(errs)) + m := make(map[string]*FieldError, len(errs)) // Convert errs to a map where the key is -
and the value // is the error. If an error already exists in the map with the same key, // then the paths will be merged. for _, e := range errs { - k := key(&e) + k := key(e) if v, ok := m[k]; ok { // Found a match, merge the keys. v.Paths = mergePaths(v.Paths, e.Paths) - m[k] = v } else { // Does not exist in the map, save the error. m[k] = e @@ -255,7 +256,7 @@ func merge(errs []FieldError) []FieldError { } // Take the map made previously and flatten it back out again. - newErrs := make([]FieldError, 0, len(m)) + newErrs := make([]*FieldError, 0, len(m)) for _, v := range m { // While we have access to the merged paths, sort them too. sort.Slice(v.Paths, func(i, j int) bool { return v.Paths[i] < v.Paths[j] }) @@ -335,3 +336,12 @@ func ErrInvalidKeyName(value, fieldPath string, details ...string) *FieldError { Details: strings.Join(details, ", "), } } + +// ErrOutOfBoundsValue constructs a FieldError for a field that has received an +// out of bound value. +func ErrOutOfBoundsValue(value, lower, upper, fieldPath string) *FieldError { + return &FieldError{ + Message: fmt.Sprintf("expected %s <= %s <= %s", lower, value, upper), + Paths: []string{fieldPath}, + } +} diff --git a/vendor/github.com/knative/pkg/controller/controller.go b/vendor/github.com/knative/pkg/controller/controller.go index 7145b152..820744b0 100644 --- a/vendor/github.com/knative/pkg/controller/controller.go +++ b/vendor/github.com/knative/pkg/controller/controller.go @@ -25,7 +25,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" @@ -136,12 +135,11 @@ func (c *Impl) EnqueueControllerOf(obj interface{}) { } } -// EnqueueLabelOf returns with an Enqueue func that takes a resource, -// identifies its controller resource through given namespace and name labels, -// converts it into a namespace/name string, and passes that to EnqueueKey. -// Callers should pass in an empty string as namespace label key for obj -// whose controller is of cluster-scoped resource. -func (c *Impl) EnqueueLabelOf(namespaceLabel, nameLabel string) func(obj interface{}) { +// EnqueueLabelOfNamespaceScopedResource returns with an Enqueue func that +// takes a resource, identifies its controller resource through given namespace +// and name labels, converts it into a namespace/name string, and passes that +// to EnqueueKey. The controller resource must be of namespace-scoped. +func (c *Impl) EnqueueLabelOfNamespaceScopedResource(namespaceLabel, nameLabel string) func(obj interface{}) { return func(obj interface{}) { object, err := kmeta.DeletionHandlingAccessor(obj) if err != nil { @@ -152,7 +150,7 @@ func (c *Impl) EnqueueLabelOf(namespaceLabel, nameLabel string) func(obj interfa labels := object.GetLabels() controllerKey, ok := labels[nameLabel] if !ok { - c.logger.Infof("Object %s/%s does not have a referring name label %s", + c.logger.Debugf("Object %s/%s does not have a referring name label %s", object.GetNamespace(), object.GetName(), nameLabel) return } @@ -160,12 +158,40 @@ func (c *Impl) EnqueueLabelOf(namespaceLabel, nameLabel string) func(obj interfa if namespaceLabel != "" { controllerNamespace, ok := labels[namespaceLabel] if !ok { - c.logger.Infof("Object %s/%s does not have a referring namespace label %s", + c.logger.Debugf("Object %s/%s does not have a referring namespace label %s", object.GetNamespace(), object.GetName(), namespaceLabel) return } - controllerKey = fmt.Sprintf("%s/%s", controllerNamespace, controllerKey) + c.EnqueueKey(fmt.Sprintf("%s/%s", controllerNamespace, controllerKey)) + return + } + + // Pass through namespace of the object itself if no namespace label specified. + // This is for the scenario that object and the parent resource are of same namespace, + // e.g. to enqueue the revision of an endpoint. + c.EnqueueKey(fmt.Sprintf("%s/%s", object.GetNamespace(), controllerKey)) + } +} + +// EnqueueLabelOfClusterScopedResource returns with an Enqueue func +// that takes a resource, identifies its controller resource through +// given name label, and passes it to EnqueueKey. +// The controller resource must be of cluster-scoped. +func (c *Impl) EnqueueLabelOfClusterScopedResource(nameLabel string) func(obj interface{}) { + return func(obj interface{}) { + object, err := kmeta.DeletionHandlingAccessor(obj) + if err != nil { + c.logger.Error(err) + return + } + + labels := object.GetLabels() + controllerKey, ok := labels[nameLabel] + if !ok { + c.logger.Debugf("Object %s/%s does not have a referring name label %s", + object.GetNamespace(), object.GetName(), nameLabel) + return } c.EnqueueKey(controllerKey) @@ -188,10 +214,10 @@ func (c *Impl) Run(threadiness int, stopCh <-chan struct{}) error { logger := c.logger logger.Info("Starting controller and workers") for i := 0; i < threadiness; i++ { - go wait.Until(func() { + go func() { for c.processNextWorkItem() { } - }, time.Second, stopCh) + }() } logger.Info("Started workers") @@ -208,74 +234,52 @@ func (c *Impl) processNextWorkItem() bool { if shutdown { return false } - - // We wrap this block in a func so we can defer c.base.WorkQueue.Done. - err := func(obj interface{}) error { - startTime := time.Now() - // Send the metrics for the current queue depth - c.statsReporter.ReportQueueDepth(int64(c.WorkQueue.Len())) - - // We call Done here so the workqueue knows we have finished - // processing this item. We also must remember to call Forget if we - // do not want this work item being re-queued. For example, we do - // not call Forget if a transient error occurs, instead the item is - // put back on the workqueue and attempted again after a back-off - // period. - defer c.WorkQueue.Done(obj) - - // We expect strings to come off the workqueue. These are of the - // form namespace/name. We do this as the delayed nature of the - // workqueue means the items in the informer cache may actually be - // more up to date that when the item was initially put onto the - // workqueue. - key, ok := obj.(string) - if !ok { - // As the item in the workqueue is actually invalid, we call - // Forget here else we'd go into a loop of attempting to - // process a work item that is invalid. - c.WorkQueue.Forget(obj) - c.logger.Errorf("expected string in workqueue but got %#v", obj) - c.statsReporter.ReportReconcile(time.Now().Sub(startTime), "[InvalidKeyType]", falseString) - return nil - } - - var err error - defer func() { - status := trueString - if err != nil { - status = falseString - } - c.statsReporter.ReportReconcile(time.Now().Sub(startTime), key, status) - }() - - // Embed the key into the logger and attach that to the context we pass - // to the Reconciler. - logger := c.logger.With(zap.String(logkey.Key, key)) - ctx := logging.WithLogger(context.TODO(), logger) - - // Run Reconcile, passing it the namespace/name string of the - // resource to be synced. - if err = c.Reconciler.Reconcile(ctx, key); err != nil { - c.handleErr(err, key) - return fmt.Errorf("error syncing %q: %v", key, err) + key := obj.(string) + + startTime := time.Now() + // Send the metrics for the current queue depth + c.statsReporter.ReportQueueDepth(int64(c.WorkQueue.Len())) + + // We call Done here so the workqueue knows we have finished + // processing this item. We also must remember to call Forget if + // reconcile succeeds. If a transient error occurs, we do not call + // Forget and put the item back to the queue with an increased + // delay. + defer c.WorkQueue.Done(key) + + var err error + defer func() { + status := trueString + if err != nil { + status = falseString } - - // Finally, if no error occurs we Forget this item so it does not - // get queued again until another change happens. - c.WorkQueue.Forget(obj) - c.logger.Infof("Successfully synced %q", key) - return nil - }(obj) - - if err != nil { - c.logger.Error(zap.Error(err)) + c.statsReporter.ReportReconcile(time.Now().Sub(startTime), key, status) + }() + + // Embed the key into the logger and attach that to the context we pass + // to the Reconciler. + logger := c.logger.With(zap.String(logkey.Key, key)) + ctx := logging.WithLogger(context.TODO(), logger) + + // Run Reconcile, passing it the namespace/name string of the + // resource to be synced. + if err = c.Reconciler.Reconcile(ctx, key); err != nil { + c.handleErr(err, key) + logger.Errorf("Reconcile failed. Time taken: %v.", time.Now().Sub(startTime)) return true } + // Finally, if no error occurs we Forget this item so it does not + // have any delay when another change happens. + c.WorkQueue.Forget(key) + logger.Infof("Reconcile succeeded. Time taken: %v.", time.Now().Sub(startTime)) + return true } -func (c *Impl) handleErr(err error, key interface{}) { +func (c *Impl) handleErr(err error, key string) { + c.logger.Error(zap.Error(err)) + // Re-queue the key if it's an transient error. if !IsPermanentError(err) { c.WorkQueue.AddRateLimited(key) diff --git a/vendor/github.com/knative/pkg/controller/stats_reporter.go b/vendor/github.com/knative/pkg/controller/stats_reporter.go index cb71eab0..0f4a4c37 100644 --- a/vendor/github.com/knative/pkg/controller/stats_reporter.go +++ b/vendor/github.com/knative/pkg/controller/stats_reporter.go @@ -123,7 +123,7 @@ func (r *reporter) ReportReconcile(duration time.Duration, key, success string) } stats.Record(ctx, reconcileCountStat.M(1)) - stats.Record(ctx, reconcileLatencyStat.M(int64(duration))) + stats.Record(ctx, reconcileLatencyStat.M(int64(duration/time.Millisecond))) return nil } diff --git a/vendor/github.com/knative/pkg/test/spoof/spoof.go b/vendor/github.com/knative/pkg/test/spoof/spoof.go index 9c94b3c5..071617a5 100644 --- a/vendor/github.com/knative/pkg/test/spoof/spoof.go +++ b/vendor/github.com/knative/pkg/test/spoof/spoof.go @@ -30,6 +30,7 @@ import ( "github.com/knative/pkg/test/logging" zipkin "github.com/knative/pkg/test/zipkin" + "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" @@ -43,10 +44,14 @@ const ( requestInterval = 1 * time.Second requestTimeout = 5 * time.Minute // TODO(tcnghia): These probably shouldn't be hard-coded here? - ingressName = "knative-ingressgateway" ingressNamespace = "istio-system" ) +// Temporary work around the upgrade test issue for knative/serving#2434. +// TODO(lichuqiang): remove the backward compatibility for knative-ingressgateway +// once knative/serving#2434 is merged +var ingressNames = []string{"knative-ingressgateway", "istio-ingressgateway"} + // Response is a stripped down subset of http.Response. The is primarily useful // for ResponseCheckers to inspect the response body without consuming it. // Notably, Body is a byte slice instead of an io.ReadCloser. @@ -121,10 +126,18 @@ func New(kubeClientset *kubernetes.Clientset, logger *logging.BaseLogger, domain // GetServiceEndpoint gets the endpoint IP or hostname to use for the service func GetServiceEndpoint(kubeClientset *kubernetes.Clientset) (*string, error) { var endpoint string - ingress, err := kubeClientset.CoreV1().Services(ingressNamespace).Get(ingressName, metav1.GetOptions{}) + var ingress *v1.Service + var err error + for _, ingressName := range ingressNames { + ingress, err = kubeClientset.CoreV1().Services(ingressNamespace).Get(ingressName, metav1.GetOptions{}) + if err == nil { + break + } + } if err != nil { return nil, err } + ingresses := ingress.Status.LoadBalancer.Ingress if len(ingresses) != 1 { return nil, fmt.Errorf("Expected exactly one ingress load balancer, instead had %d: %s", len(ingresses), ingresses) @@ -132,7 +145,7 @@ func GetServiceEndpoint(kubeClientset *kubernetes.Clientset) (*string, error) { ingressToUse := ingresses[0] if ingressToUse.IP == "" { if ingressToUse.Hostname == "" { - return nil, fmt.Errorf("Expected ingress loadbalancer IP or hostname for %s to be set, instead was empty", ingressName) + return nil, fmt.Errorf("Expected ingress loadbalancer IP or hostname for %s to be set, instead was empty", ingress.Name) } endpoint = ingressToUse.Hostname } else { @@ -232,4 +245,4 @@ func (sc *SpoofingClient) LogZipkinTrace(traceID string) error { sc.logger.Infof(prettyJSON.String()) return nil -} \ No newline at end of file +} diff --git a/vendor/github.com/knative/pkg/test/zipkin/util.go b/vendor/github.com/knative/pkg/test/zipkin/util.go index 5d41e5db..a06df7e2 100644 --- a/vendor/github.com/knative/pkg/test/zipkin/util.go +++ b/vendor/github.com/knative/pkg/test/zipkin/util.go @@ -27,8 +27,8 @@ import ( "github.com/knative/pkg/test/logging" "go.opencensus.io/trace" - "k8s.io/client-go/kubernetes" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" ) const ( @@ -68,7 +68,7 @@ func SetupZipkinTracing(kubeClientset *kubernetes.Clientset) error { return errors.New("No Zipkin Pod found on the cluster. Ensure monitoring is switched on for your Knative Setup") } - portForwardCmd := exec.Command("kubectl", "port-forward", "--namespace=" + ZipkinNamespace, zipkinPods.Items[0].Name, fmt.Sprintf("%d:%d", ZipkinPort, ZipkinPort)) + portForwardCmd := exec.Command("kubectl", "port-forward", "--namespace="+ZipkinNamespace, zipkinPods.Items[0].Name, fmt.Sprintf("%d:%d", ZipkinPort, ZipkinPort)) if err = portForwardCmd.Start(); err != nil { return fmt.Errorf("Error starting kubectl port-forward command : %v", err) @@ -107,4 +107,4 @@ func CheckZipkinPortAvailability() error { } server.Close() return nil -} \ No newline at end of file +} diff --git a/vendor/github.com/knative/pkg/webhook/webhook.go b/vendor/github.com/knative/pkg/webhook/webhook.go index 6ca41821..69fa2af9 100644 --- a/vendor/github.com/knative/pkg/webhook/webhook.go +++ b/vendor/github.com/knative/pkg/webhook/webhook.go @@ -204,12 +204,17 @@ func getOrGenerateKeyCertsFromSecret(ctx context.Context, client kubernetes.Inte // it then delegates validation to apis.Validatable on "new". func Validate(ctx context.Context) ResourceCallback { return func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error { - if hifNew, ok := new.(apis.Immutable); ok && old != nil { - hifOld, ok := old.(apis.Immutable) + if immutableNew, ok := new.(apis.Immutable); ok && old != nil { + // Copy the old object and set defaults so that we don't reject our own + // defaulting done earlier in the webhook. + old = old.DeepCopyObject().(GenericCRD) + old.SetDefaults() + + immutableOld, ok := old.(apis.Immutable) if !ok { return fmt.Errorf("unexpected type mismatch %T vs. %T", old, new) } - if err := hifNew.CheckImmutableFields(hifOld); err != nil { + if err := immutableNew.CheckImmutableFields(immutableOld); err != nil { return err } } @@ -231,6 +236,7 @@ func SetDefaults(ctx context.Context) ResourceDefaulter { if err != nil { return err } + *patches = append(*patches, patch...) return nil }