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

Using for ThirdPartyResource's #8

Closed
tiaanl opened this issue Oct 6, 2016 · 25 comments
Closed

Using for ThirdPartyResource's #8

tiaanl opened this issue Oct 6, 2016 · 25 comments

Comments

@tiaanl
Copy link

tiaanl commented Oct 6, 2016

I've started using this library and it works very well.

When extending the API with ThirdPartyResource resources, things become a major issue. Using the dynamic part doesn't work well because I don't want to convert all my objects into map[string]interface{} objects. The rest part alone also doesn't work well, because there are no Encoders/Decoders.

I'm sure all the tools required to start working with my own resources is available, but get them all up and running is just a major pain point currently.

Right now I don't know enough of the internals to be productive towards a solution, but any solution would be MUCH appreciated.

@lavalamp
Copy link
Member

lavalamp commented Oct 6, 2016

@smarterclayton @krousey Do we have a convenient go struct <-> TPR codec laying around somewhere?

@jimmidyson
Copy link
Member

/cc @fabxc

@tiaanl
Copy link
Author

tiaanl commented Oct 7, 2016

Okay, so I've made progress on my side. Using the rest part of the library is quite useful, but the biggest problem is still plenty of little details that is not really reported when not done correctly.

Doing a GET for a list of objects you need the following into object:

type ProjectList struct {
    unversioned.TypeMeta `json:",inline"`
    unversioned.ListMeta `json:"metadata,omitempty"`

    Items []Project `json:"items"`
}

Notice the unversioned.ListMeta here. Without it, you will just not get the right result or something like not a registered type. Very confusing.

The final issue I have here still is that when I try to pull that same list with this call:

type ProjectSpec struct {
    Description string `json:"description"`
}

type Project struct {
    unversioned.TypeMeta `json:",inline"`
    api.ObjectMeta        `json:"metadata,omitempty"`

    Spec ProjectSpec `json:"spec"`
}


    projectList := ProjectList{}
    err := ah.Client.Rest.Get().
        Namespace("default").
        Resource("projects").
        Do().
        Into(&projectList)

The list object gets the right details, but the actual items do not. They are just blank with no data. I've figured out that it is the ObjectMeta type that is causing this error. If I recreate my own ObjectMeta struct, then it works perfectly. So I think there is another interface-detecting mechanism somewhere in the decoding that doesn't like the ObjectMeta type.

If I get all of this to work, I would definitely like to write a doc for how to use all this.

@deads2k
Copy link
Contributor

deads2k commented Oct 7, 2016

The list object gets the right details, but the actual items do not. They are just blank with no data. I've figured out that it is the ObjectMeta type that is causing this error. If I recreate my own ObjectMeta struct, then it works perfectly. So I think there is another interface-detecting mechanism somewhere in the decoding that doesn't like the ObjectMeta type.

You might be getting bit by ugorji codec generation. We had a problem with it downstream that we resolved by carrying a patch that deleted the generated codecs until we generated them for our types.

Also, if you're using third party resources, make sure you're aware of the known issues/missing features: kubernetes/enhancements#95

@justinsb
Copy link
Member

justinsb commented Oct 9, 2016

I hit the same thing, and copying ObjectMeta into my package fixes the problem (thanks for the tip @tiaanl)

Do we know why this happens? Is there an issue tracking this @deads2k?

@deads2k
Copy link
Contributor

deads2k commented Oct 10, 2016

Do we know why this happens? Is there an issue tracking this @deads2k?

I think it's because there's an interface check that passes because the object is anonymously included, but I haven't dug into this case to make sure. I don't know of an issue. We discovered it in OpenShift and didn't think it was reasonable to try to force reversion or object shape change to accommodate it. I think we fixed the correctness problems in the codec generator, so it ought to be safe to use for generating a codec now.

@lavalamp
Copy link
Member

It's ugorji--either run ugorji to generate en/decoders for your types, or
add a top level Decode() (double check the name) method that calls the
default json machinery.

