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

k8s.io/client-go vendors k8s.io/apimachinery which breaks build #83

Closed
davecheney opened this issue Jan 31, 2017 · 32 comments
Closed

k8s.io/client-go vendors k8s.io/apimachinery which breaks build #83

davecheney opened this issue Jan 31, 2017 · 32 comments

Comments

@davecheney
Copy link

TL;DR: libraries must not vendor their dependencies

It is not possible to build programs that vendor k8s.io/client-go as the latter has vendored k8s.io/apimachinery.

Sample program:

package main

import (
        "flag"
        "fmt"
        "log"
        "os"
        "path/filepath"
        "time"

        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
        "k8s.io/client-go/tools/clientcmd"

        "k8s.io/client-go/kubernetes"
)

func main() {
        kubeconfig := flag.String("kubeconfig", filepath.Join(os.Getenv("HOME"), ".kube", "config"), "absolute path to the kubeconfig file")

        flag.Parse()
        // uses the current context in kubeconfig
        config, err := clientcmd.BuildConfigFromFlags("", *kubeconfig)
        check(err)

        clientset, err := kubernetes.NewForConfig(config)
        check(err)

        for {
                pods, err := clientset.Core().Pods("").List(metav1.ListOptions{})
                check(err)

                fmt.Printf("There are %d pods in the cluster\n", len(pods.Items))
                time.Sleep(10 * time.Second)
        }
}

func check(err error) {
        if err != nil {
                log.Fatalf("fatal: %v", err)
        }
}

This program will not compile because the vendored copy of k8s.io/apimachinery (https://github.com/kubernetes/client-go/tree/master/vendor/k8s.io/apimachinery) aliases the copy in my own project, this breaks type equality.

# stash.atlassian.com/scm/kube/rds-resource-provider/cmd/postgres-resource-provider
cmd/postgres-resource-provider/main.go:29: cannot use "stash.atlassian.com/scm/kube/rds-resource-provider/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions literal (type "stash.atlassian.com/scm/kube/rds-resource-provider/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions) as type "stash.atlassian.com/scm/kube/rds-resource-provider/vendor/k8s.io/client-go/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions in argument to clientset.Core().Pods("").List
Makefile:11: recipe for target 'build' failed
make: *** [build] Error 2
@lavalamp
Copy link
Member

TL;DR: go's vendoring system is fundamentally broken.

