From 29f167b3cf7f10e0e3ec2deeb92906aed914dfed Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Fri, 16 Jun 2023 09:03:22 +0100 Subject: [PATCH] further clarifications and updates to the proliferation design doc Signed-off-by: Ashley Davis --- design/20230302.gomod.md | 90 ++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 54 deletions(-) diff --git a/design/20230302.gomod.md b/design/20230302.gomod.md index e0d3f79f5..43b507dea 100644 --- a/design/20230302.gomod.md +++ b/design/20230302.gomod.md @@ -10,7 +10,7 @@ The intention here is to describe what we did and what we discovered, with an ey - 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 +- The fewer dependencies a go module has, the easier it is to maintain - It's OK if people can't import our binaries as go modules ### Solution @@ -20,18 +20,26 @@ The intention here is to describe what we did and what we discovered, with an ey - 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 +- We call `github.com/cert-manager/cert-manager` the **core module** +- We call all other new modules **secondary modules** ### Pros - Each binary can be patched independently - Side effects of a patch are limited to one binary when only that binary has the dependency + - For example, consider updating Helm before go module proliferation + - Updating the Helm version alone won't affect anything which doesn't import Helm + - **But:** Updating Helm also brings in Helm's updated dependencies which _would_ affect other binaries + - E.g. we and Helm depend on the k8s libraries + - That means that bumping Helm forces a bump of all k8s APIs for _all_ binaries + - With proliferation, bumping Helm would still bump the k8s libraries - but _only_ for cmctl! - This includes forking a dependency or needing to `replace` one - - Gives us more control over our own destiny + - In summary: Proliferation gives us more control over our own destiny - Core go.mod dependencies are reduced - All importers of `github.com/cert-manager/cert-manager` have fewer transitive dependencies - Reduced chance of dependency conflicts for all importers - - Including us - in subprojects! + - Including us - in our 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 @@ -42,11 +50,11 @@ The intention here is to describe what we did and what we discovered, with an ey ### Cons -- Using local `replace` statements for binaries will break imports for those binaries - - We assume this won't be too destructive in most cases +- Using local `replace` statements for binaries will break external importers of those binaries + - We assume this won't be too destructive in most cases (since we don't see many importers of those binaries) - 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 + - That would require two PRs in the event that we need to change the secondary module and the core module at the same time + - If the secondary module 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 @@ -98,13 +106,16 @@ We can create several new Go modules so that each binary we build can have disti `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`. +### Terminology + +Currently cert-manager has one module name: `github.com/cert-manager/cert-manager`. This import path is widely used and +we can't break imports of this module. We'll call this the **"core" module.** + +This proposal also introduces several new modules which depend on the core module. We'll call these "secondary" modules. + ### Solution Detail First, we'll add a go.mod file for each binary we ship under `cmd/` - `acmesolver`, `cainjector`, `controller`, `ctl` and `webhook`. @@ -143,37 +154,10 @@ cmd 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). - -```text -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). +As part of this process there will be several import paths which will need to be fixed, but nothing should break. ### Workflow Example: Changing a Binary @@ -181,10 +165,10 @@ NB: See `Importing cert-manager / Development Experience` below for an explorati 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. +would require changes both to at least one secondary module (e.g. the controller) and to the core cert-manager module. 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 +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. @@ -211,7 +195,7 @@ module - or anyone who was previously importing `github.com/cert-manager/cert-ma ### 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 +**In short:** Module replacements help developers but 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](https://stackoverflow.com/a/71984158) to better understand the options we have @@ -228,20 +212,17 @@ For example we could look at the `controller` component which would depend on th 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. +For this example involving `cmd/controller` it might well be acceptable for us to break 3rd party imports but for other +modules that might not be reasonable. In that case, we'll always have a fallback; using a 'regular' import of the core module. -**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. +This would mean that we create two PRs for a change; the first changes the core module, and the second updates the secondary +module to import the new core module version created by the previous PR. + +UPDATE: As we implemented this design, it was decided that we didn't want to break imports of `cmctl` because it was +used in several other cert-manager subprojects - so cmctl uses the approach described above. #### 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. @@ -327,7 +308,7 @@ There's probably some low hanging fruit we could pick here, but we're unlikely t 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 +## Addendum: Groundwork for `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). @@ -335,7 +316,8 @@ 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. +this proposal and we don't propose to do it as part of this work. But the implementation of this design might inform +how we could approach solving the `pkg/apis` problem. ## Footnotes