Remove defaulting for leader election ID

[alvaroaleman]

Currently, the leader election ID defaults to controller-leader-election-helper. Since it is convenient to not configure anything for the leader election ID, it will crash whenever two controller-runtime based controllers run in the same namespace or have the same namespace configured for the leader election (e.G. kube-system).

This is especially bad since there is no logging about the lock being held by a different controller, so from a users perspective this looks like the controller just froze.

[coderanger]

#444 gives us another option here, since we could use the controller name in the default value.

[alvaroaleman]

#444 is a nice change, but when ppl use a kubebuilder-generated controller, there is a decent chance,two controllers will have the same name.

Regardless of that thought, the leader election package itself should not have a defaulting, because that is very easy to get wrong and silently cause issues later on.

[coderanger]

Hmm, would have to double check but I would think the name string would be set based on the controller name you scaffolded. So conflicts seem unlikely other than maybe people following the tutorial?

[alvaroaleman]

Hmm, would have to double check but I would think the name string would be set based on the controller name you scaffolded. So conflicts seem unlikely other than maybe people following the tutorial?

IIRC, the controller name is based on the resource you care for, so there is a somewhat decent chance there will be e.G. multiple persistent-volume-controllers.

I think for the scaffolding this is not a problem at all, because it could just generate some random string as suffix, this is mainly an issue for ppl who directly use controller-runtime.

[mengqiy]

I agree that doing defaulting in leader election in controller-runtime can be error-prone.

BTW, we are trying to use leader election with deployment in kubebuilder by default. We are using downward API to use the same namespace as the pod.
https://github.com/kubernetes-sigs/kubebuilder/pull/727/files#diff-f66e269e20dbb6060baad28302f5a3a3R59

[DirectXMan12]

So, at the very least we should log something. That's easy to do, and you want that anyway.

I think controller name is probably sane here -- the pattern we scaffold out in KB is a namespace per manager (NB: leader election defaults to the in-cluster namespace provided along with the SA token), which provides some amount of defense against this, plus name per controller provides further defense. Furthermore, leader election isn't on by default.

Thus, to have issues, you have to either

a) not be using kubebuilder, enabled leader election, not change the defaults, and run multiple controllers with the same name in the same namespace

or

b) override the config that kubebuilder scaffolds out to specify a non-unique namespace

I view this as a kind of "you've turned on leader election, so be aware of what you're doing". I'm happy to hear rebuttals to this line of logic though.

[alvaroaleman]

I think controller name is probably sane here -- the pattern we scaffold out in KB is a namespace per manager

I would think issues arise here mainly for ppl that use controller-runtime but do not use kubebuilder. This probably raises the bigger philosophical question if its fine to do changes in controller-runtime that are are not required for ppl that use kubebuilder for scaffolding.

Thus, to have issues, you have to either
a) not be using kubebuilder, enabled leader election, not change the defaults, and run multiple controllers with the same name in the same namespace
or
b) override the config that kubebuilder scaffolds out to specify a non-unique namespace

A combination of both actually and the fact that the "controller-name for leadereletion pull is not meged": We have a controller that uses controller-runtime and not kubebuilder, runs outside of the cluster it talks to and creates its leader-election lock in kube-system. The user of that cluster then deployed another controller that was scaffolded with kubebuilder but changed (I guess?) to create its lock in kube-system as well.

you've turned on leader election, so be aware of what you're doing

Thats a valid POV, another valid POV is that we should make it as hard as possible to not do the wrong thing. And today, its imho pretty easy to get this wrong.

[DirectXMan12]

if its fine to do changes in controller-runtime that are are not required for ppl that use kubebuilder for scaffolding.

In general, I try to assume that people will use CR without KB, and make that easy. The question mainly comes from when the two conflict 🤷‍♀️

Thats a valid POV, another valid POV is that we should make it as hard as possible to not do the wrong thing. And today, its imho pretty easy to get this wrong.

I generally agree with that sentiment. My main hesitance is that without a default, you now need to know how to name the leader election lock, even in the semi-common (as far as i'm aware) case of running in separate namespaces. I'm not terribly opposed to removing the default (and scaffolding it back out in KB), I'm just debating whether or not this makes things significantly more annoying for the common case. Every thing you make someone manually specify is extra cognitive overhead. On the other hand, once we have per-controller LE, the point's kinda moot, since the LE name is at best a prefix.

TL;DR: If the default is a significant foot-gun for multiple people, I'm fine removing it. I am/was just trying to weigh the impact on the different use cases.

[fejta-bot]

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.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[alvaroaleman]

TL;DR: If the default is a significant foot-gun for multiple people, I'm fine removing it. I am/was just trying to weigh the impact on the different use cases.

TBH I don't really know how to find that out. Send a mail? Ask in Slack? Wait if anyone else shows up on this issue?

[DirectXMan12]

Maybe send out a mail? I'm not particularly sure either. We could send out a query on the KB mailing list, and direct them to vote/weigh in here.