✨ Adds a tool for linting OpenAPI schemas on CRD resources #628

munnerz · 2021-09-30T16:38:07Z

This PR adds a new CLI tool named crd-linter which will lint/run checks against CRD resources and their corresponding schema.

Initially this includes just 4 linters to check for:

SchemaProvided: ensures a schema is provided for all API versions
PreserveUnknownFields: ensures that provideUnknownFields and x-kubernetes-preserve-unknown-fields is not used
MaxLengthStrings: ensures string typed fields have a maxLength specified
MaxItemsArrays: ensures array typed fields have a maxItems specified

This is all in the effort to ensure a higher quality of CRDs, which helps protect apiservers against e.g. too much data being stored in a CRD, or ever-growing number of items in arrays that can cause scalability issues.

These linters aren't aimed at all being "best practice", but providing this tool allows teams to check for potential issues/better understand APIs they are installing and where issues may be found.

I'm not sure where this is best to live, nor am I wedded to any parts of the design of it, so welcome any and all feedback 😄

ref https://kubernetes.slack.com/archives/C0EG7JC6T/p1628592082067800

k8s-ci-robot · 2021-09-30T16:38:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: munnerz
To complete the pull request process, please assign vincepri after the PR has been reviewed.
You can assign the PR to them by writing /assign @vincepri in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

munnerz · 2021-10-01T08:43:06Z

Just one failure left, and I'm not sure if it makes sense to change the code here:

cmd/crd-linter/exceptions/exceptions.go:77:9: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	return ioutil.WriteFile(path, []byte(l.String()), 0644)

erikgb · 2021-10-02T20:07:40Z

Just one failure left, and I'm not sure if it makes sense to change the code here:
cmd/crd-linter/exceptions/exceptions.go:77:9: G306: Expect WriteFile permissions to be 0600 or less (gosec)

	return ioutil.WriteFile(path, []byte(l.String()), 0644)

IMO this should be considered a false positive and suppressed with an annotation.

cmd/crd-linter/linters/preserve_unknown_fields.go

cmd/crd-linter/exceptions/exceptions.go

erikgb · 2021-10-03T10:04:37Z

cmd/crd-linter/exceptions/exceptions.go

+	return &ExceptionList{Exceptions: make([]Exception, 0)}
+}
+
+// LoadFromFile will load a pipe (|) separated list of Exceptions from the


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.

👍 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?

@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?

cmd/crd-linter/linters/interface.go

munnerz · 2021-10-25T10:21:48Z

Thanks for the review comments, I'll be updating the PR this week 😄

munnerz · 2021-11-02T11:51:46Z

I've addressed all the feedback - there's just the one open question on the serialisation format for the exceptions file left :)

k8s-ci-robot · 2021-11-02T11:52:00Z

@munnerz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-tools-test-master	`2f473e2`	link	true	`/test pull-controller-tools-test-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-11-11T04:44:06Z

@munnerz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

camilamacedo86

Have we an enhancement proposal for this one?

camilamacedo86 · 2021-12-10T09:26:09Z

cmd/crd-linter/README.md

+| 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    |


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

camilamacedo86 · 2021-12-10T09:30:39Z

cmd/crd-linter/README.md

+For example:
+
+```
+crd-linter --path ./path/to/crds/


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?

roee88 · 2022-01-06T11:32:39Z

cmd/crd-linter/linters/preserve_unknown_fields.go

+func (p NoPreserveUnknownFields) Description() string {
+	return "Fields should avoid using 'preserveUnknownFields' as it means any data can be persisted into the Kubernetes apiserver " +
+		"without any guards on the size and type of data. Setting this to true is no longer permitted in the 'v1' API version of " +
+		"CustomResourceDefinitions, and as such, it should not be used: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-pruning"
+}
+
+func (p NoPreserveUnknownFields) Execute(crd *v1.CustomResourceDefinition) WarningList {
+	var errs []Warning
+	if crd.Spec.PreserveUnknownFields {
+		errs = append(errs, Warning{Message: "spec.preserveUnknownFields is set to 'true'"})
+	}
+	return append(errs, recurseAllSchemas(crd.Spec.Versions, func(props v1.JSONSchemaProps, path string) []Warning {
+		if props.XPreserveUnknownFields != nil && *props.XPreserveUnknownFields == true {
+			return newWarningList(fmt.Sprintf("%s.x-kubernetes-preserve-unknown-fields is set to 'true'", path))
+		}
+		return nil
+	})...)
+}


The description mentions that v1 forbids it, but that seems to be true only to spec.preserveUnknownFields and not to x-kubernetes-preserve-unknown-fields on specific properties. Am I missing something?

k8s-triage-robot · 2022-04-06T12:03:52Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

erikgb · 2022-04-06T12:14:13Z

/remove-lifecycle stale

I would like to use this tool! 😊

k8s-triage-robot · 2022-07-05T13:09:45Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-08-04T14:09:25Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-09-03T15:08:27Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-09-03T15:08:31Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Add crd-linter tool

57b5fe5

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 30, 2021

k8s-ci-robot requested review from mengqiy and pwittrock September 30, 2021 16:38

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 30, 2021

crd-linter: add 4 initial linters

f4adb3c

munnerz force-pushed the crd-linter branch from 9e7658c to f4adb3c Compare October 1, 2021 08:38

Fix golangci-lint failures by aliasing imports

70c7eb3

erikgb reviewed Oct 3, 2021

View reviewed changes

brownleej mentioned this pull request Oct 22, 2021

Adding linting checks for CRDs FoundationDB/fdb-kubernetes-operator#954

Closed

Address review feedback

2f473e2

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021

camilamacedo86 reviewed Dec 10, 2021

View reviewed changes

roee88 reviewed Jan 6, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 6, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 6, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 5, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 4, 2022

k8s-ci-robot closed this Sep 3, 2022

✨ Adds a tool for linting OpenAPI schemas on CRD resources #628

✨ Adds a tool for linting OpenAPI schemas on CRD resources #628

Uh oh!

Conversation

munnerz commented Sep 30, 2021

Uh oh!

k8s-ci-robot commented Sep 30, 2021

Uh oh!

munnerz commented Oct 1, 2021

Uh oh!

erikgb commented Oct 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikgb Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

munnerz Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

erikgb Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

munnerz commented Oct 25, 2021

Uh oh!

munnerz commented Nov 2, 2021

Uh oh!

k8s-ci-robot commented Nov 2, 2021

Uh oh!

k8s-ci-robot commented Nov 11, 2021

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

roee88 Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

k8s-triage-robot commented Apr 6, 2022

Uh oh!

erikgb commented Apr 6, 2022

Uh oh!

k8s-triage-robot commented Jul 5, 2022

Uh oh!

k8s-triage-robot commented Aug 4, 2022

Uh oh!

k8s-triage-robot commented Sep 3, 2022

Uh oh!

k8s-ci-robot commented Sep 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants