javascript icon indicating copy to clipboard operation
javascript copied to clipboard

Patch fails on custom resouce with UnsupportedMediaType error

Open dominic-p opened this issue 2 years ago • 10 comments

Describe the bug When I try to run a patch operation on a custom resource, I get an error like this:

the body of the request was in an unknown format - accepted media types include: application/json-patch+json, application/merge-patch+json, application/apply-patch+yaml

Some Googling took me to this issue which indicates that the solution is to use a different patch type for custom resources. It looks like right now the strategic merge patch is the default for all patch requests.

Of course, I can override the headers which is great. But, could that code be made "smarter" to recognize if we are patching a custom resource and then default to a different patch strategy?

Client Version 0.16.3

Server Version 1.23.7

dominic-p avatar Jul 03 '22 05:07 dominic-p

@dominic-p can you send an example of the code that you are using? I'm not sure if there is a way to make this more automatic but we can look into it.

brendandburns avatar Jul 05 '22 15:07 brendandburns

Thanks for the response. My code right now is pretty custom. I'm using a function like the apply example to deploy YAML to the cluster. When I call that function with a Custom Resource (in my case a ClusterIssuer from cert-manager), I simply use the headers override on the patch function to use a different content-type.

I'm still pretty new to this code base (and kubernetes in general), so I'm not sure what would be involved in this. In my mind, if there were a magic function like isCustomResource() that could look at a spec and return true or false, then we could simply check that on the patch function and chose a default content-type header appropriately.

If that function doesn't exist or would be brittle/hard to write then this probably doesn't make a lot of sense to attempt.

dominic-p avatar Jul 05 '22 16:07 dominic-p

Thanks for the response. My code right now is pretty custom. I'm using a function like the apply example to deploy YAML to the cluster. When I call that function with a Custom Resource (in my case a ClusterIssuer from cert-manager), I simply use the headers override on the patch function to use a different content-type.

I'm still pretty new to this code base (and kubernetes in general), so I'm not sure what would be involved in this. In my mind, if there were a magic function like isCustomResource() that could look at a spec and return true or false, then we could simply check that on the patch function and chose a default content-type header appropriately.

If that function doesn't exist or would be brittle/hard to write then this probably doesn't make a lot of sense to attempt.

For me as a user, I think API functions for patching custom resource / subresources better off encapsulating the precise patch types and ruling out strategyMerge type in dealing with CR.

It looks that many API functions in the package were designed to let users have more control over the behaviours.

So in this case I think users could maintain the general api group/version and realize the isCustomResource() function.

pillumina avatar Jul 06 '22 01:07 pillumina

Thanks for the details!

For context isCustomResource() would be tricky to write correctly. It is impossible structurally to tell the difference between a CRD and a built in type based on group/apiVersion

The only real way to do it likely would be to check in a file containing all built-in versions, but that file doesn't actually exist anywhere so it would have to be manually maintained.

Perhaps one option would option might be to add a class like PatchUtils in the Java client which could abstract some of the patching logic and make it easier to have better error messages in the case where you hit this problem.

I will think on it some more, and in the meantime, we'll leave this issue so hopefully it's easier for the next person to find.

brendandburns avatar Jul 06 '22 03:07 brendandburns

Thanks for the explanation; I was kind of afraid of that. The only thought I had was looking at the gen/model dir. That almost looks like a complete list of built in types. Perhaps that could be leveraged somehow? Like I said, I'm still pretty new to this, so feel free to disregard this idea if it's way off base.

dominic-p avatar Jul 06 '22 16:07 dominic-p

@dominic-p yeah, that's what the Java client does (https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/util/ModelMapper.java)

The main problem is that the api group/version isn't actually written in any of those gen/model files. You can infer it from the name of the class (mostly), so it would be possible to write a pre-processing script that would scrape all of the file names and then generate the list of built-in API group/versions.

The Java client uses reflection to do this at runtime, but unfortunately I don't think that's feasible in Typescript.

brendandburns avatar Jul 07 '22 14:07 brendandburns

Yikes, ok, yeah, that's not pretty. :) I think you're right this is probably a non starter.

Last question though: what about the swagger.json file? The definitions key looks promising. It feels like since so much of the code is auto generated we could hook into that generation process and output what we need. But, it's definitely starting to look like too much work to be worth it at this point.

dominic-p avatar Jul 07 '22 17:07 dominic-p

Yeah, in theory we could either scrape/generate from the swagger file, or we could modify the existing generator. (https://github.com/OpenAPITools/openapi-generator) we're already in the process of (slowly) switching generators from one that uses request to one that uses fetch so it may not be the right time, unless we decide to write our own generator (which we did in the csharp client, fwiw)

brendandburns avatar Jul 10 '22 00:07 brendandburns

Got it. Yeah, I think the win just isn't big enough to justify the work. I'll make a quick PR to add some documentation to the apply example. Maybe that will help others prepare for this easier.

dominic-p avatar Jul 11 '22 19:07 dominic-p

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

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

/lifecycle stale

k8s-triage-robot avatar Oct 09 '22 19:10 k8s-triage-robot

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

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

/lifecycle rotten

k8s-triage-robot avatar Nov 08 '22 20:11 k8s-triage-robot

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-triage-robot avatar Dec 08 '22 20:12 k8s-triage-robot

@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/test-infra repository.

k8s-ci-robot avatar Dec 08 '22 20:12 k8s-ci-robot

check raw example : https://github.com/kubernetes-client/javascript/blob/0.18.0/examples/raw-example.js

abdennour avatar Feb 13 '23 02:02 abdennour