Enable vendoring.

[jkh52]

Enable vendoring.

Motivation: isolation, reliability, and reproducibility.

Note to reviewer: the first commit is large, but all manual edits are in the second commit.

[elmiko]

talking about this at the office hours today, a question has arisen about why we might want to add vendoring. one reason, is to help be more explicit about the supply chain and dependencies. also to help reduce errors that might happen from our dependencies changing between when we build and when an upstream dependency might update.

to simplify the vendor flags, if we want, we could GOFLAGS=-mod=vendor environment variable.

also from the docs https://go.dev/ref/mod

At go 1.14 or higher, automatic [vendoring](https://go.dev/ref/mod#vendoring) may be enabled. If the file vendor/modules.txt is present and consistent with go.mod, there is no need to explicitly use the -mod=vendor flag.

[jkh52]

/assign @ipochi

[jkh52]

/retest

[jkh52]

Based on discussion I changed things so that:

• Makefile uses GOFLAGS = -mod=vendor environment
  • reduce clutter
• Dockerfiles directly use -mod=vendor flag
  • ensure builds are hermetic

Happy to tweak further, otherwise I think this is ready.

[jparrill]

Reviewing the PR, the default behaviour from go 1.11 and you're vendoring is that it has by default this -mod=vendor added. But if that so I would say to remove it in all the cases or add it in all the cases (I'm in the side of add it in all the cases.)

Not just for build but also for go test commands in the makefile.

I think this is not in the scope of this pr but I also would add something like:

GO=GO111MODULE=on GOFLAGS=-mod=vendor go
GO_BUILD_RECIPE=CGO_ENABLED=1 $(GO) build .....

And this $(GO_BUILD_RECIPE) add them in all the cases to be consistent. Maybe I can drop a following up PR fixing some things like this one.

I would like to have other community opinions like from @cheftako or @ipochi to have more points of view.

[jkh52]

Reviewing the PR, the default behaviour from go 1.11 and you're vendoring is that it has by default this -mod=vendor added. But if that so I would say to remove it in all the cases or add it in all the cases (I'm in the side of add it in all the cases.)

Not just for build but also for go test commands in the makefile.

This was a good suggestion, because it caught the absence of vendor/ in the konnectivity-client module. Latest upload vendors both modules, and runs tests with the flag.

I think this is not in the scope of this pr but I also would add something like:

GO=GO111MODULE=on GOFLAGS=-mod=vendor go
GO_BUILD_RECIPE=CGO_ENABLED=1 $(GO) build .....

And this $(GO_BUILD_RECIPE) add them in all the cases to be consistent. Maybe I can drop a following up PR fixing some things like this one.

+1 to keeping deeper cleanup separate from this PR. We should improve consistency between Makefile and Dockerfiles as well (example: GO111MODULE=on).

[cheftako]

Reviewing the PR, the default behaviour from go 1.11 and you're vendoring is that it has by default this -mod=vendor added. But if that so I would say to remove it in all the cases or add it in all the cases (I'm in the side of add it in all the cases.)
Not just for build but also for go test commands in the makefile.

This was a good suggestion, because it caught the absence of vendor/ in the konnectivity-client module. Latest upload vendors both modules, and runs tests with the flag.

I think adding vendor consistently for the rest of the ANP seems like a good idea. It not only makes the build chain clear but it means when doing code inspection/debugging you have the correct version of the code present cutting down on confusion due to dependency version discrepancies. Konnectivity-client is a bit different. It is not an executable but a library. The version of its dependencies are determined by the build of the executables which pull it in. So I worry that at best pulling a vendor directory into konnectivity-client is misleading. At worst it might cause problems for things like merging it into the K/K KAS build.

[jkh52]

Reviewing the PR, the default behaviour from go 1.11 and you're vendoring is that it has by default this -mod=vendor added. But if that so I would say to remove it in all the cases or add it in all the cases (I'm in the side of add it in all the cases.)
Not just for build but also for go test commands in the makefile.

This was a good suggestion, because it caught the absence of vendor/ in the konnectivity-client module. Latest upload vendors both modules, and runs tests with the flag.

