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.

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

kubeconform fails for object value=null #123

Open
eloo opened this issue Aug 24, 2022 · 20 comments
Open

kubeconform fails for object value=null #123

eloo opened this issue Aug 24, 2022 · 20 comments

Comments

@eloo
Copy link

eloo commented Aug 24, 2022

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
instrumenta/kubeval#168

@eyarz
Copy link
Contributor

eyarz commented Aug 24, 2022

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

@yannh
Copy link
Owner

yannh commented Aug 24, 2022

Gosh @eyarz you're fast 🤣 💯 Same question ;)

@eloo
Copy link
Author

eloo commented Aug 25, 2022

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

eyarz commented Aug 25, 2022

          "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
Copy link
Author

eloo commented Aug 25, 2022

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

eyarz commented Aug 25, 2022

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

eloo commented Aug 25, 2022

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

eyarz commented Aug 25, 2022

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

eloo commented Aug 25, 2022

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

eyarz commented Aug 25, 2022

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

eloo commented Aug 25, 2022

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

eyarz commented Aug 25, 2022

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

eloo commented Aug 25, 2022

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

yannh commented Apr 22, 2023

@eloo if you ever find the time, could you give this tool a shot and see if it works better for you? #182 🙏

@eloo-abi
Copy link

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

eloo-abi commented Oct 19, 2023

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

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

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

@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: instrumenta/openapi2jsonschema#44, instrumenta/kubernetes-json-schema#25.

@eloo-abi
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants