Migration Design: Rename Bundle to ClusterBundle

[erikgb]

Proposal on how to implement #63.

The proposed solution is based on input from "the greater community". 🥇

[cert-manager-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[erikgb]

/hold

We want to discuss this proposal in our next bi-weekly meeting and allow some time to add missing details.

[damsien]

The design looks good to me 👍
I wanted to open a discussion about the migration path because trust-manager uses helm chart to manage installation & upgrades.

Suppose that an user is currently using the version X. We introduce the version X+1 and then Y. After a few weeks, the user do an helm repo udpate and sees that the version Y is available. If he directly upgrade to the version Y, he will skip the patch and all of its Bundles will not be served and unusable.

[erikgb]

Thanks for reviewing @damsien! ❤️

Suppose that an user is currently using the version X. We introduce the version X+1 and then Y. After a few weeks, the user do an helm repo udpate and sees that the version Y is available. If he directly upgrade to the version Y, he will skip the patch and all of its Bundles will not be served and unusable.

That's why I named it version Y, and not version X+2. 😉 You are addressing a real risk, and we should allow some time (maybe many releases) for users to migrate from Bundle to ClusterBundle. And clearly document the required upgrade path.

[evankanderson]

Since I suggested a bunch of the ideas here, I've added a few thoughts/comments, but feel free to ignore any input that's not helpful -- it's your project, not mine. 😁

[SgtCoDFish]

I think this is brilliantly written and it does fill me with confidence to see this written down! The link to the chat on k8s slack was enlightening too, thanks for that!

General comment: if we implemented this design as written, the only reason for someone to migrate to ClusterBundle is because we (the maintainers) are saying they have to migrate.

There's no new features or anything to draw people to the new name, it's just a thing we changed. That doesn't mean we can't make this change (or that this is a bad change generally), but I think it's better if there's a reason for people to want to migrate, not just a reason they have migrate.

I wrote in one of my review comments about us adding ClusterBundle and making no effort to migrate (yet). If we did that - and started only adding new features to ClusterBundle - eventually, there'd be a reason that people would want to migrate. I think that's worth considering at least? But I wouldn't block progress over this, and this design is great either way!

What do you think? (sorry that's a lot of words)

[erikgb]

There's no new features or anything to draw people to the new name, it's just a thing we changed. That doesn't mean we can't make this change (or that this is a bad change generally), but I think it's better if there's a reason for people to want to migrate, not just a reason they have migrate.

This is addressed by the motivation IMO. Doing this rename will allow us more flexibility in adding new (and already requested) features. In particular a new namespace-scoped Bundle resource. We should also improve the API while migrating, which will, at least IMO, reduce complexity significantly - both in code and testing. And simplicity is always a good platform for adding new features.

If the proposed API changes during migration is accepted, we will provide one new feature:

• Support for multiple target keys of the same type/format

[SgtCoDFish]

/lgtm

I don't see any reason we couldn't proceed with this personally, thanks for the effort on this Erik!

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

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 [SgtCoDFish]

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

[cert-manager-prow]

New changes are detected. LGTM label has been removed.

[ThatsMrTalbot]

Could you add an example YAML of how the new spec will look?

[erikgb]

Could you add an example YAML of how the new spec will look?

It makes to add an example of how we could improve the API when migrating to ClusterBundle. At the same time, this design is mainly a proposal on how to migrate from Bundle to ClusterBundle. The API improvements are not mandatory, but just a very attractive optional thing to do at the same time. 😉

[inteon]

There are two conflicting goals here:

• we do not want to break the behavior when deleting the Bundle resource
• we want to support users who remove the Bundle resource and start using the ClusterBundle instead.

[inteon]

An alternative approach here would be to add owner references and document how to remove those owner references when you want to start using the ClusterBundle as the new "source of truth".

[inteon]

If "owner references" are too complex, we can also implement our own garbage collector (eg. using labels).

[erikgb]

Why do you consider this as "conflicting goals"? I did not mention "do not want to break the behavior when deleting the Bundle resource" as a goal specifically, did I? 🤔 Considering the alpha state of Bundle API, I don't see a big harm in potentially orphaning some resources that can easily be deleted by the user as a workaround. The alternative could be a lot more destructive.

As a user, I don't want my ClusterBundle to be deleted when the corresponding Bundle is deleted. Maybe I am missing your point. Please clarify!

[inteon]

I think we should also add a clear message in the Bundle .status stating that using the Bundle is deprecated.
Also, the validation webhook should throw a warning explaining that the Bundle resources are deprecated.

[erikgb]

CRDs supports deprecating APIs/versions, and we can tailor the warning message. Isn't this enough?

[ThatsMrTalbot]

Im pretty sure the CRD deprecation warning tells you every time you run kubectl against it, we can test this though to make sure thats true with all the commands (describe, get, apply). If that is the case then I think a CRD deprecation warning is enough

[ThatsMrTalbot]

I've often thought that instead of having spec.target.configMap and spec.target.secret we should just have spec.targets which is an array of object references including apiVersion and kind.

This makes supporting future sources much easier, say for example if we wanted to add a plugin system like cert-manager has.

[erikgb]

Interesting! Could you draw an example? 😊

[ThatsMrTalbot]

🤦 I was thinking of sources not targets, for example:

apiVersion: trust-manager.io/v1alpha1
kind: ClusterBundle
metadata:
  name: my-bundle
spec:
  sources:
  # Using apiVersion/kind means we can utilise this same array for 
  # any future "extensions"/"plugins".
  #
  # Loading from other sources could be a valuable feature and this API change
  # would enable that option in the future.
  - apiVersion: v1
    kind: ConfigMap
    name: my-ca
    key: ca.crt
  ...

In my head it would mean that AWS for example could release an operator that exposes the AWSPrivateCABundleSource that a user creates to reference their private ca. Then by referencing that AWSPrivateCABundleSource object in a bundle we pick up the CA.

That kind of support would be added in the longer term, not this piece of work, but the API change could enable that future.

[erikgb]

Good input, thanks! I suggest moving the detailed API changes discussion to #242. I have also opened #486, which will be modified to change the initially unserved ClusterBundle - after it's introduced.

For this design, I suggest we agree that we should consider changing the API. And not exactly what we want to change. Are you ok with this approach?