Remove dependency of the API onto the typed cluster-api client #2914

alvaroaleman · 2019-03-04T10:20:55Z

What this PR does / why we need it:

Removes the dependency to the typed cluster api clientset from everywhere except the IPAM controller. The purpose of this effort is to reduce the transient dependency on the client-go version that is used by the cluster-api project.

IMHO the code also gets easier as we don't need a variety of typed clients but instead can use just one client for everything.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Documentation:

Release note:

none

Upstream removed the triple package[0], in order to be able to upgrade client-go we need to have a downsream version. [0] kubernetes/kubernetes#70966

* split kubermatic alerts into master and seed * rebuild outdated alerting rules

alvaroaleman · 2019-03-04T10:22:44Z

api/pkg/handler/node_v1.go

-func findMachineAndNode(name string, machineClient clusterv1alpha1clientset.Interface, kubeClient kubernetes.Interface) (*clusterv1alpha1.Machine, *corev1.Node, error) {
-	machineList, err := machineClient.ClusterV1alpha1().Machines(metav1.NamespaceSystem).List(metav1.ListOptions{IncludeUninitialized: true})
-	if err != nil {
+func findMachineAndNode(ctx context.Context, name string, client ctrlruntimeclient.Client) (*clusterv1alpha1.Machine, *corev1.Node, error) {


What has to be noted here is that so far the machineClient was sometimes an impersoanted client and sometimes not, but the kubeClient has always been an admin client. I am not completely sure if its okay to change it like this but then again, why should one of the clients have more permissions than the user?

cc @p0lyn0mial

only the legacy endpoints should use admin machineClient, the new endpoints must use impersonated version. kubeClient is only used (node_v1) by nodes endpoints to find corresponding machines and as we don't add RBAC Roles/Bindings to nodes we use the admin client.

I dont follow. So is it okay to use or not use the same impersonation for the kubeClient as for the machineClient?

no, no impersonation for kubeClient - there are no RBAC Roles/Bindings in place for node resources.

Any objections against extending the existing roles to allow a get on node resources?

sure go ahead, we were planing to add RBAC to more resources if not all in the future.

alvaroaleman · 2019-03-04T11:19:54Z

/retest

alvaroaleman · 2019-03-04T12:09:17Z

/retest

alvaroaleman · 2019-03-04T12:41:49Z

/retest

alvaroaleman · 2019-03-04T14:01:25Z

/assign @zreigz

zreigz · 2019-03-04T15:00:59Z

I think something is missing here. I have errors Error 500: failed to load machines from cluster: no kind is registered for the type v1alpha1.MachineList in scheme "k8s.io/client-go/kubernetes/scheme/register.go:61"

zreigz · 2019-03-05T10:00:01Z

yes, it's my internal problem for @p0lyn0mial everything works fine. Also @maciaszczykm has the same problem with running our system locally. For him also usercluster-controller can not start. I will investigate it later.
/lgtm

kubermatic-bot · 2019-03-05T10:00:04Z

LGTM label has been added.

Git tree hash: 48b7a70d0a197b3a3674f9b851b1a868f2e7be03

p0lyn0mial · 2019-03-05T10:22:09Z

api/pkg/cluster/client/client.go

+	GetApiextensionsClient(*kubermaticv1.Cluster, ...ConfigOption) (apiextensionsclientset.Interface, error)
+	GetAdmissionRegistrationClient(*kubermaticv1.Cluster, ...ConfigOption) (admissionregistrationclientset.AdmissionregistrationV1beta1Interface, error)
+	GetKubeAggregatorClient(*kubermaticv1.Cluster, ...ConfigOption) (aggregationclientset.Interface, error)
+	GetDynamicClient(*kubermaticv1.Cluster, ...ConfigOption) (ctrlruntimeclient.Client, error)


this is really nice, the interface for interacting with API is clean :) What's the current status of the library (controller-runtime/pkg/client) ? Isn't in alpha ? Are there any known limitations ?

Isn't in alpha ?

No, where did you get that from?

We already use it in various places like e.G.:

User Cluster Controller and User Cluster RBAC controller

Openshift controller

Partly in the cluster controller (dynamic cache from controller-runtime)

We plan to eventually make all controllers controller-runtime based because it is a lot easier to use than direct client-go and also reduces the amount of boilerplate needed for controllers drastically, making the code faster to read and the actual business logic easier to find. So if there are actual issues I would vote to just fix them upstream.

It is also used by e.G. the cluster-api project. I can't find anything in the docs that says it was alpha.

Are there any known limitations ?

The fake client seems to be a bit more limited and there is no DeleteCollection although there is a open WIP PR for that. Other than that none that I am aware of.

Usually major number of 0 is used to indicate pre release versions, see https://github.com/kubernetes-sigs/controller-runtime/releases

other than that it looks really nice :)

p0lyn0mial · 2019-03-05T10:23:01Z

yes, it's my internal problem for @p0lyn0mial everything works fine

ack, tested on my local machine and it actually works ! :)

