18 KiB
Design: More Modules
NB: This design doc follows from a Hackathon by @SgtCoDFish and @inteon.
The intention here is to describe what we did and what we discovered, with an eye to seeking consensus and merging upstream.
In Short
Assumptions / Axioms
- It's hard or impossible to upgrade our dependencies months after a release
- We won't change our conservative approach to backports
- Having fewer dependencies in our go modules is a good thing
- It's OK if people can't import our binaries as go modules
Solution
- Create a go module for each binary
- Create go modules for integration and e2e tests
- Utilise local replace statements where possible
- i.e. Binaries have a local replace for the core cert-manager module
- This breaks imports of those binaries but means changes only require one PR
Pros
-
Each binary can be patched independently
- Side effects of a patch are limited to one binary when only that binary has the dependency
- This includes forking a dependency or needing to
replaceone - Gives us more control over our own destiny
-
Core go.mod dependencies are reduced
- All importers of
github.com/cert-manager/cert-managerhave fewer transitive dependencies - Reduced chance of dependency conflicts for all importers
- Including us - in subprojects!
- Many people need to import cert-manager! (pkg/apis, etc).
- We might split things more in the future - this is a good first step
- All importers of
-
Lays the groundwork for further splitting out binaries / packages
- This is the start of what we'll do if we want cmctl to be its own repo
- Or splitting
pkg/apisinto a separate module - Or splitting issuers into a module (to isolate cloud SDK dependencies)
Cons
-
Using local
replacestatements for binaries will break imports for those binaries- We assume this won't be too destructive in most cases
- If we need to make binaries importable again, we can change them to use regular import statements
- That would require two PRs in the event that we need to change the binary + the core module at the same time
- If the binary would've ended up in a separate repo anyway (e.g. cmctl) we'd have done this eventually
-
Increased complexity in working with the codebase
- E.g.
go test ./...no longer tests everything, since it won't recurse into modules - This can be alleviated with some Makefile work -
make testcan still test everything - Go Workspaces (
go.work) can also help in development environments to make things simpler
- E.g.
Longer Form Problem Statement
In short: Some of our dependencies are complex which makes them hard to upgrade in already-released versions
Upgrading the dependencies of even simple Go projects can be tricky and for a more complex project like cert-manager it can be impossible to upgrade dependencies for older releases while satisfying all constraints that we place on ourselves as maintainers.
In our case, these constraints are to:
- Minimise / eliminate CVE reports for any supported release of cert-manager
- Be conservative about upgrades, and avoid major version bumps in already-released software
Since we have one go.mod file for all of our built binaries, it's not possible for us to be selective about upgrades,
either. If, say, only the controller component were to report as having a critical vulnerability, we'd have no
way of fixing only that one vulnerability while leaving everything else untouched.
Essentially, our current project layout forces us to made difficult choices whenever we need to upgrade things.
Problem Example
In short: An example of how upgrades can be particularly difficult in some cases, with no good options.
At the time of writing, cert-manager 1.10 is still in support and depends on Helm because it's imported by cmctl (and
only cmctl). We can see the dependency in go.mod.
There's a vulnerability reported for Helm v3.10.3 ([1]) which we'd like to patch, but the only version with a fix available is Helm v3.11.1.
Between Helm 3.10 and 3.11, several of Helm's dependencies were upgraded, and crucially Helm has some of the same dependencies that cert-manager does. That means that we can't easily just upgrade Helm.
Running go get -u helm.sh/helm/v3 produces 56 different upgrades of other dependencies. Notably, it bumps our
Kubernetes dependencies from v0.25.2 to v0.26.0 but there are several other changes.
(NB: Helm is just an example here and we could have problems with any package)
Proposed Solution: Go Module Proliferation
In short: Add several new go.mod files so individual components can be patched independently
We can create several new Go modules so that each binary we build can have distinct dependencies. This would mean that
cmctl having a dependency on Helm would only affect cmctl and wouldn't force us to change any of the other
components we build in order to patch a Helm vulnerability.
In addition to separating out the binaries, we could also separate out the "core" issuer implementations into a
distinct module (since they bring so many external dependencies and currently live in pkg/ where they'll be part of
the core cert-manager go.mod file).
Plus, where we have testing-only dependencies (e.g. for integration or end-to-end tests) we could create a test module
so that those test dependencies don't pollute the main go.mod.
Solution Detail
First, we'll add a go.mod file for each binary we ship under cmd/ - acmesolver, cainjector, controller, ctl and webhook.
These new modules should resolve to having identical dependencies to what they currently have (i.e. we shouldn't bump any versions at this stage).
cmd
├── acmesolver
│ ├── ...
│ ├── go.mod
│ ├── go.sum
│ ├── main.go
├── cainjector
│ ├── ...
│ ├── go.mod
│ ├── go.sum
│ └── main.go
├── controller
│ ├── ...
│ ├── go.mod
│ ├── go.sum
│ └── main.go
├── ctl
│ ├── go.mod
│ ├── go.sum
│ ├── main.go
│ └── ...
└── webhook
├── go.mod
├── go.sum
├── main.go
└── ...
These changes will also require tweaks to how modules are built and tested, which will be done in our Makefile.
Plus there are some imports of cmd under test/ which will need to be changed or updated.
After these changes, running go mod tidy on the core cert-manager module should clean a lot of dependencies but will
leave many SDKs since they're depended on by issuer logic which is in pkg/.
Next, we'll abstract this issuer-specific logic into its own module. In the Hackathon where this concept was originally
explored, we named this module intree (as in, in-tree issuers). Any references to this code will be removed from pkg,
which should remove dependencies on cloud SDKs from cert-manager's core module, leaving them only in the new controller
module (note that separating these issuers into a separate binary is a separate proposal. This proposal keeps those issuers
in the controller component for now).
intree
└── issuers
├── acme
│ ├── ...
│ ├── go.mod
│ ├── go.sum
│ └── issuer
│ └── ...
├── vault
│ ├── ...
│ ├── go.mod
│ └── go.sum
└── venafi
├── ...
├── go.mod
└── go.sum
There'll also be some test imports at this point which will need to be moved around (some detail below).
Workflow Example: Changing a Binary
NB: See Importing cert-manager / Development Experience below for an exploration of the problems we face here and reasoning
behind the proposed solution.
As an example of the kind of change being discussed, imagine adding a new field to our CRDs along with a feature gate. This would require changes both to the binaries (e.g. the controller) and to the core cert-manager packages.
In order to avoid having to make two PRs for this kind of change we propose to explicitly state that any external import of
any of the new modules under cmd is not supported. By breaking this kind of external import, we can use the replace directive
in the new go.mod files for each of the binaries to refer to the cert-manager package in the same repository.
This means that every change to pkg/ will automatically be picked up by all of the binaries that we build and test in CI.
An example of the replace directive is given below:
module github.com/cert-manager/cert-manager/controller-binary
go 1.19
replace github.com/cert-manager/cert-manager => ../../
require (
github.com/cert-manager/cert-manager v0.0.0-00010101000000-000000000000
...
)
To be clear: using replace directives like this will break anyone who tries to import the github.com/cert-manager/cert-manager/controller-binary
module - or anyone who was previously importing github.com/cert-manager/cert-manager/cmd/controller before this proposal.
Potential Issues
Importing cert-manager / Development Experience
In short: Module replacements help developers but don't aren't respected by imports, meaning some changes could need two PRs or we'd have to break anyone importing certain modules
Useful Reference: It helps to read this StackOverflow comment to better understand the options we have
The simplest development experience when working with multiple Go modules at once is to use either the replace directive
in go.mod or the use directive in go.work to point to local versions of a module. This allows both modules to be
developed in parallel on a local machine.
For modules which we don't think should ever be imported by third parties, replace directives would work so that those modules always use the version of cert-manager which is at the same commit as those modules.
For example we could look at the controller component which would depend on the core cert-manager module. Its
go.mod might look like the example given above under "Workflow Example: Changing a Binary".
An issue with this approach is that the replace statement wouldn't be respected if anyone imports the controller module
from a 3rd party project. Instead, that 3rd party would see an error relating to an unknown version of cert-manager.
For cmd/controller it might be acceptable for us to break 3rd party imports - that's a decision we'd have to make -
but for other modules that might not be reasonable. For example, if we created a new module for each of the built-in
issuers, it would depend on the core cert-manager module but would also need to be imported by the controller
component.
In that case we'd not be able to use replace directives and we'd need to refer to specific cert-manager versions.
That would in turn mean that in order to make a change to both the issuer module and the core module, we'd have to first
create a PR for the core module, get that merged and then create a PR for the issuers module which includes an update
to the issuers go.mod to point at the new cert-manager version.
Potential Solution for Developer Experience: Dynamic go.work
(Untested)
We could introduce a make target which generates one or more go.work files locally to point all modules at local
development versions. This doesn't help with having to raise two PRs for a change, but it does help minimise the
burden of testing changes locally.
This could mean that users won't notice if they forget to bump their go.mod files to point at a new release of the core
module - but tests should fail in CI to alert them of this problem.
Running Tests
In short: Multiple modules in one repo break go test ./...
Part of the migration to Make enabled the use of go test for testing. Under the hood, our make targets essentially
use go test themselves.
The issue is that go test won't recurse into other modules. If we make cmd/controller a separate module, then
go test ./pkg/... ./cmd/... won't run any of the tests in cmd/controller. Any existing uses of go test ./...
which intend to test everything will silently start to not test everything.
This can be mitigated by leaning more heavily on make; we can have make test run the tests for every module. It's a
shame to lose the ability to test everything with go test in this way, but the tradeoff ultimately seems worth it.
Test Modules
In short: The test/ directory could (should) be a module but part of it is imported elsewhere.
The test/ directory at the root of the cert-manager repo today has several purposes.
test/unit provides a library which is imported by several other packages, to aid with setting up data for unit tests.
For example, pkg/controller/certificatesigningrequests/ca/ca_test.go imports the test/unit/gen package to aid
with generating test data. test/internal has similar content to test/unit, but focusing more on utility functions.
test/integration and test/e2e implement actual tests which are designed to run against cert-manager but which don't
fit under the category of unit tests. These test directories have external dependencies including on cloudflare-go and
the Hashicorp Vault API along with imports for the cmctl and cert-manager webhook code.
Essentially, the test/ directory has both actual tests and test utility code. The actual tests import several
areas of cert-manager which become external modules under these proposals, and the utility code is imported by the core
cert-manager module.
Solution: Split Test Code
Since there are two types of code in test/, we can split it.
There are known external importers
of test/unit/ which means it's difficult to move that without breaking people.
As such, we could move test/e2e and test/integration - or we could make them both independent modules and keep them where they are.
The diff for the main repository go.mod after separating out the tests is presented in footnote [2].
Increased Time to Patch Everything
Having multiple go.mod files wil mean that when we share a dependency across many components (such as the Kubernetes
libraries) we'll have to update multiple files rather than just one. Alternatively, if we update a dependency for the
core go.mod file we'll maybe want to also update every other go.mod which imports that one.
Other Considered Approaches
Being Less Conservative
The main issue we face with upgrading older versions of cert-manager is that we self-impose strict conservatism when it comes to any kind of backport. In this view, any change for any reason is inherently seen as bad and to be avoided, even if that change has no runtime impact for users.
We don't need to do this. While we wouldn't seek to make major version upgrades in backports just for the fun of it, we could choose to accept a larger subset of backports and rely on our tests to confirm that the change is sound.
This doesn't solve the problems of allowing independent control over the dependencies of different binaries, though, and doesn't reduce the attack surface of any of our components.
Aggressively Reducing Dependencies
Rather than isolating dependencies, we could remove them by e.g. vendoring subsets of their code into our repo. This gives us a huge amount of control and allows us to preserve backwards compatibility very easily.
It also creates a huge burden for us to maintain that vendored code, which is a drawback. We'd still have to track e.g. Helm to see if there are any relevant vulnerabilities reported, and then we'd have to go and actually fix them ourselves. If upstream code diverged significantly we might be left on our own trying to work out how to fix bugs - or even trying to work out if we even have a bug.
There's probably some low hanging fruit we could pick here, but we're unlikely to be able to fully remove a big chunk of our dependencies. That means the problem won't go away - and there's always the chance that we need to add new dependencies down the road.
Stretch Goal: pkg/apis Module
We've talked before about creating a separate pkg/apis module or repo to improve the experience for users who need
to import that specific path (which is common).
Module proliferation could be a solution here by making that path a new module.
Changing the pkg/apis module isn't really related to reducing dependencies so it's a little different to the rest of
this proposal but it might be an easy solution.
Footnotes
[1] cert-manager likely isn't actively vulnerable to these specific Helm CVEs, but it's easy to imagine something being reported which it's actually vulnerable to and which we'd have to upgrade.
[2] The diff from separating integration and e2e tests into their own modules:
diff --git a/go.mod b/go.mod
index c95d5fbe3..ef3fcfc64 100644
--- a/go.mod
+++ b/go.mod
@@ -10,7 +10,6 @@ require (
github.com/Venafi/vcert/v4 v4.0.0-00010101000000-000000000000
github.com/akamai/AkamaiOPEN-edgegrid-golang v1.2.2
github.com/aws/aws-sdk-go v1.44.179
- github.com/cloudflare/cloudflare-go v0.58.1
github.com/cpu/goacmedns v0.1.1
github.com/digitalocean/godo v1.93.0
github.com/go-ldap/ldap/v3 v3.4.4
@@ -22,14 +21,10 @@ require (
github.com/kr/pretty v0.3.1
github.com/miekg/dns v1.1.50
github.com/mitchellh/go-homedir v1.1.0
- github.com/munnerz/crd-schema-fuzz v1.0.0
github.com/onsi/ginkgo/v2 v2.7.0
- github.com/onsi/gomega v1.24.2
github.com/pavlo-v-chernykh/keystore-go/v4 v4.4.1
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.14.0
- github.com/segmentio/encoding v0.3.6
- github.com/sergi/go-diff v1.3.1
github.com/spf13/cobra v1.6.1
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.8.1
@@ -203,7 +198,7 @@ require (
github.com/rubenv/sql-migrate v1.2.0 // indirect
github.com/russross/blackfriday/v2 v2.1.0 // indirect
github.com/ryanuber/go-glob v1.0.0 // indirect
- github.com/segmentio/asm v1.1.3 // indirect
+ github.com/sergi/go-diff v1.3.1 // indirect
github.com/shopspring/decimal v1.2.0 // indirect
github.com/sirupsen/logrus v1.9.0 // indirect
github.com/spf13/cast v1.4.1 // indirect