From 02bbe042b2724923014f885f3cbeac2d672044e9 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 10 Aug 2020 11:40:48 +0000 Subject: [PATCH 1/3] Use runtime-controller client interface Signed-off-by: Dennis Marttinen --- .../controller/wksctl/machine_controller.go | 61 ++++++++++--------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/pkg/apis/wksprovider/controller/wksctl/machine_controller.go b/pkg/apis/wksprovider/controller/wksctl/machine_controller.go index 9906a881..db621c05 100644 --- a/pkg/apis/wksprovider/controller/wksctl/machine_controller.go +++ b/pkg/apis/wksprovider/controller/wksctl/machine_controller.go @@ -27,7 +27,6 @@ import ( corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -203,7 +202,8 @@ func (a *MachineController) connectTo(ctx context.Context, c *existinginfrav1.Ex } func (a *MachineController) sshKey(ctx context.Context) ([]byte, error) { - secret, err := a.clientSet.CoreV1().Secrets(a.controllerNamespace).Get(ctx, controllerSecret, metav1.GetOptions{}) + var secret v1.Secret + err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: controllerSecret}, &secret) if err != nil { return nil, gerrors.Wrap(err, "failed to get WKS' secret") } @@ -225,7 +225,8 @@ type kubeadmJoinSecrets struct { } func (a *MachineController) kubeadmJoinSecrets(ctx context.Context) (*kubeadmJoinSecrets, error) { - secret, err := a.clientSet.CoreV1().Secrets(a.controllerNamespace).Get(ctx, controllerSecret, metav1.GetOptions{}) + var secret v1.Secret + err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: controllerSecret}, &secret) if err != nil { return nil, gerrors.Wrap(err, "failed to get WKS' secret") } @@ -236,12 +237,12 @@ func (a *MachineController) kubeadmJoinSecrets(ctx context.Context) (*kubeadmJoi }, nil } -func (a *MachineController) updateKubeadmJoinSecrets(ctx context.Context, ID string) error { +func (a *MachineController) updateKubeadmJoinSecrets(ctx context.Context, ID string, secret *corev1.Secret) error { len := base64.StdEncoding.EncodedLen(len(ID)) enc := make([]byte, len) base64.StdEncoding.Encode(enc, []byte(ID)) patch := []byte(fmt.Sprintf("{\"data\":{\"%s\":\"%s\"}}", bootstrapTokenID, enc)) - _, err := a.clientSet.CoreV1().Secrets(a.controllerNamespace).Patch(ctx, controllerSecret, types.StrategicMergePatchType, patch, metav1.PatchOptions{}) + err := a.client.Patch(ctx, secret, client.RawPatch(types.StrategicMergePatchType, patch)) if err != nil { log.Debugf("failed to patch wks secret %s %v", patch, err) } @@ -251,7 +252,8 @@ func (a *MachineController) updateKubeadmJoinSecrets(ctx context.Context, ID str func (a *MachineController) token(ctx context.Context, ID string) (string, error) { ns := "kube-system" name := fmt.Sprintf("%s%s", bootstrapapi.BootstrapTokenSecretPrefix, ID) - secret, err := a.clientSet.CoreV1().Secrets(ns).Get(ctx, name, metav1.GetOptions{}) + secret := &v1.Secret{} + err := a.client.Get(ctx, client.ObjectKey{Namespace: ns, Name: name}, secret) if err != nil { // The secret may have been removed if it expired so we will generate a new one log.Debugf("failed to find original bootstrap token %s/%s, generating a new one", ns, name) @@ -300,18 +302,18 @@ func (a *MachineController) installNewBootstrapToken(ctx context.Context, ns str if err != nil { return nil, gerrors.Errorf("failed to create new bootstrap token %s/%s", ns, secret.ObjectMeta.Name) } - s, err := a.clientSet.CoreV1().Secrets(ns).Create(ctx, secret, metav1.CreateOptions{}) + err = a.client.Create(ctx, secret) if err != nil { return nil, gerrors.Errorf("failed to install new bootstrap token %s/%s", ns, secret.ObjectMeta.Name) } - tokenID, ok := s.Data[bootstrapapi.BootstrapTokenIDKey] + tokenID, ok := secret.Data[bootstrapapi.BootstrapTokenIDKey] if !ok { - return nil, gerrors.Errorf("token-id not found %s/%s", s.ObjectMeta.Namespace, s.ObjectMeta.Name) + return nil, gerrors.Errorf("token-id not found %s/%s", secret.ObjectMeta.Namespace, secret.ObjectMeta.Name) } - if err := a.updateKubeadmJoinSecrets(ctx, string(tokenID)); err != nil { - return nil, gerrors.Errorf("Failed to update wks join token %s/%s", s.ObjectMeta.Namespace, s.ObjectMeta.Name) + if err := a.updateKubeadmJoinSecrets(ctx, string(tokenID), secret); err != nil { + return nil, gerrors.Errorf("Failed to update wks join token %s/%s", secret.ObjectMeta.Namespace, secret.ObjectMeta.Name) } - return s, nil + return secret, nil } // Delete the machine. If no error is returned, it is assumed that all dependent resources have been cleaned up. @@ -348,7 +350,7 @@ func (a *MachineController) delete(ctx context.Context, c *existinginfrav1.Exist }); err != nil { return err } - if err = a.clientSet.CoreV1().Nodes().Delete(ctx, node.Name, metav1.DeleteOptions{}); err != nil { + if err := a.client.Delete(ctx, node); err != nil { return err } a.recordEvent(machine, corev1.EventTypeNormal, "Delete", "deleted machine %s", machine.Name) @@ -609,8 +611,8 @@ func (a *MachineController) getNodePlan(ctx context.Context, provider *existingi } func (a *MachineController) getAuthConfigMap(ctx context.Context) (*v1.ConfigMap, error) { - client := a.clientSet.CoreV1().ConfigMaps(a.controllerNamespace) - maps, err := client.List(ctx, metav1.ListOptions{}) + var maps corev1.ConfigMapList + err := a.client.List(ctx, &maps, &client.ListOptions{Namespace: a.controllerNamespace}) if err != nil { return nil, err } @@ -626,8 +628,8 @@ func (a *MachineController) getAuthSecrets(ctx context.Context, authConfigMap *v authSecrets := map[string]resource.SecretData{} for _, authType := range []string{"authentication", "authorization"} { secretName := authConfigMap.Data[authType+"-secret-name"] - client := a.clientSet.CoreV1().Secrets(a.controllerNamespace) - secret, err := client.Get(ctx, secretName, metav1.GetOptions{}) + var secret v1.Secret + err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: secretName}, &secret) // TODO: retry several times like the old code did (?) // TODO: check whether it is a not-found response if err != nil { @@ -643,12 +645,12 @@ func (a *MachineController) getAuthSecrets(ctx context.Context, authConfigMap *v func (a *MachineController) getProviderConfigMaps(ctx context.Context, provider *existinginfrav1.ExistingInfraCluster) (map[string]*v1.ConfigMap, error) { fileSpecs := provider.Spec.OS.Files - client := a.clientSet.CoreV1().ConfigMaps(a.controllerNamespace) configMaps := map[string]*v1.ConfigMap{} for _, fileSpec := range fileSpecs { mapName := fileSpec.Source.ConfigMap if _, seen := configMaps[mapName]; !seen { - configMap, err := client.Get(ctx, mapName, metav1.GetOptions{}) + configMap := &corev1.ConfigMap{} + err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: mapName}, configMap) if err != nil { return nil, err } @@ -754,15 +756,15 @@ func nodeVersion(node *corev1.Node) string { func (a *MachineController) uncordon(ctx context.Context, node *corev1.Node) error { contextLog := log.WithFields(log.Fields{"node": node.Name}) - client := a.clientSet.CoreV1().Nodes() retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - result, getErr := client.Get(ctx, node.Name, metav1.GetOptions{}) + var result v1.Node + getErr := a.client.Get(ctx, client.ObjectKey{Name: node.Name}, &result) if getErr != nil { contextLog.Errorf("failed to read node info, can't reschedule: %v", getErr) return getErr } result.Spec.Unschedulable = false - _, updateErr := client.Update(ctx, result, metav1.UpdateOptions{}) + updateErr := a.client.Update(ctx, &result) if updateErr != nil { contextLog.Errorf("failed to reschedule node: %v", updateErr) return updateErr @@ -820,15 +822,15 @@ func (a *MachineController) removeNodeLabel(ctx context.Context, node *corev1.No func (a *MachineController) modifyNode(ctx context.Context, node *corev1.Node, updater func(node *corev1.Node)) error { contextLog := log.WithFields(log.Fields{"node": node.Name}) - client := a.clientSet.CoreV1().Nodes() retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - result, getErr := client.Get(ctx, node.Name, metav1.GetOptions{}) + result := &v1.Node{} + getErr := a.client.Get(ctx, client.ObjectKey{Name: node.Name}, result) if getErr != nil { contextLog.Errorf("failed to read node info, assuming unsafe to update: %v", getErr) return getErr } updater(result) - _, updateErr := client.Update(ctx, result, metav1.UpdateOptions{}) + updateErr := a.client.Update(ctx, result) if updateErr != nil { contextLog.Errorf("failed attempt to update node annotation: %v", updateErr) return updateErr @@ -890,7 +892,8 @@ func hasTaint(node *corev1.Node, value string) bool { } func (a *MachineController) findNodeByID(ctx context.Context, machineID, systemUUID string) (*corev1.Node, error) { - nodes, err := a.clientSet.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) + var nodes v1.NodeList + err := a.client.List(ctx, &nodes) if err != nil { return nil, gerrors.Wrap(err, "failed to list nodes") } @@ -922,7 +925,8 @@ func (a *MachineController) getMasterNode(ctx context.Context) (*corev1.Node, er } func (a *MachineController) getMasterNodes(ctx context.Context) ([]*corev1.Node, error) { - nodes, err := a.clientSet.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) + var nodes v1.NodeList + err := a.client.List(ctx, &nodes) if err != nil { return nil, gerrors.Wrap(err, "failed to list nodes") } @@ -962,7 +966,8 @@ func (a *MachineController) isControllerNode(ctx context.Context, node *corev1.N } func (a *MachineController) getControllerNodeName(ctx context.Context) (string, error) { - pods, err := a.clientSet.CoreV1().Pods(a.controllerNamespace).List(ctx, metav1.ListOptions{}) + var pods v1.PodList + err := a.client.List(ctx, &pods, &client.ListOptions{Namespace: a.controllerNamespace}) if err != nil { return "", err } From a78800c416648432eeb08ac7e4fb2b3ac79d44f8 Mon Sep 17 00:00:00 2001 From: Dennis Marttinen Date: Mon, 10 Aug 2020 16:44:48 +0300 Subject: [PATCH 2/3] Fix duplicate import of k8s.io/api/core/v1 Signed-off-by: Dennis Marttinen --- .../controller/wksctl/machine_controller.go | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/pkg/apis/wksprovider/controller/wksctl/machine_controller.go b/pkg/apis/wksprovider/controller/wksctl/machine_controller.go index db621c05..d0791633 100644 --- a/pkg/apis/wksprovider/controller/wksctl/machine_controller.go +++ b/pkg/apis/wksprovider/controller/wksctl/machine_controller.go @@ -25,7 +25,6 @@ import ( bootstraputils "github.com/weaveworks/wksctl/pkg/utilities/kubeadm" "github.com/weaveworks/wksctl/pkg/utilities/version" corev1 "k8s.io/api/core/v1" - v1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -202,7 +201,7 @@ func (a *MachineController) connectTo(ctx context.Context, c *existinginfrav1.Ex } func (a *MachineController) sshKey(ctx context.Context) ([]byte, error) { - var secret v1.Secret + var secret corev1.Secret err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: controllerSecret}, &secret) if err != nil { return nil, gerrors.Wrap(err, "failed to get WKS' secret") @@ -225,7 +224,7 @@ type kubeadmJoinSecrets struct { } func (a *MachineController) kubeadmJoinSecrets(ctx context.Context) (*kubeadmJoinSecrets, error) { - var secret v1.Secret + var secret corev1.Secret err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: controllerSecret}, &secret) if err != nil { return nil, gerrors.Wrap(err, "failed to get WKS' secret") @@ -252,7 +251,7 @@ func (a *MachineController) updateKubeadmJoinSecrets(ctx context.Context, ID str func (a *MachineController) token(ctx context.Context, ID string) (string, error) { ns := "kube-system" name := fmt.Sprintf("%s%s", bootstrapapi.BootstrapTokenSecretPrefix, ID) - secret := &v1.Secret{} + secret := &corev1.Secret{} err := a.client.Get(ctx, client.ObjectKey{Namespace: ns, Name: name}, secret) if err != nil { // The secret may have been removed if it expired so we will generate a new one @@ -519,7 +518,7 @@ func (a *MachineController) kubeadmUpOrDowngrade(ctx context.Context, machine *c return nil } -func (a *MachineController) prepareForMasterUpdate(ctx context.Context, node *v1.Node) error { +func (a *MachineController) prepareForMasterUpdate(ctx context.Context, node *corev1.Node) error { // Check if it's safe to update a master if err := a.checkMasterHAConstraint(ctx, node); err != nil { return gerrors.Wrap(err, "Not enough available master nodes to allow master update") @@ -610,7 +609,7 @@ func (a *MachineController) getNodePlan(ctx context.Context, provider *existingi return plan, nil } -func (a *MachineController) getAuthConfigMap(ctx context.Context) (*v1.ConfigMap, error) { +func (a *MachineController) getAuthConfigMap(ctx context.Context) (*corev1.ConfigMap, error) { var maps corev1.ConfigMapList err := a.client.List(ctx, &maps, &client.ListOptions{Namespace: a.controllerNamespace}) if err != nil { @@ -624,11 +623,11 @@ func (a *MachineController) getAuthConfigMap(ctx context.Context) (*v1.ConfigMap return nil, nil } -func (a *MachineController) getAuthSecrets(ctx context.Context, authConfigMap *v1.ConfigMap) (map[string]resource.SecretData, error) { +func (a *MachineController) getAuthSecrets(ctx context.Context, authConfigMap *corev1.ConfigMap) (map[string]resource.SecretData, error) { authSecrets := map[string]resource.SecretData{} for _, authType := range []string{"authentication", "authorization"} { secretName := authConfigMap.Data[authType+"-secret-name"] - var secret v1.Secret + var secret corev1.Secret err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: secretName}, &secret) // TODO: retry several times like the old code did (?) // TODO: check whether it is a not-found response @@ -643,9 +642,9 @@ func (a *MachineController) getAuthSecrets(ctx context.Context, authConfigMap *v return authSecrets, nil } -func (a *MachineController) getProviderConfigMaps(ctx context.Context, provider *existinginfrav1.ExistingInfraCluster) (map[string]*v1.ConfigMap, error) { +func (a *MachineController) getProviderConfigMaps(ctx context.Context, provider *existinginfrav1.ExistingInfraCluster) (map[string]*corev1.ConfigMap, error) { fileSpecs := provider.Spec.OS.Files - configMaps := map[string]*v1.ConfigMap{} + configMaps := map[string]*corev1.ConfigMap{} for _, fileSpec := range fileSpecs { mapName := fileSpec.Source.ConfigMap if _, seen := configMaps[mapName]; !seen { @@ -757,7 +756,7 @@ func nodeVersion(node *corev1.Node) string { func (a *MachineController) uncordon(ctx context.Context, node *corev1.Node) error { contextLog := log.WithFields(log.Fields{"node": node.Name}) retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - var result v1.Node + var result corev1.Node getErr := a.client.Get(ctx, client.ObjectKey{Name: node.Name}, &result) if getErr != nil { contextLog.Errorf("failed to read node info, can't reschedule: %v", getErr) @@ -823,7 +822,7 @@ func (a *MachineController) removeNodeLabel(ctx context.Context, node *corev1.No func (a *MachineController) modifyNode(ctx context.Context, node *corev1.Node, updater func(node *corev1.Node)) error { contextLog := log.WithFields(log.Fields{"node": node.Name}) retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - result := &v1.Node{} + result := &corev1.Node{} getErr := a.client.Get(ctx, client.ObjectKey{Name: node.Name}, result) if getErr != nil { contextLog.Errorf("failed to read node info, assuming unsafe to update: %v", getErr) @@ -844,7 +843,7 @@ func (a *MachineController) modifyNode(ctx context.Context, node *corev1.Node, u return nil } -func (a *MachineController) checkMasterHAConstraint(ctx context.Context, nodeBeingUpdated *v1.Node) error { +func (a *MachineController) checkMasterHAConstraint(ctx context.Context, nodeBeingUpdated *corev1.Node) error { nodes, err := a.getMasterNodes(ctx) if err != nil { // If we can't read the nodes, return the error so we don't @@ -869,7 +868,7 @@ func (a *MachineController) checkMasterHAConstraint(ctx context.Context, nodeBei // we compare Nodes by name, because name is required to be unique and // uids will differ if we manage to delete and recreate the object. -func sameNode(a, b *v1.Node) bool { +func sameNode(a, b *corev1.Node) bool { return a.Name == b.Name } @@ -892,7 +891,7 @@ func hasTaint(node *corev1.Node, value string) bool { } func (a *MachineController) findNodeByID(ctx context.Context, machineID, systemUUID string) (*corev1.Node, error) { - var nodes v1.NodeList + var nodes corev1.NodeList err := a.client.List(ctx, &nodes) if err != nil { return nil, gerrors.Wrap(err, "failed to list nodes") @@ -925,7 +924,7 @@ func (a *MachineController) getMasterNode(ctx context.Context) (*corev1.Node, er } func (a *MachineController) getMasterNodes(ctx context.Context) ([]*corev1.Node, error) { - var nodes v1.NodeList + var nodes corev1.NodeList err := a.client.List(ctx, &nodes) if err != nil { return nil, gerrors.Wrap(err, "failed to list nodes") @@ -966,7 +965,7 @@ func (a *MachineController) isControllerNode(ctx context.Context, node *corev1.N } func (a *MachineController) getControllerNodeName(ctx context.Context) (string, error) { - var pods v1.PodList + var pods corev1.PodList err := a.client.List(ctx, &pods, &client.ListOptions{Namespace: a.controllerNamespace}) if err != nil { return "", err From e5ab4f8b71ab22f0c923d66b9a9b2f2e8989eb34 Mon Sep 17 00:00:00 2001 From: Dennis Marttinen Date: Mon, 10 Aug 2020 16:57:52 +0300 Subject: [PATCH 3/3] Use stack allocation where possible Signed-off-by: Dennis Marttinen --- .../controller/wksctl/machine_controller.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/apis/wksprovider/controller/wksctl/machine_controller.go b/pkg/apis/wksprovider/controller/wksctl/machine_controller.go index d0791633..49607807 100644 --- a/pkg/apis/wksprovider/controller/wksctl/machine_controller.go +++ b/pkg/apis/wksprovider/controller/wksctl/machine_controller.go @@ -648,12 +648,12 @@ func (a *MachineController) getProviderConfigMaps(ctx context.Context, provider for _, fileSpec := range fileSpecs { mapName := fileSpec.Source.ConfigMap if _, seen := configMaps[mapName]; !seen { - configMap := &corev1.ConfigMap{} - err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: mapName}, configMap) + var configMap corev1.ConfigMap + err := a.client.Get(ctx, client.ObjectKey{Namespace: a.controllerNamespace, Name: mapName}, &configMap) if err != nil { return nil, err } - configMaps[mapName] = configMap + configMaps[mapName] = &configMap } } return configMaps, nil @@ -822,14 +822,14 @@ func (a *MachineController) removeNodeLabel(ctx context.Context, node *corev1.No func (a *MachineController) modifyNode(ctx context.Context, node *corev1.Node, updater func(node *corev1.Node)) error { contextLog := log.WithFields(log.Fields{"node": node.Name}) retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - result := &corev1.Node{} - getErr := a.client.Get(ctx, client.ObjectKey{Name: node.Name}, result) + var result corev1.Node + getErr := a.client.Get(ctx, client.ObjectKey{Name: node.Name}, &result) if getErr != nil { contextLog.Errorf("failed to read node info, assuming unsafe to update: %v", getErr) return getErr } - updater(result) - updateErr := a.client.Update(ctx, result) + updater(&result) + updateErr := a.client.Update(ctx, &result) if updateErr != nil { contextLog.Errorf("failed attempt to update node annotation: %v", updateErr) return updateErr