On Mon, Oct 10, 2016 at 4:37 AM, David Eads notifications@github.com
wrote:

Do we know why this happens? Is there an issue tracking this @deads2k
https://github.com/deads2k?

I think it's because there's an interface check that passes because the
object is anonymously included, but I haven't dug into this case to make
sure. I don't know of an issue. We discovered it in OpenShift and didn't
think it was reasonable to try to force reversion or object shape change to
accommodate it. I think we fixed the correctness problems in the codec
generator, so it ought to be safe to use for generating a codec now.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#8 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAngliBN6xCuAk7JiJnMqysUShjYDzvhks5qyiNfgaJpZM4KP30A
.

@wfarr
Copy link
Contributor

wfarr commented Oct 21, 2016

add a top level Decode() (double check the name) method that calls the default json machinery

I'm encountering similar issues to others in this issue.

Please excuse my ignorance here. I'm playing catch-up on the internals of this repository still. Would you be able to point me towards what the Decode() method should be added to? Is it my custom types being passed to Into()?

I can also publish some of the code that I've been working on for our use case publicly to demonstrate what I'm doing (or more likely doing wrong 😉 ) if context is lacking (that said, my work is modeled pretty closely after what I've read from others here in this issue).

@caesarxuchao
Copy link
Member

Would you be able to point me towards what the Decode() method should be added to? Is it my custom types being passed to Into()?

Yes.

Or you can take a look at this file, which is the generated ugorji code in the k8s repository, you can generate similar code for your types. The code is generated using this script. (I think write your own Decode() is easier, k8s uses ugorji because it's more efficient than the default json package).

@caesarxuchao
Copy link
Member

Paste the slack discussion here since it might be useful for other people.

As ugorji doc says:
"if value implements encoding.TextUnmarshaler and format is a text format, call its UnmarshalText method"

So you need to implement the UnmarshalText() method for your type, you can use json.Unmarshal to implement it. For example:

type YourType struct {
}

func (yourType *YourType) UnmarshalText(data []byte) error {
  return  json.Unmarshal(data, yourType)
}

@tiaanl
Copy link
Author

tiaanl commented Oct 30, 2016

I created a repo to test just accessing the third party resources.

https://github.com/tiaanl/kube-tpr-demo

It has a few simple comands to interact with k8s.

The first issue with using the dynamic package is solved by registering my types with api.Schema.

The only last issue is still the decoding of the json into the correct types. Ive tried all the recommendations in this thread, except for generating that huge file for my type, which tbh is entirely overkill for trying to create something this simple.

Being able to store your meta data inside k8s, using the api versioning and all the other benefits you get for free would be fantastic. No more separate etcd with boilerplate code you have to write yourself.

Any help on this issue would be greatly appreciated.

@caesarxuchao
Copy link
Member

@tiaanl I heard the UnmarshalText method I pasted above didn't work. I'll debug it these days and report back here, since it's blocking so many people.

@wfarr
Copy link
Contributor

wfarr commented Oct 30, 2016

Thanks so much! I know the indirection and back-and-forth can be really
frustrating and time consuming. I appreciate your attention and effort
towards helping suss these issues out. ❤️
On Sun, Oct 30, 2016 at 6:08 PM Chao Xu notifications@github.com wrote:

@tiaanl https://github.com/tiaanl I heard the UnmarshalText method I
pasted above didn't work. I'll debug it these days and report back here,
since it's blocking so many people.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#8 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAAyxHQQugCTzfwTc_HnmKGL6MV-Rd1ks5q5RVMgaJpZM4KP30A
.

@caesarxuchao
Copy link
Member

Finally got it work. See "Problem 1" and "Problem 2" in the code: See wfarr/k8s-tpr-playground#1.

@caesarxuchao
Copy link
Member

Reported issue ugorji/go#178

@wfarr
Copy link
Contributor

wfarr commented Oct 31, 2016

I can definitely confirm these work-arounds are functional. 👍

The serialization mostly seems to work with cache.NewInformer() as well (something we expect to use). I've pushed up some code demonstrating this at https://github.com/wfarr/k8s-tpr-playground/blob/master/main_test.go#L36-L86

The current reflector errors with cache.NewInformer() on TPRs:

E1031 10:11:23.642558   66896 reflector.go:214] github.com/wfarr/k8s-tpr-playground/vendor/k8s.io/client-go/1.5/tools/cache/reflector.go:109: Unable to understand list result &main.ExampleList{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, Metadata:unversioned.ListMeta{SelfLink:"", ResourceVersion:""}, Items:[]main.Example{main.Example{TypeMeta:unversioned.TypeMeta{Kind:"Example", APIVersion:"wfarr.systems/v1"}, Metadata:api.ObjectMeta{Name:"example1", GenerateName:"", Namespace:"default", SelfLink:"/apis/wfarr.systems/v1/namespaces/default/examples/example1", UID:"ca182ddf-9d1e-11e6-9df1-42010af0008b", ResourceVersion:"3644329", Generation:0, CreationTimestamp:unversioned.Time{Time:time.Time{sec:63613263424, nsec:0, loc:(*time.Location)(0x13236e0)}}, DeletionTimestamp:(*unversioned.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"extensions/v1beta1\",\"metadata\":{\"name\":\"example1\",\"namespace\":\"default\",\"creationTimestamp\":null},\"data\":\"eyJhcGlWZXJzaW9uIjoid2ZhcnIuc3lzdGVtcy92MSIsImtpbmQiOiJFeGFtcGxlIiwibWV0YWRhdGEiOnsibmFtZSI6ImV4YW1wbGUxIn0sInNwZWMiOnsiYmFyIjp0cnVlLCJmb28iOiJIZWxsbyJ9fQ==\"}"}, OwnerReferences:[]api.OwnerReference(nil), Finalizers:[]string(nil), ClusterName:""}, Spec:main.ExampleSpec{Foo:"Hello", Bar:true}}}}: object does not implement the List interfaces

Of note, the contained []Example as *ExampleList.Items is deserialized perfectly, so it just seems to be unhappy with the TypeMeta or ListMeta on *ExampleList itself blowing up right now? I've poked around at a few attempts at fixing this (and will keep doing so), but any guidance on this would be much appreciated.

@wfarr
Copy link
Contributor

wfarr commented Nov 3, 2016

Good news!

I was able to get cache.NewInformer() working with this stuff by performing similar workarounds against my ExampleList type.

Namely:

  • Stopped embedding the ListMeta
  • Added GetListMeta() to satisfy the List interface

wfarr/k8s-tpr-playground@3b0d143#diff-7ddfb3e035b42cd70649cc33393fe32c demonstrates a working example

@caesarxuchao
Copy link
Member

That's very cool!

@smarterclayton
Copy link
Contributor

We are going to nuke ugorji in 1.6 (now that we have protobuf support) and
that will get around the problem mentioned above of having to implement the
deserializer.

On Thu, Nov 3, 2016 at 4:18 PM, Chao Xu notifications@github.com wrote:

That's very cool!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p8XmdTFockbqcv_SRzbWmat8SSAfks5q6kGKgaJpZM4KP30A
.

@rmohr
Copy link
Contributor

rmohr commented Nov 7, 2016

@caesarxuchao @wfarr thx for your examples. Can also confirm that #29 works.

@caesarxuchao
Copy link
Member

Thanks @rmohr for the confirmation. The example got merged.

justinsb added a commit to justinsb/kops that referenced this issue Dec 7, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 7, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 8, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 9, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 9, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 12, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 12, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 12, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
justinsb added a commit to justinsb/kops that referenced this issue Dec 15, 2016
This will work around some apimachinery bugs
(kubernetes/client-go#8)
hongchaodeng added a commit to hongchaodeng/etcd-operator that referenced this issue Feb 14, 2017
There is known issue with TPR in client-go:
   kubernetes/client-go#8
 Workarounds:
 - We include `Metadata` field in object explicitly.
 - Copy the solutions from client-go TPR examples to work around a
known problem with third-party resources and ugorji.
```go
type ClusterListCopy ClusterList
type ClusterCopy Cluster

func (c *Cluster) UnmarshalJSON(data []byte) error {
	tmp := ClusterCopy{}
	err := json.Unmarshal(data, &tmp)
	if err != nil {
		return err
	}
	tmp2 := Cluster(tmp)
	*c = tmp2
	return nil
}

func (cl *ClusterList) UnmarshalJSON(data []byte) error {
	tmp := ClusterListCopy{}
	err := json.Unmarshal(data, &tmp)
	if err != nil {
		return err
	}
	tmp2 := ClusterList(tmp)
	*cl = tmp2
	return nil
}
```
hongchaodeng added a commit to hongchaodeng/etcd-operator that referenced this issue Feb 14, 2017
There is known issue with TPR in client-go:
   kubernetes/client-go#8
 Workarounds:
 - We include `Metadata` field in object explicitly.
 - Copy the solutions from client-go TPR examples to work around a
known problem with third-party resources and ugorji.
```go
type ClusterListCopy ClusterList
type ClusterCopy Cluster

func (c *Cluster) UnmarshalJSON(data []byte) error {
	tmp := ClusterCopy{}
	err := json.Unmarshal(data, &tmp)
	if err != nil {
		return err
	}
	tmp2 := Cluster(tmp)
	*c = tmp2
	return nil
}

func (cl *ClusterList) UnmarshalJSON(data []byte) error {
	tmp := ClusterListCopy{}
	err := json.Unmarshal(data, &tmp)
	if err != nil {
		return err
	}
	tmp2 := ClusterList(tmp)
	*cl = tmp2
	return nil
}
```
@hongchaodeng
Copy link
Contributor

hongchaodeng commented Feb 14, 2017

@caesarxuchao

I don't think this is fixed. Users of TPR client still have the problem. When defining types, they need to understand the workarounds and put them in their code. But there isn't any docs for it. There isn't any easy way for user to write a TPR client either.

Before we have a complete fix for it, can we keep this open? Or shall we open another to track the issue?

@nilebox
Copy link

nilebox commented Mar 9, 2017

Just had the same problem both with 2.0.0 and master revisions, and found two workarounds (both work).

If I recreate my own ObjectMeta struct, then it works perfectly. So I think there is another interface-detecting mechanism somewhere in the decoding that doesn't like the ObjectMeta type.

Indeed, some implicit interface seems to be the root cause of the issue. So the workarounds are:

  1. Create own copy of ObjectMeta type and use it instead of the one provided by client-go. Seems boilerplaty, and we still need this type to implement ObjectMetaAccessor interface:
// Required to satisfy ObjectMetaAccessor interface
func (e *Example) GetObjectMeta() metav1.Object {
	return &e.Metadata
}

So you will have to manually create a copy of this object to satisfy the interface.

  1. Just rename the ObjectMeta field to anything different, for example call it Metadata. It just works.

We are going to nuke ugorji in 1.6 (now that we have protobuf support)

Great news! Debugging problems like this is not fun.

@caesarxuchao
Copy link
Member

@nilebox thanks for sharing the workarounds. Did you see the tpr example in this repo? Is it not working? I'm trying to determine if we should update the example, or if we should move it to a more obvious place.

@nilebox
Copy link

nilebox commented Mar 9, 2017

@caesarxuchao Yes, I saw this example, and it should work (since you explicitly called a field Metadata). But since there are no comments around it, I didn't expect the naming is important here.

I created my type based on the Kubernetes Deployment type which has an ObjectMeta field. So I ran into problem tracked in this issue, and renaming the field solved it.

I would suggest to add some comment or link to this issue to your example so newcomers would pay attention to it.

asymmetric pushed a commit to kinvolk-archives/awstpr that referenced this issue May 23, 2017
sttts pushed a commit to sttts/client-go that referenced this issue Nov 21, 2017
Use types from openshift/api
sttts pushed a commit to sttts/client-go that referenced this issue Dec 22, 2017
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