yannh / kubeconform

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

kubeconform fails for object value=null #123

Open eloo opened 2 years ago

eloo commented 2 years ago

Hi, just have seen that key: null is validated correctly. Also in kubeval there was similar issue where it is indicated that key: null is a valid object or at least should be expected https://github.com/instrumenta/kubeval/issues/168

eyarz commented 2 years ago

Can you provide an example of a manifest with key: null that should not pass the schema validation?

yannh commented 2 years ago

Gosh @eyarz you're fast :rofl: :100: Same question ;)

eloo commented 2 years ago

Hi, yes of course i have an example ;)

claim.yml

# Source: microservice/templates/redis/redis.yaml
apiVersion: project.cloud.test/v1
kind: RedisInstance
metadata:
  name: test-release-name-project-dev-redis
  namespace: project-dev
  annotations:
    helm.sh/resource-policy: "keep"
spec:
  parameters:
    redisClusterName: "test-release-na-project-dev-redis"
    region: eu-central-1
    plan: micro
    deletionPolicy: Delete
    cacheSubnetGroupName: vpc_cache_subnet_group-UNABLE-RESOLVE-FROM-SECRET
    cacheParameterGroupName: elysium-redis6-x
    securityGroupIds:
      - vpc_cache_security_group
      - UNABLE-RESOLVE-FROM-SECRET
  compositionUpdatePolicy: Automatic
  compositionRevisionRef: null
  writeConnectionSecretToRef:
    name: test-release-name-project-dev-redis-connection

xrd.json

{
    "properties": {
      "apiVersion": {
        "type": "string"
      },
      "kind": {
        "type": "string"
      },
      "metadata": {
        "type": "object"
      },
      "spec": {
        "properties": {
          "compositionRef": {
            "properties": {
              "name": {
                "type": "string"
              }
            },
            "required": [
              "name"
            ],
            "type": "object",
            "additionalProperties": false
          },
          "compositionRevisionRef": {
            "properties": {
              "name": {
                "type": "string"
              }
            },
            "required": [
              "name"
            ],
            "type": "object",
            "additionalProperties": false
          },
          "compositionSelector": {
            "properties": {
              "matchLabels": {
                "additionalProperties": {
                  "type": "string"
                },
                "type": "object"
              }
            },
            "required": [
              "matchLabels"
            ],
            "type": "object",
            "additionalProperties": false
          },
          "compositionUpdatePolicy": {
            "default": "Automatic",
            "enum": [
              "Automatic",
              "Manual"
            ],
            "type": "string"
          },
          "parameters": {
            "properties": {
              "cacheParameterGroupName": {
                "description": "CacheParameterGroupName specifies the name of the parameter group to associate with this replication group.",
                "type": "string"
              },
              "cacheSubnetGroupName": {
                "type": "string"
              },
              "deletionPolicy": {
                "description": "Deletion policy for the AWS resources",
                "type": "string"
              },
              "plan": {
                "type": "string"
              },
              "redisClusterName": {
                "type": "string"
              },
              "region": {
                "type": "string"
              },
              "securityGroupIds": {
                "description": "SecurityGroupIds specifies one or more Amazon VPC security groups associated with this replication group. Use this parameter only when you are creating a replication group in an Amazon VPC.",
                "items": {
                  "type": "string"
                },
                "type": "array"
              },
              "tags": {
                "description": "AWS resource tags",
                "items": {
                  "properties": {
                    "key": {
                      "type": "string"
                    },
                    "value": {
                      "type": "string"
                    }
                  },
                  "type": "object",
                  "additionalProperties": false
                },
                "type": "array"
              }
            },
            "required": [
              "cacheParameterGroupName",
              "redisClusterName",
              "region",
              "plan",
              "cacheSubnetGroupName",
              "securityGroupIds"
            ],
            "type": "object",
            "additionalProperties": false
          },
          "publishConnectionDetailsTo": {
            "properties": {
              "configRef": {
                "default": {
                  "name": "default"
                },
                "properties": {
                  "name": {
                    "type": "string"
                  }
                },
                "type": "object",
                "additionalProperties": false
              },
              "metadata": {
                "properties": {
                  "annotations": {
                    "additionalProperties": {
                      "type": "string"
                    },
                    "type": "object"
                  },
                  "labels": {
                    "additionalProperties": {
                      "type": "string"
                    },
                    "type": "object"
                  },
                  "type": {
                    "type": "string"
                  }
                },
                "type": "object",
                "additionalProperties": false
              },
              "name": {
                "type": "string"
              }
            },
            "required": [
              "name"
            ],
            "type": "object",
            "additionalProperties": false
          },
          "resourceRef": {
            "properties": {
              "apiVersion": {
                "type": "string"
              },
              "kind": {
                "type": "string"
              },
              "name": {
                "type": "string"
              }
            },
            "required": [
              "apiVersion",
              "kind",
              "name"
            ],
            "type": "object",
            "additionalProperties": false
          },
          "writeConnectionSecretToRef": {
            "properties": {
              "name": {
                "type": "string"
              }
            },
            "required": [
              "name"
            ],
            "type": "object",
            "additionalProperties": false
          }
        },
        "required": [
          "parameters"
        ],
        "type": "object",
        "additionalProperties": false
      },
      "status": {
        "properties": {
          "conditions": {
            "description": "Conditions of the resource.",
            "items": {
              "properties": {
                "lastTransitionTime": {
                  "format": "date-time",
                  "type": "string"
                },
                "message": {
                  "type": "string"
                },
                "reason": {
                  "type": "string"
                },
                "status": {
                  "type": "string"
                },
                "type": {
                  "type": "string"
                }
              },
              "required": [
                "lastTransitionTime",
                "reason",
                "status",
                "type"
              ],
              "type": "object",
              "additionalProperties": false
            },
            "type": "array"
          },
          "connectionDetails": {
            "properties": {
              "lastPublishedTime": {
                "format": "date-time",
                "type": "string"
              }
            },
            "type": "object",
            "additionalProperties": false
          }
        },
        "type": "object",
        "additionalProperties": false
      }
    },
    "required": [
      "spec"
    ],
    "type": "object"
  }