We are working on switching to the new dep vendoring management tool, after which this should be acceptable. (You'll run dep ensure and it will flatten the dependencies.)

If we don't vendor, then you'll have a very difficult time getting a set of dependencies that actually works. We describe this in the top level README. This is a huge source of pain for us--we just did some code moves to other packages which are causing the current set of difficulties.

@lavalamp
Copy link
Member

(And I know the go doctrine is "have stable public apis and make new repos when you break them" but we are untangling a big ball of spaghetti and that's just totally impractical for us.)

@davecheney
Copy link
Author

@lavalamp no argument there, vendor/ is utterly broken for library writers. /cc @spf13

We are working on switching to the new dep vendoring management tool, after which this should be acceptable. (You'll run dep ensure and it will flatten the dependencies.)

This tool is marked pre alpha, are you sure that is prudent?

Can you at least update the vendored copy of k8s.io/apimachinery ?

@deads2k
Copy link
Contributor

deads2k commented Jan 31, 2017

Can you at least update the vendored copy of k8s.io/apimachinery ?

apimachinery updates a lot faster than client-go because of the way the process goes. client-go is the one that needs updating, but because of the merge queue process its difficult to do. See my comment here for fixing client-go sync kubernetes/kubernetes#39504 (comment)

@caesarxuchao
Copy link
Member

This program will not compile because the vendored copy of k8s.io/apimachinery (https://github.com/kubernetes/client-go/tree/master/vendor/k8s.io/apimachinery) aliases the copy in my own project

@davecheney do you use a vendor package management tool? Both godep and glide will flatten the dependencies.

However, godep won't handle version conflicts in dependencies. This is why we want to switch to dep or glide, which supports specifying a range of versions, lowing the probability of conflicts.

For now, can you use the release-2.0 branch of client-go? That branch doesn't depend on k8s.io/apimachinary.

@davecheney
Copy link
Author

davecheney commented Feb 1, 2017 via email

@lavalamp
Copy link
Member

lavalamp commented Feb 2, 2017

A version of client-go with the apimachinery package flattened back out is in github.com/lavalamp/client-go-flat. (go get github.com/lavalamp/client-go-flat should work.)

Is this approach something we should automate and officialize?

@seh
Copy link

seh commented Feb 2, 2017

Using Glide with its --strip-vendor flag allows me to use kubernetes/client-go together with kubernetes/apimachinery, per @caesarxuchao's suggestion above.

@deads2k
Copy link
Contributor

deads2k commented Feb 2, 2017

@lavalamp @smarterclayton @liggitt in the experiment you've created here: https://github.com/lavalamp/client-go-flat/, how would another library writer who generated his clients using our client generator against the apimachinery repo go about creating a compatible client? The signatures for anything that uses an apimachinery type like RESTClient https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/client.go#L61-L89 will be incompatible.

As we add aggregated API servers, we expect this to be more common, not less.

@deads2k
Copy link
Contributor

deads2k commented Feb 2, 2017

@lavalamp I think we ought to try to deleting the content of vendor/k8s.io/apimachinery, but leaving its manifest entry present. With the synchronized rollout that @caesarxuchao is working on, go get-ing from master should give matched sets instead of client-go lagging. By leaving the manifest entry, anyone trying to vendor would be able to pin the exact version we want. And we wouldn't face the "we generated incompatible clients" problem.

@caesarxuchao
Copy link
Member

@deads2k Leaving only apimachinery out of vendor/ is strange. If a user use git clone to download client-go, missing only apimachinery in vendor/ will cause confusion.

The synchronized rollout work (kubernetes/test-infra#1784) will keep client-go/vendor/k8s.io/apimachinery up-to-date, so keeping the whole vendor/ folder is possible.

We could also remove the entire vendor/ folder (#19 (comment)), but I see two problems: 1. go get user will get incompatible dependencies. 2. we'll need to provide dependency manifests for multiple vendor package management tools. 2 is solvable, just requires some maintenance efforts.

@deads2k
Copy link
Contributor

deads2k commented Feb 3, 2017

@caesarxuchao is there another option that will allow different client-go libraries from other projects to inter-operate?

@lavalamp
Copy link
Member

lavalamp commented Feb 3, 2017

ok, I think flattening out the dependencies like we talked about at the SIG is not a workable plan.

I think our approach should be:

  • document
  • users must use a version control tool, dep, glide, or godep
  • continuously test so that every supported branch always always always works.

@errordeveloper
Copy link
Member

I currently need to have set exact revisions of this repo and apimachinery in my glide.yaml, in fact I never wanted to care about two packages and how they relate to each other just about makes sense to me as a user... Looks like master is broken at the moment, and simply referencing latest revisions like I did a few days ago didn't work. Perhaps there are still many usability improvements that are waiting to be implemented. I think basically it's whenever you want to construct a object natively (not from YAML), then you have to pull in apimachinery.

@errordeveloper
Copy link
Member

@seh I am also using glide up -v...

@deads2k
Copy link
Contributor

deads2k commented Feb 6, 2017

I currently need to have set exact revisions of this repo and apimachinery in my glide.yaml, in fact I never wanted to care about two packages and how they relate to each other just about makes sense to me as a user... Looks like master is broken at the moment, and simply referencing latest revisions like I did a few days ago didn't work. Perhaps there are still many usability improvements that are waiting to be implemented. I think basically it's whenever you want to construct a object natively (not from YAML), then you have to pull in apimachinery.

@caesarxuchao has kubernetes/test-infra#1784 to change the client-go publishing infrastructure so that it stops lagging behind.

@lavalamp
Copy link
Member

@rubenv
Copy link

rubenv commented Feb 13, 2017

@lavalamp That's not publicly viewable?

@lavalamp
Copy link
Member

lavalamp commented Feb 13, 2017 via email

@ericchiang
Copy link
Contributor

ericchiang commented Feb 15, 2017

edit: redacted. I misunderstood what tags import what.

It looks like v2.0.0-alpha.1 doesn't specify a version of k8s.io/apimachinery[0] in it's Godeps file so tools like glide can't determine the version needed. @errordeveloper that's why you're glide command doesn't work.

Here's an attempt to compile the out of cluster example[1]:

$ cat glide.yaml 
package: github.com/ericchiang/client-go-example
import:
- package: k8s.io/client-go
  version: v2.0.0-alpha.1
$ ls
glide.yaml  main.go
$ glide up -v
[INFO]	Downloading dependencies. Please wait...
[INFO]	--> Fetching updates for k8s.io/client-go.
[INFO]	--> Setting version for k8s.io/client-go to v2.0.0-alpha.1.
[INFO]	Resolving imports
[INFO]	--> Fetching updates for github.com/go-openapi/spec.
[INFO]	--> Fetching updates for github.com/gogo/protobuf.
[INFO]	--> Fetching updates for github.com/google/gofuzz.
[INFO]	--> Fetching updates for k8s.io/apimachinery.
[INFO]	Found Godeps.json file in /home/eric/.glide/cache/src/https-k8s.io-client-go
[INFO]	--> Parsing Godeps metadata...
# ...
$ go install
./main.go:30: cannot use "github.com/ericchiang/client-go-example/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions literal (type "github.com/ericchiang/client-go-example/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions) as type "github.com/ericchiang/client-go-example/vendor/k8s.io/client-go/pkg/api/v1".ListOptions in argument to clientset.CoreV1().Pods("").List

Good news, this is fixed on master![2] So installing from that SHA works fine because glide can pull the right version of k8s.io/apimachinery.

$ cat glide.yaml 
package: github.com/ericchiang/client-go-example
import:
- package: k8s.io/client-go
  version: 8b466d64c5da37e0d3985b907d812e1f58cb41cb
$ glide up -v
$ go install
# no error

[0] https://github.com/kubernetes/client-go/blob/v2.0.0-alpha.1/Godeps/Godeps.json
[1] https://github.com/kubernetes/client-go/blob/8b466d64c5da37e0d3985b907d812e1f58cb41cb/examples/out-of-cluster/main.go
[2]

"Rev": "7080e31e90e981181435294bca96c80a37db8941"

@caesarxuchao
Copy link
Member

@ericchiang v2.0.0 doesn't depend on k8s.io/apimachinery, why would glide try to import apimachinery?

@ericchiang
Copy link
Contributor

@caesarxuchao ah my bad.

So master imports k8s.io/apimachinery but v2.0.0-alpha.1 doesn't? I guess I'm confused about what the tag branch means.

@caesarxuchao
Copy link
Member

Right. v2.0.0 is reflect kuberentes 1.5, when k8s.io/apimanichery doesn't exist yet.

@sttts
Copy link
Contributor

sttts commented Feb 16, 2017

Maybe most of you have seen this already: @lavalamp wrote a good summary about ways to vendor client-go: https://github.com/kubernetes/client-go/blob/master/INSTALL.md

@lavalamp
Copy link
Member

Given my changes to README.md & INSTALL.md, and the fact that @caesarxuchao has cut & blessed the v2.0.0 tag and added some detail to CHANGELOG.md, I think we can finally consider this resolved. Separate issues have been filed for a few remaining tasks (e.g. CI for the install instructions).

@rubenv
Copy link

rubenv commented Feb 16, 2017

@lavalamp I've tried following your instructions in INSTALL.md (I'm one of those pesky users who just wants to use go get and doesn't vendor).

I don't think it's even possible. It states:

If you want to write a simple script, don't care about a reproducible client library install, don't mind getting head (which may be less stable than a particular release) and don't share any dependencies with client-go

It's not possible to not share dependencies. For instance, getting a Pod has the following signature (as part of PodInterface):

Get(name string, options meta_v1.GetOptions) (*v1.Pod, error)

meta_v1 is k8s.io/apimachinery/pkg/apis/meta/v1.

That's hardly obscure as a use case.
I might be doing something wrong, but I don't think it's currently possible to write any meaningful application with just go get.

Am I missing something?

@lavalamp
Copy link
Member

@rubenv thanks for pointing that out. I forgot about it in the stack of other things that were broken.

@rubenv
Copy link

rubenv commented Mar 9, 2017

@lavalamp have you found any time to look into this? It's currently still impossible to use client-go without vendoring.

@sttts
Copy link
Contributor

sttts commented Mar 9, 2017

@rubenv this is being worked on in kubernetes/test-infra#1784 and related PRs by @caesarxuchao.

Until that is merged and runs daily, client-go master branch continues to be kind of broken. Not really satisfying now, but we are getting near to a solution.

@rubenv
Copy link

rubenv commented Mar 9, 2017

Cool, subscribing. Thanks a lot for the effort and apologies for the complaining :-)

@caesarxuchao
Copy link
Member

Now if a user go get k8s.io/client-go in a clean GOPATH (meaning k8s.io/apimachinery doesn't already exist in the GOPATH), (s)he will not hit the problem described in the issue. I think we can consider the issue resolved.

@pbarker
Copy link
Contributor

pbarker commented Apr 5, 2017

So, I'm sure you all are familiar with https://github.com/ericchiang/k8s, they manage to provide from what I can tell the same functionality with only two dependencies. I am curious from a design perspective what advantage your numerous dependencies have over their two dependency solution?

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