Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 51 additions & 0 deletions cmd/crd-linter/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Linter for Kubernetes CustomResourceDefinition resources

The crd-linter encompasses a number of linters that can be run against
Kubernetes CustomResourceDefinition (CRD) objects to help ensure best-practices
are followed when CRDs are authored.

## Usage

```
$ crd-linter --help
Usage of crd-linter:
--exceptions-file string Path to a list of exceptions for linter failures
--linters strings Full list of linters to run against discovered CustomResourceDefinitions (default [SchemaProvided,PreserveUnknownFields,MaxLengthStrings,MaxItemsArrays])
--output-exceptions If true, an exception list file will be written to the file denoted by '--exceptions-file'
--path string Path to recursively search for CustomResourceDefinition objects
```

The crd-linter must be pointed at a directory containing YAML files with CRD
object(s) defined in them using the `--path` flag.

For example:

```
crd-linter --path ./path/to/crds/
Copy link
Member

Choose a reason for hiding this comment

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

This project is responsible for gen resources?
could you add in the first comment why do you think that this project ought to also linter the resources? I think we need here to know when and why it would be required and used?

Also, would that be a new bin or it would also be hipped with controller-gen? Have we an enhancement proposal for it? and then, why add support to lint only the CRD and not all manifests generated by the project?

```

### Linters

The following linters are supported:

| Name | Description |
|-----------------------|---------------------------------------------------------------------|
| MaxLengthStrings | Ensures that all 'string' type fields have a `maxLength` option set |
| MaxItemsArrays | Ensures that all 'array' type fields have a `maxItems` option set |
| SchemaProvided | Ensures that all versions of all CRDs provide an OpenAPI schema |
| PreserveUnknownFields | Ensures that no fields within a CRD schema permit unknown fields |
Copy link
Member

Choose a reason for hiding this comment

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

Is not this attr only valid for v1beta1 CRDs? This project no longer supports or work with v1beta1


### Exceptions

An exceptions file may be provided which will permit certain rule violations to
be skipped. This is useful when gradually improving/fixing issues within a
repository of CRDs, whilst not permitting new violations.

You can generate an exceptions file by setting `--output-exceptions=true`
whilst also providing a `--exceptions-file`.
This will output all linter failures to the named exceptions file, so that
future runs of the linter will skip these failures.

Use this in combination with the `--linters` flag to generate an exceptions
file that only contains a subset of linter failures (e.g. only the nosiest or
least problematic failures).
60 changes: 60 additions & 0 deletions cmd/crd-linter/evaluator/evaluate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package evaluator

import (
"sigs.k8s.io/controller-tools/cmd/crd-linter/exceptions"
"sigs.k8s.io/controller-tools/cmd/crd-linter/linters"
"sigs.k8s.io/controller-tools/cmd/crd-linter/reader"
)

func Evaluate(list linters.LinterList, eval []reader.CRDToEvaluate, excepted *exceptions.ExceptionList) ResultsList {
results := make([]Results, len(eval))
for i, e := range eval {
results[i] = Results{
Evaluated: e,
}
for _, l := range list {
errs := l.Execute(e.CustomResourceDefinition)
if len(errs) == 0 {
continue
}

// If no exceptions are provided, skip the filtering logic
if excepted == nil {
results[i].Violations = append(results[i].Violations, ViolationList{
Linter: l,
Violations: errs,
})
continue
}

// Check results against the exception list
var filteredErrs []string
for _, err := range errs {
if !excepted.IsExcepted(e.OriginalFilename, e.CustomResourceDefinition.Name, l.Name(), err) {
filteredErrs = append(filteredErrs, err)
}
}
results[i].Violations = append(results[i].Violations, ViolationList{
Linter: l,
Violations: filteredErrs,
})
}
}
return results
}
93 changes: 93 additions & 0 deletions cmd/crd-linter/evaluator/types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package evaluator

import (
"sort"

"sigs.k8s.io/controller-tools/cmd/crd-linter/exceptions"
"sigs.k8s.io/controller-tools/cmd/crd-linter/linters"
"sigs.k8s.io/controller-tools/cmd/crd-linter/reader"
)

type Results struct {
Evaluated reader.CRDToEvaluate

Violations []ViolationList
}

func (r *Results) HasViolations() bool {
if len(r.Violations) == 0 {
return false
}
for _, v := range r.Violations {
if len(v.Violations) > 0 {
return true
}
}
return false
}

func (r *Results) ViolatedLinters() []ViolationList {
var list []ViolationList
for _, v := range r.Violations {
if len(v.Violations) == 0 {
continue
}

list = append(list, v)
}
return list
}

type ResultsList []Results

func (results ResultsList) ToExceptionList() *exceptions.ExceptionList {
list := exceptions.NewExceptionList()
for _, r := range results {
if !r.HasViolations() {
continue
}

for _, l := range r.Violations {
for _, v := range l.Violations {
list.Add(r.Evaluated.OriginalFilename, r.Evaluated.CustomResourceDefinition.Name, l.Linter.Name(), v)
}
}
}
sort.SliceStable(list.Exceptions, func(i, j int) bool {
return list.Exceptions[i].String() < list.Exceptions[j].String()
})
return list
}