p0lyn0mial · 2019-03-05T11:24:37Z

/lgtm
/approve

kron4eg · 2019-03-05T11:57:56Z

/approve

mrIncompetent · 2019-03-05T12:26:42Z

api/cmd/kubermatic-api/main.go

@@ -114,9 +114,14 @@ func createInitProviders(options serverRunOptions) (providers, error) {
 			kubermaticSeedInformerFactory := kubermaticinformers.NewSharedInformerFactory(kubermaticSeedClient, informer.DefaultInformerResyncPeriod)
 			defaultImpersonationClientForSeed := kubernetesprovider.NewKubermaticImpersonationClient(cfg)

+			userClusterConnectionProvider, err := client.NewExternal(kubeInformerFactory.Core().V1().Secrets().Lister())
+			if err != nil {
+				glog.Fatalf("failed to get usreClusterConnectionProvider: %v", err)


typo + return an error here. Only die with a fatal inside the main func

mrIncompetent · 2019-03-05T12:35:16Z

api/pkg/controller/rbac-user-cluster/reconciler.go

@@ -25,6 +25,7 @@ type reconciler struct {

 // Reconcile creates and updates ClusterRoles and ClusterRoleBinding to achieve the desired state
 func (r *reconciler) Reconcile(resourceName string) error {
+	glog.Infof("Reconciling RBAC for %s", resourceName)


Following https://github.com/kubernetes/community/blob/b3349d5b1354df814b67bbdee6890477f3c250cb/contributors/devel/logging.md

this might rather be a glog.V(4) ? WDYT?

mrIncompetent · 2019-03-05T12:39:42Z

/approve

p0lyn0mial · 2019-03-05T12:49:07Z

api/pkg/controller/rbac-user-cluster/mapper.go

@@ -58,6 +58,11 @@ func GenerateRBACClusterRole(resourceName string) (*rbacv1.ClusterRole, error) {
 				Resources: []string{machinedeployments, machines},
 				Verbs:     verbs,
 			},
+			{
+				APIGroups: []string{""},
+				Resources: []string{"nodes"},


i'm going to add an issue to our backlog to create different verbs for different groups as we do for other resources.

/cc @zreigz

I don't follow. This is a per-resource setting, not a per-group one. And what's the advantage of factoring it out?

no, this is per-group (this method will be called for each group we have - owners, editors, viewers), see: https://github.com/kubermatic/kubermatic/blob/master/api/pkg/controller/rbac-user-cluster/controller.go#L24

the advantage is a unified access to a cluster, for example readers can only view resources.

p0lyn0mial · 2019-03-05T12:49:48Z

/lgtm
/approve

kubermatic-bot · 2019-03-05T12:49:50Z

LGTM label has been added.

Git tree hash: 8969734a4c19f3390f0de534a91e8fa712c22c57

kubermatic-bot · 2019-03-05T12:49:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kron4eg, mrIncompetent, p0lyn0mial

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kron4eg,mrIncompetent,p0lyn0mial]
~~api/cmd/OWNERS~~ [mrIncompetent]
~~api/cmd/kubermatic-api/OWNERS~~ [p0lyn0mial]
~~api/pkg/controller/OWNERS~~ [mrIncompetent]
~~api/pkg/controller/monitoring/OWNERS~~ [kron4eg]
~~api/pkg/controller/rbac-user-cluster/OWNERS~~ [p0lyn0mial]
~~api/pkg/handler/OWNERS~~ [p0lyn0mial]
~~api/pkg/provider/kubernetes/OWNERS~~ [p0lyn0mial]
~~api/pkg/resources/OWNERS~~ [mrIncompetent]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mrIncompetent · 2019-03-05T14:10:47Z

Sorry for the delay!
/lgtm

kubermatic-bot · 2019-03-05T14:10:50Z

LGTM label has been added.

Git tree hash: 6414cb692a3f710a22ddba9c73fe9c93fba08191

alvaroaleman and others added 15 commits March 3, 2019 13:13

Use downstream triple package

bdfaab6

Upstream removed the triple package[0], in order to be able to upgrade client-go we need to have a downsream version. [0] kubernetes/kubernetes#70966

WIP

d2c3390

Update fixtures

2b443a1

Override cluster-api dependency

72a7564

Fix linting

4268f9a

Split Kubermatic alerts into master and seed (#2909)

001c829

* split kubermatic alerts into master and seed * rebuild outdated alerting rules

Remove clusterv1alpha1 clientset dependecy from cluster controller

758cf96

Remove unused script

6ea6ac9

Use ctrlruntimeClient in pkg/handler/node_v1.go

1702af2

Finish moving handler to ctrlruntimeClient

71524e8

Remove GetAdminKubernetesClientForCustomerCluster from Clusterprovider

8fd9b1a

Use dynamic client for machines in conformance tester

13d5b79

Fix handler tests

ef2006b

Remove unused script

3413bde

Merge branch 'master' into remove-dependency-cluster-client-mergede

259c0e6

kubermatic-bot added release-note-none Denotes a PR that doesn't merit a release note. team/lifecycle size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 4, 2019

alvaroaleman commented Mar 4, 2019

View reviewed changes

Fix linting

458a5e7

alvaroaleman changed the title ~~Remove dependency cluster client mergede~~ Remove dependency of the API onto the typed cluster-api client Mar 4, 2019

Actually fix linting error

14b3a36

Register schemes

b03eb0c

kubermatic-bot assigned zreigz Mar 4, 2019

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019

p0lyn0mial reviewed Mar 5, 2019

View reviewed changes

kubermatic-bot assigned p0lyn0mial Mar 5, 2019

mrIncompetent reviewed Mar 5, 2019

View reviewed changes

Dont fatalf outside of main

8cc557f

kubermatic-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019

mrIncompetent reviewed Mar 5, 2019

View reviewed changes

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2019

p0lyn0mial reviewed Mar 5, 2019

View reviewed changes

kubermatic-bot requested a review from zreigz March 5, 2019 12:49

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019

Print reconciling info on level four

5cd0d79

kubermatic-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019

kubermatic-bot assigned mrIncompetent Mar 5, 2019

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019

kubermatic-bot merged commit 2916668 into master Mar 5, 2019

kubermatic-bot deleted the remove-dependency-cluster-client-mergede branch March 5, 2019 14:14

kdomanski unassigned p0lyn0mial Jun 4, 2019

scheeles unassigned mrIncompetent Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency of the API onto the typed cluster-api client #2914

Remove dependency of the API onto the typed cluster-api client #2914

alvaroaleman commented Mar 4, 2019

alvaroaleman Mar 4, 2019

zreigz Mar 4, 2019

p0lyn0mial Mar 4, 2019

alvaroaleman Mar 4, 2019

p0lyn0mial Mar 4, 2019

alvaroaleman Mar 4, 2019

p0lyn0mial Mar 5, 2019

alvaroaleman Mar 5, 2019

alvaroaleman commented Mar 4, 2019

alvaroaleman commented Mar 4, 2019

alvaroaleman commented Mar 4, 2019

alvaroaleman commented Mar 4, 2019

zreigz commented Mar 4, 2019

zreigz commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019

p0lyn0mial Mar 5, 2019

alvaroaleman Mar 5, 2019

p0lyn0mial Mar 5, 2019

p0lyn0mial commented Mar 5, 2019

p0lyn0mial commented Mar 5, 2019

kron4eg commented Mar 5, 2019

mrIncompetent Mar 5, 2019

alvaroaleman Mar 5, 2019

mrIncompetent Mar 5, 2019

alvaroaleman Mar 5, 2019

mrIncompetent commented Mar 5, 2019

p0lyn0mial Mar 5, 2019

alvaroaleman Mar 5, 2019

p0lyn0mial Mar 5, 2019

p0lyn0mial commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019

mrIncompetent commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019

Remove dependency of the API onto the typed cluster-api client #2914

Remove dependency of the API onto the typed cluster-api client #2914

Conversation

alvaroaleman commented Mar 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Mar 4, 2019

alvaroaleman commented Mar 4, 2019

alvaroaleman commented Mar 4, 2019

alvaroaleman commented Mar 4, 2019

zreigz commented Mar 4, 2019

zreigz commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Mar 5, 2019

p0lyn0mial commented Mar 5, 2019

kron4eg commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrIncompetent commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019

mrIncompetent commented Mar 5, 2019

kubermatic-bot commented Mar 5, 2019