command:

kubeconform -schema-location default -schema-location xrd.json --strict claim.yml

result:

claim.yml - RedisInstance test-release-name-project-dev-redis is invalid: For field spec.compositionRevisionRef: Invalid type. Expected: object, given: null
eyarz commented 2 years ago
          "compositionRevisionRef": {
            "properties": {
              "name": {
                "type": "string"
              }
            },
            "required": [
              "name"

According to the JSON schema you attached, the key compositionRevisionRef must contain a property (object) with the key name (here is a valid example).

Therefore, it's the expected behavior that Kubconform will fail when spec.compositionRevisionRef: null.

eloo commented 2 years ago

@eyarz are you sure? because the compositionRevisionRef itself is not required so setting the key to null should be fine

further this is example is accepted by kubernetes and kubectl without any issues

imho a valid violation would be:

spec.compositionRevisionRef: {}
eyarz commented 2 years ago

I'm not a JSON schema expert, but I know that the keyword required means it's mandatory.

But we need to clarify something, Kubeconfrom checks manifest against a provided schema(s). If you think spec.compositionRevisionRef: {} should pass, you can just fix your schema. There isn't any bug to fix here.

If you provide me with the CRD, I can look into why spec.compositionRevisionRef: {} is acceptable by kubectl/K8s.

eloo commented 2 years ago

@eyarz i guess we have a misunderstanding here

spec.compositionRevisionRef: {} should NOT pass -> because we provide a object without name

but

spec.compositionRevisionRef: null should pass -> because we remove the whole key

and spec.compositionRevisionRef itself is NOT required.. only spec.compositionRevisionRef.name is required if spec.compositionRevisionRef is present

but with spec.compositionRevisionRef: null spec.compositionRevisionRef is not present

e.g. if you remove the line spec.compositionRevisionRef: null kubeconform is happy.. but it should also be happy with spec.compositionRevisionRef: null in the claim.yml

eyarz commented 2 years ago

Thank you for the clarification, now we are synced :)

The problem is that your schema states that spec.compositionRevisionRef should be an object and null type is not an object according to the JSON schema draft.

So the issue is still that the manifest doesn't match the schema - you can also check it out here: image

eloo commented 2 years ago

Hi,

okay.. i have seen that when i would change the json to be like this

            "type": ["null", "object"],

its working..

but now its getting curious :P we are using your script to translate the CRDs into json schemas https://raw.githubusercontent.com/yannh/kubeconform/master/scripts/openapi2jsonschema.py

so maybe this script needs to be updated? so maybe all not required keys should have "null" additional as type

eyarz commented 2 years ago

It's not my script so I can't take credit for it 😅

The interesting question is if this is also how it's defined in the CRD or if it's something that is happening when translating the openAPI into JSON schema.

If you give a link to the CRD itself, I can look into it :)

