Fix Content negotiation incorrect when Accept header uses type parame… #50603

shiywang · 2017-08-14T09:13:08Z

Fixes #50519
@smarterclayton @liggitt still wip, I'll add some unit test soon, and simplify the logic

sttts · 2017-08-14T09:46:16Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go

@@ -282,10 +310,16 @@ func NegotiateMediaTypeOptions(header string, accepted []AcceptedMediaType, endp
 				clause.Type == "*" && clause.SubType == "*":
 				// TODO: should we prefer the first type with no unrecognized options?  Do we need to ignore unrecognized


the comment can go , can't it?

oh, now I got it, thank you, yes, will do that.

smarterclayton · 2017-08-14T22:31:49Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go

@@ -273,19 +300,29 @@ func NegotiateMediaTypeOptions(header string, accepted []AcceptedMediaType, endp
 	}

 	clauses := goautoneg.ParseAccept(header)
+	candidateMediaTypeList := []candidateMediaType{}


make this a var candidates []candidateMediaType array so that an allocation is not required in all cases.

smarterclayton · 2017-08-14T22:33:23Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go

+			return retVal, true
+		}
+		//remove element
+		candidateMediaTypeList = append(candidateMediaTypeList[:index], candidateMediaTypeList[index+1:]...)


It would be better to swap index with the last item, then truncate the array by one. This code will be called on every request and some efficiency matters.

smarterclayton · 2017-08-14T22:36:17Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go

