Make more use of api-server validation

Signed-off-by: Erik Godding Boye <[email protected]>
cert-manager · Nov 22, 2024 · ec915d4 · ec915d4
1 parent b1a0474
commit ec915d4
Show file tree

Hide file tree

Showing 7 changed files with 336 additions and 356 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
@@ -138,6 +138,11 @@ spec:
                             x-kubernetes-map-type: atomic
                         type: object
                         x-kubernetes-map-type: atomic
+                        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
@@ -209,6 +214,11 @@ spec:
                             x-kubernetes-map-type: atomic
                         type: object
                         x-kubernetes-map-type: atomic
+                        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.
@@ -222,10 +232,16 @@ spec:
                         type: boolean
                     type: object
                     x-kubernetes-map-type: atomic
+                    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-list-type: atomic
+                  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:
@@ -343,6 +359,13 @@ spec:
                         - 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)'
+                    - message: additional format keys must be unique
+                      rule: '!has(self.additionalFormats) || ![has(self.additionalFormats.jks), has(self.additionalFormats.pkcs12)].all(x,x) || self.additionalFormats.jks.key != self.additionalFormats.pkcs12.key'
+                    - message: additional format keys must be different from configMap/secret keys
+                      rule: '!has(self.additionalFormats) || !((has(self.configMap) ? [self.configMap.key] : []) + (has(self.secret) ? [self.secret.key] : [])).exists(k,(has(self.additionalFormats.jks) && self.additionalFormats.jks.key == k) || (has(self.additionalFormats.pkcs12) && self.additionalFormats.pkcs12.key == k))'
               required:
                 - sources
                 - target

diff --git a/docs/api/api.md b/docs/api/api.md
@@ -122,7 +122,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#L126-L136>)
+## type [AdditionalFormats](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L131-L141>)
 
 AdditionalFormats specifies any additional formats to write to the target
 
@@ -205,7 +205,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#L214-L253>)
+## type [BundleCondition](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L221-L260>)
 
 BundleCondition contains condition information for a Bundle.
 
@@ -312,9 +312,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#L74-L99>)
+## type [BundleSource](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L76-L101>)
 
-BundleSource is the set of sources whose data will be appended and synced to the BundleTarget in all Namespaces. \+structType=atomic
+BundleSource is the set of sources whose data will be appended and synced to the BundleTarget in all Namespaces. \+structType=atomic \+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 {
@@ -364,7 +364,7 @@ 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-L69>)
+## type [BundleSpec](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L60-L70>)
 
 BundleSpec defines the desired state of a Bundle.
 
