skaffold labels erroneously added to CRD validation schema metadata field

[andsens]

I am trying to get cert-manager up and running with skaffold+kustomize, but am running into an issue with skaffold adding labels to a CRD manifest. kustomize build . | kubectl apply -f - works fine, so I
think this is skaffold itself. Either it modifies the manifest or kustomize gets some additional flags.

Expected behavior

The manifest validates.

Actual behavior

skaffold errors out with:

error: error validating "STDIN": error validating data: ValidationError(CustomResourceDefinition.spec.validation.openAPIV3Schema.properties.metadata): unknown field "labels" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.JSONSchemaProps; if you choose to ignore these errors, turn validation off with --validate=false

Information

• Skaffold version: v0.24.0
• Operating system: MacOS 10.14.3
• Contents of skaffold.yaml:

apiVersion: skaffold/v1beta6
kind: Config
deploy:
  kustomize:
    path: .

Steps to reproduce the behavior

1. skaffold run this gist.

Full log

$ skaffold run
Starting build...
Build complete in 40.014µs
Starting test...
Test complete in 7.814µs
Starting deploy...
kubectl client version: 1.13
error: error validating "STDIN": error validating data: ValidationError(CustomResourceDefinition.spec.validation.openAPIV3Schema.properties.metadata): unknown field "labels" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.JSONSchemaProps; if you choose to ignore these errors, turn validation off with --validate=false
Deploy complete in 644.45351ms
You can also run [skaffold run --tail] to get the logs

[andsens]

I'm having trouble figuring out what the actual manifest looks like. Is there a way to do a dry-run with skaffold and retrieve the generated manifest?

[tejal29]

@andsens. Right now, there isnt. But this is good feature to have.
I will track this request in another issue #1739
However, you can run skaffold deploy -v=debug to see the generated manifests on the stdout.

[andsens]

Yup, found the issue with -v=debug:

[...]
  validation:
    openAPIV3Schema:
      properties:
        apiVersion:
          description: 'APIVersion defines the versioned schema of this representation
            of an object. Servers should convert recognized schemas to the latest
            internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
          type: string
        kind:
          description: 'Kind is a string value representing the REST resource this
            object represents. Servers may infer this from the endpoint the client
            submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
          type: string
        metadata:
          labels:
            cleanup: "true"
            skaffold-builder: local
            skaffold-deployer: kustomize
            skaffold-tag-policy: git-commit
            tail: "true"
          type: object
        spec:
[...]

In the original it looks like this:

[...]
  validation:
    openAPIV3Schema:
      properties:
        apiVersion:
          description: 'APIVersion defines the versioned schema of this representation
            of an object. Servers should convert recognized schemas to the latest
            internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
          type: string
        kind:
          description: 'Kind is a string value representing the REST resource this
            object represents. Servers may infer this from the endpoint the client
            submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
          type: string
        metadata:
          type: object
        spec:
[...]

Full debug output.

[nkubala]

yeah, we probably shouldn't be doing this. we're using the k8s dynamic client to apply labels to all k8s objects created with skaffold, but this won't work for CRDs. we'll need to figure out a way to determine if an object was created with a CRD and skip labeling if it was.

[briandealwis]

I think the code that's doing this is ManifestList's SetLabels(), which is called from Kustomize:

https://github.com/GoogleContainerTools/skaffold/blob/163f2e893148b8e66be1ab503831f7d334799279/pkg/skaffold/deploy/kustomize.go#L133

This isn't the same as the labeller used in Helm.

[daniyalj]

running into the same issue. Im deploying rbac-manager with skaffold with the following:

$ cat skaffold.yaml 
apiVersion: skaffold/v1beta8
kind: Config
deploy:
  kubectl:
    manifests:
      - manifests/rbac-manager/01-rbac-manager.yaml

01-rbac-manager.yaml is here

error is:

clusterrole.rbac.authorization.k8s.io/rbac-manager unchanged
clusterrolebinding.rbac.authorization.k8s.io/rbac-manager unchanged
error: error validating "STDIN": error validating data: ValidationError(CustomResourceDefinition.spec.validation.openAPIV3Schema.properties.metadata): unknown field "labels" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.JSONSchemaProps; if you choose to ignore these errors, turn validation off with --validate=false
FATA[0000] deploy failed: kubectl apply: exit status 1 

[daniyalj]

Skaffold is creating labels in my crd:

