content-type for patch commands #959

tomplus · 2019-09-22T01:06:45Z

Hi all.

In this PR I'd like to discuss possible solution for problems with selecting content-type when K8s' objects are patching. This problem is nicely documented in #866 and tomplus/kubernetes_asyncio#68

This idea is to introduce a new parameter in ApiClient to be able to control a return from the select_header_content_type method and remove the problematic detection from rest.py. Optionally we can add this as an argument to each patch method.
It'll be breaking change if an application uses patch_* methods.

Please take a look. If we accept the solution I prepare PR in openapi-generator and temporary patch for this repo.

cc: @roycaihw @yliaog @HaraldGustafsson @nolar

Thanks.

tomplus · 2019-09-22T01:09:36Z

kubernetes/e2e_test/test_client.py

@@ -55,9 +55,12 @@ class TestClient(unittest.TestCase):
    def setUpClass(cls):
        cls.config = base.get_e2e_configuration()

+    def setUp(self):
+        self.client = api_client.ApiClient(configuration=self.config,
+                                           prefered_content_type="application/strategic-merge-patch+json")


We can use it as a default to make it backward compatible.

Assume you meant to have it as default in the ApiClient?

IIUC, defaulting to one content type doesn't make it backward compatible, since the old behavior was picking one of the two content types based on the body type. We have to keep the old behavior if we want to avoid breaking the old clients

IMO backward compatibility is not possible, because current behavior doesn't work for custom resources. If we assume that strategic-merg-patch is more common we can set is as a default. Other clients will have to change the preferred content type only...

IMO backward compatibility is not possible, because current behavior doesn't work for custom resources

the current behavior allows JSON patch for custom resources, right? Can we keep the following behavior if prefered_content_type is None?

for native types, a client can send json-patch+json if the body is a list, or strategic-merge-patch+json if the body is a map

for custom resources, a client can send merge-patch+json only

In future, we need to allow a client to send merge-patch+json and apply-patch+yaml for both native types and custom resources as well

Sure, I prepare changes for such approach.

I see one drawbacks - we'll keep the "magic" logic in rest.py and it won't be such generic to add it to the open-generator. But let's discuss it later with the code.

Thanks.

if the select_header_content_type takes the body and method as well then it can be intelligent about it's choice. We should be prepared for that custom resources will support strategic merge in the future when such strategy is defined in the CRD. So it is still a good idea to introduce the option to set the preferred type.

Note that the previous supported type for custom resources was application/merge-patch+json and not json-patch+json which was newly added. So if you should have a default for custom objects you should select application/merge-patch+json if you want backward compatibility.

Note that the previous supported type for custom resources was application/merge-patch+json and not json-patch+json which was newly added. So if you should have a default for custom objects you should select application/merge-patch+json if you want backward compatibility

Ah yes, you're right. Thanks!

Now I’m leaning towards to expose content_type to each request (for patch mainly) to bypass the select_content_type function. This function now accepts "method" and "body" to try to select the better header. K8s introduces strategic-merge-patch which is k8s specific and this part has to be patched - instead of patching rest.py. Let me know what you think. Thanks.

roycaihw · 2019-09-23T20:03:12Z

kubernetes/e2e_test/test_client.py

@@ -55,9 +55,12 @@ class TestClient(unittest.TestCase):
    def setUpClass(cls):
        cls.config = base.get_e2e_configuration()

+    def setUp(self):
+        self.client = api_client.ApiClient(configuration=self.config,
+                                           prefered_content_type="application/strategic-merge-patch+json")


IIUC, defaulting to one content type doesn't make it backward compatible, since the old behavior was picking one of the two content types based on the body type. We have to keep the old behavior if we want to avoid breaking the old clients

roycaihw · 2019-09-23T20:14:31Z

examples/custom_object.py

-    api = client.CustomObjectsApi()
+    # passing prefered_content_type to choose the required type of patch
+    api_client = client.ApiClient(prefered_content_type="application/merge-patch+json")
+    api = client.CustomObjectsApi(api_client)


What if someone wants to send different types of patch in their client? Is it possible to expose an argument in each API to specify the content type for each request?

Sure, we can add it to each patch method but I thought that something like the default-content-type could help to write more consistent code. I've never mixed type of patch in my code yet.

