Cluster-api: Enable optional use of local Kubernetes API connection in NodeRef controller

[embik]

/kind feature

Describe the solution you'd like
The NodeRef controller requires a <Cluster Name>-kubeconfig secret for access to remote Kubernetes clusters. It fails if no kubeconfig secret is available.

As discussed in #cluster-api (Slack), the legacy Node controller did not require that secret, but rather connected via the local Kubernetes cluster (which hosts the Cluster API resources). This behaviour makes sense in some situations for the new NodeRef controller as well IMHO. We implemented the Machine API to allow a cluster to resize itself and therefore Machine resources always reference local Node resources.

Our implementation creates a "dummy" cluster.local Cluster resource and references this for the NodeRef controller to work, but it doesn't make much sense to me to create a dedicated cluster.local-kubeconfig secret that contains credentials for the _local_ cluster. A flag --noderef-local-cluster or something like that could enable usage of the local Kubernetes API connection in case no kubeconfig secret exists instead of failure (which is the current and default behavior).

[detiber]

/assign @vincepri

[ncdc]

What if we try to use the secret, and if we can't find it, we fall back to trying to use the local/in-cluster credentials?

[detiber]

@ncdc that makes sense to me

[joonas]

I'd be interested in taking a pass at implementing this

[vincepri]

@joonas Reach out if you have any questions 😊, I also think some logic needs to change in the MachineSet Controller, given that we check for the Node's readiness status

[joonas]

@vincepri thanks, I'll do a bit of spelunking when I find the time and get back to you with any questions

/assign @joonas

[ncdc]

@embik in v0.2.x (v1alpha2), Cluster API creates the kubeconfig secret for you. Will that work for you?

[ncdc]

We are no longer maintaining v0.1.x. I'm going to remove the milestone so we can retriage this.

/milestone clear

[ncdc]

@embik ping re https://github.com/kubernetes-sigs/cluster-api/issues/1242#issuecomment-535638398?

[vincepri]

/priority awaiting-more-evidence

[justinsb]

I do think this one feels like it could be legitimate to help with adoption - I'm wondering if it's possible to use something like e.g. GKE to manage the control plane, but then run the machine controller to use MachineDeployments with that cluster. In this case it would be nice to default to using the in-cluster k8s configuration, and to not have the cluster-controller create a secret at all. Having a secret floating about that we don't use feels like it would make people unhappy from a security point of view.

I don't know whether this is a long-term scenario, but our plan for kops involves this - step 1 is to use cluster-api just for nodes (optionally), step 2 is full management including the control plane.

[embik]

@ncdc sorry for the late reply! As @justinsb pointed out I feel like creating secrets that are not strictly necessary is not ideal. I'm not working on the machine api implementation that required this anymore though. I'm pinging the people that this might be relevant to.

[ncdc]

I think I'm ok looking first for the secret, then falling back to in-cluster config if the secret does not exist.

[ncdc]

/help

[k8s-ci-robot]

@ncdc:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ncdc]

I'd say the methods in controllers/remote (e.g. NewClusterClient) should be used everywhere we talk to a workload cluster. We should consolidate any/all logic that potentially falls back to the in-cluster config into this package, and probably rename it to workload. In other words (suggested method names & behavior):

• workload.NewClusterClient should try to create a client using the kubeconfig secret. If the secret doesn't exist, just return the management cluster client.
• workload.NewClusterRESTConfig should follow the same logic as above, returning a *RESTConfig

[vincepri]

Instead of falling back, which is harder to test because the secret call has to fail first and it could lead into confusion. What if we add a new spec field or annotation to enable to in cluster client behavior?

[roehlc]

Thanks for pinging me @embik .
I'm also thinking @justinsb argument for not creating a secret that isn't used is important.
I feel like falling back to the in-cluster config would be the ideal solution. If that is to difficult to test, maybe something like an additional annotation or field as suggested by @vincepri could also work.
Looking forward to progress or more ideas to get this feature done.

[ncdc]

I think this might get easier in v1alpha3 and not need a field/annotation. If Cluster.Spec.ControlPlaneRef is nil, use in-cluster config. Otherwise, require & expect the kubeconfig secret. This does require that anyone upgrading a v1alpha2 cluster that has control plane machines to v1alpha3 would need to create a new ControlPlane resource (to adopt the alpha2 control plane machines), but I'd hope that would be reasonable.

[justinsb]

@ncdc did you mean controlPlaneRef == nil or controlPlaneRef != nil? Technically today if controlPlaneRef == nil that's precisely when we generate a kubeconfig in reconcileKubeconfig; I'm not sure if that is an artifact of backwards compatibility though.

At a higher level - I'm not sure I understand why I wouldn't describe my control plane in the one-cluster scenario (which is when we want to use in-cluster config, AIUI).

I do like the suggestion to use a field (or annotation), it just isn't clear to me why controlPlaneRef is it! But I'm also only now getting up to speed on the control-plane stuff...

[ncdc]

@justinsb I did mean == nil, but after thinking about it more, I'm not sure we can easily and accurately determine the desired behavior 100% of the time. There may be situations where you have a controlPlaneRef but you want to use the in-cluster config (my original thought was if you had a control plane provider, you'd always want to use the secret).

I think it's probably best to add a field in the cluster spec to indicate if you want to use in-cluster config or not.

[justinsb]

Thanks @ncdc - I'll try to work on a straw-man PR!

[vincepri]

@justinsb did you have any updates here?

[vincepri]

/priority backlog

[vincepri]

Closing for now due to inactivity, feel free to reopen

/close

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

Closing for now due to inactivity, feel free to reopen

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.