Pipeline: Pipeline depends on packages with bad k8s imports, preventing minor upgrades

Created on 27 Aug 2019 · 12Comments · Source: tektoncd/pipeline

See https://github.com/kubernetes/kubernetes/issues/81878 and/or https://github.com/google/go-containerregistry/issues/496

The reconciler imports go-containerregistry's k8schain package, which has an unsupported k8s.io/kubernetes import.

$ go mod why k8s.io/kubernetes/pkg/credentialprovider/secrets

github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1
github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1.test
github.com/tektoncd/pipeline/test/builder
github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/resources
github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/entrypoint
github.com/google/go-containerregistry/pkg/authn/k8schain
k8s.io/kubernetes/pkg/credentialprovider/secrets

This makes it impossible for downstream pipeline consumers to do minor module upgrades:

$ go get -u k8s.io/test-infra # or anything else which imports tektoncd/pipeline
...
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go: k8s.io/[email protected]: unknown revision v0.0.0
go get: error loading module requirements

kinbug

Source

fejta

Most helpful comment

If k8s.io/kubernetes/... imports are not recommended, is that documented anywhere? It could be useful to migrate those to some internal path so it's clearer they're not meant for external consumption.

ImJasonH on 28 Aug 2019

👍3

All 12 comments

@fejta (repeat of question over email): how much of Pipelines does Prow need to import? Is it possible for Prow to import /depend on less of Pipelines?

More context provided by @fejta for this issue:

kubernetes/kubernetes#81878 -- any module which has an indirect dependency on k8s.io/kubernetes cannot do minor upgrades
tektoncd/pipeline#1244 -- pipeline imports go-containerregistry's k8s.io/kubernetes dependency
google/go-containerregistry#496 -- containerregistry imports k8s.io/kubernetes/pkg/credentialprovider/secrets
@mattmoor @jonjohnsonjr

bobcatfish on 28 Aug 2019

From go mod why it looks like go-containerregistry (and therefore credentialprovider) is only needed for tests (github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1.test). Would it be possible for Prow to prune test files from its Pipelines dep, while we look into removing the dep from go-containerregistry?

ImJasonH on 28 Aug 2019

Prow imports are:

type definitions: https://github.com/kubernetes/test-infra/blob/496ca514635406625f04f18c713dcf162ca8dce1/prow/apis/prowjobs/v1/types.go#L28
the generated client: https://github.com/kubernetes/test-infra/blob/496ca514635406625f04f18c713dcf162ca8dce1/prow/cmd/pipeline/main.go#L35-L37

For knative-build integration they now use the controller-manager to create a client at runtime, rather than using the generated one: https://github.com/kubernetes/test-infra/blob/496ca514635406625f04f18c713dcf162ca8dce1/prow/cmd/build/main.go#L129

Looks like we just need to get the problematic imports out of the types + generated client if possible, and/or Prow can switch to using a dynamic client.

bobcatfish on 28 Aug 2019

From go mod why it looks like go-containerregistry (and therefore credentialprovider) is only needed for tests (github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1.test). Would it be possible for Prow to prune test files from its Pipelines dep, while we look into removing the dep from go-containerregistry?

I'm completely wrong, Pipelines imports k8schain (the package at the middle of this dependency bomb) in github.com/tektoncd/pipeline/pkg/reconciler/taskrun/entrypoint

ImJasonH on 28 Aug 2019

👍3

Rather than knowingly breaking a wide range of tools (knative, tekton, kaniko, ...) by moving things to internal, maybe project the library out of k/k? I think we'd be very happy to move references to that.

mattmoor on 28 Aug 2019

/assign
I think the problem should show up for recent version of tektoncd/pipeline… I'll try to bump the dependency in kubernetes/test-infra

vdemeester on 6 Sep 2019

Hum interesting, go mod doesn't really look into what package is used and what is not… So even if test-infra doesn't import anything in tektoncd/pipeline that uses go-containerregistry, it lists it as an indirect dependency…

vdemeester on 6 Sep 2019

😕2

Quick update, https://github.com/google/go-containerregistry/pull/649 might be the temporary fix (waiting for https://github.com/kubernetes/enhancements/pull/1406) to remove that import path.

vdemeester on 10 Jan 2020

@fejta I think depending on latest pipeline once 0.10 is released should fix this :angel:

vdemeester on 21 Jan 2020

https://github.com/kubernetes/test-infra/pull/16099 is merged, 0.10 is released, We can close :tada:
/close

vdemeester on 7 Feb 2020

@vdemeester: Closing this issue.

In response to this:

https://github.com/kubernetes/test-infra/pull/16099 is merged, 0.10 is released, We can close :tada:
/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.