Make more use of api-server validation

Signed-off-by: Erik Godding Boye <[email protected]>
cert-manager · Nov 16, 2024 · 78f63d5 · 78f63d5
1 parent 41dc93d
commit 78f63d5
Show file tree

Hide file tree

Showing 7 changed files with 229 additions and 223 deletions.
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
@@ -87,6 +87,7 @@ spec:
                             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 +136,9 @@ 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)'
                       inLine:
                         description: InLine is a simple string to append as the source data.
                         type: string
@@ -155,6 +159,7 @@ spec:
                             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 +208,9 @@ 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)'
                       useDefaultCAs:
                         description: |-
                           UseDefaultCAs, when true, requests the default CA bundle to be used as a source.
@@ -215,7 +223,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 +246,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 +264,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 +282,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 +308,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
@@ -125,7 +125,7 @@ func Resource(resource string) schema.GroupResource
 Resource takes an unqualified resource and returns a Group qualified GroupResource
 
 <a name="AdditionalFormats"></a>
-## type [AdditionalFormats](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L120-L128>)
+## type [AdditionalFormats](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L125-L133>)
 
 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.
 
 <a name="BundleCondition"></a>
-## type [BundleCondition](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L206-L245>)
+## type [BundleCondition](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L214-L253>)
 
 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.
 
 <a name="BundleSource"></a>
-## type [BundleSource](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L70-L95>)
+## type [BundleSource](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L74-L99>)
 
-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.
 
 <a name="BundleSpec"></a>
-## type [BundleSpec](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L60-L66>)
+## type [BundleSpec](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L60-L69>)
 
 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.
 
 <a name="BundleStatus"></a>
-## type [BundleStatus](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L189-L203>)
+## type [BundleStatus](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L197-L211>)
 
 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.
 
 <a name="BundleTarget"></a>
-## type [BundleTarget](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L99-L117>)
+## type [BundleTarget](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L104-L122>)
 
-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.
 
 <a name="JKS"></a>
-## type [JKS](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L130-L139>)
+## type [JKS](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L135-L144>)
 
 
 
@@ -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.
 
 <a name="KeySelector"></a>
-## type [KeySelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L183-L186>)
+## type [KeySelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L190-L194>)
 
 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.
 
 <a name="NamespaceSelector"></a>
-## type [NamespaceSelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L152-L157>)
+## type [NamespaceSelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L157-L162>)
 
 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.
 
 <a name="PKCS12"></a>
-## type [PKCS12](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L141-L149>)
+## type [PKCS12](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L146-L154>)
 
 
 
@@ -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.
 
 <a name="SourceObjectKeySelector"></a>
-## type [SourceObjectKeySelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L161-L180>)
+## type [SourceObjectKeySelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L167-L187>)
 
-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\}"
 
 ```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

diff --git 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,12 @@ 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}"
 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
@@ -182,6 +189,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
@@ -86,25 +86,12 @@ 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"))
 			}
@@ -115,53 +102,14 @@ func (v *validator) validate(obj runtime.Object) (admission.Warnings, error) {
 
 		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 {
@@ -185,18 +133,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{}{}