I think adding vendor consistently for the rest of the ANP seems like a good idea. It not only makes the build chain clear but it means when doing code inspection/debugging you have the correct version of the code present cutting down on confusion due to dependency version discrepancies. Konnectivity-client is a bit different. It is not an executable but a library. The version of its dependencies are determined by the build of the executables which pull it in. So I worry that at best pulling a vendor directory into konnectivity-client is misleading. At worst it might cause problems for things like merging it into the K/K KAS build.

Good thinking, I checked and it seems that there should not be k/k problems.

I tried updating k/k with:

./hack/pin-dependency.sh sigs.k8s.io/apiserver-network-proxy/konnectivity-client=github.com/jkh52/apiserver-network-proxy/konnectivity-client vendor
./hack/update-codegen.sh
./hack/update-vendor.sh

as hoped, the vendor/sigs.k8s.io/apiserver-network-proxy/konnectivity-client contents did not change. I double checked, and there are no vendor directories anywhere in https://github.com/kubernetes/kubernetes/tree/master/vendor.

https://go.dev/ref/mod#vendoring says Unlike [vendoring in GOPATH mode](https://go.dev/s/go15vendor), the go command ignores vendor directories in locations other than the main module’s root directory. So it makes sense that k/k vendoring would avoid taking recursive vendor cruft.

That said, it does seem heavy handed to vendor the pure client lib, just in support of a single test konnectivity-client/pkg/client/client_test.go.

[jkh52]

Reviewing the PR, the default behaviour from go 1.11 and you're vendoring is that it has by default this -mod=vendor added. But if that so I would say to remove it in all the cases or add it in all the cases (I'm in the side of add it in all the cases.)
Not just for build but also for go test commands in the makefile.

This was a good suggestion, because it caught the absence of vendor/ in the konnectivity-client module. Latest upload vendors both modules, and runs tests with the flag.

I think adding vendor consistently for the rest of the ANP seems like a good idea. It not only makes the build chain clear but it means when doing code inspection/debugging you have the correct version of the code present cutting down on confusion due to dependency version discrepancies. Konnectivity-client is a bit different. It is not an executable but a library. The version of its dependencies are determined by the build of the executables which pull it in. So I worry that at best pulling a vendor directory into konnectivity-client is misleading. At worst it might cause problems for things like merging it into the K/K KAS build.

Good thinking, I checked and it seems that there should not be k/k problems.

I tried updating k/k with:

./hack/pin-dependency.sh sigs.k8s.io/apiserver-network-proxy/konnectivity-client=github.com/jkh52/apiserver-network-proxy/konnectivity-client vendor
./hack/update-codegen.sh
./hack/update-vendor.sh

as hoped, the vendor/sigs.k8s.io/apiserver-network-proxy/konnectivity-client contents did not change. I double checked, and there are no vendor directories anywhere in https://github.com/kubernetes/kubernetes/tree/master/vendor.

https://go.dev/ref/mod#vendoring says Unlike [vendoring in GOPATH mode](https://go.dev/s/go15vendor), the go command ignores vendor directories in locations other than the main module’s root directory. So it makes sense that k/k vendoring would avoid taking recursive vendor cruft.

That said, it does seem heavy handed to vendor the pure client lib, just in support of a single test konnectivity-client/pkg/client/client_test.go.

Choices:

1. Vendor both modules (it is more consistent, but could confuse some readers)
2. Vendor only the top level module, do not vendor the client lib module

I still lean toward 1, because it makes the repo consistent (all builds and tests run against locally available dependencies) but I can switch to 2 if @cheftako feels strongly, or if others also prefer it.

@ipochi or @jparrill do either of you have an opinion?

[jparrill]

do either of you have an opinion?

Well, @cheftako knows better than me the project, so I think He will have the point here :).

[jkh52]

do either of you have an opinion?

Well, @cheftako knows better than me the project, so I think He will have the point here :).

Thanks. I'll switch to option 2 since there is better consensus there.

[jkh52]

/assign @cheftako

[cheftako]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, jkh52

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [cheftako,jkh52]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment