Outdated validation for k8s HPA

[artem-nefedov]

Describe the bug

Given this manifest (builtin kubernetes resource, not a CRD), I get the server complaining about "metrics" field: Yamlls: Property metrics is not allowed. [513].

apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: foo
spec:
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: foo
  minReplicas: 2
  maxReplicas: 3
  metrics:
  - type: Resource
    resource:
      name: cpu
      target:
        type: Utilization
        averageUtilization: 80

I think the problem is that it tries to compare it with older autoscaling/v1 spec, where this property was in fact not allowed. In v2, this is a valid manifest.

Environment

• Windows
• Mac
• Linux
• other (please specify)

yaml-language-server version 1.21.0, used in integration with helm-ls

[datho7561]

Unfortunately, I can't replicate this problem. It seems to be working correctly for me.

[datho7561]

Wait, I was able to reproduce the bug. Try adding an extra newline at the end of the file. That got rid of the errors for me. Saving, then removing the newline makes the problem return. I don't know what's wrong, but it's specific to helm-ls (I can't recreate the problem when working with a non-helm kubernetes resource).

[datho7561]

See mrjosh/helm-ls#155 maybe?

[artem-nefedov]

I apologize for the confusion. Actually, just using plain document doesn't show the error, but I assumed that's because helm-ls doesn't send the document to yamlls at all in this case. I already have a newline at the end, tried adding more and that didn't change anything.

To get reproduction with helm-ls, there are 2 ways that consistently work for me:

1. Option 1: Add an actually incorrect field, e.g.:

apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: foo
spec:
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: foo
  minReplicas: 2
  maxReplicas: 3
  foo: bar
  metrics:
  - type: Resource
    resource:
      name: cpu
      target:
        type: Utilization
        averageUtilization: 80

In this case, yamlls complains that both "foo" and "metrics" are not allowed, but it should be only "foo".

2. Option 2: Add any inline template, e.g.:

apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: foo
spec:
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: foo
  minReplicas: 2
  maxReplicas: {{ 3 }}
  metrics:
  - type: Resource
    resource:
      name: cpu
      target:
        type: Utilization
        averageUtilization: 80

[datho7561]

Actually, just using plain document doesn't show the error

I've been able to reproduce the error appearing with a plain document

but I assumed that's because helm-ls doesn't send the document to yamlls at all in this case.

At least for VS Code, yaml-language-server isn't registered to work on Helm documents at all, since the parser will choke on any Helm templating that's present. Even if the file is a Helm template with no specific Helm syntax, the language server won't activate, because yaml-language-server has no way of identifying Helm template sections.

[artem-nefedov]

in integration with helm-ls, yaml-language-server also doesn't activate by itself for helm manifests. Instead, helm-ls will first render the template and then send resulting document to yamlls. I assumed (possibly incorrectly) that it was not doing that for the plain document.

At the same time, when the error does happen, it clearly comes from yamlls, not helm-ls itself. I don't understand how helm-ls can influence the parsing of yamlls in a way that result specifically in detecting allowed field as not allowed. Maybe by setting # yaml-language-server: comment in the rendered document? But I haven't found any evidence that it does that.

BTW, I confirmed that it does allow older field (removed in v2) "targetCPUUtilizationPercentage" while not allowing "metrics", so the problem is 100% caused by it using older v1 schema for validation instead of v2.

[datho7561]

I'm going to read helm-ls and try and understand what it's doing to reuse components of yaml-language-server

[artem-nefedov]

Update: I can sort-of reproduce this without helm-ls, just using yaml-language-server with yaml.schemas.kubernetes = ["*.yaml"] in config. It works using the method with incorrect property from #1213 (comment)

For example, with this manifest it complains that both "foo" and "metrics" are not allowed. If I remove "foo", it suddenly stops complaining about "metrics" too.

apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: foo
spec:
  foo: bar
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: foo
  minReplicas: 2
  maxReplicas: 3
  metrics:
  - type: Resource
    resource:
      name: cpu
      target:
        type: Utilization
        averageUtilization: 80

[datho7561]

Okay, I can reproduce this too. I'll take a closer look.

[datho7561]

I figured it out: our implementation doesn't use the declared Group Version Kind (GVK) to select the correct schema.

It does this instead:

• Uses this massive schema as the schema: https://github.com/yannh/kubernetes-json-schema/blob/master/v1.32.5-standalone/all.json
• Uses custom logic to pick which of the "oneOf" cases is used for validation. Normally, the first one without errors is selected, but that causes problems when selecting the schema here, since some resource types allow arbitrary entries in the spec, which means everything is valid. So instead, a bunch of heuristics are used to identify the "most likely match". None of these heuristics involves actually checking the declared Group Version Kind of the resource.

I think we should switch over to use Group Version Kind instead.

This problem is less visible in VS Code, where if you're working with Kubernetes, you're likely to have installed Microsoft's Kubernetes extension, which provides it's own, better implementation for Kubernetes schema resolution.