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

Add CEL validation to validate subcommand #5326

Merged
merged 2 commits into from
Feb 2, 2024
Merged

Add CEL validation to validate subcommand #5326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13448ed
Add CEL validation to `validate` subcommand
ezgidemirel Feb 2, 2024
7edf5fe
start passing io.Writer for ignoring output in tests
ezgidemirel Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions cmd/crank/beta/validate/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package validate

import (
"fmt"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -84,7 +83,6 @@ func (c *LocalCache) Init() error {

// Flush removes the cache directory
func (c *LocalCache) Flush() error {
fmt.Println("flushing cache directory: ", c.cacheDir)
return c.fs.RemoveAll(c.cacheDir)
}

Expand Down
6 changes: 3 additions & 3 deletions cmd/crank/beta/validate/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (c *Cmd) AfterApply() error {
}

// Run validate.
func (c *Cmd) Run(_ *kong.Context, _ logging.Logger) error { //nolint:gocyclo // stdin check makes it over the top
func (c *Cmd) Run(k *kong.Context, _ logging.Logger) error { //nolint:gocyclo // stdin check makes it over the top
if c.Resources == "-" && c.Extensions == "-" {
return errors.New("cannot use stdin for both extensions and resources")
}
Expand Down Expand Up @@ -117,7 +117,7 @@ func (c *Cmd) Run(_ *kong.Context, _ logging.Logger) error { //nolint:gocyclo //
c.CacheDir = filepath.Join(currentPath, c.CacheDir)
}

m := NewManager(c.CacheDir, c.fs)
m := NewManager(c.CacheDir, c.fs, k.Stdout)

// Convert XRDs/CRDs to CRDs and add package dependencies
if err := m.PrepExtensions(extensions); err != nil {
Expand All @@ -130,7 +130,7 @@ func (c *Cmd) Run(_ *kong.Context, _ logging.Logger) error { //nolint:gocyclo //
}

// Validate resources against schemas
if err := SchemaValidation(resources, m.crds, c.SkipSuccessResults); err != nil {
if err := SchemaValidation(resources, m.crds, c.SkipSuccessResults, k.Stdout); err != nil {
return errors.Wrapf(err, "cannot validate resources")
}

Expand Down
10 changes: 7 additions & 3 deletions cmd/crank/beta/validate/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package validate

import (
"fmt"
"io"

"github.com/spf13/afero"
apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Expand Down Expand Up @@ -47,14 +48,15 @@ const (
type Manager struct {
fetcher ImageFetcher
cache Cache
writer io.Writer

crds []*apiextv1.CustomResourceDefinition
deps map[string]bool // One level dependency images
confs map[string]bool // Configuration images
}

// NewManager returns a new Manager
func NewManager(cacheDir string, fs afero.Fs) *Manager {
func NewManager(cacheDir string, fs afero.Fs, w io.Writer) *Manager {
m := &Manager{}

m.cache = &LocalCache{
Expand All @@ -63,7 +65,7 @@ func NewManager(cacheDir string, fs afero.Fs) *Manager {
}

m.fetcher = &Fetcher{}

m.writer = w
m.crds = make([]*apiextv1.CustomResourceDefinition, 0)
m.deps = make(map[string]bool)
m.confs = make(map[string]bool)
Expand Down Expand Up @@ -218,7 +220,9 @@ func (m *Manager) cacheDependencies() error {
continue
}

fmt.Printf("package schemas does not exist, downloading: %s\n", image)
if _, err := fmt.Fprintln(m.writer, "package schemas does not exist, downloading: ", image); err != nil {
return errors.Wrapf(err, errWriteOutput)
}

layer, err := m.fetcher.FetchBaseLayer(image)
if err != nil {
Expand Down
120 changes: 83 additions & 37 deletions cmd/crank/beta/validate/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

@phisco phisco Feb 2, 2024 •
edited

Choose a reason for hiding this comment

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

what do you think about the following logic instead:

  • no name, no annotation => ""
  • no name, but annotation => "" (annotation)
  • name, no annotation => "name"
  • name and annotation => "name" (annotation)

both name and annotation could be useful to function users as they can now set the name of resources too.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 (%q) the name so that if both the annotation and the name are missing you don get a dangling comma and it's easier to see the name is missing: iam.aws.upbound.io/v1beta1, Kind=User, validated successfully => iam.aws.upbound.io/v1beta1, Kind=User, "" validated successfully.