From 8b219a45b2d97013f4c4d7ff87f6ac6c67f78508 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 1 Feb 2022 17:03:37 +0000 Subject: [PATCH] Fix AdditationOutputFormat validation, and adds unit tests. Use correct feature set Signed-off-by: joshvanl --- .../apis/certmanager/validation/BUILD.bazel | 6 +- .../certmanager/validation/certificate.go | 41 ++++--- .../validation/certificate_test.go | 114 ++++++++++++++++++ 3 files changed, 146 insertions(+), 15 deletions(-) diff --git a/internal/apis/certmanager/validation/BUILD.bazel b/internal/apis/certmanager/validation/BUILD.bazel index df5458396..a06472c56 100644 --- a/internal/apis/certmanager/validation/BUILD.bazel +++ b/internal/apis/certmanager/validation/BUILD.bazel @@ -17,7 +17,7 @@ go_library( "//internal/apis/certmanager:go_default_library", "//internal/apis/certmanager/validation/util:go_default_library", "//internal/apis/meta:go_default_library", - "//internal/controller/feature:go_default_library", + "//internal/webhook/feature:go_default_library", "//pkg/api/util:go_default_library", "//pkg/apis/acme:go_default_library", "//pkg/apis/certmanager:go_default_library", @@ -31,6 +31,7 @@ go_library( "@io_k8s_apimachinery//pkg/api/validation:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1/validation:go_default_library", "@io_k8s_apimachinery//pkg/runtime:go_default_library", + "@io_k8s_apimachinery//pkg/util/sets:go_default_library", "@io_k8s_apimachinery//pkg/util/validation/field:go_default_library", ], ) @@ -49,7 +50,9 @@ go_test( "//internal/apis/acme:go_default_library", "//internal/apis/certmanager:go_default_library", "//internal/apis/meta:go_default_library", + "//internal/webhook/feature:go_default_library", "//pkg/apis/certmanager/v1:go_default_library", + "//pkg/util/feature:go_default_library", "//pkg/util/pki:go_default_library", "//test/unit/gen:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", @@ -57,6 +60,7 @@ go_test( "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_apimachinery//pkg/util/validation/field:go_default_library", + "@io_k8s_component_base//featuregate/testing:go_default_library", ], ) diff --git a/internal/apis/certmanager/validation/certificate.go b/internal/apis/certmanager/validation/certificate.go index 5956af468..44ead2b84 100644 --- a/internal/apis/certmanager/validation/certificate.go +++ b/internal/apis/certmanager/validation/certificate.go @@ -26,11 +26,12 @@ import ( apivalidation "k8s.io/apimachinery/pkg/api/validation" metavalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" internalcmapi "github.com/jetstack/cert-manager/internal/apis/certmanager" cmmeta "github.com/jetstack/cert-manager/internal/apis/meta" - "github.com/jetstack/cert-manager/internal/controller/feature" + "github.com/jetstack/cert-manager/internal/webhook/feature" "github.com/jetstack/cert-manager/pkg/api/util" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" utilfeature "github.com/jetstack/cert-manager/pkg/util/feature" @@ -78,19 +79,6 @@ func ValidateCertificateSpec(crt *internalcmapi.CertificateSpec, fldPath *field. default: el = append(el, field.Invalid(fldPath.Child("privateKey", "algorithm"), crt.PrivateKey.Algorithm, "must be either empty or one of rsa or ecdsa")) } - if crt.AdditionalOutputFormats != nil { - if !utilfeature.DefaultFeatureGate.Enabled(feature.AdditionalCertificateOutputFormats) { - el = append(el, field.Forbidden(fldPath.Child("AdditionalOutputFormat"), "Feature gate AdditionalCertificateOutputFormats must be enabled")) - } - check := make(map[string]bool) - for _, val := range crt.AdditionalOutputFormats { - if _, exists := check[string(val.Type)]; !exists { - check[string(val.Type)] = true - } else { - el = append(el, field.Invalid(fldPath.Child("AdditionalOutputFormats"), crt.AdditionalOutputFormats, "Duplicate Type in additionalOutputFormats")) - } - } - } } if crt.Duration != nil || crt.RenewBefore != nil { @@ -112,6 +100,8 @@ func ValidateCertificateSpec(crt *internalcmapi.CertificateSpec, fldPath *field. } } + el = append(el, validateAdditionalOutputFormats(crt, fldPath)...) + return el } @@ -224,3 +214,26 @@ func ValidateDuration(crt *internalcmapi.CertificateSpec, fldPath *field.Path) f } return el } + +func validateAdditionalOutputFormats(crt *internalcmapi.CertificateSpec, fldPath *field.Path) field.ErrorList { + var el field.ErrorList + + if !utilfeature.DefaultFeatureGate.Enabled(feature.AdditionalCertificateOutputFormats) { + if len(crt.AdditionalOutputFormats) > 0 { + el = append(el, field.Forbidden(fldPath.Child("additionalOutputFormats"), "feature gate AdditionalCertificateOutputFormats must be enabled")) + } + return el + } + + // Ensure the set of output formats is unique, keyed on "Type". + aofSet := sets.NewString() + for _, val := range crt.AdditionalOutputFormats { + if aofSet.Has(string(val.Type)) { + el = append(el, field.Duplicate(fldPath.Child("additionalOutputFormats").Key("type"), string(val.Type))) + continue + } + aofSet.Insert(string(val.Type)) + } + + return el +} diff --git a/internal/apis/certmanager/validation/certificate_test.go b/internal/apis/certmanager/validation/certificate_test.go index 2d83e53fd..39d699b65 100644 --- a/internal/apis/certmanager/validation/certificate_test.go +++ b/internal/apis/certmanager/validation/certificate_test.go @@ -25,10 +25,13 @@ import ( admissionv1 "k8s.io/api/admission/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" + featuregatetesting "k8s.io/component-base/featuregate/testing" internalcmapi "github.com/jetstack/cert-manager/internal/apis/certmanager" cmmeta "github.com/jetstack/cert-manager/internal/apis/meta" + "github.com/jetstack/cert-manager/internal/webhook/feature" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + utilfeature "github.com/jetstack/cert-manager/pkg/util/feature" "github.com/stretchr/testify/assert" ) @@ -784,3 +787,114 @@ func TestValidateDuration(t *testing.T) { }) } } + +func Test_validateAdditionalOutputFormats(t *testing.T) { + tests := map[string]struct { + featureEnabled bool + spec *internalcmapi.CertificateSpec + expErr field.ErrorList + }{ + "if feature disabled and no formats defined, expect no error": { + featureEnabled: false, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{}, + }, + expErr: nil, + }, + "if feature disabled and 1 format defined, expect error": { + featureEnabled: false, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{ + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + }, + }, + expErr: field.ErrorList{ + field.Forbidden(field.NewPath("spec", "additionalOutputFormats"), "feature gate AdditionalCertificateOutputFormats must be enabled"), + }, + }, + "if feature disabled and multiple formats defined, expect error": { + featureEnabled: false, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{ + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + {Type: internalcmapi.CertificateOutputFormatType("bar")}, + {Type: internalcmapi.CertificateOutputFormatType("random")}, + }, + }, + expErr: field.ErrorList{ + field.Forbidden(field.NewPath("spec", "additionalOutputFormats"), "feature gate AdditionalCertificateOutputFormats must be enabled"), + }, + }, + "if feature enabled and no formats defined, expect no error": { + featureEnabled: true, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{}, + }, + expErr: nil, + }, + "if feature enabled and single format defined, expect no error": { + featureEnabled: true, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{ + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + }, + }, + expErr: nil, + }, + "if feature enabled and multiple unique formats defined, expect no error": { + featureEnabled: true, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{ + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + {Type: internalcmapi.CertificateOutputFormatType("bar")}, + {Type: internalcmapi.CertificateOutputFormatType("random")}, + }, + }, + expErr: nil, + }, + "if feature enabled and multiple formats defined but 2 non-unique, expect error": { + featureEnabled: true, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{ + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + {Type: internalcmapi.CertificateOutputFormatType("bar")}, + {Type: internalcmapi.CertificateOutputFormatType("random")}, + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + }, + }, + expErr: field.ErrorList{ + field.Duplicate(field.NewPath("spec", "additionalOutputFormats").Key("type"), "foo"), + }, + }, + "if feature enabled and multiple formats defined but multiple non-unique, expect error": { + featureEnabled: true, + spec: &internalcmapi.CertificateSpec{ + AdditionalOutputFormats: []internalcmapi.CertificateAdditionalOutputFormat{ + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + {Type: internalcmapi.CertificateOutputFormatType("bar")}, + {Type: internalcmapi.CertificateOutputFormatType("random")}, + {Type: internalcmapi.CertificateOutputFormatType("random")}, + {Type: internalcmapi.CertificateOutputFormatType("foo")}, + {Type: internalcmapi.CertificateOutputFormatType("bar")}, + {Type: internalcmapi.CertificateOutputFormatType("bar")}, + {Type: internalcmapi.CertificateOutputFormatType("123")}, + {Type: internalcmapi.CertificateOutputFormatType("456")}, + }, + }, + expErr: field.ErrorList{ + field.Duplicate(field.NewPath("spec", "additionalOutputFormats").Key("type"), "random"), + field.Duplicate(field.NewPath("spec", "additionalOutputFormats").Key("type"), "foo"), + field.Duplicate(field.NewPath("spec", "additionalOutputFormats").Key("type"), "bar"), + field.Duplicate(field.NewPath("spec", "additionalOutputFormats").Key("type"), "bar"), + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, feature.AdditionalCertificateOutputFormats, test.featureEnabled)() + gotErr := validateAdditionalOutputFormats(test.spec, field.NewPath("spec")) + assert.Equal(t, test.expErr, gotErr) + }) + } +}