From bffebe2cb6d9697380d600e7bbf03f8bb87ec52e Mon Sep 17 00:00:00 2001 From: irbekrm Date: Thu, 29 Apr 2021 09:51:06 +0100 Subject: [PATCH] Calls to validating webhook can now return warnings Adds warnings to the top level validating functions' signatures Signed-off-by: irbekrm --- pkg/api/util/BUILD.bazel | 1 + pkg/internal/api/validation/BUILD.bazel | 5 +- pkg/internal/api/validation/registry.go | 53 ++++++---- pkg/internal/api/validation/registry_test.go | 92 ++++++++++------- pkg/internal/api/validation/warning.go | 21 ++++ pkg/internal/apis/acme/validation/BUILD.bazel | 1 + .../apis/acme/validation/challenge.go | 8 +- .../apis/acme/validation/challenge_test.go | 9 +- pkg/internal/apis/acme/validation/order.go | 8 +- .../apis/acme/validation/order_test.go | 29 ++++-- .../identity/certificaterequests/BUILD.bazel | 2 + .../certificaterequests.go | 11 ++- .../certificaterequests_test.go | 37 ++++--- .../apis/certmanager/validation/BUILD.bazel | 2 + .../certmanager/validation/certificate.go | 11 ++- .../validation/certificate_test.go | 21 +++- .../validation/certificaterequest.go | 11 ++- .../validation/certificaterequest_test.go | 99 ++++++++++--------- .../certmanager/validation/clusterissuer.go | 15 +-- .../apis/certmanager/validation/issuer.go | 29 +++--- .../certmanager/validation/issuer_test.go | 18 ++-- .../testdata/apis/testgroup/v2/validation.go | 7 +- .../apis/testgroup/validation/BUILD.bazel | 1 + .../apis/testgroup/validation/validation.go | 13 ++- .../testgroup/validation/validation_test.go | 45 ++++++--- pkg/webhook/handlers/validation.go | 10 +- 26 files changed, 366 insertions(+), 193 deletions(-) create mode 100644 pkg/internal/api/validation/warning.go diff --git a/pkg/api/util/BUILD.bazel b/pkg/api/util/BUILD.bazel index dc14963d5..2c9ad44b6 100644 --- a/pkg/api/util/BUILD.bazel +++ b/pkg/api/util/BUILD.bazel @@ -8,6 +8,7 @@ go_library( "issuers.go", "names.go", "usages.go", + "warnings.go", ], importpath = "github.com/jetstack/cert-manager/pkg/api/util", visibility = ["//visibility:public"], diff --git a/pkg/internal/api/validation/BUILD.bazel b/pkg/internal/api/validation/BUILD.bazel index c81632d0c..fb5af0436 100644 --- a/pkg/internal/api/validation/BUILD.bazel +++ b/pkg/internal/api/validation/BUILD.bazel @@ -2,7 +2,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["registry.go"], + srcs = [ + "registry.go", + "warning.go", + ], importpath = "github.com/jetstack/cert-manager/pkg/internal/api/validation", visibility = ["//pkg:__subpackages__"], deps = [ diff --git a/pkg/internal/api/validation/registry.go b/pkg/internal/api/validation/registry.go index afe545837..521648cca 100644 --- a/pkg/internal/api/validation/registry.go +++ b/pkg/internal/api/validation/registry.go @@ -35,8 +35,8 @@ type Registry struct { validateUpdateRegister map[schema.GroupVersionKind]ValidateUpdateFunc } -type ValidateFunc func(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList -type ValidateUpdateFunc func(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList +type ValidateFunc func(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, WarningList) +type ValidateUpdateFunc func(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, WarningList) // NewRegistry creates a new empty registry, backed by the provided Scheme. func NewRegistry(scheme *runtime.Scheme) *Registry { @@ -93,26 +93,29 @@ func (r *Registry) AddValidateUpdateFunc(obj runtime.Object, fn ValidateUpdateFu // calling the validation functions. // Any validation functions registered for the objects internal API version // will be run against the object regardless of version. -func (r *Registry) Validate(req *admissionv1.AdmissionRequest, obj runtime.Object, requestVersion schema.GroupVersionKind) field.ErrorList { +func (r *Registry) Validate(req *admissionv1.AdmissionRequest, obj runtime.Object, requestVersion schema.GroupVersionKind) (field.ErrorList, WarningList) { versioned, internal := r.lookupValidateFuncs(requestVersion) if versioned == nil && internal == nil { - return nil + return nil, nil } targetObj, internalObj, err := r.convert(obj, requestVersion) if err != nil { - return internalError(err) + return internalError(err), nil } el := field.ErrorList{} + warnings := WarningList{} if versioned != nil { - el = append(el, versioned(req, targetObj)...) + e, w := versioned(req, targetObj) + el, warnings = append(el, e...), append(warnings, w...) } if internal != nil { - el = append(el, internal(req, internalObj)...) + e, w := internal(req, internalObj) + el, warnings = append(el, e...), append(warnings, w...) } - return el + return el, warnings } // ValidateUpdate will run all update validation functions registered for the @@ -122,31 +125,34 @@ func (r *Registry) Validate(req *admissionv1.AdmissionRequest, obj runtime.Objec // calling the validation functions. // Any validation functions registered for the objects internal API version // will be run against the object regardless of version. -func (r *Registry) ValidateUpdate(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object, requestVersion schema.GroupVersionKind) field.ErrorList { +func (r *Registry) ValidateUpdate(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object, requestVersion schema.GroupVersionKind) (field.ErrorList, []string) { versioned, internal := r.lookupValidateUpdateFuncs(requestVersion) if versioned == nil && internal == nil { - return nil + return nil, nil } targetOldObj, internalOldObj, err := r.convert(oldObj, requestVersion) if err != nil { - return internalError(err) + return internalError(err), nil } targetObj, internalObj, err := r.convert(obj, requestVersion) if err != nil { - return internalError(err) + return internalError(err), nil } el := field.ErrorList{} + warnings := WarningList{} if versioned != nil { - el = append(el, versioned(req, targetOldObj, targetObj)...) + e, w := versioned(req, targetOldObj, targetObj) + el, warnings = append(el, e...), append(warnings, w...) } if internal != nil { - el = append(el, internal(req, internalOldObj, internalObj)...) + e, w := internal(req, internalOldObj, internalObj) + el, warnings = append(el, e...), append(warnings, w...) } - return el + return el, warnings } func (r *Registry) lookupValidateFuncs(gvk schema.GroupVersionKind) (versioned ValidateFunc, internal ValidateFunc) { @@ -170,8 +176,12 @@ func (r *Registry) appendValidate(gvk schema.GroupVersionKind, fn ValidateFunc) return } - r.validateRegister[gvk] = func(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { - return append(existing(req, obj), fn(req, obj)...) + // If a ValidateFunc for GVK already exists, build a new ValidateFunc that + // will return both the results of the new and old ValidateFunc. + r.validateRegister[gvk] = func(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, WarningList) { + e, w := existing(req, obj) + newE, newW := fn(req, obj) + return append(e, newE...), append(w, newW...) } } @@ -182,8 +192,13 @@ func (r *Registry) appendValidateUpdate(gvk schema.GroupVersionKind, fn Validate return } - r.validateUpdateRegister[gvk] = func(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList { - return append(existing(req, oldObj, obj), fn(req, oldObj, obj)...) + // If a ValidateUpdateFunc for GVK already exists, build a new + // ValidateUpdateFunc that will return both the results of the new and old + // ValidateUpdateFunc. + r.validateUpdateRegister[gvk] = func(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, WarningList) { + e, w := existing(req, oldObj, obj) + newE, newW := fn(req, oldObj, obj) + return append(e, newE...), append(w, newW...) } } diff --git a/pkg/internal/api/validation/registry_test.go b/pkg/internal/api/validation/registry_test.go index 919a249cb..bc19066b8 100644 --- a/pkg/internal/api/validation/registry_test.go +++ b/pkg/internal/api/validation/registry_test.go @@ -18,6 +18,7 @@ package validation_test import ( "fmt" + "reflect" "testing" admissionv1 "k8s.io/api/admission/v1" @@ -41,14 +42,17 @@ var ( func TestValidateType(t *testing.T) { reg := validation.NewRegistry(scheme) called := false - utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { called = true - return nil + return nil, nil })) - errs := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) > 0 { t.Errorf("expected to not get an error but got: %v", errs.ToAggregate()) } + if len(warnings) > 0 { + t.Errorf("expected no warnings but got: %+v", warnings) + } if !called { t.Errorf("expected registered validation function to run but it did not") } @@ -59,22 +63,25 @@ func TestValidateTypeMultiple(t *testing.T) { called1 := false called2 := false calledInternal := false - utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { called1 = true - return nil + return nil, nil })) - utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { called2 = true - return nil + return nil, nil })) - utilruntime.Must(reg.AddValidateFunc(&cmapiinternal.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateFunc(&cmapiinternal.Certificate{}, func(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { calledInternal = true - return nil + return nil, nil })) - errs := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) > 0 { t.Errorf("expected to not get an error but got: %v", errs.ToAggregate()) } + if len(warnings) > 0 { + t.Errorf("expected to not get any warnings but got %+v", warnings) + } if !called1 || !called2 { t.Errorf("expected registered validation function to run but it did not") } @@ -88,22 +95,25 @@ func TestValidateUpdateTypeMultiple(t *testing.T) { called1 := false called2 := false calledInternal := false - utilruntime.Must(reg.AddValidateUpdateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, _, _ runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateUpdateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, _, _ runtime.Object) (field.ErrorList, validation.WarningList) { called1 = true - return nil + return nil, nil })) - utilruntime.Must(reg.AddValidateUpdateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, _, _ runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateUpdateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, _, _ runtime.Object) (field.ErrorList, validation.WarningList) { called2 = true - return nil + return nil, nil })) - utilruntime.Must(reg.AddValidateUpdateFunc(&cmapiinternal.Certificate{}, func(_ *admissionv1.AdmissionRequest, _, _ runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateUpdateFunc(&cmapiinternal.Certificate{}, func(_ *admissionv1.AdmissionRequest, _, _ runtime.Object) (field.ErrorList, validation.WarningList) { calledInternal = true - return nil + return nil, nil })) - errs := reg.ValidateUpdate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.ValidateUpdate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) > 0 { t.Errorf("expected to not get an error but got: %v", errs.ToAggregate()) } + if len(warnings) > 0 { + t.Errorf("expected to not get any warnings but got: %v", warnings) + } if !called1 || !called2 { t.Errorf("expected registered validation function to run but it did not") } @@ -115,33 +125,40 @@ func TestValidateUpdateTypeMultiple(t *testing.T) { func TestValidateUpdateType(t *testing.T) { reg := validation.NewRegistry(scheme) called := false - utilruntime.Must(reg.AddValidateUpdateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateUpdateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, validation.WarningList) { called = true - return nil + return nil, nil })) - errs := reg.ValidateUpdate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.ValidateUpdate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) > 0 { t.Errorf("expected to not get an error but got: %v", errs.ToAggregate()) } + if len(warnings) > 0 { + t.Errorf("expected to not get any warnings but got %+v", warnings) + } if !called { t.Errorf("expected registered validation function to run but it did not") } } -func TestValidateTypeReturnsErrors(t *testing.T) { +func TestValidateTypeReturnsErrorsAndWarnings(t *testing.T) { reg := validation.NewRegistry(scheme) called := false expectedErr := field.InternalError(nil, fmt.Errorf("failed")) - utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { + expectedWarnings := validation.WarningList{"test warning"} + utilruntime.Must(reg.AddValidateFunc(&cmapi.Certificate{}, func(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { called = true - return field.ErrorList{expectedErr} + return field.ErrorList{expectedErr}, expectedWarnings })) - errs := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) != 1 { t.Error("expected to get an error but got none") } else if err := errs[0]; err.Error() != expectedErr.Error() { t.Errorf("expected error to be %q but got %q", expectedErr.Error(), err.Error()) } + if !reflect.DeepEqual(warnings, expectedWarnings) { + t.Errorf("expected warnings %+#v got %+#v", expectedWarnings, warnings) + } if !called { t.Errorf("expected registered validation function to run but it did not") } @@ -150,14 +167,17 @@ func TestValidateTypeReturnsErrors(t *testing.T) { func TestValidateInternalType(t *testing.T) { reg := validation.NewRegistry(scheme) called := false - utilruntime.Must(reg.AddValidateFunc(&cmapiinternal.Certificate{}, func(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { + utilruntime.Must(reg.AddValidateFunc(&cmapiinternal.Certificate{}, func(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { called = true - return nil + return nil, nil })) - errs := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) > 0 { t.Errorf("expected to not get an error but got: %v", errs.ToAggregate()) } + if len(warnings) > 0 { + t.Errorf("expected to not get any warnings but got %+v", warnings) + } if !called { t.Errorf("expected registered internal validation function to run for external type but it did not") } @@ -165,24 +185,30 @@ func TestValidateInternalType(t *testing.T) { func TestValidateNoErrorNoneRegistered(t *testing.T) { reg := validation.NewRegistry(scheme) - errs := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.Validate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) > 0 { t.Errorf("expected to not get an error but got: %v", errs.ToAggregate()) } + if len(warnings) > 0 { + t.Errorf("expected to not get any warnings but got: %+v", warnings) + } } func TestValidateUpdateNoErrorNoneRegistered(t *testing.T) { reg := validation.NewRegistry(scheme) - errs := reg.ValidateUpdate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) + errs, warnings := reg.ValidateUpdate(&admissionv1.AdmissionRequest{}, &cmapi.Certificate{}, &cmapi.Certificate{}, cmapi.SchemeGroupVersion.WithKind("Certificate")) if len(errs) > 0 { t.Errorf("expected to not get an error but got: %v", errs.ToAggregate()) } + if len(warnings) > 0 { + t.Errorf("exptected to not get any warnings but got: %+v", warnings) + } } func TestValidateUnrecognisedType(t *testing.T) { reg := validation.NewRegistry(scheme) - err := reg.AddValidateFunc(&corev1.Pod{}, func(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { - return nil + err := reg.AddValidateFunc(&corev1.Pod{}, func(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { + return nil, nil }) if err == nil { t.Errorf("expected to get an error but did not") @@ -191,8 +217,8 @@ func TestValidateUnrecognisedType(t *testing.T) { func TestValidateUpdateUnrecognisedType(t *testing.T) { reg := validation.NewRegistry(scheme) - err := reg.AddValidateUpdateFunc(&corev1.Pod{}, func(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList { - return nil + err := reg.AddValidateUpdateFunc(&corev1.Pod{}, func(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, validation.WarningList) { + return nil, nil }) if err == nil { t.Errorf("expected to get an error but did not") diff --git a/pkg/internal/api/validation/warning.go b/pkg/internal/api/validation/warning.go new file mode 100644 index 000000000..46c673465 --- /dev/null +++ b/pkg/internal/api/validation/warning.go @@ -0,0 +1,21 @@ +/* +Copyright 2020 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +// WarningList is a list of warnings that will be returned by the validating webhook. +// See https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#webhook-request-and-response +type WarningList []string diff --git a/pkg/internal/apis/acme/validation/BUILD.bazel b/pkg/internal/apis/acme/validation/BUILD.bazel index 02815f20e..ba7567d8f 100644 --- a/pkg/internal/apis/acme/validation/BUILD.bazel +++ b/pkg/internal/apis/acme/validation/BUILD.bazel @@ -26,6 +26,7 @@ go_test( ], embed = [":go_default_library"], deps = [ + "//pkg/internal/api/validation:go_default_library", "//pkg/internal/apis/acme:go_default_library", "@io_k8s_apimachinery//pkg/util/validation/field:go_default_library", "@io_k8s_utils//pointer:go_default_library", diff --git a/pkg/internal/apis/acme/validation/challenge.go b/pkg/internal/apis/acme/validation/challenge.go index 956a9df9b..0c26d2b8f 100644 --- a/pkg/internal/apis/acme/validation/challenge.go +++ b/pkg/internal/apis/acme/validation/challenge.go @@ -23,21 +23,23 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmacme "github.com/jetstack/cert-manager/pkg/internal/apis/acme" ) -func ValidateChallengeUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) field.ErrorList { +func ValidateChallengeUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) (field.ErrorList, validation.WarningList) { old, ok := oldObj.(*cmacme.Challenge) new := newObj.(*cmacme.Challenge) // if oldObj is not set, the Update operation is always valid. if !ok || old == nil { - return nil + return nil, nil } + var warnings validation.WarningList el := field.ErrorList{} if !reflect.DeepEqual(old.Spec, new.Spec) { el = append(el, field.Forbidden(field.NewPath("spec"), "challenge spec is immutable after creation")) } - return el + return el, warnings } diff --git a/pkg/internal/apis/acme/validation/challenge_test.go b/pkg/internal/apis/acme/validation/challenge_test.go index c182b3a0c..657f77931 100644 --- a/pkg/internal/apis/acme/validation/challenge_test.go +++ b/pkg/internal/apis/acme/validation/challenge_test.go @@ -22,6 +22,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmacme "github.com/jetstack/cert-manager/pkg/internal/apis/acme" ) @@ -29,6 +30,7 @@ func TestValidateChallengeUpdate(t *testing.T) { scenarios := map[string]struct { old, new *cmacme.Challenge errs []*field.Error + warnings validation.WarningList }{ "allows setting challenge spec for the first time": { new: &cmacme.Challenge{ @@ -67,7 +69,7 @@ func TestValidateChallengeUpdate(t *testing.T) { } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - errs := ValidateChallengeUpdate(nil, s.old, s.new) + errs, warnings := ValidateChallengeUpdate(nil, s.old, s.new) if len(errs) != len(s.errs) { t.Errorf("Expected %v but got %v", s.errs, errs) return @@ -75,9 +77,12 @@ func TestValidateChallengeUpdate(t *testing.T) { for i, e := range errs { expectedErr := s.errs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected errors %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, s.warnings) { + t.Errorf("Expected warnings %+#v but got %+#v", s.warnings, warnings) + } }) } } diff --git a/pkg/internal/apis/acme/validation/order.go b/pkg/internal/apis/acme/validation/order.go index d87606a1d..8461b322b 100644 --- a/pkg/internal/apis/acme/validation/order.go +++ b/pkg/internal/apis/acme/validation/order.go @@ -23,21 +23,23 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmacme "github.com/jetstack/cert-manager/pkg/internal/apis/acme" ) -func ValidateOrderUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) field.ErrorList { +func ValidateOrderUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) (field.ErrorList, validation.WarningList) { old, ok := oldObj.(*cmacme.Order) new := newObj.(*cmacme.Order) // if oldObj is not set, the Update operation is always valid. if !ok || old == nil { - return nil + return nil, nil } el := field.ErrorList{} + var warnings validation.WarningList el = append(el, ValidateOrderSpecUpdate(old.Spec, new.Spec, field.NewPath("spec"))...) el = append(el, ValidateOrderStatusUpdate(old.Status, new.Status, field.NewPath("status"))...) - return el + return el, warnings } func ValidateOrderSpecUpdate(old, new cmacme.OrderSpec, fldPath *field.Path) field.ErrorList { diff --git a/pkg/internal/apis/acme/validation/order_test.go b/pkg/internal/apis/acme/validation/order_test.go index df4e08629..7a33cf6b5 100644 --- a/pkg/internal/apis/acme/validation/order_test.go +++ b/pkg/internal/apis/acme/validation/order_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmacme "github.com/jetstack/cert-manager/pkg/internal/apis/acme" ) @@ -43,39 +44,47 @@ func testImmutableOrderField(t *testing.T, fldPath *field.Path, setter func(*cma expectedErrs := []*field.Error{ field.Forbidden(fldPath, "field is immutable once set"), } + var expectedWarnings validation.WarningList old := &cmacme.Order{} new := &cmacme.Order{} setter(old, testValueOptionOne) setter(new, testValueOptionTwo) - errs := ValidateOrderUpdate(nil, old, new) + errs, warnings := ValidateOrderUpdate(nil, old, new) if len(errs) != len(expectedErrs) { - t.Errorf("Expected %v but got %v", expectedErrs, errs) + t.Errorf("Expected errors %v but got %v", expectedErrs, errs) return } for i, e := range errs { expectedErr := expectedErrs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected error %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, expectedWarnings) { + t.Errorf("Expected warnings %+#v but got %+#v", expectedWarnings, warnings) + } }) t.Run("should allow updates to "+fldPath.String()+" if not already set", func(t *testing.T) { expectedErrs := []*field.Error{} + var expectedWarnings validation.WarningList old := &cmacme.Order{} new := &cmacme.Order{} setter(old, testValueNone) setter(new, testValueOptionOne) - errs := ValidateOrderUpdate(nil, old, new) + errs, warnings := ValidateOrderUpdate(nil, old, new) if len(errs) != len(expectedErrs) { - t.Errorf("Expected %v but got %v", expectedErrs, errs) + t.Errorf("Expected errors %v but got %v", expectedErrs, errs) return } for i, e := range errs { expectedErr := expectedErrs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected error %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, expectedWarnings) { + t.Errorf("Expected warnings %+#v but got %+#v", expectedWarnings, warnings) + } }) } @@ -200,6 +209,7 @@ func TestValidateCertificateUpdate(t *testing.T) { scenarios := map[string]struct { old, new *cmacme.Order errs []*field.Error + warnings validation.WarningList }{ "allows all updates if old is nil": { new: &cmacme.Order{ @@ -211,7 +221,7 @@ func TestValidateCertificateUpdate(t *testing.T) { } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - errs := ValidateOrderUpdate(nil, s.old, s.new) + errs, warnings := ValidateOrderUpdate(nil, s.old, s.new) if len(errs) != len(s.errs) { t.Errorf("Expected %v but got %v", s.errs, errs) return @@ -219,9 +229,12 @@ func TestValidateCertificateUpdate(t *testing.T) { for i, e := range errs { expectedErr := s.errs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected errors %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, s.warnings) { + t.Errorf("Expected warnings %+#v but got %+#v", s.warnings, warnings) + } }) } } diff --git a/pkg/internal/apis/certmanager/identity/certificaterequests/BUILD.bazel b/pkg/internal/apis/certmanager/identity/certificaterequests/BUILD.bazel index d8276dc5d..11d30eeb1 100644 --- a/pkg/internal/apis/certmanager/identity/certificaterequests/BUILD.bazel +++ b/pkg/internal/apis/certmanager/identity/certificaterequests/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager/identity/certificaterequests", visibility = ["//pkg:__subpackages__"], deps = [ + "//pkg/internal/api/validation:go_default_library", "//pkg/internal/apis/certmanager:go_default_library", "//pkg/util:go_default_library", "@io_k8s_api//admission/v1:go_default_library", @@ -34,6 +35,7 @@ go_test( srcs = ["certificaterequests_test.go"], embed = [":go_default_library"], deps = [ + "//pkg/internal/api/validation:go_default_library", "//pkg/internal/apis/certmanager:go_default_library", "@io_k8s_api//admission/v1:go_default_library", "@io_k8s_api//authentication/v1:go_default_library", diff --git a/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go b/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go index f62c4a26c..d59812fff 100644 --- a/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go +++ b/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go @@ -26,11 +26,13 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" "github.com/jetstack/cert-manager/pkg/util" ) -func ValidateCreate(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { +func ValidateCreate(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList cr := obj.(*cmapi.CertificateRequest) fldPath := field.NewPath("spec") @@ -48,7 +50,7 @@ func ValidateCreate(req *admissionv1.AdmissionRequest, obj runtime.Object) field el = append(el, field.Forbidden(fldPath.Child("extra"), "extra identity must be that of the requester")) } - return el + return el, warnings } func extrasMatch(crExtra map[string][]string, reqExtra map[string]authenticationv1.ExtraValue) bool { @@ -70,7 +72,8 @@ func extrasMatch(crExtra map[string][]string, reqExtra map[string]authentication return true } -func ValidateUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) field.ErrorList { +func ValidateUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList oldCR, newCR := oldObj.(*cmapi.CertificateRequest), newObj.(*cmapi.CertificateRequest) fldPath := field.NewPath("spec") @@ -88,7 +91,7 @@ func ValidateUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Obje el = append(el, field.Forbidden(fldPath.Child("extra"), "extra identity cannot be changed once set")) } - return el + return el, warnings } func MutateCreate(req *admissionv1.AdmissionRequest, obj runtime.Object) { diff --git a/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests_test.go b/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests_test.go index 097b950ec..ade8d07f2 100644 --- a/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests_test.go +++ b/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests_test.go @@ -24,6 +24,7 @@ import ( authenticationv1 "k8s.io/api/authentication/v1" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" ) @@ -31,9 +32,10 @@ func TestValidateCreate(t *testing.T) { fldPath := field.NewPath("spec") tests := map[string]struct { - req *admissionv1.AdmissionRequest - cr *cmapi.CertificateRequest - want field.ErrorList + req *admissionv1.AdmissionRequest + cr *cmapi.CertificateRequest + wantE field.ErrorList + wantW validation.WarningList }{ "if identity fields don't match that of requester, should fail": { req: &admissionv1.AdmissionRequest{ @@ -58,7 +60,7 @@ func TestValidateCreate(t *testing.T) { }, }, }, - want: field.ErrorList{ + wantE: field.ErrorList{ field.Forbidden(fldPath.Child("uid"), "uid identity must be that of the requester"), field.Forbidden(fldPath.Child("username"), "username identity must be that of the requester"), field.Forbidden(fldPath.Child("groups"), "groups identity must be that of the requester"), @@ -88,15 +90,18 @@ func TestValidateCreate(t *testing.T) { }, }, }, - want: nil, + wantE: nil, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - got := ValidateCreate(test.req, test.cr) - if !reflect.DeepEqual(got, test.want) { - t.Errorf("ValidateCreate() = %v, want %v", got, test.want) + gotE, gotW := ValidateCreate(test.req, test.cr) + if !reflect.DeepEqual(gotE, test.wantE) { + t.Errorf("errors from ValidateCreate() = %v, want %v", gotE, test.wantE) + } + if !reflect.DeepEqual(gotW, test.wantW) { + t.Errorf("warnings from ValidateCreate() = %v, want %v", gotW, test.wantW) } }) } @@ -107,7 +112,8 @@ func TestValidateUpdate(t *testing.T) { tests := map[string]struct { oldCR, newCR *cmapi.CertificateRequest - want field.ErrorList + wantE field.ErrorList + wantW validation.WarningList }{ "if identity fields don't match that of the old CertificateRequest, should fail": { oldCR: &cmapi.CertificateRequest{ @@ -132,7 +138,7 @@ func TestValidateUpdate(t *testing.T) { }, }, }, - want: field.ErrorList{ + wantE: field.ErrorList{ field.Forbidden(fldPath.Child("uid"), "uid identity cannot be changed once set"), field.Forbidden(fldPath.Child("username"), "username identity cannot be changed once set"), field.Forbidden(fldPath.Child("groups"), "groups identity cannot be changed once set"), @@ -162,15 +168,18 @@ func TestValidateUpdate(t *testing.T) { }, }, }, - want: nil, + wantE: nil, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - got := ValidateUpdate(nil, test.newCR, test.oldCR) - if !reflect.DeepEqual(got, test.want) { - t.Errorf("ValidateUpdate() = %v, want %v", got, test.want) + gotE, gotW := ValidateUpdate(nil, test.newCR, test.oldCR) + if !reflect.DeepEqual(gotE, test.wantE) { + t.Errorf("errors from ValidateUpdate() = %v, want %v", gotE, test.wantE) + } + if !reflect.DeepEqual(gotW, test.wantW) { + t.Errorf("warnings from ValidateUpdate() = %v, want %v", gotW, test.wantW) } }) } diff --git a/pkg/internal/apis/certmanager/validation/BUILD.bazel b/pkg/internal/apis/certmanager/validation/BUILD.bazel index d072fa1b0..0b518f62f 100644 --- a/pkg/internal/apis/certmanager/validation/BUILD.bazel +++ b/pkg/internal/apis/certmanager/validation/BUILD.bazel @@ -42,7 +42,9 @@ go_test( ], embed = [":go_default_library"], deps = [ + "//pkg/api/util:go_default_library", "//pkg/apis/certmanager/v1:go_default_library", + "//pkg/internal/api/validation:go_default_library", "//pkg/internal/apis/acme:go_default_library", "//pkg/internal/apis/certmanager:go_default_library", "//pkg/internal/apis/meta:go_default_library", diff --git a/pkg/internal/apis/certmanager/validation/certificate.go b/pkg/internal/apis/certmanager/validation/certificate.go index b0496aa13..482db4845 100644 --- a/pkg/internal/apis/certmanager/validation/certificate.go +++ b/pkg/internal/apis/certmanager/validation/certificate.go @@ -27,6 +27,7 @@ import ( "github.com/jetstack/cert-manager/pkg/api/util" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" internalcmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" cmmeta "github.com/jetstack/cert-manager/pkg/internal/apis/meta" ) @@ -86,16 +87,18 @@ func ValidateCertificateSpec(crt *internalcmapi.CertificateSpec, fldPath *field. return el } -func ValidateCertificate(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { +func ValidateCertificate(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList crt := obj.(*internalcmapi.Certificate) allErrs := ValidateCertificateSpec(&crt.Spec, field.NewPath("spec")) - return allErrs + return allErrs, warnings } -func ValidateUpdateCertificate(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList { +func ValidateUpdateCertificate(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList crt := obj.(*internalcmapi.Certificate) allErrs := ValidateCertificateSpec(&crt.Spec, field.NewPath("spec")) - return allErrs + return allErrs, warnings } func validateIssuerRef(issuerRef cmmeta.ObjectReference, fldPath *field.Path) field.ErrorList { diff --git a/pkg/internal/apis/certmanager/validation/certificate_test.go b/pkg/internal/apis/certmanager/validation/certificate_test.go index 7ccf89eac..4abf6beeb 100644 --- a/pkg/internal/apis/certmanager/validation/certificate_test.go +++ b/pkg/internal/apis/certmanager/validation/certificate_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" internalcmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" cmmeta "github.com/jetstack/cert-manager/pkg/internal/apis/meta" ) @@ -48,8 +49,9 @@ func int32Ptr(i int32) *int32 { func TestValidateCertificate(t *testing.T) { fldPath := field.NewPath("spec") scenarios := map[string]struct { - cfg *internalcmapi.Certificate - errs []*field.Error + cfg *internalcmapi.Certificate + errs []*field.Error + warnings validation.WarningList }{ "valid basic certificate": { cfg: &internalcmapi.Certificate{ @@ -514,15 +516,24 @@ func TestValidateCertificate(t *testing.T) { } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - errs := ValidateCertificate(nil, s.cfg) + errs, warnings := ValidateCertificate(nil, s.cfg) if len(errs) != len(s.errs) { - t.Errorf("Expected %v but got %v", s.errs, errs) + t.Errorf("Expected errors %v but got %v", s.errs, errs) return } + if len(warnings) != len(s.warnings) { + t.Errorf("Expected warnings %v but got %v", s.warnings, warnings) + } for i, e := range errs { expectedErr := s.errs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected error %v but got %v", expectedErr, e) + } + } + for i, w := range warnings { + expectedWarning := s.warnings[i] + if w != expectedWarning { + t.Errorf("Expected warning %q but got %q", expectedWarning, w) } } }) diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest.go b/pkg/internal/apis/certmanager/validation/certificaterequest.go index 2a55400e8..5a8a31d2a 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest.go @@ -30,6 +30,7 @@ import ( "github.com/jetstack/cert-manager/pkg/apis/acme" "github.com/jetstack/cert-manager/pkg/apis/certmanager" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" cmmeta "github.com/jetstack/cert-manager/pkg/internal/apis/meta" "github.com/jetstack/cert-manager/pkg/util" @@ -38,16 +39,18 @@ import ( var defaultInternalKeyUsages = []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment} -func ValidateCertificateRequest(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { +func ValidateCertificateRequest(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList cr := obj.(*cmapi.CertificateRequest) allErrs := ValidateCertificateRequestSpec(&cr.Spec, field.NewPath("spec"), true) allErrs = append(allErrs, ValidateCertificateRequestApprovalCondition(cr.Status.Conditions, field.NewPath("status", "conditions"))...) - return allErrs + return allErrs, warnings } -func ValidateUpdateCertificateRequest(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) field.ErrorList { +func ValidateUpdateCertificateRequest(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList oldCR, newCR := oldObj.(*cmapi.CertificateRequest), newObj.(*cmapi.CertificateRequest) var el field.ErrorList @@ -66,7 +69,7 @@ func ValidateUpdateCertificateRequest(_ *admissionv1.AdmissionRequest, oldObj, n el = append(el, field.Forbidden(field.NewPath("spec"), "cannot change spec after creation")) } - return el + return el, warnings } func validateCertificateRequestAnnotations(objA, objB *cmapi.CertificateRequest, fieldPath *field.Path) field.ErrorList { diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go index cad2f196f..d7088b66a 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cminternal "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" cminternalmeta "github.com/jetstack/cert-manager/pkg/internal/apis/meta" "github.com/jetstack/cert-manager/pkg/util/pki" @@ -62,7 +63,8 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { tests := map[string]struct { oldCR, newCR *cminternal.CertificateRequest - want field.ErrorList + wantE field.ErrorList + wantW validation.WarningList }{ "if CertificateRequest spec and cert-manager.io annotations change, error": { oldCR: baseCR.DeepCopy(), @@ -77,7 +79,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))), }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(field.NewPath("metadata", "annotations", "cert-manager.io/foo"), "cannot change cert-manager annotation after creation"), field.Forbidden(field.NewPath("spec"), "cannot change spec after creation"), }, @@ -95,7 +97,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))), }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(field.NewPath("metadata", "annotations", "acme.cert-manager.io/bar"), "cannot change cert-manager annotation after creation"), field.Forbidden(field.NewPath("spec"), "cannot change spec after creation"), }, @@ -103,7 +105,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { "if CertificateRequest spec and annotations do not change, don't error": { oldCR: baseCR.DeepCopy(), newCR: baseCR.DeepCopy(), - want: nil, + wantE: nil, }, "CertificateRequest with single Approved=true condition that doesn't change, shouldn't error": { oldCR: &cminternal.CertificateRequest{ @@ -134,7 +136,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: nil, + wantE: nil, }, "CertificateRequest with single Denied=true condition that doesn't change, shouldn't error": { oldCR: &cminternal.CertificateRequest{ @@ -165,7 +167,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: nil, + wantE: nil, }, "CertificateRequest with single Approved=false condition that changes, should error": { oldCR: &cminternal.CertificateRequest{ @@ -198,7 +200,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, "'Approved' condition may not be modified once set"), }, }, @@ -233,7 +235,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, "'Denied' condition may not be modified once set"), field.Invalid(fldPathConditions.Child("Denied"), nil, `"Denied" condition may only be set to True`), }, @@ -269,7 +271,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, "'Denied' condition may not be modified once set"), }, }, @@ -304,7 +306,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, "'Approved' condition may not be modified once set"), }, }, @@ -333,7 +335,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: nil, + wantE: nil, }, "CertificateRequest with no condition that changes to Denied=true, shouldn't error": { oldCR: &cminternal.CertificateRequest{ @@ -360,7 +362,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { }, }, }, - want: nil, + wantE: nil, }, "CertificateRequest with single Approved=true condition that is removed, should error": { oldCR: &cminternal.CertificateRequest{ @@ -386,7 +388,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { Conditions: []cminternal.CertificateRequestCondition{}, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, "'Approved' condition may not be modified once set"), }, }, @@ -414,7 +416,7 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { Conditions: []cminternal.CertificateRequestCondition{}, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, "'Denied' condition may not be modified once set"), }, }, @@ -422,16 +424,19 @@ func TestValidateCertificateRequestUpdate(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - got := ValidateUpdateCertificateRequest(nil, test.oldCR, test.newCR) - for i := range got { - if got[i].Type != field.ErrorTypeForbidden { + gotE, gotW := ValidateUpdateCertificateRequest(nil, test.oldCR, test.newCR) + for i := range gotE { + if gotE[i].Type != field.ErrorTypeForbidden { // filter out the value so it does not print the full CSR in tests - got[i].BadValue = nil + gotE[i].BadValue = nil } } - if !reflect.DeepEqual(got, test.want) { - t.Errorf("ValidateUpdateCertificateRequest() = %v, want %v", got, test.want) + if !reflect.DeepEqual(gotE, test.wantE) { + t.Errorf("errors from ValidateUpdateCertificateRequest() = %v, want %v", gotE, test.wantE) + } + if !reflect.DeepEqual(gotW, test.wantW) { + t.Errorf("warnings from ValidateUpdateCertificateRequest() = %#+v, want %#+v", gotW, test.wantW) } }) } @@ -442,8 +447,9 @@ func TestValidateCertificateRequest(t *testing.T) { fldPathConditions := field.NewPath("status", "conditions") tests := map[string]struct { - cr *cminternal.CertificateRequest - want field.ErrorList + cr *cminternal.CertificateRequest + wantE field.ErrorList + wantW validation.WarningList }{ "Test csr with no usages": { cr: &cminternal.CertificateRequest{ @@ -453,7 +459,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: nil, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "Test csr with double signature usages": { cr: &cminternal.CertificateRequest{ @@ -463,7 +469,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "Test csr with double extended usages": { cr: &cminternal.CertificateRequest{ @@ -473,7 +479,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "Test csr with reordered usages": { cr: &cminternal.CertificateRequest{ @@ -483,7 +489,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageServerAuth, cminternal.UsageClientAuth, cminternal.UsageKeyEncipherment, cminternal.UsageDigitalSignature}, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "Test csr that is CA with usages set": { cr: &cminternal.CertificateRequest{ @@ -494,7 +500,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageCertSign}, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "Test csr that is CA but no cert sign in usages": { cr: &cminternal.CertificateRequest{ @@ -505,7 +511,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageClientAuth, cminternal.UsageServerAuth}, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "Error on csr not having all usages": { cr: &cminternal.CertificateRequest{ @@ -515,7 +521,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth}, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Invalid(fldPath.Child("request"), nil, "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[3] != []certmanager.KeyUsage[4]]"), }, }, @@ -527,7 +533,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment}, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Invalid(fldPath.Child("request"), nil, "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[4] != []certmanager.KeyUsage[2]]"), }, }, @@ -540,7 +546,7 @@ func TestValidateCertificateRequest(t *testing.T) { Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageClientAuth, cminternal.UsageServerAuth}, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "CertificateRequest with single Approved=true condition, shouldn't error": { cr: &cminternal.CertificateRequest{ @@ -559,7 +565,7 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "CertificateRequest with single Denied=true condition, shouldn't error": { cr: &cminternal.CertificateRequest{ @@ -578,7 +584,7 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{}, + wantE: []*field.Error{}, }, "CertificateRequest with single Approved=false condition, should error": { cr: &cminternal.CertificateRequest{ @@ -598,7 +604,7 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Invalid(fldPathConditions.Child("Approved"), nil, `"Approved" condition may only be set to True`), }, @@ -621,7 +627,7 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Invalid(fldPathConditions.Child("Denied"), nil, `"Denied" condition may only be set to True`), }, @@ -649,7 +655,7 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Invalid(field.NewPath("status", "conditions", "Approved"), nil, `"Approved" condition may only be set to True`), field.Invalid(field.NewPath("status", "conditions", "Denied"), nil, @@ -680,7 +686,7 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, "both 'Denied' and 'Approved' conditions cannot coexist"), }, }, @@ -707,7 +713,7 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, `multiple "Approved" conditions present`), }, }, @@ -734,22 +740,25 @@ func TestValidateCertificateRequest(t *testing.T) { }, }, }, - want: []*field.Error{ + wantE: []*field.Error{ field.Forbidden(fldPathConditions, `multiple "Denied" conditions present`), }, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - got := ValidateCertificateRequest(nil, test.cr) - for i := range got { - if got[i].Type != field.ErrorTypeForbidden { + gotE, gotW := ValidateCertificateRequest(nil, test.cr) + for i := range gotE { + if gotE[i].Type != field.ErrorTypeForbidden { // filter out the value so it does not print the full CSR in tests - got[i].BadValue = nil + gotE[i].BadValue = nil } } - if !reflect.DeepEqual(got, test.want) { - t.Errorf("ValidateCertificateRequest() = %v, want %v", got, test.want) + if !reflect.DeepEqual(gotE, test.wantE) { + t.Errorf("errors from ValidateCertificateRequest() = %v, want %v", gotE, test.wantE) + } + if !reflect.DeepEqual(test.wantW, gotW) { + t.Errorf("warnings from ValidateCertificateRequest() = %v, want %v", gotW, test.wantW) } }) } diff --git a/pkg/internal/apis/certmanager/validation/clusterissuer.go b/pkg/internal/apis/certmanager/validation/clusterissuer.go index f7e149ee8..f0d9fd542 100644 --- a/pkg/internal/apis/certmanager/validation/clusterissuer.go +++ b/pkg/internal/apis/certmanager/validation/clusterissuer.go @@ -21,19 +21,20 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" ) -// Validation functions for cert-manager v1alpha2 ClusterIssuer types +// Validation functions for cert-manager ClusterIssuer types. -func ValidateClusterIssuer(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { +func ValidateClusterIssuer(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { iss := obj.(*cmapi.ClusterIssuer) - allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) - return allErrs + allErrs, warnings := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) + return allErrs, warnings } -func ValidateUpdateClusterIssuer(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList { +func ValidateUpdateClusterIssuer(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, validation.WarningList) { iss := obj.(*cmapi.ClusterIssuer) - allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) - return allErrs + allErrs, warnings := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) + return allErrs, warnings } diff --git a/pkg/internal/apis/certmanager/validation/issuer.go b/pkg/internal/apis/certmanager/validation/issuer.go index e113c0856..301d88591 100644 --- a/pkg/internal/apis/certmanager/validation/issuer.go +++ b/pkg/internal/apis/certmanager/validation/issuer.go @@ -26,6 +26,8 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + apiutil "github.com/jetstack/cert-manager/pkg/api/util" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmacme "github.com/jetstack/cert-manager/pkg/internal/apis/acme" "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager/validation/util" @@ -34,23 +36,24 @@ import ( // Validation functions for cert-manager Issuer types. -func ValidateIssuer(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { +func ValidateIssuer(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { iss := obj.(*certmanager.Issuer) - allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) - return allErrs + allErrs, warnings := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) + return allErrs, warnings } -func ValidateUpdateIssuer(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList { +func ValidateUpdateIssuer(_ *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, validation.WarningList) { iss := obj.(*certmanager.Issuer) - allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) - return allErrs + allErrs, warnings := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) + return allErrs, warnings } -func ValidateIssuerSpec(iss *certmanager.IssuerSpec, fldPath *field.Path) field.ErrorList { +func ValidateIssuerSpec(iss *certmanager.IssuerSpec, fldPath *field.Path) (field.ErrorList, validation.WarningList) { return ValidateIssuerConfig(&iss.IssuerConfig, fldPath) } -func ValidateIssuerConfig(iss *certmanager.IssuerConfig, fldPath *field.Path) field.ErrorList { +func ValidateIssuerConfig(iss *certmanager.IssuerConfig, fldPath *field.Path) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList numConfigs := 0 el := field.ErrorList{} if iss.ACME != nil { @@ -58,7 +61,8 @@ func ValidateIssuerConfig(iss *certmanager.IssuerConfig, fldPath *field.Path) fi el = append(el, field.Forbidden(fldPath.Child("acme"), "may not specify more than one issuer type")) } else { numConfigs++ - el = append(el, ValidateACMEIssuerConfig(iss.ACME, fldPath.Child("acme"))...) + e, w := ValidateACMEIssuerConfig(iss.ACME, fldPath.Child("acme")) + el, warnings = append(el, e...), append(warnings, w...) } } if iss.CA != nil { @@ -97,10 +101,11 @@ func ValidateIssuerConfig(iss *certmanager.IssuerConfig, fldPath *field.Path) fi el = append(el, field.Required(fldPath, "at least one issuer must be configured")) } - return el + return el, warnings } -func ValidateACMEIssuerConfig(iss *cmacme.ACMEIssuer, fldPath *field.Path) field.ErrorList { +func ValidateACMEIssuerConfig(iss *cmacme.ACMEIssuer, fldPath *field.Path) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList el := field.ErrorList{} if len(iss.PrivateKey.Name) == 0 { el = append(el, field.Required(fldPath.Child("privateKeySecretRef", "name"), "private key secret name is a required field")) @@ -122,7 +127,7 @@ func ValidateACMEIssuerConfig(iss *cmacme.ACMEIssuer, fldPath *field.Path) field el = append(el, ValidateACMEIssuerChallengeSolverConfig(&sol, fldPath.Child("solvers").Index(i))...) } - return el + return el, warnings } func ValidateACMEIssuerChallengeSolverConfig(sol *cmacme.ACMEChallengeSolver, fldPath *field.Path) field.ErrorList { diff --git a/pkg/internal/apis/certmanager/validation/issuer_test.go b/pkg/internal/apis/certmanager/validation/issuer_test.go index b68ae33ea..f2e549ff5 100644 --- a/pkg/internal/apis/certmanager/validation/issuer_test.go +++ b/pkg/internal/apis/certmanager/validation/issuer_test.go @@ -25,6 +25,8 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation/field" + apiutil "github.com/jetstack/cert-manager/pkg/api/util" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" cmacme "github.com/jetstack/cert-manager/pkg/internal/apis/acme" cmapi "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager" cmmeta "github.com/jetstack/cert-manager/pkg/internal/apis/meta" @@ -106,8 +108,9 @@ func TestValidateVaultIssuerConfig(t *testing.T) { func TestValidateACMEIssuerConfig(t *testing.T) { fldPath := field.NewPath("") scenarios := map[string]struct { - spec *cmacme.ACMEIssuer - errs []*field.Error + spec *cmacme.ACMEIssuer + errs []*field.Error + warnings validation.WarningList }{ "valid acme issuer": { spec: &validACMEIssuer, @@ -316,7 +319,7 @@ func TestValidateACMEIssuerConfig(t *testing.T) { } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - errs := ValidateACMEIssuerConfig(s.spec, fldPath) + errs, warnings := ValidateACMEIssuerConfig(s.spec, fldPath) if len(errs) != len(s.errs) { t.Errorf("Expected %v but got %v", s.errs, errs) return @@ -327,6 +330,7 @@ func TestValidateACMEIssuerConfig(t *testing.T) { t.Errorf("Expected %v but got %v", expectedErr, e) } } + assert.Equal(t, s.warnings, warnings) }) } } @@ -334,8 +338,9 @@ func TestValidateACMEIssuerConfig(t *testing.T) { func TestValidateIssuerSpec(t *testing.T) { fldPath := field.NewPath("") scenarios := map[string]struct { - spec *cmapi.IssuerSpec - errs field.ErrorList + spec *cmapi.IssuerSpec + errs field.ErrorList + warnings validation.WarningList }{ "valid ca issuer": { spec: &cmapi.IssuerSpec{ @@ -427,8 +432,9 @@ func TestValidateIssuerSpec(t *testing.T) { } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - gotErrs := ValidateIssuerSpec(s.spec, fldPath) + gotErrs, warnings := ValidateIssuerSpec(s.spec, fldPath) assert.Equal(t, s.errs, gotErrs) + assert.Equal(t, s.warnings, warnings) }) } } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go b/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go index e25f1bce7..54df85848 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go +++ b/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go @@ -20,13 +20,16 @@ import ( admissionv1 "k8s.io/api/admission/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/jetstack/cert-manager/pkg/internal/api/validation" ) -func ValidateTestType(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { +func ValidateTestType(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { el := field.ErrorList{} + warnings := validation.WarningList{} tt := obj.(*TestType) if tt.TestField == DisallowedTestFieldValue { el = append(el, field.Invalid(field.NewPath("testField"), tt.TestField, "value not allowed")) } - return el + return el, warnings } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/validation/BUILD.bazel b/pkg/webhook/handlers/testdata/apis/testgroup/validation/BUILD.bazel index c0f4b37fa..e63b67131 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/validation/BUILD.bazel +++ b/pkg/webhook/handlers/testdata/apis/testgroup/validation/BUILD.bazel @@ -23,6 +23,7 @@ go_test( srcs = ["validation_test.go"], embed = [":go_default_library"], deps = [ + "//pkg/internal/api/validation:go_default_library", "//pkg/webhook/handlers/testdata/apis/testgroup:go_default_library", "//pkg/webhook/handlers/testdata/apis/testgroup/v1:go_default_library", "@io_k8s_apimachinery//pkg/util/validation/field:go_default_library", diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go b/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go index 2ae47036b..0f5de9508 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go +++ b/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go @@ -21,29 +21,32 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" "github.com/jetstack/cert-manager/pkg/webhook/handlers/testdata/apis/testgroup" v1 "github.com/jetstack/cert-manager/pkg/webhook/handlers/testdata/apis/testgroup/v1" ) -func ValidateTestType(_ *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList { +func ValidateTestType(_ *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList testType := obj.(*testgroup.TestType) el := field.ErrorList{} if testType.TestField == v1.TestFieldValueNotAllowed { el = append(el, field.Invalid(field.NewPath("testField"), testType.TestField, "invalid value")) } - return el + return el, warnings } -func ValidateTestTypeUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) field.ErrorList { +func ValidateTestTypeUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime.Object) (field.ErrorList, validation.WarningList) { + var warnings validation.WarningList old, ok := oldObj.(*testgroup.TestType) new := newObj.(*testgroup.TestType) // if oldObj is not set, the Update operation is always valid. if !ok || old == nil { - return nil + return nil, warnings } el := field.ErrorList{} if old.TestFieldImmutable != "" && old.TestFieldImmutable != new.TestFieldImmutable { el = append(el, field.Forbidden(field.NewPath("testFieldImmutable"), "field is immutable once set")) } - return el + return el, warnings } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation_test.go b/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation_test.go index a2642728b..cee07d399 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation_test.go +++ b/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation_test.go @@ -22,14 +22,16 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/jetstack/cert-manager/pkg/internal/api/validation" "github.com/jetstack/cert-manager/pkg/webhook/handlers/testdata/apis/testgroup" v1 "github.com/jetstack/cert-manager/pkg/webhook/handlers/testdata/apis/testgroup/v1" ) func TestValidateTestType(t *testing.T) { scenarios := map[string]struct { - obj *testgroup.TestType - errs []*field.Error + obj *testgroup.TestType + errs []*field.Error + warnings validation.WarningList }{ "does not allow testField to be TestFieldValueNotAllowed": { obj: &testgroup.TestType{ @@ -42,17 +44,20 @@ func TestValidateTestType(t *testing.T) { } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - errs := ValidateTestType(nil, s.obj) + errs, warnings := ValidateTestType(nil, s.obj) if len(errs) != len(s.errs) { - t.Errorf("Expected %v but got %v", s.errs, errs) + t.Errorf("Expected errors %v but got %v", s.errs, errs) return } for i, e := range errs { expectedErr := s.errs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected error %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, s.warnings) { + t.Errorf("Expected warnings %+#v but got %+#v", s.warnings, warnings) + } }) } } @@ -65,6 +70,7 @@ func TestValidateTestTypeUpdate(t *testing.T) { scenarios := map[string]struct { old, new *testgroup.TestType errs []*field.Error + warnings validation.WarningList }{ "allows all updates if old is nil": { new: &testgroup.TestType{ @@ -74,17 +80,20 @@ func TestValidateTestTypeUpdate(t *testing.T) { } for n, s := range scenarios { t.Run(n, func(t *testing.T) { - errs := ValidateTestTypeUpdate(nil, s.old, s.new) + errs, warnings := ValidateTestTypeUpdate(nil, s.old, s.new) if len(errs) != len(s.errs) { - t.Errorf("Expected %v but got %v", s.errs, errs) + t.Errorf("Expected errors %v but got %v", s.errs, errs) return } for i, e := range errs { expectedErr := s.errs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected error %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, s.warnings) { + t.Errorf("Expected warnings %+#v but got %+#v", s.warnings, warnings) + } }) } } @@ -102,6 +111,7 @@ const ( // is not set. func testImmutableTestTypeField(t *testing.T, fldPath *field.Path, setter func(*testgroup.TestType, testValue)) { t.Run("should reject updates to "+fldPath.String(), func(t *testing.T) { + var expectedWarnings validation.WarningList expectedErrs := []*field.Error{ field.Forbidden(fldPath, "field is immutable once set"), } @@ -109,34 +119,41 @@ func testImmutableTestTypeField(t *testing.T, fldPath *field.Path, setter func(* new := &testgroup.TestType{} setter(old, testValueOptionOne) setter(new, testValueOptionTwo) - errs := ValidateTestTypeUpdate(nil, old, new) + errs, warnings := ValidateTestTypeUpdate(nil, old, new) if len(errs) != len(expectedErrs) { - t.Errorf("Expected %v but got %v", expectedErrs, errs) + t.Errorf("Expected errors %v but got %v", expectedErrs, errs) return } for i, e := range errs { expectedErr := expectedErrs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected error %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, expectedWarnings) { + t.Errorf("Expected warnings %+#v got %+#v", expectedWarnings, warnings) + } }) t.Run("should allow updates to "+fldPath.String()+" if not already set", func(t *testing.T) { + var expectedWarnings validation.WarningList expectedErrs := []*field.Error{} old := &testgroup.TestType{} new := &testgroup.TestType{} setter(old, testValueNone) setter(new, testValueOptionOne) - errs := ValidateTestTypeUpdate(nil, old, new) + errs, warnings := ValidateTestTypeUpdate(nil, old, new) if len(errs) != len(expectedErrs) { - t.Errorf("Expected %v but got %v", expectedErrs, errs) + t.Errorf("Expected errors %v but got %v", expectedErrs, errs) return } for i, e := range errs { expectedErr := expectedErrs[i] if !reflect.DeepEqual(e, expectedErr) { - t.Errorf("Expected %v but got %v", expectedErr, e) + t.Errorf("Expected error %v but got %v", expectedErr, e) } } + if !reflect.DeepEqual(warnings, expectedWarnings) { + t.Errorf("Expected warnings %+#v but got %+#v", expectedWarnings, warnings) + } }) } diff --git a/pkg/webhook/handlers/validation.go b/pkg/webhook/handlers/validation.go index 562e48b6b..b7db0f663 100644 --- a/pkg/webhook/handlers/validation.go +++ b/pkg/webhook/handlers/validation.go @@ -101,15 +101,19 @@ func (r *registryBackedValidator) Validate(ctx context.Context, admissionSpec *a } } errs := field.ErrorList{} + var warnings validation.WarningList if admissionSpec.Operation == admissionv1.Create { // perform validation on new version of resource - errs = append(errs, r.registry.Validate(admissionSpec, obj, gvk)...) + e, w := r.registry.Validate(admissionSpec, obj, gvk) + errs, warnings = append(errs, e...), append(warnings, w...) } else if admissionSpec.Operation == admissionv1.Update { // perform update validation on resource - errs = append(errs, r.registry.ValidateUpdate(admissionSpec, oldObj, obj, gvk)...) + e, w := r.registry.ValidateUpdate(admissionSpec, oldObj, obj, gvk) + errs, warnings = append(errs, e...), append(warnings, w...) } + // TODO: implement warnings for Plugin interface // If no validation errors occurred, perform plugin checks. if len(errs) == 0 { for _, plugin := range r.plugins { @@ -119,6 +123,8 @@ func (r *registryBackedValidator) Validate(ctx context.Context, admissionSpec *a } } + status.Warnings = warnings + // return with allowed = false if any errors occurred if err := errs.ToAggregate(); err != nil { status.Allowed = false