-
Notifications
You must be signed in to change notification settings - Fork 905
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.
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
Add CEL validation to validate
subcommand
#5326
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,102 +17,148 @@ limitations under the License. | |
package validate | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"io" | ||
|
||
ext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" | ||
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema" | ||
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" | ||
"k8s.io/apiextensions-apiserver/pkg/apiserver/validation" | ||
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
"k8s.io/apimachinery/pkg/runtime/schema" | ||
runtimeschema "k8s.io/apimachinery/pkg/runtime/schema" | ||
celconfig "k8s.io/apiserver/pkg/apis/cel" | ||
|
||
"github.com/crossplane/crossplane-runtime/pkg/errors" | ||
|
||
"github.com/crossplane/crossplane/internal/controller/apiextensions/composite" | ||
) | ||
|
||
func newValidators(crds []*extv1.CustomResourceDefinition) (map[schema.GroupVersionKind][]validation.SchemaValidator, error) { | ||
validators := map[schema.GroupVersionKind][]validation.SchemaValidator{} | ||
const ( | ||
errWriteOutput = "cannot write output" | ||
) | ||
|
||
func newValidatorsAndStructurals(crds []*extv1.CustomResourceDefinition) (map[runtimeschema.GroupVersionKind][]*validation.SchemaValidator, map[runtimeschema.GroupVersionKind]*schema.Structural, error) { | ||
validators := map[runtimeschema.GroupVersionKind][]*validation.SchemaValidator{} | ||
structurals := map[runtimeschema.GroupVersionKind]*schema.Structural{} | ||
|
||
for i := range crds { | ||
internal := &ext.CustomResourceDefinition{} | ||
if err := extv1.Convert_v1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(crds[i], internal, nil); err != nil { | ||
return nil, err | ||
return nil, nil, err | ||
} | ||
|
||
// Top-level and per-version schemas are mutually exclusive. Therefore, we will use both if they are present. | ||
// Top-level and per-version schemas are mutually exclusive. | ||
for _, ver := range internal.Spec.Versions { | ||
var sv validation.SchemaValidator | ||
var err error | ||
|
||
gvk := schema.GroupVersionKind{ | ||
gvk := runtimeschema.GroupVersionKind{ | ||
Group: internal.Spec.Group, | ||
Version: ver.Name, | ||
Kind: internal.Spec.Names.Kind, | ||
} | ||
|
||
// Version specific validation rules | ||
if ver.Schema != nil && ver.Schema.OpenAPIV3Schema != nil { | ||
sv, _, err = validation.NewSchemaValidator(ver.Schema.OpenAPIV3Schema) | ||
if err != nil { | ||
return nil, err | ||
} | ||
var s *ext.JSONSchemaProps | ||
switch { | ||
case internal.Spec.Validation != nil: | ||
s = internal.Spec.Validation.OpenAPIV3Schema | ||
case ver.Schema != nil && ver.Schema.OpenAPIV3Schema != nil: | ||
s = ver.Schema.OpenAPIV3Schema | ||
default: | ||
// TODO log a warning here, it should never happen | ||
continue | ||
} | ||
|
||
validators[gvk] = append(validators[gvk], sv) | ||
sv, _, err = validation.NewSchemaValidator(s) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
// Top level validation rules | ||
if internal.Spec.Validation != nil { | ||
sv, _, err = validation.NewSchemaValidator(internal.Spec.Validation.OpenAPIV3Schema) | ||
if err != nil { | ||
return nil, err | ||
} | ||
validators[gvk] = append(validators[gvk], &sv) | ||
|
||
validators[gvk] = append(validators[gvk], sv) | ||
structural, err := schema.NewStructural(s) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
structurals[gvk] = structural | ||
} | ||
} | ||
|
||
return validators, nil | ||
return validators, structurals, nil | ||
} | ||
|
||
// SchemaValidation validates the resources against the given CRDs | ||
func SchemaValidation(resources []*unstructured.Unstructured, crds []*extv1.CustomResourceDefinition, skipSuccessLogs bool) error { | ||
schemaValidators, err := newValidators(crds) | ||
func SchemaValidation(resources []*unstructured.Unstructured, crds []*extv1.CustomResourceDefinition, skipSuccessLogs bool, w io.Writer) error { //nolint:gocyclo // printing the output increases the cyclomatic complexity a little bit | ||
schemaValidators, structurals, err := newValidatorsAndStructurals(crds) | ||
if err != nil { | ||
return errors.Wrap(err, "cannot create schema validators") | ||
} | ||
|
||
failure, warning := 0, 0 | ||
failure, missingSchemas := 0, 0 | ||
|
||
for i, r := range resources { | ||
resourceValidators, ok := schemaValidators[r.GetObjectKind().GroupVersionKind()] | ||
gvk := r.GetObjectKind().GroupVersionKind() | ||
sv, ok := schemaValidators[gvk] | ||
if !ok { | ||
warning++ | ||
fmt.Println("[!] could not find CRD/XRD for: " + r.GroupVersionKind().String()) | ||
missingSchemas++ | ||
if _, err := fmt.Fprintf(w, "[!] could not find CRD/XRD for: %s\n", r.GroupVersionKind().String()); err != nil { | ||
return errors.Wrap(err, errWriteOutput) | ||
} | ||
|
||
continue | ||
} | ||
|
||
rf := 0 | ||
for _, v := range resourceValidators { | ||
re := v.Validate(&resources[i]) | ||
for _, e := range re.Errors { | ||
|
||
for _, v := range sv { | ||
re := validation.ValidateCustomResource(nil, r, *v) | ||
for _, e := range re { | ||
rf++ | ||
fmt.Printf("[x] validation error %s, %s : %s\n", r.GroupVersionKind().String(), r.GetAnnotations()[composite.AnnotationKeyCompositionResourceName], e.Error()) | ||
if _, err := fmt.Fprintf(w, "[x] schema validation error %s, %s : %s\n", r.GroupVersionKind().String(), getResourceName(r), e.Error()); err != nil { | ||
return errors.Wrap(err, errWriteOutput) | ||
} | ||
} | ||
} | ||
|
||
if rf == 0 && !skipSuccessLogs { | ||
fmt.Printf("[✓] %s, %s validated successfully\n", r.GroupVersionKind().String(), r.GetAnnotations()[composite.AnnotationKeyCompositionResourceName]) | ||
} else { | ||
failure++ | ||
s := structurals[gvk] // if we have a schema validator, we should also have a structural | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feels like we could merge the two maps by having a dedicated struct wrapping both the schemaValidator and the then CEL validator |
||
|
||
celValidator := cel.NewValidator(s, true, celconfig.PerCallLimit) | ||
re, _ = celValidator.Validate(context.TODO(), nil, s, resources[i].Object, nil, celconfig.PerCallLimit) | ||
for _, e := range re { | ||
rf++ | ||
if _, err := fmt.Fprintf(w, "[x] CEL validation error %s, %s : %s\n", r.GroupVersionKind().String(), getResourceName(r), e.Error()); err != nil { | ||
return errors.Wrap(err, errWriteOutput) | ||
} | ||
} | ||
|
||
if rf == 0 && !skipSuccessLogs { | ||
if _, err := fmt.Fprintf(w, "[✓] %s, %s validated successfully\n", r.GroupVersionKind().String(), getResourceName(r)); err != nil { | ||
return errors.Wrap(err, errWriteOutput) | ||
} | ||
} else { | ||
failure++ | ||
} | ||
} | ||
} | ||
|
||
fmt.Printf("%d error, %d warning, %d success cases\n", failure, warning, len(resources)-failure-warning) | ||
if _, err := fmt.Fprintf(w, "Total %d resources: %d missing schemas, %d success cases, %d failure cases\n", len(resources), missingSchemas, len(resources)-failure-missingSchemas, failure); err != nil { | ||
return errors.Wrap(err, errWriteOutput) | ||
} | ||
|
||
if failure > 0 { | ||
return errors.New("could not validate all resources") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func getResourceName(r *unstructured.Unstructured) string { | ||
if r.GetName() != "" { | ||
return r.GetName() | ||
} | ||
|
||
// fallback to composition resource name | ||
return r.GetAnnotations()[composite.AnnotationKeyCompositionResourceName] | ||
} | ||
Comment on lines
+157
to
+164
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you think about the following logic instead:
both name and annotation could be useful to function users as they can now set the name of resources too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer keeping it simple and not making the output hard to read. Since there can be only one resource with the same name and GVK, users can check the annotation if they want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it would be hard to read and in a scenario where I'm piping the render output into this there would be no manifests to check, I'd have to rerun the command without piping to see both. At least I'd quote ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest having this return
(struct, error)
next time someone touches this code. The return values are long and hard to follow.