gardener / gardener

Homogeneous Kubernetes clusters at scale on any infrastructure using hosted control planes.
https://gardener.cloud
Apache License 2.0
2.93k stars 480 forks source link

Error patching ControllerDeployment #10267

Open dimityrmirchev opened 3 months ago

dimityrmirchev commented 3 months ago

How to categorize this issue?

/area control-plane /kind bug

What happened: kubectl apply -f for ControllerDeployment fails.

What you expected to happen: The apply command to succeed.

How to reproduce it (as minimally and precisely as possible):

Get the spec for ControllerDeployment from https://github.com/gardener/gardener-extension-provider-aws/blob/83233144f83d88221aaa4a3a724b160ae2a39d4d/example/controller-registration.yaml

apiVersion: core.gardener.cloud/v1
kind: ControllerDeployment
metadata:
  name: provider-aws
helm:
  rawChart: ...
  values:
    image:
      tag: v1.57.0-dev
k apply -f depl.yaml
controllerdeployment.core.gardener.cloud/provider-aws created

Change .helm.values.image.tag to "v1.57.0-dev-1".

k apply -f depl.yaml
warning: error calculating patch from openapi v3 spec: unable to find api field "image"
Error from server: error when applying patch:
{"helm":{"values":{"image":{"tag":"v1.57.0-dev-1"}}},"metadata":{"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"core.gardener.cloud/v1\",\"helm\":{\"rawChart\":\"...\",\"values\":{\"image\":{\"tag\":\"v1.57.0-dev-1\"}}},\"kind\":\"ControllerDeployment\",\"metadata\":{\"annotations\":{},\"name\":\"provider-aws\"}}\n"}}}
to:
Resource: "core.gardener.cloud/v1, Resource=controllerdeployments", GroupVersionKind: "core.gardener.cloud/v1, Kind=ControllerDeployment"
Name: "provider-aws", Namespace: ""
for: "depl.yaml": error when patching "depl.yaml": unable to find api field in struct JSON for the json field "image

This log is also observed from the gardener-apiserver container.

Anything else we need to know?:

Environment:

rfranzke commented 3 months ago

cc @timebertt @maboehm

maboehm commented 3 months ago

I was able to reproduce it, but I probably won't have time to fix / investigate until the end of next week. Seems like some APIServer does not like to patch *apiextensionsv1.JSON

acumino commented 2 months ago

Since apiextensionsv1.JSON requires a raw JSON format, it should be updated as below in the yaml file -

apiVersion: core.gardener.cloud/v1
kind: ControllerDeployment
metadata:
  name: provider-aws
helm:
  rawChart: <chart>
  values: |
    {
      "image": {
        "tag": "v1.57.0-dev-1"
      }
    }

or we need to change the value type of the Value in HelmControllerDeployment.

Nuckal777 commented 2 months ago

I just hit this bug, it's counterintuitive. Interestingly, kubectl edit allows changing the YAML representation. What speaks against specifying values as map[string]any?

Setting the values as a JSON object doesn't work well together with certain values, e.g. imageVectorOverwrite, which expects a YAML object.

maboehm commented 2 months ago

The issue is caused by strategic merge patching. Thats why kubectl edit work, and so does kubectl apply --server-side. There were similar issues for Helm in the past: https://github.com/helm/helm/issues/3382

maboehm commented 2 months ago

So I finally got a chance to really dig into it. I'd say this is not really a bug, but we could do better.

kubectl apply attempts to "guess" if it can "strategic merge patch" (SMP) or if it has to fallback to "merge patch". For CRDs, SMP is not supported, so the type *apiextensionsv1.JSON works fine there (and is used e.g. by flux HelmReleases). But ControllerDeployments pass that check, so it attempts a SMP, but fails when looking up the patch metadata. You can see that when increasing the verbosity (-v 7) when running kubectl apply.

SMP fails, because the JSON type defines no type, and only has a description in the OpenAPI spec.:

"io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.JSON": {
    "description": "JSON represents any valid JSON value. These types are supported: bool, int64, float64, string, []interface{}, map[string]interface{} and nil."
}

Upstream, this is explicitly configured by implementing OpenAPISchemaType() here. There is a TODO comment to at some point change this once anyOf is supported. (more on that later)

I tried to copy the type and exlicitly return []string{"object}. This results in a "free form object":

 "com.github.gardener.gardener.pkg.apis.core.v1.JSON": {
    "description": "JSON represents any valid JSON value. These types are supported: bool, int64, float64, string, []interface{}, map[string]interface{} and nil.",
    "type": "object"
},

With that, kubectl apply correctly falls back to application/merge-patch+json, and kubectl patch still does not work. Server-side apply still works as well. By the way, in the OpenAPI spec, io.k8s.apimachinery.pkg.runtime.RawExtension is also a "free-form" object.

So what can we do? For g/g, we could just implement our own type, e.g. HelmValues, that is essentially apiextensionsv1.JSON, but says in the OpenAPI spec that it is an object. (This is probably even more correct, since we never want this field to be for example just a number). A bit annoying is that we need to repeat this type in core, corev1,corev1beta1,operatorv1alpha1 or need to find a place to define that type centrally.

We could also think about contributing this upstream. kube-openapi supports oneOf types since 2 years ago. And I believe even if the TODO comment says they are waiting for anyOf, I think oneOf is actually more correct (see docs), which can be provided by implementing OpenAPIV3OneOfTypes() []string

Long story short - we can help kubectl apply to correctly identify which patch method to use by either implementing our own type or contributing a change upstream.

ialidzhikov commented 3 weeks ago

In case someone is looking how to workaround this issue in an extension repo using skaffold based local setup: https://github.com/gardener/gardener-extension-registry-cache/pull/277.