search

cert-manager / trust-manager

trust-manager is an operator for distributing trust bundles across a Kubernetes cluster.
https://cert-manager.io/docs/projects/trust-manager/
Apache License 2.0
243 stars 65 forks source link

chore: reconcile Bundle status unconditionally #236

Closed erikgb closed 6 months ago

erikgb commented 9 months ago

This is my next step towards simpler code after our SSA-migration. The main change in this PR is that an already synced Bundle now will submit a probable no-op patch of Bundle status to the api-server. The number of bundle resources in a cluster is likely limited, and the simplifications of controller code should justify the slightly additional load on the api-server. Already in this PR, I propose deleting two utility functions with tests, but there is probably more that could be cleaned up around the sync of target configmaps and secrets. Please let me know if this could/should be done in this PR. If not, I will submit a follow-up PR.

/cc @inteon @SgtCoDFish

jetstack-bot commented 9 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from erikgb. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/cert-manager/trust-manager/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
erikgb commented 9 months ago

Do you know what the effect of this change is wrt. apiserver-load at scale? I think we should do a bit of testing before we start to unconditionally patch.

If we have a reconciliation loop caused by bugs in code or controllers fighting over resources, this could cause issues, but I consider it quite unlikely. And IMO those problems should not be hidden by conditionals in the reconcile func. But more testing can never harm. How do you think this can be tested better?

SgtCoDFish commented 9 months ago

Summarising what I remember from the standup: I think we agreed that there's a case to be made for this (not least because it should be possible for some tooling to inspect the completed patch at the end of the process and not send if it's a no-op, which would mitigate the increased load on the API server that this could introduce).

We also agreed that this was lower priority than a future PR to use upstream condition types and lower priority than #239 - so we'll focus on that future condition PR and then on #239 for now!

jetstack-bot commented 8 months ago

PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
erikgb commented 6 months ago

It seems like there is no desire to use applyconfigurations in the maintainer team, closing.