yannh / kubeconform

A FAST Kubernetes manifests validator, with support for Custom Resources!
Apache License 2.0
2.23k stars 124 forks source link

Is -strict flag supposed to work w/ CRD validation? #171

Closed deepthawtz closed 1 year ago

deepthawtz commented 1 year ago

Hi,

I've noticed that the -strict flag when used against a CRD is not complaining about extra fields. Type validation and enum validation is working fine but if a YAML resource I'm validating has added undefined fields the -strict flag is returning valid. Is this known behavior?

I've tested w/ an older kubeconform version and I've also tried latest and v0.4.11.

deepthawtz commented 1 year ago

My schema

{
  "description": "RouteConfig contains routing configuration.",
  "properties": {
    "apiVersion": {
      "description": "APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources",
      "type": "string"
    },
    "kind": {
      "description": "Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds",
      "type": "string"
    },
    "metadata": {
      "type": "object"
    },
    "spec": {
      "description": "RouteConfigSpec describes a RouteConfig.",
      "properties": {
        "proxy_group": {
          "enum": [
            "frontend",
            "internal"
          ],
          "type": "string"
        },
        "virtual_hosts": {
          "items": {
            "description": "VirtualHost is configuration for a virtual host.",
            "properties": {
              "domains": {
                "items": {
                  "type": "string"
                },
                "type": "array"
              },
              "name": {
                "type": "string"
              },
              "routes": {
                "items": {
                  "description": "Route is configuration for a route in a virtual host.",
                  "properties": {
                    "backends": {
                      "items": {
                        "description": "RouteBackend specifies a Backend reference in a Route.",
                        "properties": {
                          "name": {
                            "type": "string"
                          },
                          "weight": {
                            "format": "int32",
                            "type": "integer"
                          }
                        },
                        "required": [
                          "name",
                          "weight"
                        ],
                        "type": "object"
                      },
                      "type": "array"
                    },
                    "host_rewrite": {
                      "type": "string"
                    },
                    "match": {
                      "properties": {
                        "exact_path": {
                          "type": "string"
                        },
                        "headers": {
                          "items": {
                            "description": "RouteHeaderMatch specifies a header matching rule.",
                            "properties": {
                              "exact_match": {
                                "type": "string"
                              },
                              "invert_match": {
                                "type": "boolean"
                              },
                              "name": {
                                "type": "string"
                              },
                              "prefix_match": {
                                "type": "string"
                              },
                              "regex_match": {
                                "type": "string"
                              },
                              "suffix_match": {
                                "type": "string"
                              }
                            },
                            "required": [
                              "name"
                            ],
                            "type": "object"
                          },
                          "type": "array"
                        },
                        "prefix": {
                          "type": "string"
                        }
                      },
                      "type": "object"
                    },
                    "prefix_rewrite": {
                      "type": "string"
                    },
                    "query_parameters": {
                      "items": {
                        "description": "RouteQueryParameterMatch specifies a query parameter matching rule.",
                        "properties": {
                          "exact_match": {
                            "description": "Match the value of the query parameter exactly.",
                            "type": "string"
                          },
                          "name": {
                            "description": "Name of a query parameter to match against.",
                            "type": "string"
                          },
                          "regex_match": {
                            "description": "Regular expression the value must match.",
                            "type": "string"
                          }
                        },
                        "required": [
                          "name"
                        ],
                        "type": "object"
                      },
                      "type": "array"
                    },
                    "rate_limit_zone": {
                      "type": "string"
                    },
                    "request_headers_to_add": {
                      "items": {
                        "properties": {
                          "key": {
                            "type": "string"
                          },
                          "value": {
                            "type": "string"
                          }
                        },
                        "required": [
                          "key",
                          "value"
                        ],
                        "type": "object"
                      },
                      "type": "array"
                    },
                    "timeout": {
                      "type": "string"
                    }
                  },
                  "required": [
                    "backends",
                    "match"
                  ],
                  "type": "object"
                },
                "type": "array"
              }
            },
            "required": [
              "domains",
              "name",
              "routes"
            ],
            "type": "object"
          },
          "type": "array"
        }
      },
      "required": [
        "proxy_group",
        "virtual_hosts"
      ],
      "type": "object"
    }
  },
  "required": [
    "spec"
  ],
  "type": "object"
}

my resource I'm trying to get to fail because it is defining a property at the wrong nesting level host_rewrite

---
apiVersion: routing.venmo.com/v1
kind: RouteConfig
metadata:
  name: foo
spec:
  proxy_group: internal
  virtual_hosts:
  - domains:
    - foo
    name: foo
    routes:
    - match:
        host_rewrite: bar # I want this to fail because it is defined at the wrong nesting level
        prefix: /
      backends:
      - name: outpost-internal-backend
        weight: 100
deepthawtz commented 1 year ago
$ kubeconform -verbose -summary -kubernetes-version 1.23.9 -schema-location "routeconfig-routing-v1.json" -strict routeconfigs.yaml
/tmp/proxy-config/templates/routeconfigs.yaml - RouteConfig foo is valid
Summary: 1 resource found in 1 file - Valid: 1, Invalid: 0, Errors: 0, Skipped: 0
yannh commented 1 year ago

Hi @deepthawtz , that's a really good question - ideally it would work, but currently it won't. Kubeconform vaildates against Kubernetes manifests against schemas in that repo - https://github.com/yannh/kubernetes-json-schema . The tooling generates both a "strict" and a "non strict" version of the schemas. When you specify -strict, kubeconform will use the "strict" version of the schemas in that repository.

When you use CRDs however, you use a schema you generated yourself, and it depends how that schema was generated.

As for https://github.com/datreeio/CRDs-catalog , it is good input and they might be interested in providing both a strict and non-strict version of their schemas. I would suggest opening a bug there asking for their opinion.

yannh commented 1 year ago

The parts in this script https://github.com/yannh/openapi2jsonschema/blob/master/openapi2jsonschema/command.py#L190 that refer to the "strict" mode should likely be also implemented in https://github.com/yannh/kubeconform/blob/master/scripts/openapi2jsonschema.py so users could chose between strict and non strict mode when converting to JSON Schema. Is it correct to assume you used this tool to generate the schema you are validating against? Best, Yann

deepthawtz commented 1 year ago

I'm using controller-gen crd to generate the CRD schemas. After looking into it more I realize that kubeconform -strict will work properly if the CRD schema contains "additionalProperties": false in whatever type: object you want to reject additional properties for. So I've found a workaround for now and looking into how I can generate "strict" schemas from our Go code directly (somehow).

deepthawtz commented 1 year ago

After adding "additionalProperties": false in the correct spots in the schema I am able to correctly fail the validation which is what I wanted to do.

kubeconform -verbose -summary -strict -schema-location "schema/routeconfig-routing-v1.json" -strict /tmp/proxy-config/templates/routeconfigs.yaml
/tmp/proxy-config/templates/routeconfigs.yaml - RouteConfig outpost-internal is invalid: For field spec.virtual_hosts.0.routes.0.match: Additional property host_rewrite is not allowed
/tmp/proxy-config/templates/routeconfigs.yaml - RouteConfig internal-proxy-health is valid
/tmp/proxy-config/templates/routeconfigs.yaml - RouteConfig frontend-proxy-health is invalid: For field spec.virtual_hosts.0.routes.0.match: Additional property timeout is not allowed
Summary: 3 resources found in 1 file - Valid: 1, Invalid: 2, Errors: 0, Skipped: 0
yannh commented 1 year ago

Sooo you got it to work? Controller-gen generates an OpenAPI file, not a JSON schema directly? You need to convert it to JSON schema, correct?

deepthawtz commented 1 year ago

yes, probably a better way to do the conversion but I'm doing this:

ruby -rjson -ryaml -e "puts JSON.pretty_generate(YAML.load_file('schema/some-openapi.yaml'))" | jq ".spec.versions[0].schema.openAPIV3Schema" > schema.json

yannh commented 1 year ago

That's very interesting thank you! I am currently in touch with some members of the JSONSchema spec team who are explaining me that the schema in the OpenAPIV3 CRD is very close to being a JSON schema but not exactly :D Apparently from OpenAPIv3.1 what you are suggesting should be :100: If this works for you though :+1:

deepthawtz commented 1 year ago

We can close this ticket since it doesn't look like any issue w/ kubeconform. The -strict flag works on CRD schemas, the schema just needs to set "additionalProperties": false under any objects they want to reject additional fields for. That was the key here. Thanks for the help.

FrancoisPoinsot commented 1 year ago

I am having an issue with validating emissary's custom resources using the schema defined there in datree/CRD-catalog schema that have been generated, as far as I understand, using the script in this current repo

And it seems to me the schema generated using this python script is always "strict". Because additionalProperties is set to false when missing: https://github.com/yannh/kubeconform/blob/master/scripts/openapi2jsonschema.py#L28 There is only 1 override option for the root object.

Am I correct saying you can only generate "strict" variante of the schema using this openapi2jsonschema.py ? If yes, would you be interested in a PR to add an option to generate "non-strict" schema ?