kubernetes-client / csharp

Officially supported dotnet Kubernetes Client library
Apache License 2.0
1.1k stars 295 forks source link

KubernetesClient 7.x patching broken #772

Closed rogeralsing closed 2 years ago

rogeralsing commented 2 years ago

https://github.com/asynkron/protoactor-dotnet/issues/1416

After upgrading to KubernetesClient 7.0.8, we are seeing the following error returned from k8s when trying to patch pod labels:

{
    "kind": "Status",
    "apiVersion": "v1",
    "metadata": {},
    "status": "Failure",
    "message": "json: cannot unmarshal object into Go value of type jsonpatch.Patch", //<----- this error
    "reason": "BadRequest",
    "code": 400
}

After adding a delegating logger to the HttpClient, we are seeing this payload being sent:

{
    "operations": [
        {
            "value": {
                "label xyz":"value xyz"
            },
            "operationType": "Replace",         //<---- This entry is not part of the Patch spec
            "path": "/Metadata/Labels",
            "op": "replace"
        }
    ],
    "contractResolver": {}
}

Downgrading to KubernetesClient 6.0.8 works, and does not have this invalid extra entry:

{
    "operations": [
        {
            "value": {
                "label xyz":"value xyz"
            },
            "path": "/Metadata/Labels",
            "op": "replace"
        }
    ],
    "contractResolver": {}
}
tg123 commented 2 years ago

starting from 7.x, the sdk switched to system.text.json could you please check if your json patch library supports system.text.json?

rogeralsing commented 2 years ago

This is the code we use to make the call:

var patch = new JsonPatchDocument<V1Pod>(); //Microsoft.AspNetCore.JsonPatch
patch.Replace(x => x.Metadata.Labels, labels); 
await kubernetes.PatchNamespacedPodAsync(new V1Patch(patch, V1Patch.PatchType.JsonPatch), podName, podNamespace);

Is there anything we should change on our end here?

rogeralsing commented 2 years ago

image

Ok, so maybe this is a case of RTFM from us. Does this mean that using KubernetesClient 7.x+ on a v1.22 k8s cluster. should fail for the above reason? Or is it intended to be backwards compat?

rogeralsing commented 2 years ago

image

Our mistake, the OperationBase used by the asp.net patch infra uses Json.NET.

tg123 commented 2 years ago

you may want to take a look at https://www.nuget.org/packages/JsonPatch.Net/

here is test using it https://github.com/kubernetes-client/csharp/blob/8f4ee45cf38e572c56b76b684804381e13d4eac3/tests/E2E.Tests/MnikubeTests.cs#L121

Paciv commented 2 years ago

I also discovered the behavior change (serializer change from Newtonsoft to System.Text.Json). As specified here: https://docs.microsoft.com/en-us/aspnet/core/web-api/jsonpatch?view=aspnetcore-6.0#add-support-for-json-patch-when-using-systemtextjson "The System.Text.Json-based input formatter doesn't support JSON Patch" [sic]

Yes, the problem can be worked around by using another third party supporting it, but that can also be solved serializing with Newtonsoft before the k8s.csharp call:

var patch = new JsonPatchDocument<V1Pod>();
patch.ContractResolver = new DefaultContractResolver
{
    NamingStrategy = new CamelCaseNamingStrategy()
};
patch.Replace(x => x.Metadata.Labels, labels);
var patchString = Newtonsoft.Json.JsonConvert.SerializeObject(patch);
await kubernetes.PatchNamespacedPodAsync(new V1Patch(patchString, V1Patch.PatchType.JsonPatch), podName, podNamespace);

The ContractResolver is needed, because previously API object were decorated with Newtonsoft attributes containing the camelcase name, and it is now using System.Text.Json attributes.

Still, it is a bit sad to change the serializer on a Patch object to a serializer that doesn't support Json Patch.