eloo commented 2 years ago

crd.yml

apiVersion: apiextensions.k8s.io/v1
  kind: CustomResourceDefinition
  metadata:
    creationTimestamp: "2022-05-11T09:59:12Z"
    generation: 1
    name: redisinstances.project.cloud.test
    ownerReferences:
    - apiVersion: apiextensions.crossplane.io/v1
      controller: true
      kind: CompositeResourceDefinition
      name: xredisinstances.project.cloud.test
      uid: 93b2c0c2-537d-47dd-8e5b-431e1adcb17c
    resourceVersion: "226839108"
    uid: c34ede3c-d2bb-43be-ba94-0a7a40b3fd97
  spec:
    conversion:
      strategy: None
    group: project.cloud.test
    names:
      categories:
      - claim
      kind: RedisInstance
      listKind: RedisInstanceList
      plural: redisinstances
      singular: redisinstance
    scope: Namespaced
    versions:
    - additionalPrinterColumns:
      - jsonPath: .status.conditions[?(@.type=='Ready')].status
        name: READY
        type: string
      - jsonPath: .spec.writeConnectionSecretToRef.name
        name: CONNECTION-SECRET
        type: string
      - jsonPath: .metadata.creationTimestamp
        name: AGE
        type: date
      name: v1
      schema:
        openAPIV3Schema:
          properties:
            apiVersion:
              type: string
            kind:
              type: string
            metadata:
              type: object
            spec:
              properties:
                compositionRef:
                  properties:
                    name:
                      type: string
                  required:
                  - name
                  type: object
                compositionRevisionRef:
                  properties:
                    name:
                      type: string
                  required:
                  - name
                  type: object
                compositionSelector:
                  properties:
                    matchLabels:
                      additionalProperties:
                        type: string
                      type: object
                  required:
                  - matchLabels
                  type: object
                compositionUpdatePolicy:
                  default: Automatic
                  enum:
                  - Automatic
                  - Manual
                  type: string
                parameters:
                  properties:
                    cacheParameterGroupName:
                      description: CacheParameterGroupName specifies the name of the
                        parameter group to associate with this replication group.
                      type: string
                    cacheSubnetGroupName:
                      type: string
                    deletionPolicy:
                      description: Deletion policy for the AWS resources
                      type: string
                    plan:
                      type: string
                    redisClusterName:
                      type: string
                    region:
                      type: string
                    securityGroupIds:
                      description: SecurityGroupIds specifies one or more Amazon VPC
                        security groups associated with this replication group. Use
                        this parameter only when you are creating a replication group
                        in an Amazon VPC.
                      items:
                        type: string
                      type: array
                    tags:
                      description: AWS resource tags
                      items:
                        properties:
                          key:
                            type: string
                          value:
                            type: string
                        type: object
                      type: array
                  required:
                  - cacheParameterGroupName
                  - redisClusterName
                  - region
                  - plan
                  - cacheSubnetGroupName
                  - securityGroupIds
                  type: object
                publishConnectionDetailsTo:
                  properties:
                    configRef:
                      default:
                        name: default
                      properties:
                        name:
                          type: string
                      type: object
                    metadata:
                      properties:
                        annotations:
                          additionalProperties:
                            type: string
                          type: object
                        labels:
                          additionalProperties:
                            type: string
                          type: object
                        type:
                          type: string
                      type: object
                    name:
                      type: string
                  required:
                  - name
                  type: object
                resourceRef:
                  properties:
                    apiVersion:
                      type: string
                    kind:
                      type: string
                    name:
                      type: string
                  required:
                  - apiVersion
                  - kind
                  - name
                  type: object
                writeConnectionSecretToRef:
                  properties:
                    name:
                      type: string
                  required:
                  - name
                  type: object
              required:
              - parameters
              type: object
            status:
              properties:
                conditions:
                  description: Conditions of the resource.
                  items:
                    properties:
                      lastTransitionTime:
                        format: date-time
                        type: string
                      message:
                        type: string
                      reason:
                        type: string
                      status:
                        type: string
                      type:
                        type: string
                    required:
                    - lastTransitionTime
                    - reason
                    - status
                    - type
                    type: object
                  type: array
                connectionDetails:
                  properties:
                    lastPublishedTime:
                      format: date-time
                      type: string
                  type: object
              type: object
          required:
          - spec
          type: object
      served: true
      storage: true
      subresources:
        status: {}