kind: CustomResourceDefinition
metadata:
  labels:
    app: rbac-manager
    cleanup: "true"
    docker-api-version: "1.39"
    skaffold-builder: local
    skaffold-deployer: kubectl
    skaffold-tag-policy: git-commit
    tail: "true"
  name: rbacdefinitions.rbacmanager.reactiveops.io
spec:
  group: rbacmanager.reactiveops.io
  names:
    kind: RBACDefinition
    plural: rbacdefinitions
  scope: Cluster
  validation:
    openAPIV3Schema:
      properties:
        apiVersion:
          type: string
        kind:
          type: string
        metadata:
          labels:
            cleanup: "true"
            docker-api-version: "1.39"
            skaffold-builder: local
            skaffold-deployer: kubectl
            skaffold-tag-policy: git-commit
            tail: "true"
          type: object
        rbacBindings:
          items:
            properties:

I am not using Kustomize

[adrienjt]

FWIW, this is a regression. No problem with v0.14.0.

(I've had no reason to upgrade for a long time, decided to upgrade to v0.29.0 to avoid too many changes at once when I do need to upgrade... no luck, I'm going to stick with v0.14.0 a little longer.)

[nkubala]

turns out this is happening because skaffold is adding labels to all sections in any manifest that use metadata as the key, including those in the validation section for a CRD. obviously this is bad because we should never be touching the CRD validation section.

a fix for this would be checking for validation sections when we're recursively parsing the manifest, and skipping those sections outright since we shouldn't be modifying them. PRs welcome as this should be a pretty easy fix, or else I'll try and get to it next week.

[tejal29]

I have a PR out for this which is in draft right now. https://github.com/GoogleContainerTools/skaffold/pull/2060

[afirth]

borks kubebuilder :(

[chainlink]

Looks like this is stalled

[tejal29]

@chainlink yes. i will try to resurrect this this week.

[tejal29]

re-opening,
Since implementation is not yet done.

[zmarouf]

Hitting the same issue. Is there a way to just not add skaffold labels at all?
Edit:
Right now I'm doing this to circumvent the kustomize issue but it means skaffold doesn't manage my deployments :(

❯ skaffold build -q -o build.yaml
❯ cat build.yaml | jq '{images: [.builds[] | {name: .imageName, newName: .imageName, newTag: .tag | split(":")[1]}]}' | yq r - > images.yaml # stedolan/jq
❯ yq merge kustomization.yaml images.yaml -x -i # mikefarah/yq
❯ kustomize apply .

[kunickiaj]

Is this coming any closer to resolution?

[mgoltzsche]

With the logic that is currently in place a Replacer could be added that excludes object containing kind: CustomResourceDefinition using a corresponding Matcher. However this allows only to exclude CRDs from the transformation completely - not only some parts of it - while it should still be labeled. Also currently the Matcher must be added to every Replacer although so far there does not seem to be a need to have a different exclusion/preservation rule per Replacer impl (labelsSetter, imageSaver, imageReplacer, namespaceCollector). This behaviour should depend on the resource type. Matching key names anywhere within any resource is rather fuzzy.
Hence I suggest to move the exclusion functionality into a resource type dependent traversal strategy. This would allow the precise definition of exclusion rules and not traverse parts that should be kept untouched. A CRD-specialized implementation could be selected for each manifest by kind and implemented so that it simply traverses the first-level fields only to set the labels on the CRD while leaving the spec with the schema untouched. The Replacer interface could be a more generally called FieldVisitor and its methods reduced to Visit which may aggregate or transform a received object.

[rmmcnulty9]

On v1.4.0 I am still seeing this problem when trying to deploy KEDA ScaledObjects (https://keda.sh/scalers/rabbitmq-queue/). Their CRD structure has a metadata tag that Skaffold wants to put labels on which is invalid for the CRD.

The skaffold error message is:

 - spec.triggers.metadata.labels in body must be of type string: "object"

[rmmcnulty9]

Update: This also happens on v1.5.0

Should this issue be reopened or should I submit a new issue?

[mgoltzsche]

Currently, in case the resource is not a CRD, the transforming FieldVisitor is wrapped into a decorator that applies it recursively.
To make this more reliable it should be wrapped/recursive only when the resource's kind matches one of a predefined set of kinds that require the recursive transformations.

[mgoltzsche]

@rmmcnulty9 I've created a corresponding PR #3833.