+}
+
+func mostSpecificMediaType(l []candidateMediaType) (candidateMediaType, int) {
+	max := struct {


don't use a struct for this when two variables are sufficient

smarterclayton · 2017-08-14T22:36:50Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go

+	clauses  goautoneg.Accept
+}
+
+func mostSpecificMediaType(l []candidateMediaType) (candidateMediaType, int) {


just return int, returning a struct is not necessary.

krzyzacy · 2017-08-16T01:09:54Z

/retest

shiywang · 2017-08-16T02:12:50Z

@smarterclayton updated, ptal

I think this can be solved correctly by having the delegation in the loop

I'm not quit understand that delegation in the loop things, do you mean using delegation pattern will make code more elegant ?

smarterclayton · 2017-08-16T02:32:50Z

I don't remember, may have edited the comment after reading the code more.

smarterclayton · 2017-08-16T02:37:20Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go

+			return retVal, true
+		}
+		// swap index with the last item, then truncate the array by one.
+		candidates[index], candidates[len(candidates)-1] = candidates[len(candidates)-1], candidates[index]


oops, "swap" was incorrect. You can take the last array item and assign it to the current location. Then truncate.

If we don't accept index, then we will never accept index and so it doesn't need to be preserved.

shiywang · 2017-08-18T10:56:52Z

@smarterclayton updated, how about this ?

shiywang · 2017-08-29T03:11:47Z

@smarterclayton @sttts mind take a look ?

sttts · 2017-08-29T06:43:31Z

@smarterclayton can you take another look?

liggitt · 2017-08-29T15:45:49Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go

+func (emptyEndpointRestrictions) AllowsConversion(gvk schema.GroupVersionKind) bool {
+	if gvk.GroupVersion() == metav1alpha1.SchemeGroupVersion {
+		switch gvk.Kind {
+		case "Table", "PartialObjectMetadata", "PartialObjectMetadataList":


hmm, I didn't expect to see specific types here

actually, isn't the point of emptyEndpointRestrictions to avoid matching any specific type request?

I think the reason I add specific types here is same as here: (a temporary solution )

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

Line 83 in fe13072

func (scope *RequestScope) AllowsConversion(gvk schema.GroupVersionKind) bool {

, if we avoid matching any specific type, then the regression test like this in unit test here

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate_test.go

Line 210 in 46648a5

"Accept": []string{"application/json, text/plain;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io"},

will be no meaning. since no matter how long and most specific your Accept header is, even they are correct form, like application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io the unit test will always pick the short one, since acceptMediaTypeOptions always return false

@liggitt do you have any suggestions ?

The linked unit test should be selecting application/json. In the absence of qualifiers, the first matching, valid accept header should be used

The function you linked should only return true if the endpoint supports those conversions (all objects support partial object meta, only endpoints that have a TableConvertor should support the table object).

The emptyEndpointRestrictions is used in both NegotiateOutputSerializer and NegotiateOutputStreamSerializer.

NegotiateOutputSerializer is called in the discovery endpoints. They don't support Table Convertor. The types served don't have ObjectMeta at all, so the original code is more appropriate.

NegotiateOutputStreamSerializer is called by the watch handler. Shouldn't the watch handler instead get the endpoint restrictions from the scope? @smarterclayton

@shiywang As for the unit test, I think you can add a test testing NegotiateOutputMediaType, and add custom endpoint restrictions in your test matrix.

Empty should not support anything. Both restrictions should be provided by the scope, and the underlying type has to support the rules for partial metadata. Only types that implement the meta.Object interface are candidates for PartialObjectMetadata. Only storage implementations with ConvertToTable should support TableOutput.

@shiywang As for the unit test, I think you can add a test testing NegotiateOutputMediaType, and add custom endpoint restrictions in your test matrix.

Thanks @caesarxuchao I will add a custom endpoint restriction and fall back to original code of emptyEndpointRestrictions

The linked unit test should be selecting application/json. In the absence of qualifiers, the first matching, valid accept header should be used

Why the first matching, shouldn't be most specific matching? left a question here.

k8s-github-robot · 2017-08-30T22:09:37Z

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", "release-note-experimental" or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

k8s-github-robot · 2017-10-30T08:20:54Z

This PR hasn't been active in 60 days. It will be closed in 29 days (Nov 28, 2017).

cc @caesarxuchao @deads2k @shiywang

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

dims · 2017-12-11T13:53:04Z

/test pull-kubernetes-unit

jberkus · 2017-12-11T21:18:11Z

This PR has not been updated in several days, and does not appear to be a blocker issue. As such, it is being downgraded to pause it out of 1.9.0. If this is in error, please update the issue, approve and merge it in the next 16 hours. Thanks!

/priority important-soon

jberkus · 2017-12-11T21:20:21Z

/remove-priority critical-urgent

jberkus · 2017-12-11T21:36:10Z

Updating per SIG:

/remove-priority important-soon

/priority critical-urgent

k8s-github-robot · 2017-12-11T21:38:36Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@caesarxuchao @deads2k @shiywang @smarterclayton

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

smarterclayton · 2017-12-11T21:40:49Z

Looks good to me as well, thanks.

k8s-ci-robot · 2017-12-11T21:41:34Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: caesarxuchao, shiywang

Associated issue: #50519

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

staging/src/k8s.io/apiserver/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-12-12T00:26:00Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-12-12T01:57:54Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

liggitt · 2017-12-12T04:15:30Z

I added these tests and expected the detailed type to be selected:

+               {
+                       req: &http.Request{
+                               Header: http.Header{
+                                       "Accept": []string{"application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io, application/json"},
+                               },
+                       },
+                       contentType: "application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io",
+                       ns:          &fakeNegotiater{serializer: fakeCodec, types: []string{"application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io", "application/json"}},
+                       serializer:  fakeCodec,
+               },

failed with negotiate_test.go:278: 14: expected application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io, got: application/json

+               {
+                       req: &http.Request{
+                               Header: http.Header{
+                                       "Accept": []string{"application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io"},
+                               },
+                       },
+                       contentType: "application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io",
+                       ns:          &fakeNegotiater{serializer: fakeCodec, types: []string{"application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io", "application/json"}},
+                       serializer:  fakeCodec,
+               },

failed with negotiate_test.go:260: 15: failed: only the following media types are accepted: application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io, application/json

am I misunderstanding the tests?

shiywang · 2017-12-12T05:22:33Z

hi @liggitt, in your case
{"application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io, application/json"},

the "application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io should be picked, but not because it's most specific, instead it's user's specific order. if you agree, the reason is I explained here #50603 (comment), then I will add custom endpoint restrictions in test matrix.

if you mean user specify
{"application/json,application/json;as=PartialObjectMetadataList;v=v1alpha1;g=meta.k8s.io"},
also the longer one should be picked, I think we also have a pr to address that here https://bitbucket.org/ww/goautoneg/pull-requests/2/add-rfc7231-section-532-most-specific/diff, but seems @smarterclayton not agree with.

liggitt · 2017-12-12T06:53:28Z

Hmm... added #57081 and the fallback worked as I expected. I'm confused why the negotiate unit test fails in what appears to be an identical case

shiywang · 2017-12-12T07:39:33Z

Hmm... added #57081 and the fallback worked as I expected.

you mean expectation of "right behavior" ? anyway I will take a look that test.

I'm confused why the negotiate unit test fails in what appears to be an identical case

I also add a test enhance pr here: #57086 unit test should not only check test.contentType but also m.Convert

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 14, 2017

k8s-github-robot assigned deads2k and caesarxuchao Aug 14, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Aug 14, 2017

sttts reviewed Aug 14, 2017

View reviewed changes

shiywang force-pushed the loop branch from 8fc7893 to 8024492 Compare August 14, 2017 16:35

smarterclayton reviewed Aug 14, 2017

View reviewed changes

shiywang force-pushed the loop branch 2 times, most recently from 51b6e05 to e245a78 Compare August 16, 2017 01:07

shiywang force-pushed the loop branch from e245a78 to 52f74e4 Compare August 16, 2017 02:08

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 16, 2017

shiywang force-pushed the loop branch from 52f74e4 to 706b94b Compare August 16, 2017 02:21

shiywang changed the title ~~[WIP] Fix Content negotiation incorrect when Accept header uses type parame…~~ Fix Content negotiation incorrect when Accept header uses type parame… Aug 16, 2017

smarterclayton reviewed Aug 16, 2017

View reviewed changes

shiywang force-pushed the loop branch from ec47ee9 to 46648a5 Compare August 18, 2017 02:52

liggitt reviewed Aug 29, 2017

View reviewed changes

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 30, 2017

k8s-github-robot added milestone/needs-attention and removed milestone/needs-attention labels Dec 7, 2017

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Dec 11, 2017

k8s-github-robot added the milestone/incomplete-labels label Dec 11, 2017

k8s-ci-robot removed the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 11, 2017

k8s-github-robot added milestone/removed and removed milestone/incomplete-labels labels Dec 11, 2017

k8s-github-robot removed this from the v1.9 milestone Dec 11, 2017

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Dec 11, 2017

smarterclayton added this to the v1.9 milestone Dec 11, 2017

k8s-github-robot removed the milestone/removed label Dec 11, 2017

smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2017

k8s-github-robot merged commit ee13444 into kubernetes:master Dec 12, 2017

shiywang deleted the loop branch December 12, 2017 03:05

shiywang mentioned this pull request Dec 12, 2017

add custom endpoint restrictions in test matrix #57086

Closed

liggitt mentioned this pull request Jun 28, 2018

Kubectl 1.11.0 is unable to get pods or nodes on clusters running 1.8.4 #65575

Closed

		@@ -282,10 +310,16 @@ func NegotiateMediaTypeOptions(header string, accepted []AcceptedMediaType, endp
		clause.Type == "" && clause.SubType == "":
		// TODO: should we prefer the first type with no unrecognized options? Do we need to ignore unrecognized

Fix Content negotiation incorrect when Accept header uses type parame… #50603

Fix Content negotiation incorrect when Accept header uses type parame… #50603

Conversation

shiywang commented Aug 14, 2017 • edited

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

krzyzacy commented Aug 16, 2017

shiywang commented Aug 16, 2017

smarterclayton commented Aug 16, 2017

Choose a reason for hiding this comment

shiywang commented Aug 18, 2017

shiywang commented Aug 29, 2017

sttts commented Aug 29, 2017

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

shiywang Aug 30, 2017 • edited

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

shiywang Nov 27, 2017 • edited

Choose a reason for hiding this comment

k8s-github-robot commented Aug 30, 2017

k8s-github-robot commented Oct 30, 2017

dims commented Dec 11, 2017

jberkus commented Dec 11, 2017

jberkus commented Dec 11, 2017

jberkus commented Dec 11, 2017

k8s-github-robot commented Dec 11, 2017

smarterclayton commented Dec 11, 2017

k8s-ci-robot commented Dec 11, 2017

k8s-github-robot commented Dec 12, 2017

k8s-github-robot commented Dec 12, 2017

liggitt commented Dec 12, 2017 • edited

shiywang commented Dec 12, 2017 • edited

liggitt commented Dec 12, 2017

shiywang commented Dec 12, 2017 • edited

shiywang commented Aug 14, 2017 •

edited

shiywang Aug 30, 2017 •

edited

shiywang Nov 27, 2017 •

edited

liggitt commented Dec 12, 2017 •

edited

shiywang commented Dec 12, 2017 •

edited

shiywang commented Dec 12, 2017 •

edited