kubeadm: ensure proper parsing of SSR username

- Split the code that tries to get node name from SSR into
a new function getNodeNameFromSSR(). Unit test the function.
- Fix error that the "system:nodes:" prefix was not trimmed.
- Fix mislearding errors around FetchInitConfigurationFromCluster.
This function performs multiple actions, and the "get node"
action can also be of type apierrors.NotFound(). This creates
confusion in the returned error in enforceRequirement during
upgrade. Fix this problem.

Author: neolit123

cmd/kubeadm/app/cmd/upgrade/common.go

@@ -26,8 +26,6 @@ import (
 	"github.com/pkg/errors"
 	"github.com/spf13/pflag"
 
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	fakediscovery "k8s.io/client-go/discovery/fake"
 	clientset "k8s.io/client-go/kubernetes"
@@ -96,11 +94,6 @@ func enforceRequirements(flagSet *pflag.FlagSet, flags *applyPlanFlags, args []s
 
 	initCfg, err := configutil.FetchInitConfigurationFromCluster(client, printer, "upgrade/config", false, false)
 	if err != nil {
-		if apierrors.IsNotFound(err) {
-			_, _ = printer.Printf("[upgrade/config] In order to upgrade, a ConfigMap called %q in the %q namespace must exist.\n", constants.KubeadmConfigConfigMap, metav1.NamespaceSystem)
-			_, _ = printer.Printf("[upgrade/config] Use 'kubeadm init phase upload-config --config your-config.yaml' to re-upload it.\n")
-			err = errors.Errorf("the ConfigMap %q in the %q namespace was not found", constants.KubeadmConfigConfigMap, metav1.NamespaceSystem)
-		}
 		return nil, nil, nil, nil, errors.Wrap(err, "[upgrade/init config] FATAL")
 	}
 

cmd/kubeadm/app/util/config/cluster.go

@@ -53,8 +53,9 @@ func FetchInitConfigurationFromCluster(client clientset.Interface, printer outpu
 	if printer == nil {
 		printer = &output.TextPrinter{}
 	}
-	printer.Printf("[%s] Reading configuration from the cluster...\n", logPrefix)
-	printer.Printf("[%s] FYI: You can look at this config file with 'kubectl -n %s get cm %s -o yaml'\n", logPrefix, metav1.NamespaceSystem, constants.KubeadmConfigConfigMap)
+	_, _ = printer.Printf("[%s] Reading configuration from the %q ConfigMap in namespace %q...\n",
+		logPrefix, constants.KubeadmConfigConfigMap, metav1.NamespaceSystem)
+	_, _ = printer.Printf("[%s] Use 'kubeadm init phase upload-config --config your-config.yaml' to re-upload it.\n", logPrefix)
 
 	// Fetch the actual config from cluster
 	cfg, err := getInitConfigurationFromCluster(constants.KubernetesDir, client, newControlPlane, skipComponentConfigs)
@@ -139,11 +140,9 @@ func GetNodeName(kubeconfigFile string) (string, error) {
 	if kubeconfigFile != "" {
 		client, err := kubeconfig.ClientSetFromFile(kubeconfigFile)
 		if err == nil {
-			ssr, err := client.AuthenticationV1().SelfSubjectReviews().
-				Create(context.Background(), &authv1.SelfSubjectReview{}, metav1.CreateOptions{})
-
-			if err == nil && ssr.Status.UserInfo.Username != "" {
-				return ssr.Status.UserInfo.Username, nil
+			nodeName, err = getNodeNameFromSSR(client)
+			if err == nil {
+				return nodeName, nil
 			}
 		}
 		nodeName, err = getNodeNameFromKubeletConfig(kubeconfigFile)
@@ -167,7 +166,7 @@ func GetNodeRegistration(kubeconfigFile string, client clientset.Interface, node
 	}
 
 	// gets the corresponding node and retrieves attributes stored there.
-	node, err := client.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{})
+	node, err := client.CoreV1().Nodes().Get(context.Background(), nodeName, metav1.GetOptions{})
 	if err != nil {
 		return errors.Wrap(err, "failed to get corresponding node")
 	}
@@ -231,6 +230,22 @@ func getNodeNameFromKubeletConfig(fileName string) (string, error) {
 	return strings.TrimPrefix(cert.Subject.CommonName, constants.NodesUserPrefix), nil
 }
 
+// getNodeNameFromSSR reads the node name from the SelfSubjectReview for a given client.
+// If the kubelet.conf is passed as fileName it can be used to retrieve the node name.
+func getNodeNameFromSSR(client clientset.Interface) (string, error) {
+	ssr, err := client.AuthenticationV1().SelfSubjectReviews().
+		Create(context.Background(), &authv1.SelfSubjectReview{}, metav1.CreateOptions{})
+	if err != nil {
+		return "", err
+	}
+	user := ssr.Status.UserInfo.Username
+	if !strings.HasPrefix(user, constants.NodesUserPrefix) {
+		return "", errors.Errorf("%q is not a node client, must have %q prefix in the name",
+			user, constants.NodesUserPrefix)
+	}
+	return strings.TrimPrefix(user, constants.NodesUserPrefix), nil
+}
+
 func getAPIEndpoint(client clientset.Interface, nodeName string, apiEndpoint *kubeadmapi.APIEndpoint) error {
 	return getAPIEndpointWithRetry(client, nodeName, apiEndpoint,
 		constants.KubernetesAPICallRetryInterval, kubeadmapi.GetActiveTimeouts().KubernetesAPICall.Duration)

cmd/kubeadm/app/util/config/cluster_test.go

@@ -29,6 +29,7 @@ import (
 
 	"github.com/pkg/errors"
 
+	authv1 "k8s.io/api/authentication/v1"
 	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -860,3 +861,75 @@ func TestGetRawAPIEndpointFromPodAnnotationWithoutRetry(t *testing.T) {
 		})
 	}
 }
+
+func TestGetNodeNameFromSSR(t *testing.T) {
+	var tests = []struct {
+		name             string
+		clientSetup      func(*clientsetfake.Clientset)
+		expectedNodeName string
+		expectedError    bool
+	}{
+		{
+			name: "valid node name",
+			clientSetup: func(clientset *clientsetfake.Clientset) {
+				clientset.PrependReactor("create", "selfsubjectreviews",
+					func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+						obj := &authv1.SelfSubjectReview{
+							Status: authv1.SelfSubjectReviewStatus{
+								UserInfo: authv1.UserInfo{
+									Username: kubeadmconstants.NodesUserPrefix + "foo",
+								},
+							},
+						}
+						return true, obj, nil
+					})
+			},
+			expectedNodeName: "foo",
+			expectedError:    false,
+		},
+		{
+			name: "SSR created but client is not a node",
+			clientSetup: func(clientset *clientsetfake.Clientset) {
+				clientset.PrependReactor("create", "selfsubjectreviews",
+					func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+						obj := &authv1.SelfSubjectReview{
+							Status: authv1.SelfSubjectReviewStatus{
+								UserInfo: authv1.UserInfo{
+									Username: "foo",
+								},
+							},
+						}
+						return true, obj, nil
+					})
+			},
+			expectedNodeName: "",
+			expectedError:    true,
+		},
+		{
+			name: "error creating SSR",
+			clientSetup: func(clientset *clientsetfake.Clientset) {
+				clientset.PrependReactor("create", "selfsubjectreviews",
+					func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+						return true, nil, errors.New("")
+					})
+			},
+			expectedNodeName: "",
+			expectedError:    true,
+		},
+	}
+	for _, rt := range tests {
+		t.Run(rt.name, func(t *testing.T) {
+			client := clientsetfake.NewSimpleClientset()
+			rt.clientSetup(client)
+
+			nodeName, err := getNodeNameFromSSR(client)
+
+			if (err != nil) != rt.expectedError {
+				t.Fatalf("expected error: %+v, got: %+v", rt.expectedError, err)
+			}
+			if rt.expectedNodeName != nodeName {
+				t.Fatalf("expected nodeName: %s, got: %s", rt.expectedNodeName, nodeName)
+			}
+		})
+	}
+}