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

Consider decoupling helm-controller from resulting binary #148

Open
mallardduck opened this issue Jan 21, 2025 · 0 comments
Open

Consider decoupling helm-controller from resulting binary #148

mallardduck opened this issue Jan 21, 2025 · 0 comments

Comments

@mallardduck
Copy link
Member

Per title, this issue/RFE is simply for our team to consider the potential for fully removing the embedded helm-controller.

Summary

The primary objective is to de-couple PromFed from helm-controller - specifically in a manner that decouples from "a specific helm-controller version". So instead the HelmChart functionality would be owned by a more explicitly defined controller - external of using any "embedded" one from PromFed. In turn this reduces complexity around PromFed maintenance and compatibility.

Important Context:

  • helm-controller supports either: a single global controller watching every NS, or many NS scoped instances.
  • helm-controller doesn't support a mixture of both of these modes in the same cluster.
  • The existing PromFed embedded helm-controller is always locked at a single version (at build time).
  • k3s/RKE2 will update helm-controller versions on patch releases.
  • The existing PromFed embedded helm-controller both:
    • Installs under a new name, and
    • Installs under a namespace, so becomes namespace scoped.
  • Technically, the ManagedBy annotation that helm-controller supports does allow multiple controller instances to have overlapping namespace scopes w/o conflict via Alternative names.
    • In other words, mixing global and namespaced ones works with specific steps taken - as that's partially how PromFed can co-exist with global ones with proper configs.
    • The lease mechanism will allow both: global and/or many ns-scoped instances to hold a lease.
      • Using both together isn't officially supposed, as mentioned above, but technically possible as long as each NS specific instance has a custom ControllerName set to differentiate ManagedBy and ensure global instance won't touch the ones for NS-scoped controllers.
      • The current helm-controller lease mechanism will not allow locks for: multiple global intances, or multiple ns-scoped instances within the same ns. (In this case, a second Global instance lock will overlap with the first and similarly any additional ns-scoped in the same NS overlap.)

Options 1. Remove helm-controller as a godep for PromFed, make it a sibling container instead

This option, at a high level, entails keeping the existing functionality we surface to PromFed users today. However doing so in a much different mechanism at a k8s level. Instead of a true "embedded" controller in PromFed binary, it would become a new helm-controller pod in the deployment. Still "embedded" but not statically at compile time.

Pros:

  • Removes the concern about if PromFed manages CRDs for HelmChart at all (i.e. now the only CRDs it cares about are ones it created),
  • End-users can change the version of helm-controller by updating the image tag,
  • User-story for "how to configure in complex situation" has a much easier answer
  • Complex faults/errors situaitons w/ multiple helm-controllers incorrectly configured become more clear
    • It still might be PromFed chart related, but potential config issues are more obvious w/ PromFed deploying a helm-controller pod.
  • 3 Simple Supported Config Setups:
    • PromFed Chart only possible helm-controller on Cluster: The helm-controller w/ PromFed must be used (now a pod).
    • PromFed Chart on Cluster with existing Global Deployed helm-controller: Cannot have PromFed's helm-controller enabled.
      • This matches my understanding - after talking with @brandond - RE mixing global and namespace scoped helm-controller instances.
      • Essentially the PromFed Embedded Controller (as it exists today) is a "namespaced instance" listening on cattle-monitoring-system; and if using a global one (built-in on k3s/rke2, or manually on any other k8s) then namespaced ones aren't allowed.
  • PromFed Chart on Cluster with Namespace scoped controller instances: User's can either: a) deploy one for PromFed manually, or b) use the PromFed one and match the helm-controller version with the existing ones on their cluster.

Cons:

  • PromFed is decoupled from HelmController but still relies on it.
    • Potentially increases matrix of supportable configs QA wants to test;
    • Doesn't have to though, if they scope their testing to the versions we set as default and the defaults used when testing on RKE2/k3s?
  • To use PromFed (w/o other helm-controllers on cluster) customers now need 2 images instead of 1 (low impact)
  • Requires changes to the charts and code to properly deprecate and switch
    • Tho there's lots of room to just interpret existing chart values into a new deployment file; doesn't necessarily require a breaking change

Option 2. Status Quo: Keep helm-controller embedded into binary at build time

Pros:

  • Not changing anything is easy

Cons:

  • Compatibility among each version of k8s that each Rancher version supports is complex
  • Certain config situations are very ambiguous and can lead to complex issues to debug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants