Controller-runtime: 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 :woman_shrugging:

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.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten

[DirectXMan12]

/remove-lifecycle rotten

[shawn-hurley]

I have not hit this issue, but the scenario seems to be something that would happen pretty often.

I wonder how "easy" it was to determine that this was the case? If it is easy to determine is it easy for a cluster-admin to fix (someone who may not have access to the controller code)?

I assume that it is not easy to fix, and therefore I would agree with the breaking change.

This leads to another concern, which is how does operator X know that its lock is unique? Could there be a case where operator author from two separate controllers choose the same lock name and now a cluster-admin who wants to deploy both is stuck? Is there something that we can do to guarantee that a lock name is for a given controller?

[alvaroaleman]

I wonder how "easy" it was to determine that this was the case?

Wasn't that obvious, as there is currently no logging for this case. Logs are only emitted by klog if you set it to v>=4. That in itself is another issue we definitely should fix.

This leads to another concern, which is how does operator X know that its lock is unique? Could there be a case where operator author from two separate controllers choose the same lock name and now a cluster-admin who wants to deploy both is stuck? Is there something that we can do to guarantee that a lock name is for a given controller?

Well in the scaffolding it would be possible to generate a UUID I guess. In cases where ppl use controller-runtime directly, we can only try make doing the wrong thing as hard as possible and IMHO its too easy right now with the default.

[pires]

To me, the problem as described by the OP does justify changing the default behavior to something that may increase cognitive load of adopters, if it prevents them from having the same or larger cognitive load when the problem surfaces.

With that said, I wonder if CR could default to a UUID while allowing an adopter to explicitly opt-in for an override? If an adopter overrides, they are cognizant of the problem(s) they may face, wouldn't you agree?

[alvaroaleman]

@pires we talked about this during kubecon and agreed that this should be done, but Solly is not present so there is no one who can approve the PR :grimacing:

With that said, I wonder if CR could default to a UUID while allowing an adopter to explicitly opt-in for an override? If an adopter overrides, they are cognizant of the problem(s) they may face, wouldn't you agree?

Kubebuilder (the scaffolding tool) could do that, controller-runtime can not really do that, as it must be both unique per controller manager and deterministic. An UUID would give the first but not the second, effectively disabling leader election as the name of the lock changes on every run.

[pires]

Oh, sorry I missed that and thanks for clarifying.

Out of curiosity, did anyone consider to actually disable leader-election by default in CR?

[alvaroaleman]

Out of curiosity, did anyone consider to actually disable leader-election by default in CR?

It is disabled by default already.

[DirectXMan12]

We discussed this at the f2f a bit, but I've had too much turkey this past week to remember exactly what the decision was. @shawn-hurley has the meeting notes I think.

[shawn-hurley]

from my notes:

The outcome that I have written down is that we want to force users to specify a unique lock name for per-controller locks. Will default to the manager’s lock name but allow people to override.