I'm okay with either way

I've dropped preferred_content _type option - it didn't support situation where different type of methods have different valid content types.

kubernetes/client/api_client.py

tomplus · 2019-10-28T07:43:34Z

examples/custom_object.py

+        namespace="default",
+        plural="crontabs",
+        body={'image': 'my-awesome-cron-image:2'},
+        content_type="application/merge-patch+json"


This parameter is not necessary - select_header_content_type can manage with it.

Custom resources support JSON patch, so this parameter can be used to hint JSON patch / JSON merge patch, right?

Yes, exactly.

What about application/apply-patch+yaml? Why aren't we allowing to override from the caller?

HaraldGustafsson · 2019-10-28T07:52:49Z

kubernetes/client/apis/custom_objects_api.py

-        header_params['Content-Type'] = self.api_client.\
-            select_header_content_type(['application/merge-patch+json'])
+        header_params['Content-Type'] = params.get('content_type', self.api_client.\
+            select_header_content_type(['application/merge-patch+json']), 'PATCH', body_params)


This all started due to that we added 'application/json-patch+json' for custom objects, so should that not be part of the list of available content types?

Also assuming this is an example on the changes to all the patch methods, since there are several more in this file.

Yes, should be but this PR uses current code from master branch (https://github.com/kubernetes-client/python/blob/master/kubernetes/client/apis/custom_objects_api.py#L1659). AFAIK @roycaihw removed this option when the problem appeared.

Yes, I use this PR to discuss final solution and then I will change the openapi-generator to add this to all methods.

I removed 'application/json-patch+json' from custom objects because the existing select_header_content_type couldn't support it without breaking old clients. With this change from @tomplus, we should be able to add 'application/json-patch+json' for custom objects

tomplus · 2019-11-08T14:55:35Z

@roycaihw Could you take a look at the last concept? I'd like to prepare generator to manage with it. Thanks.

fejta-bot · 2020-02-06T15:33:25Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

tomplus · 2020-02-06T15:39:17Z

/remove-lifecycle stale

roycaihw

@tomplus Sorry for the late response. I have some small questions to clarify. Overall I think the interface looks good.

roycaihw · 2020-02-18T20:15:08Z

examples/custom_object.py

+        namespace="default",
+        plural="crontabs",
+        body={'image': 'my-awesome-cron-image:2'},
+        content_type="application/merge-patch+json"


Custom resources support JSON patch, so this parameter can be used to hint JSON patch / JSON merge patch, right?

roycaihw · 2020-02-18T20:16:38Z

kubernetes/client/api_client.py

+                    if 'application/strategic-merge-patch+json' in content_types:
+                        return 'application/strategic-merge-patch+json'
+                    # ---
+                    return 'application/merge-patch+json'


Most native types support both strategic merge patch and JSON merge patch. Just to clarify, if a client wants to send JSON merge patch, it has to use the content_type parameter to hint, right?

Yes, it's hard to detect which type of patch has been sent so it's better to expose content_type explicit.

roycaihw · 2020-02-18T20:20:08Z

kubernetes/client/apis/core_v1_api.py

-        header_params['Content-Type'] = self.api_client.\
-            select_header_content_type(['application/json-patch+json', 'application/merge-patch+json', 'application/strategic-merge-patch+json'])
+        header_params['Content-Type'] = params.get('content_type', self.api_client.\
+            select_header_content_type(['application/json-patch+json', 'application/merge-patch+json', 'application/strategic-merge-patch+json'], 'PATCH', body_params))


Kubernetes recently added a new type application/apply-patch+yaml. However it's likely hard to auto-detect the yaml type based on the body

roycaihw · 2020-02-18T20:23:58Z

kubernetes/client/apis/custom_objects_api.py

-        header_params['Content-Type'] = self.api_client.\
-            select_header_content_type(['application/merge-patch+json'])
+        header_params['Content-Type'] = params.get('content_type', self.api_client.\
+            select_header_content_type(['application/merge-patch+json']), 'PATCH', body_params)


I removed 'application/json-patch+json' from custom objects because the existing select_header_content_type couldn't support it without breaking old clients. With this change from @tomplus, we should be able to add 'application/json-patch+json' for custom objects

fejta-bot · 2020-05-18T20:28:28Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

tomplus · 2020-05-18T23:43:41Z

/remove-lifecycle stale

lizfeed · 2020-06-15T20:50:42Z

Any update on this? this would be really handy for me.

Reference: - [kubernetes-client#866](kubernetes-client#866) - [kubernetes-client#959](kubernetes-client#959) Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>

tomplus · 2022-06-07T21:19:44Z

/remove-lifecycle stale

k8s-triage-robot · 2022-09-05T21:47:55Z

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 · 2022-10-05T22:05:37Z

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

spolcyn · 2022-10-05T22:56:08Z

/remove-lifecycle rotten

k8s-triage-robot · 2023-01-03T23:00:47Z

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 · 2023-02-02T23:46:19Z

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

aslafy-z · 2023-02-02T23:52:43Z

/remove-lifecycle rotten

k8s-triage-robot · 2023-05-04T00:17:58Z

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

This bot triages 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 PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

aslafy-z · 2023-05-04T00:29:47Z

So, we switched to golang..

k8s-triage-robot · 2023-06-03T00:37:39Z

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

This bot triages 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 PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this 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 · 2023-07-03T00:52:32Z

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

This bot triages 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 PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close

k8s-ci-robot · 2023-07-03T00:52:34Z

@k8s-triage-robot: Closed this PR.

In response to this:

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

This bot triages 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 PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

/close

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.

arcivanov · 2023-07-03T00:59:26Z

/reopen

k8s-ci-robot · 2023-07-03T00:59:28Z

@arcivanov: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

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.

Reference: - [#866](kubernetes-client/python#866) - [#959](kubernetes-client/python#959) Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 22, 2019

k8s-ci-robot requested review from roycaihw and yliaog September 22, 2019 01:06

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 22, 2019

tomplus commented Sep 22, 2019

View reviewed changes

roycaihw reviewed Sep 23, 2019

View reviewed changes

kubernetes/client/api_client.py Outdated Show resolved Hide resolved

kubernetes/client/api_client.py Outdated Show resolved Hide resolved

roycaihw assigned yliaog Sep 24, 2019

roycaihw mentioned this pull request Oct 1, 2019

patch_namespaced_config_map merge-patch #968

Closed

tomplus commented Oct 28, 2019

View reviewed changes

HaraldGustafsson reviewed Oct 28, 2019

View reviewed changes

tomplus mentioned this pull request Dec 4, 2019

UNKNOWN_BASE_TYPE in documentation for patch_* methods #1020

Closed

Datamance mentioned this pull request Dec 5, 2019

Can't replicate kubectl apply -f functionality with Python client #1028

Closed

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 6, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 6, 2020

roycaihw reviewed Feb 18, 2020

View reviewed changes

roycaihw mentioned this pull request Apr 9, 2020

Add example about using server-side apply with this client #1093

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 18, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 18, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 7, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 5, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 5, 2022

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 5, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 3, 2023

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 2, 2023

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 2, 2023

tomplus mentioned this pull request Apr 11, 2023

add JSON Patch support for namespaced custom objects #2040

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 4, 2023

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 3, 2023

k8s-ci-robot closed this Jul 3, 2023

content-type for patch commands #959

content-type for patch commands #959

Conversation

tomplus commented Sep 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roycaihw Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomplus commented Nov 8, 2019

fejta-bot commented Feb 6, 2020

tomplus commented Feb 6, 2020

roycaihw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented May 18, 2020

tomplus commented May 18, 2020

lizfeed commented Jun 15, 2020

tomplus commented Jun 7, 2022

k8s-triage-robot commented Sep 5, 2022

k8s-triage-robot commented Oct 5, 2022

spolcyn commented Oct 5, 2022

k8s-triage-robot commented Jan 3, 2023

k8s-triage-robot commented Feb 2, 2023

aslafy-z commented Feb 2, 2023

k8s-triage-robot commented May 4, 2023

aslafy-z commented May 4, 2023

k8s-triage-robot commented Jun 3, 2023

k8s-triage-robot commented Jul 3, 2023

k8s-ci-robot commented Jul 3, 2023

arcivanov commented Jul 3, 2023

k8s-ci-robot commented Jul 3, 2023

roycaihw Sep 23, 2019 •

edited

Loading