kubernetes-sigs/kubectl-validate

Resources not validated in some cases when missing .yaml extension

jjsiv opened this issue · 7 comments

jjsiv commented

What happened?

Resources sometimes will not be validated if .yaml or .yaml extension is missing on the file. This doesn't happen with all files, might be related to file size.

What did you expect to happen?

Resources are either validated properly or an error is returned indicating why they couldn't be validated. Currently it will always pass without any validation performed.

How can we reproduce it (as minimally and precisely as possible)?

I've noticed this happening with resources generated from KEDA chart. Run:
helm template keda kedacore/keda --version 2.13.2 > manifests.yaml
Make an error in any of the resources. In my cause I've just removed a letter. Run kubectl-validate:
k validate manifests.yaml --output=json
This will return an error as expected:

{
    "manifests.yaml": [
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Failure",
            "message": " \"\" is invalid: spec.template.spec.containers[0].livenessProb: Invalid value: value provided for unknown field",
            "reason": "Invalid",
            "details": {
                "causes": [
                    {
                        "reason": "FieldValueInvalid",
                        "message": "Invalid value: value provided for unknown field",
                        "field": "spec.template.spec.containers[0].livenessProb"
                    }
                ]
            },
            "code": 422
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        },
        {
            "metadata": {},
            "status": "Success"
        }
    ]
}
Error: validation failed

Now rename the file to just manifests:
mv manifests.yaml manifests
Run kubectl-validate again:
k validate manifests --output=json
This will pass with output suggesting that there is only one resource in the file:

{
    "manifests": [
        {
            "metadata": {},
            "status": "Success"
        }
    ]

Now, as a test, create a single deployment, make an error in it and try to validate it in the same way:
k create deployment test --image=nicolaka/netshoot --dry-run=server -o yaml > deployment.yaml
I've just added imagePullSecrets in the wrong place as a test:
k validate deployment.yaml --output=json

{
    "deployment.yaml": [
        {
            "metadata": {},
            "status": "Failure",
            "message": " \"\" is invalid: spec.template.spec.containers[0].imagePullSecrets: Invalid value: value provided for unknown field",
            "reason": "Invalid",
            "details": {
                "causes": [
                    {
                        "reason": "FieldValueInvalid",
                        "message": "Invalid value: value provided for unknown field",
                        "field": "spec.template.spec.containers[0].imagePullSecrets"
                    }
                ]
            },
            "code": 422
        }
    ]
}
Error: validation failed

This fails as expected. Let's try to remove the extension from file:
mv deployment.yaml deployment
Run kubectl-validate again:
k validate deployment --output=json
Here it also fails as expected:

{
    "deployment": [
        {
            "metadata": {},
            "status": "Failure",
            "message": " \"\" is invalid: spec.template.spec.containers[0].imagePullSecrets: Invalid value: value provided for unknown field",
            "reason": "Invalid",
            "details": {
                "causes": [
                    {
                        "reason": "FieldValueInvalid",
                        "message": "Invalid value: value provided for unknown field",
                        "field": "spec.template.spec.containers[0].imagePullSecrets"
                    }
                ]
            },
            "code": 422
        }
    ]
}
Error: validation failed

Anything else we need to know?

No response

Kubernetes version

$ kubectl version
# paste output here
jjsiv commented

Ok, now looking at the code I see that it does indeed just check whether either .yaml or .yml extension is present:
https://github.com/kubernetes-sigs/kubectl-validate/blob/main/pkg/utils/files.go#L9

Though I still believe that if this is the case an error should be returned when that extension is missing rather than validating only the first document from the top.

alexzielenski commented

Correct, we can only decide to split the documents in the YAML file if we know it is YAML ahead of time, and as you pointed out if we dont see a yaml extension the document is validated as a whole. In the case of multiple document YAML file only the first error is returned:

kubectl-validate/pkg/cmd/validate.go

Lines 254 to 272 in 25f6de4

if utils.IsYaml(filePath) {
documents, err := utils.SplitYamlDocuments(fileBytes)
if err != nil {
return []error{err}
}
var errs []error
for _, document := range documents {
if utils.IsEmptyYamlDocument(document) {
errs = append(errs, nil)
} else {
errs = append(errs, ValidateDocument(document, resolver))
}
}
return errs
} else {
return []error{
ValidateDocument(fileBytes, resolver),
}
}

I agree this led to confusing behavior from the tool, and I am leaning towards a file extension whitelist

jjsiv commented

Wondering now, do we need to make that decision to split the documents or not instead of always doing it? I did just try it without the conditional at all and it seems to work as expected, but perhaps there's something I'm missing

k8s-triage-robot commented

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot commented

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

k8s-ci-robot commented

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.