instrumenta / kubeval

Validate your Kubernetes configuration files, supports multiple Kubernetes versions
https://kubeval.com
Other
3.16k stars 229 forks source link

HPA Definition Problems #114

Closed jeff-minard-ck closed 5 years ago

jeff-minard-ck commented 5 years ago

(Possibly related to #57)

So, have run across this issue that, in strict mode, HPAs will fail to validate, despite being correct according to the spec.

Taken directly from https://kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale-walkthrough/:

apiVersion: autoscaling/v2beta2
kind: HorizontalPodAutoscaler
metadata:
  name: php-apache
  namespace: default
spec:
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: php-apache
  minReplicas: 1
  maxReplicas: 10
  metrics:
  - type: Resource
    resource:
      name: cpu
      target:
        type: Utilization
        averageUtilization: 50

Results in:

$ kubeval hpa.yml
The document hpa.yml contains a valid HorizontalPodAutoscaler

$ kubeval --strict hpa.yml
The document hpa.yml contains an invalid HorizontalPodAutoscaler
---> metrics: Additional property metrics is not allowed

The problem is that the JSON schema, as of today, isn't setup to deal with the fact that the metrics is a list of objects with varying types. Specifically this section:

        "additionalProperties": false,
        "required": [
          "type"
        ],
        "description": "MetricSpec specifies how to scale based on a single metric (only `type` and one other matching field should be set at once).",

A proper HPA will have both the type property and one other, according to what the type is.

I do not come to this bug tracker empty handed, however. After learning up a bit of JSON schema, it seems the solution lies in oneOf validation on the items in the metrics array.

Here's a demo schema I came up with that makes an example of how this could be modeled to function correctly:

{
  "description": "HorizontalPodAutoscalerSpec describes the desired functionality of the HorizontalPodAutoscaler.",
  "required": [
    "maxReplicas"
  ],
  "additionalProperties": false,
  "$schema": "http://json-schema.org/schema#",
  "type": "object",
  "properties": {
    "maxReplicas": {
      "type": "integer",
      "format": "int32"
    },
    "metrics": {
      "type": [
        "array",
        "null"
      ],
      "items": {
        "oneOf": [
          {
            "type": "object",
            "required": [
              "type",
              "pod"
            ],
            "additionalProperties": false,
            "properties": {
              "type": {
                "type": "string",
                "pattern": "pod"
              },
              "pod": {
                "type": "string"
              }
            }
          },
          {
            "type": "object",
            "required": [
              "type",
              "resource"
            ],
            "additionalProperties": false,
            "properties": {
              "type": {
                "type": "string",
                "pattern": "resource"
              },
              "resource": {
                "type": "string"
              }
            }
          }
        ]
      }
    }
  }
}

As tested against:

{

  "maxReplicas": 1,
  "metrics": [
    {
      "type": "resource",
      "resource": "123"
    },
    {
      "type": "pod",
      "pod": "123"
    }
  ]
}

Where you can tweak the type value and alternative pod/resource keys to watch it pass/fail for misconfigured objects. (I trialed this fun at https://www.jsonschemavalidator.net/)

I've no idea how to translate this through the code generation system, however.

claudod commented 5 years ago

Same issue for me: try to run kubeval --strict --kubernetes-version 1.12.0 (or other versions supposed to be supporting custom metrics hpa) on an hpa.yaml containing exactly the custom metrics conf example of the official k8s doc, or any other example you may find on the net, and the validation will fail with "---> metrics: Additional property metrics is not allowed".

I read the definitions.json, and it looks like io.k8s.api.autoscaling.v2beta2.MetricSpec and io.k8s.api.autoscaling.v2beta1.MetricSpec are well defined.

It really looks like a bug. Hope to have some feedback on it !

Otherwise... very nice tool ! Great job !

garethr commented 5 years ago

Thanks for flagging, I'll take a look when I get a moment at opening an upstream issue and seeing if I can patch in the schema conversion.

claudod commented 5 years ago

Upgrading to version 0.8.1, recently released, I'm not encountering any issue on HPA anymore !

garethr commented 5 years ago

@claudod thanks for letting me know.

jeff-minard-ck commented 5 years ago

Agreed; I see this working now as well. Thanks!