Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KubernetesClient 7.x patching broken #772

Closed
rogeralsing opened this issue Feb 3, 2022 · 6 comments
Closed

KubernetesClient 7.x patching broken #772

rogeralsing opened this issue Feb 3, 2022 · 6 comments

Comments

@rogeralsing
Copy link

rogeralsing commented Feb 3, 2022

asynkron/protoactor-dotnet#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
Copy link
Member

tg123 commented Feb 3, 2022

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
Copy link
Author

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
Copy link
Author

rogeralsing commented Feb 3, 2022

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
Copy link
Author

image

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

@tg123
Copy link
Member

tg123 commented Feb 3, 2022

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

here is test using it

var newlabels = new Dictionary<string, string>(pod.Metadata.Labels) { ["test"] = "test-jsonpatch" };

@tg123 tg123 pinned this issue Mar 14, 2022
@tg123 tg123 changed the title KubernetesClient 7.x patching Labels broken KubernetesClient 7.x patching broken Mar 31, 2022
@Paciv
Copy link
Contributor

Paciv commented Apr 18, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants