Client-go: Import leader election code?

[yifan-gu]

As a user, if I want to use the leader election code (https://github.com/kubernetes/kubernetes/tree/master/pkg/client/leaderelection), then I have to switch back to import from kubernetes/pkg/client because the types defined here has a different path with the types defined in kubernetes/pkg/client. e.g.

leader_election.go:33: cannot use c (type *Client) as type "github.com/yifan-gu/test/vendor/k8s.io/kubernetes/pkg/client/unversioned".Interface in field value:
    *Client does not implement "github.com/yifan-gu/test/vendor/k8s.io/kubernetes/pkg/client/unversioned".Interface (wrong type for Apps method)
        have Apps() "github.com/yifan-gu/test/vendor/k8s.io/client-go/1.4/kubernetes/typed/apps/v1alpha1".AppsInterface
        want Apps() "github.com/yifan-gu/test/vendor/k8s.io/kubernetes/pkg/client/unversioned".AppsInterface

[deads2k]

I don't think I want that leader election code escaping into the wild. Leader election has some generic utility, but I'd like to see an implementation of it that does not rely upon access to endpoints. Write access to endpoints is pretty powerful.

[yifan-gu]

Leader election has some generic utility, but I'd like to see an implementation of it that does not rely upon access to endpoints. Write access to endpoints is pretty powerful.

@deads2k Can you elaborate? I am confused why leader election is more special here than other writable API (create, update)?
Sorry I don't have much experience here.

[wongma7]

looks like reliance on endpoints has been abstracted away https://github.com/kubernetes/kubernetes/pull/33387

[xiang90]

@caesarxuchao @lavalamp Do you plan to add this API into client-go?

[caesarxuchao]

I'm OOO now. I sent https://github.com/kubernetes/kubernetes/pull/39173 before I left. I can rebase it sometime next week.

[lavalamp]

I think we should add a version of this that doesn't use the endpoints
object as the coordination point into the client. I agree with @deads2k
that having a ton of clients that we don't control trying to write to
endpoints will make our lives extremely miserable, so let's try to avoid
that.

On Fri, Dec 30, 2016 at 5:35 PM, Chao Xu [e-mail] wrote:

I'm OOO now. I sent kubernetes/kubernetes#39173
https://github.com/kubernetes/kubernetes/pull/39173 before I left. I
can rebase it sometime next week.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/client-go/issues/28#issuecomment-269842273,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglrhNfai6jMUWBbh81bHBGJ8OHe7dks5rNbFFgaJpZM4KpHxj
.

[caesarxuchao]

@deads2k @lavalamp I agree that having leaderelection package writing to endpoints is too powerful. Is there anyone working on fixing it? Is there an ETA?

If it's going to take long time to fix leaderelection, I'll prefer putting a WARNING in the package comment stating that "using this package will cause security risk", and shipping the package in client-go as is. I have been requested by downstream users to provide leader election in client-go, and they don't care about the security risk.

[hongchaodeng]

I think @caesarxuchao 's point is that there are many users relying on client-go . To users, it is an interface that does leader election in k8s native way. If users wants to write another component on top of k8s, they need leader election pacakages the same as other k8s components.

The same question on writing to endpoints issue. Is there any ETA that this will be fixed?

[caesarxuchao]

@mikedanese do you have anyone who can work on leader election?

[deads2k]

If it's going to take long time to fix leaderelection, I'll prefer putting a WARNING in the package comment stating that "using this package will cause security risk", and shipping the package in client-go as is. I have been requested by downstream users to provide leader election in client-go, and they don't care about the security risk.

I don't think that shipping a known security risk a reasonable thing to do.

[deads2k]

I am curious about the general use-case for this. Since controllers can be written to tolerate having multiple running ("normal" controllers just run duplicate work for a while), why not just have a RS with size 1? It's not like the other controllers are even warming caches. They're just sitting there doing nothing but trying to get a lease.

[xiang90]

Since controllers can be written to tolerate having multiple running

This is a bad argument. I do not think we can assume anything here. There are (or will be) more external controllers than internal controllers.

The reason for having the election endpoint is to make it easier to write controller (internal or external) to tolerate having multiple copies running, not the other way around.

Not all controllers can run duplicate work for a while for now. Controllers could assume they own the objects exclusively using election. It is unreasonable to ask people to not rely on it anymore, just because they can remove this assumption and add some extra code internally to protect it. We should make other people's life easier not harder.

Basically, some existing controllers can, now, tolerate multiple copies running since they use leader election.

If all applications and controllers can implement leasing, distributed locking logic internally, correctly and easily, then it is pointless to introduce the election endpoint at the first place.

/cc @IanLewis

[lavalamp]

I wouldn't call this a "security" risk, it's more like "cluster stability"
mixed with "future proof".

The point of having more than one controller at all times is that you need
a hot standby, otherwise if the active controller (which is the process
implementing the controllers) goes down, you're sunk. (this is used in our
own controller manager binary.)

On Tue, Jan 31, 2017 at 8:29 AM, Xiang Li [e-mail] wrote:

Since controllers can be written to tolerate having multiple running

This is a bad argument. I do not think we can assume anything here. There
are (or will be) more external controllers than internal controllers.

The reason for having the election endpoint is to make it easier to write
controller (internal or external) to tolerate having multiple copies
running, not the other way around.

Not all controllers can run duplicate work for a while for now.
Controllers could assume they own the objects exclusively using election.
It is unreasonable to ask people to not rely on it anymore, just because
they can remove this assumption and add some extra code internally to
protect it. We should make other people's life easier not harder.

Basically, some existing controllers can, now, tolerate multiple copies
running since they use leader election.

If all applications and controllers can implement leasing, distributed
locking logic internally, correctly and easily, then it is pointless to
introduce the election endpoint at the first place.

/cc @IanLewis https://github.com/IanLewis

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/client-go/issues/28#issuecomment-276413950,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglvKr-Ln_fO3GuZV9VjQ2RwE_Dkmxks5rX2FXgaJpZM4KpHxj
.

[deads2k]

a hot standby, otherwise if the active controller (which is the process
implementing the controllers) goes down, you're sunk. (this is used in our
own controller manager binary.)

As I recall the plug point, we don't even start the caches warming until its activated, so its not a very hot standby.

[lavalamp]

It won't work if apiserver goes down. But it's there running already if the
other controller manager goes down. And it doesn't need to be created or
scheduled. So it will start with very high probability.

On Tue, Jan 31, 2017 at 10:50 AM, David Eads [e-mail]
wrote:

a hot standby, otherwise if the active controller (which is the process
implementing the controllers) goes down, you're sunk. (this is used in our
own controller manager binary.)

As I recall the plug point, we don't even start the caches warming until
its activated, so its not a very hot standby.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/client-go/issues/28#issuecomment-276454774,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAngluqlt5BHDYOk4I1sAu1zfuuHvgj5ks5rX4JYgaJpZM4KpHxj
.

[mikedanese]

I wouldn't call this a "security" risk, it's more like "cluster stability"
mixed with "future proof".

It's definitely not a security risk, and the impact is not specific to the leaderelection client. It's a general scalability problem with endpoints. Apiserver traffic attributable to endpoint updates is O(num nodes * num endpoints)

[deads2k]

It's definitely not a security risk, and the impact is not specific to the leaderelection client. It's a general scalability problem with endpoints. Apiserver traffic attributable to endpoint updates is O(num nodes * num endpoints)

If memory serves, the power the manipulate endpoints implies the power to redirect traffic for a Service using them. That means anyone with endpoint setting power has MITM power. That power isn't needed to elect a leader.

[mikedanese]

A group of processes performing leader election only needs to modify the endpoints object of a service that the group fronts. It's trivial to lock down write privilege to the single endpoints object. If a process in the group is maliciously modifying endpoints then we are already sending traffic to that compromised process. If you are terribly paranoid, you can use an endpoints object that is not fronted by a service, or we could possibly move leader annotation updates to a status sub-resource.

[lavalamp]

even writes to empty endpoints objects cause distribution to kube-proxies
afaik. Even if you just change status. endpoints is just not a good choice
for this :(

On Wed, Feb 1, 2017 at 10:26 AM, Mike Danese [e-mail]
wrote:

A group of processes performing leader election only needs to modify the
endpoints object of a service that the group fronts. It's trivial to lock
down write privilege to the single endpoints object. If a process in the
group is maliciously modifying endpoints then we are already sending
traffic to a compromised process. If you are terribly paranoid, you can use
an endpoints object that is not fronted by a service, or we could possibly
move leader annotation updates to a status sub-resource.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/client-go/issues/28#issuecomment-276738589,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAngllDG9xO-DFxGtDknOCFukGu_0Cctks5rYM5BgaJpZM4KpHxj
.

[mikedanese]

@lavalamp my most recent comment is only a method to address the security concerns raised by @deads2k, not a method to solve endpoint scalability issues.

[brancz]

Apiserver traffic attributable to endpoint updates is O(num nodes * num endpoints)

I'm trying to understand this correctly. If I have a headless service (clusterIP: none, therefore not routeable), and am managing the Endpoints object either manually or through the label-selector, why does that cause any traffic to kube-proxy, if it's not routeable, it sounds like it should really just be a blob of information that is stored in etcd.

For background: in Prometheus there is the endpoints discovery, which discovers targets via the addresses in Endpoints objects. Now we are trying to figure out, whether creating (non routable/headless) Services for discovery purposes create scalability issues as mentioned above, and whether this could be optimized if it turns out to be unnecessary load on kube-proxy.

[mikedanese]

@brancz that is an optimization that doesn't exist right now. clusterIP is an attribute of a service. Endpoints are conceptually separate. kube-proxy watches all endpoints.

Now we are trying to figure out, whether creating (non routable/headless) Services for discovery purposes create scalability issues as mentioned above, and whether this could be optimized if it turns out to be unnecessary load on kube-proxy.

It would cause extra traffic when those endpoints change (e.g. pods are created/deleted or become ready/not-ready)

[aanm]

I'm trying to replace the client from kubernetes to go-client for ingress and this is drawing me back, what's the current status of this?

[timothysc]

All the more reason to have a simple component registration api imo. The primary consumer of this library are those who are writing services/controllers/operators atop. As of today, it's a hodge-podgery of rules to add services into the system. Uniformity on extensibility imo is going to become super-d-duper important. I can tell how/where a kernel is tainted, I should be able to do the same with kube.

FWIW - The leader election code is generic and can lock and anything, @mikedanese and I made sure of that, but we need "something", such as a component resource.

Folks need the ability to lock 2day, and right now they are skirting around this library to do it...

/cc @jbeda

[lavalamp]

Use a config map.

On Fri, Mar 3, 2017 at 9:04 AM, Timothy St. Clair [e-mail]
wrote:

All the more reason to have a simple component registration api imo. The
primary consumer of this library are those who are writing
services/controllers/operators atop. As of today, it's a hodge-podgery of
rules to add services into the system. Uniformity on extensibility imo is
going to become super-d-duper important. I can tell how/where a kernel is
tainted, I should be able to do the same with kube.

FWIW - The leader election code is generic and can lock and anything,
@mikedanese https://github.com/mikedanese and I made sure of that, but
we need "something", such as a component resource.

Folks need the ability to lock 2day, and right now they are skirting
around this library to do it...

/cc @jbeda https://github.com/jbeda

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/client-go/issues/28#issuecomment-284010659,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglviFz4qcU0o4AfkCzI24y4jm3sEZks5riEgQgaJpZM4KpHxj
.

[munnerz]

Hey,

So I've noticed https://github.com/kubernetes/kubernetes/pull/42666 has been merged - thanks v much @timothysc!

Would we be able to now get this code moved over/included in client-go? I'm happy to undertake the work to port it if there's nobody currently owning this!

[caesarxuchao]

@deads2k is your initial concern with leader election solved by 42666?

[deads2k]

@deads2k is your initial concern with leader election solved by 42666?

Yes, configmaps concern me a lot less. I think (but I'm not completely certain), we could even restrict a service account to a specific configmap per-leader we need to elect, right?

[ericchiang]

I think (but I'm not completely certain), we could even restrict a service account to a specific configmap per-leader we need to elect, right?

Yes, resourceName was added to RBAC when it went to beta.

[timothysc]

@munnerz

FWIW, I've given up on vendoring the client separately, and I think it's a absolute mess. I simply vendor against the main repo and fix up staging dir through scripts.

https://github.com/kubernetes/kubernetes/issues/43246

[caesarxuchao]

I updated https://github.com/kubernetes/kubernetes/pull/39173 to import the latest leaderelection package to client-go.

[caesarxuchao]

The change is merged in kubernetes main repo. The change will be published to client-go in 24 hrs.

[tamalsaha]

@caesarxuchao , is this to late to be picked up for the 1.7 branch?

[caesarxuchao]

It is too big to be cherrypicked. If someone volunteers to do the cherrypick, it'd be better to communicate it with the 1.7 release branch manager (wojtek-t) first.

[hongchaodeng]

Is it gonna be available in client-go v4.0 release? Or v5.0?

[caesarxuchao]

Without cherrypicking, it would be in v5.0.

[yifan-gu]

@caesarxuchao Thank you!

[caesarxuchao]

Bot stops working: https://github.com/kubernetes/test-infra/issues/2847#issuecomment-314963237. I'll try to reboot.

[aledbf]

@caesarxuchao any eta for this? Thanks

[caesarxuchao]

Blocked by https://github.com/kubernetes/kubernetes/pull/48497#discussion_r127312690.

[caesarxuchao]

Leaderelection was synced into the master branch. I can try to cherrypick it into kubernetes 1.7, depending on how badly people need it. No promise though, because the PR was big and the master branch has diverged from the release-1.7 quite a lot.