From f46aad2b743eeccfe29ea0d7bdf7a06c2fd5fc4b Mon Sep 17 00:00:00 2001 From: irbekrm Date: Thu, 29 Apr 2021 17:47:27 +0100 Subject: [PATCH] Implements suggestions from code review Signed-off-by: irbekrm --- pkg/api/util/warnings.go | 1 + pkg/internal/apis/acme/validation/order.go | 3 +-- .../identity/certificaterequests/certificaterequests.go | 6 ++---- pkg/internal/apis/certmanager/validation/certificate.go | 6 ++---- .../apis/certmanager/validation/certificaterequest.go | 6 ++---- .../handlers/testdata/apis/testgroup/v2/validation.go | 3 +-- .../testdata/apis/testgroup/validation/validation.go | 8 +++----- 7 files changed, 12 insertions(+), 21 deletions(-) diff --git a/pkg/api/util/warnings.go b/pkg/api/util/warnings.go index a0f5d39ff..1e0076b04 100644 --- a/pkg/api/util/warnings.go +++ b/pkg/api/util/warnings.go @@ -19,5 +19,6 @@ package util // Warning values thrown by validating webhook // https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/ const ( + // DeprecatedACMEEABKeyAlgorithmField is raised when the deprecated keyAlgorithm field for an ACME issuer's external account binding (EAB) is set. DeprecatedACMEEABKeyAlgorithmField = "ACME issuer spec field 'externalAccount.keyAlgorithm' is deprecated. The value of this field will be ignored." ) diff --git a/pkg/internal/apis/acme/validation/order.go b/pkg/internal/apis/acme/validation/order.go index 8461b322b..5f8cf13e5 100644 --- a/pkg/internal/apis/acme/validation/order.go +++ b/pkg/internal/apis/acme/validation/order.go @@ -36,10 +36,9 @@ func ValidateOrderUpdate(_ *admissionv1.AdmissionRequest, oldObj, newObj runtime } 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, warnings + return el, nil } func ValidateOrderSpecUpdate(old, new cmacme.OrderSpec, fldPath *field.Path) field.ErrorList { diff --git a/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go b/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go index d59812fff..8512d7569 100644 --- a/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go +++ b/pkg/internal/apis/certmanager/identity/certificaterequests/certificaterequests.go @@ -32,7 +32,6 @@ import ( ) func ValidateCreate(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, validation.WarningList) { - var warnings validation.WarningList cr := obj.(*cmapi.CertificateRequest) fldPath := field.NewPath("spec") @@ -50,7 +49,7 @@ func ValidateCreate(req *admissionv1.AdmissionRequest, obj runtime.Object) (fiel el = append(el, field.Forbidden(fldPath.Child("extra"), "extra identity must be that of the requester")) } - return el, warnings + return el, nil } func extrasMatch(crExtra map[string][]string, reqExtra map[string]authenticationv1.ExtraValue) bool { @@ -73,7 +72,6 @@ func extrasMatch(crExtra map[string][]string, reqExtra map[string]authentication } 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") @@ -91,7 +89,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, warnings + return el, nil } func MutateCreate(req *admissionv1.AdmissionRequest, obj runtime.Object) { diff --git a/pkg/internal/apis/certmanager/validation/certificate.go b/pkg/internal/apis/certmanager/validation/certificate.go index 482db4845..93f56c8e5 100644 --- a/pkg/internal/apis/certmanager/validation/certificate.go +++ b/pkg/internal/apis/certmanager/validation/certificate.go @@ -88,17 +88,15 @@ func ValidateCertificateSpec(crt *internalcmapi.CertificateSpec, fldPath *field. } 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, warnings + return allErrs, nil } 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, warnings + return allErrs, nil } func validateIssuerRef(issuerRef cmmeta.ObjectReference, fldPath *field.Path) field.ErrorList { diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest.go b/pkg/internal/apis/certmanager/validation/certificaterequest.go index 5a8a31d2a..c8dca75a1 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest.go @@ -40,17 +40,15 @@ import ( var defaultInternalKeyUsages = []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment} 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, warnings + return allErrs, nil } 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 @@ -69,7 +67,7 @@ func ValidateUpdateCertificateRequest(_ *admissionv1.AdmissionRequest, oldObj, n el = append(el, field.Forbidden(field.NewPath("spec"), "cannot change spec after creation")) } - return el, warnings + return el, nil } func validateCertificateRequestAnnotations(objA, objB *cmapi.CertificateRequest, fieldPath *field.Path) field.ErrorList { diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go b/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go index 54df85848..cafc017e1 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go +++ b/pkg/webhook/handlers/testdata/apis/testgroup/v2/validation.go @@ -26,10 +26,9 @@ import ( 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, warnings + return el, nil } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go b/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go index 0f5de9508..60819548a 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go +++ b/pkg/webhook/handlers/testdata/apis/testgroup/validation/validation.go @@ -27,26 +27,24 @@ import ( ) 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, warnings + return el, nil } 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, warnings + return nil, nil } 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, warnings + return el, nil }