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

Fix update rawextension decoding #14764

Merged
merged 1 commit into from
Jun 22, 2017
Merged

Fix update rawextension decoding #14764

merged 1 commit into from
Jun 22, 2017

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Jun 20, 2017

We're seeing weird things decoding update requests containing raw extensions... they're not getting converted to the internal version of the runtime object.

xref #14743

@liggitt
Copy link
Contributor Author

liggitt commented Jun 20, 2017

cc @enj @smarterclayton @deads2k

@liggitt liggitt changed the title Fix update/delete decoding WIP - Fix update/delete decoding Jun 20, 2017
@enj
Copy link
Contributor

enj commented Jun 20, 2017

So from what I have gathered the issue is that DecodeNestedObjects only has the passed in decoder for decoding, but has no into equivalent for handling conversion. Thus whatever GVK the decoder uses is seen at the REST layer (v1).

if d, ok := obj.(runtime.NestedObjectDecoder); ok {
	if err := d.DecodeNestedObjects(DirectDecoder{c.decoder}); err != nil {
		return nil, gvk, err
	}
}

The outer object is decoded and converted into the GVK of into (the internal type).

obj, gvk, err := c.decoder.Decode(data, defaultGVK, into)
if err != nil {
	return nil, gvk, err
}

if err := c.convertor.Convert(obj, into, c.decodeVersion); err != nil {
	return nil, gvk, err
}

After fixing the compile error in this PR, the update works as expected, though I did see some weird logs:

Convert_v1_PolicyRule_To_api_PolicyRule in:
k8s
#
�v1��IsPersonalSubjectAccessReview��"
&runtime.Unknown{TypeMeta:runtime.TypeMeta{APIVersion:"v1", Kind:"IsPersonalSubjectAccessReview"}, Raw:[]uint8{}, ContentEncoding:"", ContentType:""}
&runtime.Unknown{TypeMeta:runtime.TypeMeta{APIVersion:"v1", Kind:"IsPersonalSubjectAccessReview"}, Raw:[]uint8{}, ContentEncoding:"", ContentType:""}

@liggitt liggitt added this to the 3.6.0 milestone Jun 20, 2017
@@ -816,7 +816,8 @@ func UpdateResource(r rest.Updater, scope RequestScope, typer runtime.ObjectType
defaultGVK := scope.Kind
original := r.New()
trace.Step("About to convert to expected version")
obj, gvk, err := scope.Serializer.DecoderToVersion(s.Serializer, defaultGVK.GroupVersion()).Decode(body, &defaultGVK, original)
decoder := scope.Serializer.DecoderToVersion(s.Serializer, schema.GroupVersion{Group: defaultGVK.Group, Version: runtime.APIVersionInternal})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the backwards compatible change I expected

@enj
Copy link
Contributor

enj commented Jun 20, 2017

LGTM, I assume we want this upstream this as well?

@liggitt
Copy link
Contributor Author

liggitt commented Jun 20, 2017

yeah, kubernetes/kubernetes#47792

@liggitt liggitt changed the title WIP - Fix update/delete decoding Fix update rawextension decoding Jun 20, 2017
@liggitt
Copy link
Contributor Author

liggitt commented Jun 20, 2017

@enj the issue you are seeing is in the streamwatcher. It isn't related to this change, but we'll need to figure out what is going wrong with that area as well, as that can affect other watchers of items containing raw extensions.

@liggitt
Copy link
Contributor Author

liggitt commented Jun 20, 2017

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to a6c9c12

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2442/) (Base Commit: df9c0ed) (PR Branch Commit: a6c9c12)

@liggitt
Copy link
Contributor Author

liggitt commented Jun 21, 2017

upstream has LGTM
[merge][severity:blocker]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to a6c9c12

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 22, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1079/) (Base Commit: 857cfa5) (PR Branch Commit: a6c9c12) (Extended Tests: blocker) (Image: devenv-rhel7_6392)

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

Successfully merging this pull request may close these issues.

5 participants