From 3741f383e1fbd43fd61c860b06a1379d39f9b978 Mon Sep 17 00:00:00 2001 From: Erik Godding Boye Date: Sat, 16 Nov 2024 10:56:48 +0100 Subject: [PATCH] Make more use of api-server validation Signed-off-by: Erik Godding Boye --- .../crd-trust.cert-manager.io_bundles.yaml | 29 +++ docs/api/api.md | 34 ++-- pkg/apis/trust/v1alpha1/types_bundle.go | 10 + pkg/webhook/validation.go | 92 --------- pkg/webhook/validation_test.go | 175 ---------------- test/integration/bundle/integration.go | 8 +- test/integration/bundle/validation_test.go | 191 ++++++++++++++++++ 7 files changed, 256 insertions(+), 283 deletions(-) create mode 100644 test/integration/bundle/validation_test.go diff --git a/deploy/charts/trust-manager/templates/crd-trust.cert-manager.io_bundles.yaml b/deploy/charts/trust-manager/templates/crd-trust.cert-manager.io_bundles.yaml index 5211aab6..d1a0ec97 100644 --- a/deploy/charts/trust-manager/templates/crd-trust.cert-manager.io_bundles.yaml +++ b/deploy/charts/trust-manager/templates/crd-trust.cert-manager.io_bundles.yaml @@ -82,11 +82,13 @@ spec: type: boolean key: description: Key of the entry in the object's `data` field to be used. + minLength: 1 type: string name: description: |- Name is the name of the source object in the trust Namespace. This field must be left empty when `selector` is set + minLength: 1 type: string selector: description: |- @@ -135,6 +137,11 @@ spec: type: object x-kubernetes-map-type: atomic type: object + x-kubernetes-validations: + - message: must specify one and only one of {name, selector} + rule: '[has(self.name), has(self.selector)].exists_one(x,x)' + - message: must specify key or includeAllKeys + rule: '[has(self.key), has(self.includeAllKeys) && self.includeAllKeys].exists_one(x,x)' inLine: description: InLine is a simple string to append as the source data. type: string @@ -150,11 +157,13 @@ spec: type: boolean key: description: Key of the entry in the object's `data` field to be used. + minLength: 1 type: string name: description: |- Name is the name of the source object in the trust Namespace. This field must be left empty when `selector` is set + minLength: 1 type: string selector: description: |- @@ -203,6 +212,11 @@ spec: type: object x-kubernetes-map-type: atomic type: object + x-kubernetes-validations: + - message: must specify one and only one of {name, selector} + rule: '[has(self.name), has(self.selector)].exists_one(x,x)' + - message: must specify key or includeAllKeys + rule: '[has(self.key), has(self.includeAllKeys) && self.includeAllKeys].exists_one(x,x)' useDefaultCAs: description: |- UseDefaultCAs, when true, requests the default CA bundle to be used as a source. @@ -215,7 +229,15 @@ spec: defaultCAPackageVersion field of the Bundle's status field. type: boolean type: object + x-kubernetes-validations: + - message: must define exactly one source + rule: '[has(self.configMap), has(self.secret), has(self.inLine), has(self.useDefaultCAs) && self.useDefaultCAs].exists_one(x,x)' + maxItems: 100 + minItems: 1 type: array + x-kubernetes-validations: + - message: must request default CAs at most once + rule: size(self.filter(s, has(s.useDefaultCAs) && s.useDefaultCAs)) <= 1 target: description: Target is the target location in all namespaces to sync source data to. properties: @@ -230,6 +252,7 @@ spec: properties: key: description: Key is the key of the entry in the object's `data` field to be used. + minLength: 1 type: string password: default: changeit @@ -247,6 +270,7 @@ spec: properties: key: description: Key is the key of the entry in the object's `data` field to be used. + minLength: 1 type: string password: default: "" @@ -264,6 +288,7 @@ spec: properties: key: description: Key is the key of the entry in the object's `data` field to be used. + minLength: 1 type: string required: - key @@ -289,11 +314,15 @@ spec: properties: key: description: Key is the key of the entry in the object's `data` field to be used. + minLength: 1 type: string required: - key type: object type: object + x-kubernetes-validations: + - message: must define at least one target configMap/secret + rule: '[has(self.configMap), has(self.secret)].exists(x,x)' required: - sources - target diff --git a/docs/api/api.md b/docs/api/api.md index 5b913c14..d7cfbcfd 100644 --- a/docs/api/api.md +++ b/docs/api/api.md @@ -125,7 +125,7 @@ func Resource(resource string) schema.GroupResource Resource takes an unqualified resource and returns a Group qualified GroupResource -## type [AdditionalFormats]() +## type [AdditionalFormats]() AdditionalFormats specifies any additional formats to write to the target @@ -206,7 +206,7 @@ func (in *Bundle) DeepCopyObject() runtime.Object DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. -## type [BundleCondition]() +## type [BundleCondition]() BundleCondition contains condition information for a Bundle. @@ -313,9 +313,9 @@ func (in *BundleList) DeepCopyObject() runtime.Object DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. -## type [BundleSource]() +## type [BundleSource]() -BundleSource is the set of sources whose data will be appended and synced to the BundleTarget in all Namespaces. +BundleSource is the set of sources whose data will be appended and synced to the BundleTarget in all Namespaces. \+kubebuilder:validation:XValidation:rule="\[has\(self.configMap\), has\(self.secret\), has\(self.inLine\), has\(self.useDefaultCAs\) && self.useDefaultCAs\].exists\_one\(x,x\)", message="must define exactly one source" ```go type BundleSource struct { @@ -365,13 +365,16 @@ func (in *BundleSource) DeepCopyInto(out *BundleSource) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [BundleSpec]() +## type [BundleSpec]() BundleSpec defines the desired state of a Bundle. ```go type BundleSpec struct { // Sources is a set of references to data whose data will sync to the target. + // +kubebuilder:validation:MinItems=1 + // +kubebuilder:validation:MaxItems=100 + // +kubebuilder:validation:XValidation:rule="size(self.filter(s, has(s.useDefaultCAs) && s.useDefaultCAs)) <= 1", message="must request default CAs at most once" Sources []BundleSource `json:"sources"` // Target is the target location in all namespaces to sync source data to. @@ -398,7 +401,7 @@ func (in *BundleSpec) DeepCopyInto(out *BundleSpec) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [BundleStatus]() +## type [BundleStatus]() BundleStatus defines the observed state of the Bundle. @@ -439,9 +442,9 @@ func (in *BundleStatus) DeepCopyInto(out *BundleStatus) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [BundleTarget]() +## type [BundleTarget]() -BundleTarget is the target resource that the Bundle will sync all source data to. +BundleTarget is the target resource that the Bundle will sync all source data to. \+kubebuilder:validation:XValidation:rule="\[has\(self.configMap\), has\(self.secret\)\].exists\(x,x\)", message="must define at least one target configMap/secret" ```go type BundleTarget struct { @@ -484,7 +487,7 @@ func (in *BundleTarget) DeepCopyInto(out *BundleTarget) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [JKS]() +## type [JKS]() @@ -520,13 +523,14 @@ func (in *JKS) DeepCopyInto(out *JKS) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [KeySelector]() +## type [KeySelector]() KeySelector is a reference to a key for some map data object. ```go type KeySelector struct { // Key is the key of the entry in the object's `data` field to be used. + // +kubebuilder:validation:MinLength=1 Key string `json:"key"` } ``` @@ -550,7 +554,7 @@ func (in *KeySelector) DeepCopyInto(out *KeySelector) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [NamespaceSelector]() +## type [NamespaceSelector]() NamespaceSelector defines selectors to match on Namespaces. @@ -582,7 +586,7 @@ func (in *NamespaceSelector) DeepCopyInto(out *NamespaceSelector) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [PKCS12]() +## type [PKCS12]() @@ -617,15 +621,16 @@ func (in *PKCS12) DeepCopyInto(out *PKCS12) DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non\-nil. -## type [SourceObjectKeySelector]() +## type [SourceObjectKeySelector]() -SourceObjectKeySelector is a reference to a source object and its \`data\` key\(s\) in the trust Namespace. +SourceObjectKeySelector is a reference to a source object and its \`data\` key\(s\) in the trust Namespace. \+kubebuilder:validation:XValidation:rule="\[has\(self.name\), has\(self.selector\)\].exists\_one\(x,x\)", message="must specify one and only one of \{name, selector\}" \+kubebuilder:validation:XValidation:rule="\[has\(self.key\), has\(self.includeAllKeys\) && self.includeAllKeys\].exists\_one\(x,x\)", message="must specify key or includeAllKeys" ```go type SourceObjectKeySelector struct { // Name is the name of the source object in the trust Namespace. // This field must be left empty when `selector` is set //+optional + // +kubebuilder:validation:MinLength=1 Name string `json:"name,omitempty"` // Selector is the label selector to use to fetch a list of objects. Must not be set @@ -635,6 +640,7 @@ type SourceObjectKeySelector struct { // Key of the entry in the object's `data` field to be used. //+optional + // +kubebuilder:validation:MinLength=1 Key string `json:"key,omitempty"` // IncludeAllKeys is a flag to include all keys in the object's `data` field to be used. False by default. diff --git a/pkg/apis/trust/v1alpha1/types_bundle.go b/pkg/apis/trust/v1alpha1/types_bundle.go index 014839ad..71be3d71 100644 --- a/pkg/apis/trust/v1alpha1/types_bundle.go +++ b/pkg/apis/trust/v1alpha1/types_bundle.go @@ -59,6 +59,9 @@ type BundleList struct { // BundleSpec defines the desired state of a Bundle. type BundleSpec struct { // Sources is a set of references to data whose data will sync to the target. + // +kubebuilder:validation:MinItems=1 + // +kubebuilder:validation:MaxItems=100 + // +kubebuilder:validation:XValidation:rule="size(self.filter(s, has(s.useDefaultCAs) && s.useDefaultCAs)) <= 1", message="must request default CAs at most once" Sources []BundleSource `json:"sources"` // Target is the target location in all namespaces to sync source data to. @@ -67,6 +70,7 @@ type BundleSpec struct { // BundleSource is the set of sources whose data will be appended and synced to // the BundleTarget in all Namespaces. +// +kubebuilder:validation:XValidation:rule="[has(self.configMap), has(self.secret), has(self.inLine), has(self.useDefaultCAs) && self.useDefaultCAs].exists_one(x,x)", message="must define exactly one source" type BundleSource struct { // ConfigMap is a reference (by name) to a ConfigMap's `data` key(s), or to a // list of ConfigMap's `data` key(s) using label selector, in the trust Namespace. @@ -96,6 +100,7 @@ type BundleSource struct { // BundleTarget is the target resource that the Bundle will sync all source // data to. +// +kubebuilder:validation:XValidation:rule="[has(self.configMap), has(self.secret)].exists(x,x)", message="must define at least one target configMap/secret" type BundleTarget struct { // ConfigMap is the target ConfigMap in Namespaces that all Bundle source // data will be synced to. @@ -158,10 +163,13 @@ type NamespaceSelector struct { // SourceObjectKeySelector is a reference to a source object and its `data` key(s) // in the trust Namespace. +// +kubebuilder:validation:XValidation:rule="[has(self.name), has(self.selector)].exists_one(x,x)", message="must specify one and only one of {name, selector}" +// +kubebuilder:validation:XValidation:rule="[has(self.key), has(self.includeAllKeys) && self.includeAllKeys].exists_one(x,x)", message="must specify key or includeAllKeys" type SourceObjectKeySelector struct { // Name is the name of the source object in the trust Namespace. // This field must be left empty when `selector` is set //+optional + // +kubebuilder:validation:MinLength=1 Name string `json:"name,omitempty"` // Selector is the label selector to use to fetch a list of objects. Must not be set @@ -171,6 +179,7 @@ type SourceObjectKeySelector struct { // Key of the entry in the object's `data` field to be used. //+optional + // +kubebuilder:validation:MinLength=1 Key string `json:"key,omitempty"` // IncludeAllKeys is a flag to include all keys in the object's `data` field to be used. False by default. @@ -182,6 +191,7 @@ type SourceObjectKeySelector struct { // KeySelector is a reference to a key for some map data object. type KeySelector struct { // Key is the key of the entry in the object's `data` field to be used. + // +kubebuilder:validation:MinLength=1 Key string `json:"key"` } diff --git a/pkg/webhook/validation.go b/pkg/webhook/validation.go index c5f21059..fd36b2b4 100644 --- a/pkg/webhook/validation.go +++ b/pkg/webhook/validation.go @@ -19,8 +19,6 @@ package webhook import ( "context" "fmt" - "strconv" - "github.com/go-logr/logr" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -86,84 +84,6 @@ func (v *validator) validate(obj runtime.Object) (admission.Warnings, error) { path = field.NewPath("spec") ) - sourceCount := 0 - defaultCAsCount := 0 - - for i, source := range bundle.Spec.Sources { - path := path.Child("sources").Child("[" + strconv.Itoa(i) + "]") - - unionCount := 0 - - if configMap := source.ConfigMap; configMap != nil { - path := path.Child("configMap") - sourceCount++ - unionCount++ - - if len(configMap.Name) == 0 && configMap.Selector == nil { - el = append(el, field.Invalid(path, "name: ' ', selector: nil", "must validate one and only one schema (oneOf): [name, selector]. Found none valid")) - } - if len(configMap.Name) > 0 && configMap.Selector != nil { - el = append(el, field.Invalid(path, fmt.Sprintf("name: %s, selector: {}", configMap.Name), "must validate one and only one schema (oneOf): [name, selector]. Found both set")) - } - if len(configMap.Key) == 0 && !configMap.IncludeAllKeys { - el = append(el, field.Invalid(path, fmt.Sprintf("key: ' ', includeAllKeys: %t", configMap.IncludeAllKeys), "source configMap key must be defined when includeAllKeys is false")) - } - if len(configMap.Key) > 0 && configMap.IncludeAllKeys { - el = append(el, field.Invalid(path, fmt.Sprintf("key: %s, includeAllKeys: %t", configMap.Key, configMap.IncludeAllKeys), "source configMap key cannot be defined when includeAllKeys is true")) - } - } - - if secret := source.Secret; secret != nil { - path := path.Child("secret") - sourceCount++ - unionCount++ - - if len(secret.Name) == 0 && secret.Selector == nil { - el = append(el, field.Invalid(path, "name: ' ', selector: nil", "must validate one and only one schema (oneOf): [name, selector]. Found none valid")) - } - if len(secret.Name) > 0 && secret.Selector != nil { - el = append(el, field.Invalid(path, fmt.Sprintf("name: %s, selector: {}", secret.Name), "must validate one and only one schema (oneOf): [name, selector]. Found both set")) - } - if len(secret.Key) == 0 && !secret.IncludeAllKeys { - el = append(el, field.Invalid(path, fmt.Sprintf("key: ' ', includeAllKeys: %t", secret.IncludeAllKeys), "source secret key must be defined when includeAllKeys is false")) - } - if len(secret.Key) > 0 && secret.IncludeAllKeys { - el = append(el, field.Invalid(path, fmt.Sprintf("key: %s, includeAllKeys: %t", secret.Key, secret.IncludeAllKeys), "source secret key cannot be defined when includeAllKeys is true")) - } - } - - if source.InLine != nil { - sourceCount++ - unionCount++ - } - - if source.UseDefaultCAs != nil { - defaultCAsCount++ - unionCount++ - - if *source.UseDefaultCAs { - sourceCount++ - } - } - - if unionCount != 1 { - el = append(el, field.Forbidden( - path, fmt.Sprintf("must define exactly one source type for each item but found %d defined types", unionCount), - )) - } - } - - if sourceCount == 0 { - el = append(el, field.Forbidden(path.Child("sources"), "must define at least one source")) - } - - if defaultCAsCount > 1 { - el = append(el, field.Forbidden( - path.Child("sources"), - fmt.Sprintf("must request default CAs either once or not at all but got %d requests", defaultCAsCount), - )) - } - if target := bundle.Spec.Target.ConfigMap; target != nil { path := path.Child("sources") for i, source := range bundle.Spec.Sources { @@ -185,18 +105,6 @@ func (v *validator) validate(obj runtime.Object) (admission.Warnings, error) { configMap := bundle.Spec.Target.ConfigMap secret := bundle.Spec.Target.Secret - if configMap == nil && secret == nil { - el = append(el, field.Invalid(path.Child("target"), bundle.Spec.Target, "must define at least one target")) - } - - if configMap != nil && len(configMap.Key) == 0 { - el = append(el, field.Invalid(path.Child("target", "configMap", "key"), configMap.Key, "target configMap key must be defined")) - } - - if secret != nil && len(secret.Key) == 0 { - el = append(el, field.Invalid(path.Child("target", "secret", "key"), secret.Key, "target secret key must be defined")) - } - if bundle.Spec.Target.AdditionalFormats != nil { var formats = make(map[string]*trustapi.KeySelector) targetKeys := map[string]struct{}{} diff --git a/pkg/webhook/validation_test.go b/pkg/webhook/validation_test.go index 68fe2cb7..1cb47eab 100644 --- a/pkg/webhook/validation_test.go +++ b/pkg/webhook/validation_test.go @@ -41,155 +41,6 @@ func Test_validate(t *testing.T) { bundle: &corev1.Pod{}, expErr: ptr.To("expected a Bundle, but got a *v1.Pod"), }, - "no sources, no target": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{}, - }, - expErr: ptr.To(field.ErrorList{ - field.Forbidden(field.NewPath("spec", "sources"), "must define at least one source"), - field.Invalid(field.NewPath("spec", "target"), trustapi.BundleTarget{}, "must define at least one target"), - }.ToAggregate().Error()), - }, - "sources with multiple types defined in items": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - { - ConfigMap: &trustapi.SourceObjectKeySelector{Name: "test", Key: "test"}, - InLine: ptr.To("test"), - }, - {InLine: ptr.To("test")}, - { - ConfigMap: &trustapi.SourceObjectKeySelector{Name: "test", Key: "test"}, - Secret: &trustapi.SourceObjectKeySelector{Name: "test", Key: "test"}, - }, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Forbidden(field.NewPath("spec", "sources", "[0]"), "must define exactly one source type for each item but found 2 defined types"), - field.Forbidden(field.NewPath("spec", "sources", "[2]"), "must define exactly one source type for each item but found 2 defined types"), - }.ToAggregate().Error()), - }, - "empty source with no defined types": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - {}, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Forbidden(field.NewPath("spec", "sources", "[0]"), "must define exactly one source type for each item but found 0 defined types"), - field.Forbidden(field.NewPath("spec", "sources"), "must define at least one source"), - }.ToAggregate().Error()), - }, - "useDefaultCAs false, with no other defined sources": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - { - UseDefaultCAs: ptr.To(false), - }, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Forbidden(field.NewPath("spec", "sources"), "must define at least one source"), - }.ToAggregate().Error()), - }, - "useDefaultCAs requested twice": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - { - UseDefaultCAs: ptr.To(true), - }, - { - UseDefaultCAs: ptr.To(true), - }, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Forbidden(field.NewPath("spec", "sources"), "must request default CAs either once or not at all but got 2 requests"), - }.ToAggregate().Error()), - }, - "useDefaultCAs requested three times": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - { - UseDefaultCAs: ptr.To(true), - }, - { - UseDefaultCAs: ptr.To(false), - }, - { - UseDefaultCAs: ptr.To(true), - }, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Forbidden(field.NewPath("spec", "sources"), "must request default CAs either once or not at all but got 3 requests"), - }.ToAggregate().Error()), - }, - "sources names, selectors and keys are empty": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - {ConfigMap: &trustapi.SourceObjectKeySelector{Name: "", Key: ""}}, - {InLine: ptr.To("test")}, - {Secret: &trustapi.SourceObjectKeySelector{Name: "", Key: ""}}, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Invalid(field.NewPath("spec", "sources", "[0]", "configMap"), "name: ' ', selector: nil", "must validate one and only one schema (oneOf): [name, selector]. Found none valid"), - field.Invalid(field.NewPath("spec", "sources", "[0]", "configMap"), "key: ' ', includeAllKeys: false", "source configMap key must be defined when includeAllKeys is false"), - field.Invalid(field.NewPath("spec", "sources", "[2]", "secret"), "name: ' ', selector: nil", "must validate one and only one schema (oneOf): [name, selector]. Found none valid"), - field.Invalid(field.NewPath("spec", "sources", "[2]", "secret"), "key: ' ', includeAllKeys: false", "source secret key must be defined when includeAllKeys is false"), - }.ToAggregate().Error()), - }, - "sources names and selectors are both set": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - {ConfigMap: &trustapi.SourceObjectKeySelector{Name: "some-config-map", Selector: &metav1.LabelSelector{}, Key: "test"}}, - {InLine: ptr.To("test")}, - {Secret: &trustapi.SourceObjectKeySelector{Name: "some-secret", Selector: &metav1.LabelSelector{}, Key: "test"}}, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Invalid(field.NewPath("spec", "sources", "[0]", "configMap"), "name: some-config-map, selector: {}", "must validate one and only one schema (oneOf): [name, selector]. Found both set"), - field.Invalid(field.NewPath("spec", "sources", "[2]", "secret"), "name: some-secret, selector: {}", "must validate one and only one schema (oneOf): [name, selector]. Found both set"), - }.ToAggregate().Error()), - }, - "sources key is set and includeAllKeys is true": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - {ConfigMap: &trustapi.SourceObjectKeySelector{Name: "some-config-map", Key: "test", IncludeAllKeys: true}}, - {InLine: ptr.To("test")}, - {Secret: &trustapi.SourceObjectKeySelector{Name: "some-secret", Key: "test", IncludeAllKeys: true}}, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test"}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Invalid(field.NewPath("spec", "sources", "[0]", "configMap"), "key: test, includeAllKeys: true", "source configMap key cannot be defined when includeAllKeys is true"), - field.Invalid(field.NewPath("spec", "sources", "[2]", "secret"), "key: test, includeAllKeys: true", "source secret key cannot be defined when includeAllKeys is true"), - }.ToAggregate().Error()), - }, "sources defines the same configMap target": { bundle: &trustapi.Bundle{ ObjectMeta: metav1.ObjectMeta{Name: "test-bundle"}, @@ -220,32 +71,6 @@ func Test_validate(t *testing.T) { field.Forbidden(field.NewPath("spec", "sources", "[1]", "secret", "test-bundle", "test"), "cannot define the same source as target"), }.ToAggregate().Error()), }, - "target configMap key not defined": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - {InLine: ptr.To("test")}, - }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: ""}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Invalid(field.NewPath("spec", "target", "configMap", "key"), "", "target configMap key must be defined"), - }.ToAggregate().Error()), - }, - "target secret key not defined": { - bundle: &trustapi.Bundle{ - Spec: trustapi.BundleSpec{ - Sources: []trustapi.BundleSource{ - {InLine: ptr.To("test")}, - }, - Target: trustapi.BundleTarget{Secret: &trustapi.KeySelector{Key: ""}}, - }, - }, - expErr: ptr.To(field.ErrorList{ - field.Invalid(field.NewPath("spec", "target", "secret", "key"), "", "target secret key must be defined"), - }.ToAggregate().Error()), - }, "invalid namespace selector": { bundle: &trustapi.Bundle{ ObjectMeta: metav1.ObjectMeta{Name: "test-bundle-1"}, diff --git a/test/integration/bundle/integration.go b/test/integration/bundle/integration.go index 665c368d..470a5110 100644 --- a/test/integration/bundle/integration.go +++ b/test/integration/bundle/integration.go @@ -18,6 +18,7 @@ package test import ( "os" + "path/filepath" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -38,14 +39,17 @@ var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) crdsYamlFile := os.Getenv("TRUST_MANAGER_CRDS") - Expect(crdsYamlFile).NotTo(BeEmpty(), "TRUST_MANAGER_CRDS must be set to the path of the CRDs to install") + if len(crdsYamlFile) == 0 { + crdsYamlFile = filepath.Join("..", "..", "..", "_bin", "scratch", "crds") + } env = &envtest.Environment{ UseExistingCluster: ptr.To(false), CRDDirectoryPaths: []string{ crdsYamlFile, }, - Scheme: trustapi.GlobalScheme, + ErrorIfCRDPathMissing: true, + Scheme: trustapi.GlobalScheme, } _, err := env.Start() diff --git a/test/integration/bundle/validation_test.go b/test/integration/bundle/validation_test.go new file mode 100644 index 00000000..9e4db766 --- /dev/null +++ b/test/integration/bundle/validation_test.go @@ -0,0 +1,191 @@ +/* +Copyright 2021 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 test + +import ( + "context" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/klog/v2/ktesting" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Bundle Validation", func() { + var ( + ctx context.Context + cl client.Client + bundle *trustapi.Bundle + ) + BeforeEach(func() { + _, ctx = ktesting.NewTestContext(GinkgoT()) + + var err error + cl, err = client.New(env.Config, client.Options{ + Scheme: trustapi.GlobalScheme, + }) + Expect(err).NotTo(HaveOccurred()) + + bundle = &trustapi.Bundle{} + bundle.GenerateName = "validation-" + bundle.Spec.Sources = []trustapi.BundleSource{{ + UseDefaultCAs: ptr.To(true), + }} + bundle.Spec.Target = trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "ca-bundle.crt"}} + }) + + Context("Sources", func() { + It("should require at least one source", func() { + bundle.Spec.Sources = make([]trustapi.BundleSource, 0) + + expectedErr := "spec.sources: Invalid value: 0: spec.sources in body should have at least 1 items" + Expect(cl.Create(ctx, bundle)).Should(MatchError(ContainSubstring(expectedErr))) + }) + + It("should require at most one useDefaultCAs source", func() { + bundle.Spec.Sources = []trustapi.BundleSource{ + {UseDefaultCAs: ptr.To(true)}, + {UseDefaultCAs: ptr.To(true)}, + } + + expectedErr := "spec.sources: Invalid value: \"array\": must request default CAs at most once" + Expect(cl.Create(ctx, bundle)).Should(MatchError(ContainSubstring(expectedErr))) + }) + }) + + Context("Source item", func() { + DescribeTable("should require exactly one source", + func(source trustapi.BundleSource, wantErr bool) { + bundle.Spec.Sources = []trustapi.BundleSource{source} + if wantErr { + expectedErr := "spec.sources[0]: Invalid value: \"object\": must define exactly one source" + Expect(cl.Create(ctx, bundle)).Should(MatchError(ContainSubstring(expectedErr))) + } else { + Expect(cl.Create(ctx, bundle)).To(Succeed()) + } + }, + Entry("when none set", trustapi.BundleSource{}, true), + Entry("when configMap set", trustapi.BundleSource{ConfigMap: &trustapi.SourceObjectKeySelector{Name: "ca", Key: "ca.crt"}}, false), + Entry("when secret set", trustapi.BundleSource{Secret: &trustapi.SourceObjectKeySelector{Name: "ca", Key: "ca.crt"}}, false), + Entry("when inLine set", trustapi.BundleSource{InLine: ptr.To("")}, false), + Entry("when useDefaultCAs=true set", trustapi.BundleSource{UseDefaultCAs: ptr.To(true)}, false), + Entry("when useDefaultCAs=false set", trustapi.BundleSource{UseDefaultCAs: ptr.To(false)}, true), + Entry("when multiple set", trustapi.BundleSource{InLine: ptr.To(""), UseDefaultCAs: ptr.To(true)}, true), + ) + }) + + Context("Source object item", func() { + var ( + keySelectorAccessor func(*trustapi.SourceObjectKeySelector) + field string + ) + + BeforeEach(func() { + bundle.Spec.Sources = []trustapi.BundleSource{{}} + }) + + assertions := func() { + DescribeTable("should require exactly one object specifier", + func(selector *trustapi.SourceObjectKeySelector, wantErr bool) { + selector.Key = "ca.crt" + keySelectorAccessor(selector) + if wantErr { + expectedErr := "spec.sources[0].%s: Invalid value: \"object\": must specify one and only one of {name, selector}" + Expect(cl.Create(ctx, bundle)).Should(MatchError(ContainSubstring(expectedErr, field))) + } else { + Expect(cl.Create(ctx, bundle)).To(Succeed()) + } + }, + Entry("when none set", &trustapi.SourceObjectKeySelector{}, true), + Entry("when name set", &trustapi.SourceObjectKeySelector{Name: "ca"}, false), + Entry("when selector set", &trustapi.SourceObjectKeySelector{Selector: &metav1.LabelSelector{}}, false), + Entry("when both set", &trustapi.SourceObjectKeySelector{Name: "ca", Selector: &metav1.LabelSelector{}}, true), + ) + + DescribeTable("should require exactly one key specifier", + func(selector *trustapi.SourceObjectKeySelector, wantErr bool) { + selector.Name = "ca" + keySelectorAccessor(selector) + if wantErr { + expectedErr := "spec.sources[0].%s: Invalid value: \"object\": must specify key or includeAllKeys" + Expect(cl.Create(ctx, bundle)).Should(MatchError(ContainSubstring(expectedErr, field))) + } else { + Expect(cl.Create(ctx, bundle)).To(Succeed()) + } + }, + Entry("when none set", &trustapi.SourceObjectKeySelector{}, true), + Entry("when key set", &trustapi.SourceObjectKeySelector{Key: "ca.crt"}, false), + Entry("when selector set", &trustapi.SourceObjectKeySelector{IncludeAllKeys: true}, false), + Entry("when both set", &trustapi.SourceObjectKeySelector{Key: "ca.crt", IncludeAllKeys: true}, true), + ) + } + + Context("ConfigMap", func() { + BeforeEach(func() { + keySelectorAccessor = func(selector *trustapi.SourceObjectKeySelector) { + bundle.Spec.Sources[0].ConfigMap = selector + } + field = "configMap" + }) + + assertions() + }) + + Context("Secret", func() { + BeforeEach(func() { + keySelectorAccessor = func(selector *trustapi.SourceObjectKeySelector) { + bundle.Spec.Sources[0].Secret = selector + } + field = "secret" + }) + + assertions() + }) + }) + + Context("Target", func() { + DescribeTable("should require at least one target configMap/secret", + func(target trustapi.BundleTarget, wantErr bool) { + bundle.Spec.Target = target + if wantErr { + expectedErr := "spec.target: Invalid value: \"object\": must define at least one target configMap/secret" + Expect(cl.Create(ctx, bundle)).Should(MatchError(ContainSubstring(expectedErr))) + } else { + Expect(cl.Create(ctx, bundle)).To(Succeed()) + } + }, + Entry("when none set", trustapi.BundleTarget{NamespaceSelector: &trustapi.NamespaceSelector{}}, true), + Entry("when configMap set", trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "ca-bundle.crt"}}, false), + Entry("when secret set", trustapi.BundleTarget{Secret: &trustapi.KeySelector{Key: "ca-bundle.crt"}}, false), + Entry("when both set", trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "ca-bundle.crt"}, Secret: &trustapi.KeySelector{Key: "ca-bundle.crt"}}, false), + ) + + DescribeTable("should require target key", + func(target trustapi.BundleTarget, expErr string) { + bundle.Spec.Target = target + Expect(cl.Create(ctx, bundle)).Should(MatchError(ContainSubstring(expErr))) + }, + Entry("for configMap", trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{}}, "spec.target.configMap.key: Invalid value: \"\": spec.target.configMap.key in body should be at least 1 chars long"), + Entry("for secret", trustapi.BundleTarget{Secret: &trustapi.KeySelector{}}, "spec.target.secret.key: Invalid value: \"\": spec.target.secret.key in body should be at least 1 chars long"), + ) + }) +})