func (results ResultsList) HasViolations() bool {
for _, r := range results {
if r.HasViolations() {
return true
}
}
return false
}

type ViolationList struct {
// The Linter that triggered this set of violations
Linter linters.Linter

// An ordered list of violations triggered by this linter
Violations []string
}
107 changes: 107 additions & 0 deletions cmd/crd-linter/exceptions/exceptions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package exceptions

import (
"fmt"
"io/ioutil"
"strings"
)

type ExceptionList struct {
Exceptions []Exception
}

type Exception struct {
filename, crdName, linter, fieldRef string
}

func (e Exception) String() string {
return fmt.Sprintf("%s|%s|%s|%s", e.filename, e.crdName, e.linter, e.fieldRef)
}

func (l *ExceptionList) IsExcepted(filename, crdName, linter, fieldRef string) bool {
toFind := Exception{
filename: filename,
crdName: crdName,
linter: linter,
fieldRef: fieldRef,
}
for _, e := range l.Exceptions {
if e == toFind {
return true
}
}
return false
}

func (l *ExceptionList) Add(filename, crdName, linter, fieldRef string) {
if l.IsExcepted(filename, crdName, linter, fieldRef) {
return
}
l.Exceptions = append(l.Exceptions, Exception{
filename: filename,
crdName: crdName,
linter: linter,
fieldRef: fieldRef,
})
}

func (l *ExceptionList) String() string {
buf := &strings.Builder{}
for _, e := range l.Exceptions {
buf.WriteString(fmt.Sprintf("%s\n", e.String()))
}
return buf.String()
}

func (l *ExceptionList) Size() int {
return len(l.Exceptions)
}

func (l *ExceptionList) WriteToFile(path string) error {
// #nosec G306
return ioutil.WriteFile(path, []byte(l.String()), 0644)
}

func NewExceptionList() *ExceptionList {
return &ExceptionList{Exceptions: make([]Exception, 0)}
}

// LoadFromFile will load a pipe (|) separated list of Exceptions from the
Copy link
Member

Choose a reason for hiding this comment

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

This doc appears a bit confusing to me. The exception list seems to be separated by newline; one exception per line. But one exception seems to be serialized in a pipe-separated format.

Have you considered other file formats? The suggested format is very compact, but the semantics are not apparent when looking at a file.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 the format here wasn't so much chosen to be compact, but rather just to have each entry on a single line to make it clear to see how many violations there are. An example line:

path/to/the/file/jaegertracing.io_jaegers_crd.yaml|jaegers.jaegertracing.io|MaxLengthStrings|spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.collector.properties.volumes.items.properties.flexVolume.properties.fsType.maxLength is not specified on a field of type 'string'

Happy to adjust this format however you feel makes it clearer - what sort of format do you think would be better/what do you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

@munnerz Sorry for the delay. You have reasonable arguments for selecting the chosen file format, and I think it should be ok. Only downside I see, is that it is not very human readable. The format must remain stable over time, so we should decide if human readability is important now. I'll guess an IDE could be a consumer of these files. Do we have any reference or common practice to refer to?

// given file.
func LoadFromFile(path string) (*ExceptionList, error) {
data, err := ioutil.ReadFile(path)
if err != nil {
return nil, err
}

list := NewExceptionList()
entries := strings.Split(string(data), "\n")
for lineNo, e := range entries {
// skip empty lines
if len(e) == 0 {
continue
}
e := strings.Split(e, "|")
if len(e) != 4 {
return nil, fmt.Errorf("invalid Exception entry at line %d", lineNo+1)
}
list.Add(e[0], e[1], e[2], e[3])
}
return list, nil
}
53 changes: 53 additions & 0 deletions cmd/crd-linter/linters/interface.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package linters

import v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"

// A Linter evaluates a CRD resource and provides information about policy violations.
type Linter interface {
// Name returns the name of this linter, used during report output
Name() string

// Execute runs the linter against the given custom resource definition
Execute(*v1.CustomResourceDefinition) WarningList

// Description prints human-readable, actionable information and references about
// what the linter does.
Description() string
}

type Warning struct {
Message string
}

type WarningList []Warning

func (w WarningList) Len() int { return len(w) }
func (w WarningList) Swap(i, j int) { w[i], w[j] = w[j], w[i] }
func (w WarningList) Less(i, j int) bool { return w[i].Message < w[j].Message }

// Used internally to construct a warning list from a set of strings.
// This function is not exported because as the Warning struct grows, the arguments
// to the function is likely to change in backward-incompatible ways.
func newWarningList(messages ...string) WarningList {
var w WarningList
for _, msg := range messages {
w = append(w, Warning{Message: msg})
}
return w
}
Loading