eyarz commented 2 years ago

So it's the same definition in the openAPI spec:

                compositionRevisionRef:
                  properties:
                    name:
                      type: string
                  required:
                  - name

Therefore you can fix it locally or open a PR in the project that provides this CRD (crossplane?). there is nothing to fix from the Kubeconform side from what I see...

eloo commented 2 years ago

yep that snippet you have posted is correct..

but the problem is that compositionRevisionRef is not required.. so null is valid

please do not reference the name property everytime.. this is fine.. its all about the compositionRevisionRef which is optional and thus null should be valid

yannh commented 1 year ago

@eloo if you ever find the time, could you give this tool a shot and see if it works better for you? https://github.com/yannh/kubeconform/issues/182 :pray:

eloo-abi commented 12 months ago

@yannh sorry for the late response

but i have tested the script from @tricktron and its producing the same schema and thus will also not work. i guess also this is bit specific for the real use behind and not the OAS itself.

its related directly to "how is a yaml applied" and not the "result" itself.. but as this tool is specific for kubernetes it should respect this behaviour of patching

so guess the best way to fix this is to have the schema generator to add null to the types while converting

eloo-abi commented 12 months ago

@yannh i got good news..

i got it fixed by checking the script code and the upstream and have seen that you have might missed an introduction while porting the script.

in the upstream the following method is called allow_null_optional_fields https://github.com/instrumenta/openapi2jsonschema/blob/d697cbff8a25f520e125e3a5f79cb4e9b972e8ce/openapi2jsonschema/command.py#L203

but this is not called in your script, so adding the following will fix it

    schema = replace_int_or_string(schema)
+   schema = allow_null_optional_fields(schema)
    schemaJSON = json.dumps(schema, indent=2)

maybe you can add the allow_null_optional_fields to your hosted script :)

tricktron commented 11 months ago

@eloo-abi Why do you have/want to have optional null values in your crd? Can't you just remove them?

I want to understand your use case🙃.

eloo-abi commented 11 months ago

@tricktron to delete a previous set or default key by setting the value to null this is the usual way in helm (or any other kubernetes resource) to remove an already set key when applying yaml files

tricktron commented 11 months ago

@eloo-abi I see. I mostly use kustomize and helm in a gitops style and thus hardly ever need to remove an already set key. That means that every optional field should allow to be null.

I am not sure though if the provided allow_null_optional_fields code works as expected by looking at the following issues: https://github.com/instrumenta/openapi2jsonschema/issues/44, https://github.com/instrumenta/kubernetes-json-schema/issues/25.

eloo-abi commented 11 months ago

@tricktron

I mostly use kustomize and helm in a gitops style

we do use this as well but from time to time helm chart or requirements are changing and thus something need to be disabled again ;) e.g. enhanced logging in aws or something

so far i have not seen any issues in our case but i also don't have checked it that deep.

That means that every optional field should allow to be null.

but thats from the "patching" perspective this is true and must also be considered. maybe this is different for "creating" resources and "patching" resources?

namm2 commented 2 months ago

HI, I bumped into this while validating Prometheus CRDs where there are multiple properties (e.g.: serviceMonitorSelector, serviceMonitorNamespaceSelector, podMonitorSelector, etc.) that could be either null, {}, or of type v1.LabelSelector. And running kubeconform against the Prometheus CR in our env resulted many errors of invalid schema, like:

.cache/templates.yaml - ThanosRuler prometheus-ruler is invalid: problem validating schema. Check JSON formatting: jsonschema: '/spec/ruleNamespaceSelector' does not validate with https://raw.githubusercontent.com/datreeio/CRDs-catalog/main/monitoring.coreos.com/thanosruler_v1.json#/properties/spec/properties/ruleNamespaceSelector/type: expected object, but got null

The example's CRD is defined here, so the datree's jsonschema is correct.

Is there any workaround for kubeconform to pass this scenario?