Vendorless Knative #134
Comments
If this is the sole reason I would split this into a separate commit and review the commits independently Just as an FYI dropping this practice will be quite a lot of work since the majority reason we vendor deps is forpulling in non-go code artifacts Some examples:
Another extreme case is carrying patches for dependencies while we wait for them to be merged upstream. We've done that before for some K8s issues. I actually do like seeing the vendored dependencies in PRs since it gives reviewers the ability to sanity check the changes being pulled in. This makes it easier to debug at times. If we drop vendoring I would hope we put an effort to improve our tooling around visualizing the changes - ie. see this dependabot PR - ko-build/ko#663 |
That's true, but we don't have a way to enforce it and usually it's tedious (at least for me) to keep that in mind. If we keep vendor/, I found the pull request file tree does help reduce the noise though. |
upodroid
added
priority/important-soon
|
This issue is stale because it has been open for 90 days with no |
github-actions
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
knative-prow
bot
added
triage/accepted
|
/assign |
|
I will list here a couple of reasons why we should get rid of the vendor, and address some of the concerns people often raise. Reasons for removalMaintenance burden for productivityHaving the vendor directory in every project creates a huge maintenance burden for productivity. Every single tool we use (various linters, compilers, formatters etc.) needs to be properly filtered, so it will give off false-positives. Here's a live example: knative-extensions/knobots#259 (there were multiple similar issues in the past) Hard to count the contributionsCounting contributions is important for OSS projects, as it provides some insights into the health of the project. Having a Project looks datedThe enhancements in Go that made it possible to work without vendor directory safely has been made a long time ago - Go 1.13 released Sep 3, 2019 (over 3y ago). Since then, most of the code in Go has moved away from having the vendor directory. Sticking with it shows the project is old-fashioned, especially for newcomers. Remember when people attached external jar files to their Java projects, manually? That was before Maven was created to solve dependency management. The same was done for Go in 1.13+ releases. Bad code habitsHaving a vendor directory allows for relying on bad code habits, and hacks. Instead of using that just for Go code, we sometimes use it as a general tool for doing path traversal to some of the resources in the dependent projects. That's a smell, and may lead even to a vulnerability. Example of such smell can be seen in Knative-Serving: $ find . -type l -printf '%p -> ' -exec readlink {} ';' | grep vendor
./config/core/configmaps/network.yaml -> ../../../vendor/knative.dev/networking/config/config-network.yaml
./config/core/300-imagecache.yaml -> ../../vendor/knative.dev/caching/config/image.yaml
./config/core/300-resources/certificate.yaml -> ../../../vendor/knative.dev/networking/config/certificate.yaml
./config/core/300-resources/domain-claim.yaml -> ../../../vendor/knative.dev/networking/config/domain-claim.yaml
./config/core/300-resources/ingress.yaml -> ../../../vendor/knative.dev/networking/config/ingress.yaml
./config/core/300-resources/serverlessservice.yaml -> ../../../vendor/knative.dev/networking/config/serverlessservice.yaml(This is code smell, as symlinks are not cross-platform) Addressing some of the concernsVendor is needed for reproducible buildsWhen using go 1.13+ in standard configuration (with GOPROXY), the reproducible builds are already ensured. The Patching vendor files before it's patched in a dependencyPatching the project dependencies isn't a thing to do for an OSS project, IMHO. The Knative product vendors (Red Hat, Google, VMware, etc.) are there to provide such service for their customers. It is 100% expected to have vendor directories, and other kinds of code mirrors in product vendor infrastructure. Having it also in upstream, diminishes the reasons why customers should choose one of the vendor's solutions, which in turn hurts the OSS project. Sharing other no-go filesIn the long term, we would like to have a hackless project (all hacks rewritten to tools, see knative/hack#254). We have a way to achieve that, see: knative/hack#222. Other things, like symlinks and artifacts for code generation still need some work to address them without relying on vendor dir, but I think either we could use a solution similar to knative/hack#222 or have a dedicated Go tool written (maybe on upstream K8s) Some loose endsStill, we have some loose ends for R&D:
|
In addition there is a big productivity boost for day to day development when trying to debug failing tests that are coming from a different repository within Knative itself. Moreover, we still do this for some changes that didn't make into Knative pkg itself
+1 to this. Unless we have a solutions to the pain points listed in the various comments, if we really need to drop vendor I'd propose to at least give each project the final decision to chose to use or not use vendor. |
|
@pierDipi your point about debugging the code isn't valid, or I don't understand it. When you run code or tests without vendor folder, the go code is still downloaded to your home's
That's not good development practice to do so. It would be better to rely on some external interim package (fork of PKG maybe, or part of it), than patching the vendor. Anyone, person, or robot, or IDE, could easily restore the code just by calling Relying on those patches for reasons other than vulnerability patches, is just asking for trouble 😵💫. And as I said above, vulnerability patching is a job for Knative product's vendors, not the OSS project. |
Personally, I found double-checking the dependencies code an overkill. But maybe people need it... I think we can create such tooling, easily. Here's a recipe (pseudocode): if !project.HaveVendor() {
c := git.CurrentProject()
c.CheckoutTarget()
gomod.UpdateDeps()
g := git.init(go.GOMODCACHE)
g.CommitAll()
c.CheckoutPR()
gomod.UpdateDeps()
d := g.Diff()
artifacts.SaveAsArtifact(d, "dependency-changes.patch")
} |
When you need, for example, to improve the error message or try a quick fix while debugging you need to make changes to the dependency and rerun the test, I use that constantly, and no pushing to remove fork + go mod edit to poing to my fork is painful |
|
At first glance:
To help the discussion could you add some pointers to big projects with more deps than knative that have gone down that path? And why some others haven't eg. K8s?
Although I understand the benefits of goproxy, I have several questions. Which proxy do you suggest, private or public? Secondly, goproxy relies on its cache for fetching the same version what if there is an issue there or there is a ratelimit or what about limitations? Which popular OSS repos that do not commit vendor actually use GOPROXY or they just use some other mechanism? It would be nice to list here also the drawbacks that are not addressed by the proposal. |
Nothing stops you from changing the files in |
what is a good practice is IMHO not in scope for this discussion because it really depends, I brought up a concrete case for using vendor, and, I don't think vulnerability patching is not in scope for Knative |
|
@skonto: Go proxy has this value by default for go1.13+: Big companies probably should have their own goproxies, but they most likely should feed out to |
|
what I miss from this discussion is how much maintenance vendor brings to productivity tooling, do you have a concrete list of changes we have made in past 6 months to such "vendor handling tooling"? |
That works locally, how do you do that in CI? There is no risk of committing those changes since there are tests that check for vendor changes |
For CI, you shouldn't do it. Instead, commit an interim package - for example, your personal fork. |
That is clearly a productivity regression since it's not as easy to do |
A rapid and rough search: |
|
You keep saying what I should or shouldn't do but the focus here should be on being productive with the day to day development for people developing Knative and I'm bringing up concrete cases on productivity boosts while using vendor that would be removed without vendor |
I don't see how those changes are related to "checking that vendor match go.mod" and even if there is how is that causing a lot of maintenance |
I'm saying you shouldn't do it because by doing so, you are risking a huge productivity black hole for other people that will need to deal with unexpected changes in vendor directory. Both your fellow contributors, and other projects they may import your packages, may be hurt badly. |
those are quick changes to test stuff, not to be merged nor reviewed |
Then you can just call |
That was a rough search. Some of those PRs were affected by the fact we have a vendor. The point here is that every single automation (linter, compiler, formatter, test) needs to be properly filtered to not take vendor into account (or multiple ones). Here's an example issue, that's still unsolved: knative-extensions/knobots#259 |
A quick browse though GitHub trending projects:
Also, checkout this list: https://github.com/avelino/awesome-go (most of those don't have vendor). BTW. Our friends at https://github.com/sigstore also don't use vendor (cc @mattmoor @n3wscott @vaikas) The general answer would be: The big projects are slow to adapt to new ways of working |
|
Cool I saw that too, this helps to have a better understanding of adoption of the proposed approach.
How builds are done is the next question and how about the GOPROXY limitations? It would be nice to get some insights from people who have put this in practice. |
I don't understand your question: "How builds are done?". Exactly, the same:
I presume you are referring to this article you mentioned earlier? If so, then that article is no longer true. Golang have For example, this is how I install some of our private code, having the default Google's proxy set: Rate-limiting? I'm not aware of any at the default proxy. In fact, the builds are much faster with the proxy, than without it ( |
|
First and foremost, things have indeed changed since the project started as was mentioned in one of the comments in the thread. Choices that were made many years ago should be revisited indeed to see if there are better ways to do things, so thanks for raising the point. The list of all the reasons why packages were originally vendored may not be the same as it was a while ago. Some of the points here seem to be workflow preferences (seems to me, at least, so I will not really get into those. Not trying to dismiss them). This is mainly because I am no longer involved in the code day-to-day, and those that are should be able to decide what works best for them. The one thing that was originally brought up that I do not see discussed that would be good to define the workflow for is how would one handle checking what the actual changes are (I do see the code snippet, but where would that live, and who would run it, etc.). I think this is what @dprotaso is referring to, which means that instead of seeing a go.mod package version change, you will actually also see the real code changes, and while I see your go snippet there, I think these kinds of things are what folks mean it's a lot of work, so I think having some understanding of the real work across all the repositories and who's going to tackle them might make it easier to make tradeoffs. Other item that should be brought up is the tooling to handle the non-go files. |
It could be one of the tools we will deliver. Executed manually Of course, I could also be executed and archived on CI. I wonder how you deal with the issue of deps changes on sigstore project? |
|
The deps management is mainly automated by dependabot and IMHO it makes it pretty easy. If you're manually bumping the deps, there's usually a reason, and therefore you know what you're pulling in. Here's one example (just pulled randomly) where you can easily see what's being changed. Others may have different and better ways, but I like doing this.
|
|
I've been looking at loose ends that we identified to be able to remove the vendor: Kubernetes code-genThere's a rewrite of the K8s code generator scripts - see kubernetes/kubernetes#117262 (K8s 1.28+). With it, and our approach of embedding scripts (knative/hack#222) it should be possible to run the code gen without vendor. I'll also propose to rewrite the K8s code-gen to Go native code, instead of scripts. Until those came to our projects (K8s 1.28+), we could use a fork of the K8s code-gen. Knative code-genOur code-gen scripts can be rewritten to Go files, and executed with Using the files across the reposWe sometimes have symlinks from one repo to another. Most notably in serving (see #134 (comment)) The use of symlinks isn't cool, as it's not platform neutral. We would be better with having those YAMLs extracted at build time (similarly to knative/hack#222). |
|
I am glad you are taking the lead on this @cardil. My 2cents are that we can already start to work on the rewrite into golang. If some of the scripts are simple enough i would keep them as is. Another thing to think about is that rewriting would essentially get us a fork that we would have to maintain so either 1. we should upstream the changes or 2. use what they have with minimal changes to limit the amount of extra things we have to support. I agree about the symlinks, but the reality is they are being used. This should be brought up in TOC so that we get more eyes on it and kindly ask people to get away from this pattern and suggest other solutions to solve the issue that led them to symlinks. Hope to see this land sooner than later! |
|
The proposed roadmap has been presented at TOC (slides: https://docs.google.com/presentation/d/1eK7vhSRnahOq34WYkKJde8hvXMvb8eY3r76Wem5Ia9A) and was accepted. I'll update this issue description to include the issues to be tracked in this epic, which will serve as a roadmap tracker. |
cardil
changed the title
Currently, all the Knative projects are using
go mod vendor, resulting in a huge number of files in thevendordirectory.Reasons for
vendordir removalMaintenance burden for productivity
Having the vendor directory in every project creates a huge maintenance burden for productivity. Every single tool we use (various linters, compilers, formatters etc.) needs to be properly filtered, so it will give off false-positives. Here's a live example: knative-extensions/knobots#259 (there were multiple similar issues in the past)
Hard to count the contributions
Counting contributions is important for OSS projects, as it provides some insights into the health of the project. Having a
vendordir, impedes that, immensely. We need to filter out bots that contribute, and moreover, filter out the people contributions in vendor directory (or vendors in go.work is in place). Otherwise, the amount of the vendor code overwhelms any statistics.Project looks dated
The enhancements in Go that made it possible to work without vendor directory safely has been made a long time ago - Go 1.13 released Sep 3, 2019 (over 3y ago). Since then, most of the code in Go has moved away from having the vendor directory. Sticking with it shows the project is old-fashioned, especially for newcomers. Remember when people attached external jar files to their Java projects, manually? That was before Maven was created to solve dependency management. The same was done for Go in 1.13+ releases.
Bad code habits
Having a vendor directory allows for relying on bad code habits, and hacks. Instead of using that just for Go code, we sometimes use it as a general tool for doing path traversal to some of the resources in the dependent projects. That's a smell, and may lead even to a vulnerability. Example of such smell can be seen in Knative-Serving:
(This is code smell, as symlinks are not cross-platform)
Status
The proposed roadmap has been presented at TOC (slides: https://docs.google.com/presentation/d/1eK7vhSRnahOq34WYkKJde8hvXMvb8eY3r76Wem5Ia9A) and was accepted.
Roadmap
Below is a list of issues that form this Epic.
knative.dev/hackgo package shouldn't contain any go deps hack#318vendor/The text was updated successfully, but these errors were encountered: