Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Replace webhook validations with CEL validation #475

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+316 −340
Draft

WIP: Replace webhook validations with CEL validation #475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions deploy/charts/trust-manager/templates/crd-trust.cert-manager.io_bundles.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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:
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/trust/v1alpha1/types_bundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Is a +kubebuilder comment the only way of specifying these validation rules? (I'm unfamiliar generally with this CEL validation stuff)

This CEL code is super important to our UX but it's really difficult to read, understand and test it when it's embedded in a comment like this - seems like a sharp edge.

This isn't a blocker or anything, but it seems a shame if this is the only way today!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it should be possible to hack the expression directly into the CRDs, but that is not something I would do. 😆 The expressions are mainly complex because the API is suboptimal. After #486, I think the expressions could become really simple.

Thanks for reviewing @SgtCoDFish (and @juliocamarero)! But I think the best option here is to put this PR back into draft-mode and wait for a conclusion on #486.

type BundleTarget struct {
// ConfigMap is the target ConfigMap in Namespaces that all Bundle source
// data will be synced to.
Expand Down Expand Up @@ -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
Expand Down
114 changes: 1 addition & 113 deletions pkg/webhook/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package webhook
import (
"context"
"fmt"
"strconv"

"github.com/go-logr/logr"
"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
Expand Down Expand Up @@ -86,88 +85,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 {
Expand All @@ -188,51 +121,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...)

Expand Down
Loading