@@ -374,6 +374,7 @@ type BundleSpec struct {
     // +listType=atomic
     // +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.
@@ -400,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#L197-L211>)
+## type [BundleStatus](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L204-L218>)
 
 BundleStatus defines the observed state of the Bundle.
 
@@ -441,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#L103-L123>)
+## type [BundleTarget](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L108-L128>)
 
-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" \+kubebuilder:validation:XValidation:rule="\!has\(self.additionalFormats\) || \!\[has\(self.additionalFormats.jks\), has\(self.additionalFormats.pkcs12\)\].all\(x,x\) || self.additionalFormats.jks.key \!= self.additionalFormats.pkcs12.key", message="additional format keys must be unique" \+kubebuilder:validation:XValidation:rule="\!has\(self.additionalFormats\) || \!\(\(has\(self.configMap\) ? \[self.configMap.key\] : \[\]\) \+ \(has\(self.secret\) ? \[self.secret.key\] : \[\]\)\).exists\(k,\(has\(self.additionalFormats.jks\) && self.additionalFormats.jks.key == k\) || \(has\(self.additionalFormats.pkcs12\) && self.additionalFormats.pkcs12.key == k\)\)", message="additional format keys must be different from configMap/secret keys"
 
 ```go
 type BundleTarget struct {
@@ -488,7 +489,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#L140-L149>)
+## type [JKS](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L145-L154>)
 
 JKS specifies additional target JKS files \+structType=atomic
 
@@ -524,7 +525,7 @@ 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#L190-L194>)
+## type [KeySelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L197-L201>)
 
 KeySelector is a reference to a key for some map data object.
 
@@ -555,7 +556,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="PKCS12"></a>
-## type [PKCS12](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L153-L161>)
+## type [PKCS12](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L158-L166>)
 
 PKCS12 specifies additional target PKCS\#12 files \+structType=atomic
 
@@ -590,9 +591,9 @@ 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#L166-L187>)
+## type [SourceObjectKeySelector](<https://github.com/cert-manager/trust-manager/blob/main/pkg/apis/trust/v1alpha1/types_bundle.go#L173-L194>)
 
-SourceObjectKeySelector is a reference to a source object and its \`data\` key\(s\) in the trust Namespace. \+structType=atomic
+SourceObjectKeySelector is a reference to a source object and its \`data\` key\(s\) in the trust Namespace. \+structType=atomic \+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 {

diff --git a/pkg/apis/trust/v1alpha1/types_bundle.go b/pkg/apis/trust/v1alpha1/types_bundle.go
@@ -62,6 +62,7 @@ type BundleSpec struct {
 	// +listType=atomic
 	// +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.
@@ -71,6 +72,7 @@ type BundleSpec struct {
 // BundleSource is the set of sources whose data will be appended and synced to
 // the BundleTarget in all Namespaces.
 // +structType=atomic
+// +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.
@@ -100,6 +102,9 @@ 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"
+// +kubebuilder:validation:XValidation:rule="!has(self.additionalFormats) || ![has(self.additionalFormats.jks), has(self.additionalFormats.pkcs12)].all(x,x) || self.additionalFormats.jks.key != self.additionalFormats.pkcs12.key", message="additional format keys must be unique"
+// +kubebuilder:validation:XValidation:rule="!has(self.additionalFormats) || !((has(self.configMap) ? [self.configMap.key] : []) + (has(self.secret) ? [self.secret.key] : [])).exists(k,(has(self.additionalFormats.jks) && self.additionalFormats.jks.key == k) || (has(self.additionalFormats.pkcs12) && self.additionalFormats.pkcs12.key == k))", message="additional format keys must be different from configMap/secret keys"
 type BundleTarget struct {
 	// ConfigMap is the target ConfigMap in Namespaces that all Bundle source
 	// data will be synced to.
@@ -163,6 +168,8 @@ type PKCS12 struct {
 // SourceObjectKeySelector is a reference to a source object and its `data` key(s)
 // in the trust Namespace.
 // +structType=atomic
+// +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

diff --git 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"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -86,88 +84,22 @@ 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
+		path := path.Child("sources").Index(i)
 
 		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"))
-			}
 
 			errs := validation.ValidateLabelSelector(configMap.Selector, validation.LabelSelectorValidationOptions{}, path.Child("selector"))
 			el = append(el, errs...)
 		}
 
 		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"))
-			}
 
 			errs := validation.ValidateLabelSelector(secret.Selector, validation.LabelSelectorValidationOptions{}, path.Child("selector"))
 			el = append(el, errs...)
 		}
-
-		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 {
@@ -188,51 +120,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{}{}
-		if configMap != nil {
-			targetKeys[configMap.Key] = struct{}{}
-		}
-		if secret != nil {
-			targetKeys[secret.Key] = struct{}{}
-		}
-
-		// Checks for nil to avoid nil point dereference error
-		if bundle.Spec.Target.AdditionalFormats.JKS != nil {
-			formats["jks"] = &bundle.Spec.Target.AdditionalFormats.JKS.KeySelector
-		}
-
-		// Checks for nil to avoid nil point dereference error
-		if bundle.Spec.Target.AdditionalFormats.PKCS12 != nil {
-			formats["pkcs12"] = &bundle.Spec.Target.AdditionalFormats.PKCS12.KeySelector
-		}
-
-		for f, selector := range formats {
-			if selector != nil {
-				if _, ok := targetKeys[selector.Key]; ok {
-					el = append(el, field.Invalid(path.Child("target", "additionalFormats", f, "key"), selector.Key, "key must be unique in target configMap"))
-				}
-				targetKeys[selector.Key] = struct{}{}
-			}
-		}
-	}
-
 	errs := validation.ValidateLabelSelector(bundle.Spec.Target.NamespaceSelector, validation.LabelSelectorValidationOptions{}, path.Child("target", "namespaceSelector"))
 	el = append(el, errs...)