Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split Bundle controller into multiple controllers #245

Open
erikgb opened this issue Nov 26, 2023 · 3 comments
Open

Split Bundle controller into multiple controllers #245

erikgb opened this issue Nov 26, 2023 · 3 comments
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@erikgb
Copy link
Contributor

erikgb commented Nov 26, 2023

We currently have a single controller in trust-manager, and this controller has a lot of responsibilities. I can identify at least the following:

  • Prepare target configmap/secret content from sources (buildSourceBundle)
  • Create/delete target configmaps/secrets in namespaces matching namespaceSelector
  • Ensure target configmaps/secrets CA bundle content is up-to-date

Since we always reconcile bundles, this creates a long and quite complex reconcile process. I have been looking a implementing #58, and that would a least require a new controller. Introducing a new controller is doable, but we would need a lot of the logic from the existing bundle controller. And waste compute resources by building everything from scratch.

I would suggest splitting the single existing controller into:

  • Controller for bundles + watching sources configmaps/secrets (in trust namespace). The output from this controller should be a configmap/secret in trust namespace acting as a prototype for all target configmaps/secrets.
  • Controller for bundles (namespaceSelector) + watching namespaces. This controller will be responsible for creating/deleting target configmaps/secrets in namespaces matching/not-matching namespaceSelector. New targets will be created from the bundle target prototype resource.
  • Controller for bundle target prototype configmaps/secrets. It will ensure target configmaps/secrets are good copies of their prototype. Note: This controller should not create/delete target configmaps/secrets.
  • Controller for target configmaps/secrets. It should simply ensure the target configmap/secret is a good copy of it's prototype.

This might seem complex, and I am happy to discuss adjustments. 😃 I think the first controller is the most important, as it will ensure we process bundle sources once per bundle.

I see the following benefits from this refactoring:

  • More efficient processing, avoiding doing potentially heavy processing over and over again.
  • No need to have conditional operations requiring helpers with dedicated tests, ref. chore: reconcile Bundle status unconditionally #236
  • Better separation of concern. If you want to read the current bundle reconcile from start to finish you have to look through most of the code in trust-manager. 😉
  • Easier testing - as it will only be the first controller that needs to work with X.509 stuff. The rest is just technical Kubernetes controller operations - that can be tested with dummy trust bundle content.

A potential downside/challenge, is maintaining the Bundle Synced condition. But do we really need it? What about heading for the kstatus approach, where the idea is to add conditions when something is wrong.

CC @inteon @SgtCoDFish

@arsenalzp
Copy link
Contributor

Hello,
If you need help with this bunch of tasks, I kindly help you with that!

@erikgb
Copy link
Contributor Author

erikgb commented Dec 15, 2023

If you need help with this bunch of tasks, I kindly help you with that!

Thanks @arsenalzp, but we have discussed this a bit and decided to put it a bit on hold. It will increase the complexity of our controller machinery. So not sure if we ever want to do this.

@cert-manager-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
/lifecycle stale

@cert-manager-prow cert-manager-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

3 participants