Separate certificate logic for joins

With the introduction of support for external etcd comes better
certificate management. This commit separates the worker join certificate
logic from the control plane join certificate logic.

Signed-off-by: Chuck Ha <[email protected]>

Author: chuckha

controllers/kubeadmconfig_controller.go

@@ -227,7 +227,7 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 			return ctrl.Result{}, err
 		}
 
-		certificates := internalcluster.NewCertificatesForControlPlane(config.Spec.ClusterConfiguration)
+		certificates := internalcluster.NewCertificatesForInitialControlPlane(config.Spec.ClusterConfiguration)
 		if err := certificates.LookupOrGenerate(ctx, r.Client, cluster, config); err != nil {
 			log.Error(err, "unable to lookup or create cluster certificates")
 			return ctrl.Result{}, err
@@ -266,56 +266,41 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 	if config.Spec.JoinConfiguration == nil {
 		log.Info("Creating default JoinConfiguration")
 		config.Spec.JoinConfiguration = &kubeadmv1beta1.JoinConfiguration{}
-		if util.IsControlPlaneMachine(machine) {
-			config.Spec.JoinConfiguration.ControlPlane = &kubeadmv1beta1.JoinControlPlane{}
-		}
-	}
-
-	certificates := internalcluster.NewCertificatesForWorker(config.Spec.JoinConfiguration.CACertPath)
-	if err := certificates.Lookup(ctx, r.Client, cluster); err != nil {
-		log.Error(err, "unable to lookup cluster certificates")
-		return ctrl.Result{}, err
-	}
-	if err := certificates.EnsureAllExist(); err != nil {
-		return ctrl.Result{}, err
 	}
 
-	hashes, err := certificates.GetByPurpose(secret.ClusterCA).Hashes()
-	if err != nil {
-		log.Error(err, "Unable to generate Cluster CA certificate hashes")
-		return ctrl.Result{}, err
-	}
-	// TODO: move this into reconcile.Discovery so that defaults for the Discovery are all in the same place
-	if config.Spec.JoinConfiguration.Discovery.BootstrapToken == nil {
-		config.Spec.JoinConfiguration.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{}
-	}
-	config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes = hashes
+	// it's a control plane join
+	if util.IsControlPlaneMachine(machine) {
+		if config.Spec.JoinConfiguration.ControlPlane == nil {
+			config.Spec.JoinConfiguration.ControlPlane = &kubeadmv1beta1.JoinControlPlane{}
+		}
 
-	// ensure that joinConfiguration.Discovery is properly set for joining node on the current cluster
-	if err := r.reconcileDiscovery(cluster, config); err != nil {
-		if requeueErr, ok := errors.Cause(err).(capierrors.HasRequeueAfterError); ok {
-			log.Info(err.Error())
-			return ctrl.Result{RequeueAfter: requeueErr.GetRequeueAfter()}, nil
+		certificates := internalcluster.NewCertificatesForJoiningControlPlane()
+		if err := certificates.Lookup(ctx, r.Client, cluster); err != nil {
+			log.Error(err, "unable to lookup cluster certificates")
+			return ctrl.Result{}, err
+		}
+		if err := certificates.EnsureAllExist(); err != nil {
+			return ctrl.Result{}, err
 		}
-		return ctrl.Result{}, err
-	}
 
-	joindata, err := kubeadmv1beta1.ConfigurationToYAML(config.Spec.JoinConfiguration)
-	if err != nil {
-		log.Error(err, "failed to marshal join configuration")
-		return ctrl.Result{}, err
-	}
+		// ensure that joinConfiguration.Discovery is properly set for joining node on the current cluster
+		if err := r.reconcileDiscovery(cluster, config, certificates); err != nil {
+			if requeueErr, ok := errors.Cause(err).(capierrors.HasRequeueAfterError); ok {
+				log.Info(err.Error())
+				return ctrl.Result{RequeueAfter: requeueErr.GetRequeueAfter()}, nil
+			}
+			return ctrl.Result{}, err
+		}
 
-	// it's a control plane join
-	if util.IsControlPlaneMachine(machine) {
-		if config.Spec.JoinConfiguration.ControlPlane == nil {
-			return ctrl.Result{}, errors.New("Machine is a ControlPlane, but JoinConfiguration.ControlPlane is not set in the KubeadmConfig object")
+		joinData, err := kubeadmv1beta1.ConfigurationToYAML(config.Spec.JoinConfiguration)
+		if err != nil {
+			log.Error(err, "failed to marshal join configuration")
+			return ctrl.Result{}, err
 		}
 
 		log.Info("Creating BootstrapData for the join control plane")
-
 		cloudJoinData, err := cloudinit.NewJoinControlPlane(&cloudinit.ControlPlaneJoinInput{
-			JoinConfiguration: joindata,
+			JoinConfiguration: joinData,
 			Certificates:      certificates,
 			BaseUserData: cloudinit.BaseUserData{
 				AdditionalFiles:     config.Spec.Files,
@@ -335,7 +320,32 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 		return ctrl.Result{}, nil
 	}
 
-	// otherwise it is a node
+	// It's a worker join
+	certificates := internalcluster.NewCertificatesForWorker(config.Spec.JoinConfiguration.CACertPath)
+	if err := certificates.Lookup(ctx, r.Client, cluster); err != nil {
+		log.Error(err, "unable to lookup cluster certificates")
+		return ctrl.Result{}, err
+	}
+	if err := certificates.EnsureAllExist(); err != nil {
+		log.Error(err, "Missing certificates")
+		return ctrl.Result{}, err
+	}
+
+	// ensure that joinConfiguration.Discovery is properly set for joining node on the current cluster
+	if err := r.reconcileDiscovery(cluster, config, certificates); err != nil {
+		if requeueErr, ok := errors.Cause(err).(capierrors.HasRequeueAfterError); ok {
+			log.Info(err.Error())
+			return ctrl.Result{RequeueAfter: requeueErr.GetRequeueAfter()}, nil
+		}
+		return ctrl.Result{}, err
+	}
+
+	joinData, err := kubeadmv1beta1.ConfigurationToYAML(config.Spec.JoinConfiguration)
+	if err != nil {
+		log.Error(err, "failed to marshal join configuration")
+		return ctrl.Result{}, err
+	}
+
 	if config.Spec.JoinConfiguration.ControlPlane != nil {
 		return ctrl.Result{}, errors.New("Machine is a Worker, but JoinConfiguration.ControlPlane is set in the KubeadmConfig object")
 	}
@@ -350,7 +360,7 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 			PostKubeadmCommands: config.Spec.PostKubeadmCommands,
 			Users:               config.Spec.Users,
 		},
-		JoinConfiguration: joindata,
+		JoinConfiguration: joinData,
 	})
 	if err != nil {
 		log.Error(err, "failed to create a worker join configuration")
@@ -416,7 +426,7 @@ func (r *KubeadmConfigReconciler) MachineToBootstrapMapFunc(o handler.MapObject)
 // The implementation func respect user provided discovery configurations, but in case some of them are missing, a valid BootstrapToken object
 // is automatically injected into config.JoinConfiguration.Discovery.
 // This allows to simplify configuration UX, by providing the option to delegate to CABPK the configuration of kubeadm join discovery.
-func (r *KubeadmConfigReconciler) reconcileDiscovery(cluster *clusterv1.Cluster, config *bootstrapv1.KubeadmConfig) error {
+func (r *KubeadmConfigReconciler) reconcileDiscovery(cluster *clusterv1.Cluster, config *bootstrapv1.KubeadmConfig, certificates internalcluster.Certificates) error {
 	log := r.Log.WithValues("kubeadmconfig", fmt.Sprintf("%s/%s", config.Namespace, config.Name))
 
 	// if config already contains a file discovery configuration, respect it without further validations
@@ -429,6 +439,16 @@ func (r *KubeadmConfigReconciler) reconcileDiscovery(cluster *clusterv1.Cluster,
 		config.Spec.JoinConfiguration.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{}
 	}
 
+	// calculate the ca cert hashes if they are not already set
+	if len(config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes) == 0 {
+		hashes, err := certificates.GetByPurpose(secret.ClusterCA).Hashes()
+		if err != nil {
+			log.Error(err, "Unable to generate Cluster CA certificate hashes")
+			return err
+		}
+		config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes = hashes
+	}
+
 	// if BootstrapToken already contains an APIServerEndpoint, respect it; otherwise inject the APIServerEndpoint endpoint defined in cluster status
 	apiServerEndpoint := config.Spec.JoinConfiguration.Discovery.BootstrapToken.APIServerEndpoint
 	if apiServerEndpoint == "" {

controllers/kubeadmconfig_controller_test.go

@@ -400,7 +400,7 @@ func TestKubeadmConfigReconciler_Reconcile_GenerateCloudConfigData(t *testing.T)
 	}
 }
 
-// If a controlplane has an invalid JoinConfiguration then user intervention is required.
+// If a control plane has no JoinConfiguration, then we will create a default and no error will occur
 func TestKubeadmConfigReconciler_Reconcile_ErrorIfJoiningControlPlaneHasInvalidConfiguration(t *testing.T) {
 	// TODO: extract this kind of code into a setup function that puts the state of objects into an initialized controlplane (implies secrets exist)
 	cluster := newCluster("cluster")
@@ -436,8 +436,8 @@ func TestKubeadmConfigReconciler_Reconcile_ErrorIfJoiningControlPlaneHasInvalidC
 		},
 	}
 	_, err := k.Reconcile(request)
-	if err == nil {
-		t.Fatal("Expected error, got nil")
+	if err != nil {
+		t.Fatalf("Expected no error but got %v", err)
 	}
 }
 
@@ -598,6 +598,11 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
 	}
 
 	dummyCAHash := []string{"...."}
+	bootstrapToken := kubeadmv1beta1.Discovery{
+		BootstrapToken: &kubeadmv1beta1.BootstrapTokenDiscovery{
+			CACertHashes: dummyCAHash,
+		},
+	}
 	goodcluster := &clusterv1.Cluster{
 		Status: clusterv1.ClusterStatus{
 			APIEndpoints: []clusterv1.APIEndpoint{
@@ -619,7 +624,9 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
 			cluster: goodcluster,
 			config: &bootstrapv1.KubeadmConfig{
 				Spec: bootstrapv1.KubeadmConfigSpec{
-					JoinConfiguration: &kubeadmv1beta1.JoinConfiguration{},
+					JoinConfiguration: &kubeadmv1beta1.JoinConfiguration{
+						Discovery: bootstrapToken,
+					},
 				},
 			},
 			validateDiscovery: func(c *bootstrapv1.KubeadmConfig) error {
@@ -633,8 +640,8 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
 				if d.BootstrapToken.APIServerEndpoint != "example.com:6443" {
 					return errors.Errorf("BootstrapToken.APIServerEndpoint=example.com:6443 expected, got %q", d.BootstrapToken.APIServerEndpoint)
 				}
-				if d.BootstrapToken.UnsafeSkipCAVerification != true {
-					return errors.Errorf("BootstrapToken.UnsafeSkipCAVerification=true expected, got false")
+				if d.BootstrapToken.UnsafeSkipCAVerification == true {
+					return errors.Errorf("BootstrapToken.UnsafeSkipCAVerification=false expected, got true")
 				}
 				return nil
 			},
@@ -667,6 +674,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
 					JoinConfiguration: &kubeadmv1beta1.JoinConfiguration{
 						Discovery: kubeadmv1beta1.Discovery{
 							BootstrapToken: &kubeadmv1beta1.BootstrapTokenDiscovery{
+								CACertHashes:      dummyCAHash,
 								APIServerEndpoint: "bar.com:6443",
 							},
 						},
@@ -689,7 +697,8 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
 					JoinConfiguration: &kubeadmv1beta1.JoinConfiguration{
 						Discovery: kubeadmv1beta1.Discovery{
 							BootstrapToken: &kubeadmv1beta1.BootstrapTokenDiscovery{
-								Token: "abcdef.0123456789abcdef",
+								CACertHashes: dummyCAHash,
+								Token:        "abcdef.0123456789abcdef",
 							},
 						},
 					},
@@ -729,7 +738,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			err := k.reconcileDiscovery(tc.cluster, tc.config)
+			err := k.reconcileDiscovery(tc.cluster, tc.config, internalcluster.Certificates{})
 			if err != nil {
 				t.Errorf("expected nil, got error %v", err)
 			}
@@ -758,15 +767,21 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileFailureBehaviors(t
 			cluster: &clusterv1.Cluster{}, // cluster without endpoints
 			config: &bootstrapv1.KubeadmConfig{
 				Spec: bootstrapv1.KubeadmConfigSpec{
-					JoinConfiguration: &kubeadmv1beta1.JoinConfiguration{},
+					JoinConfiguration: &kubeadmv1beta1.JoinConfiguration{
+						Discovery: kubeadmv1beta1.Discovery{
+							BootstrapToken: &kubeadmv1beta1.BootstrapTokenDiscovery{
+								CACertHashes: []string{"item"},
+							},
+						},
+					},
 				},
 			},
 		},
 	}
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			err := k.reconcileDiscovery(tc.cluster, tc.config)
+			err := k.reconcileDiscovery(tc.cluster, tc.config, internalcluster.Certificates{})
 			if err == nil {
 				t.Error("expected error, got nil")
 			}
@@ -1270,7 +1285,7 @@ func createSecrets(t *testing.T, cluster *clusterv1.Cluster, owner *bootstrapv1.
 	if owner.Spec.ClusterConfiguration == nil {
 		owner.Spec.ClusterConfiguration = &kubeadmv1beta1.ClusterConfiguration{}
 	}
-	certificates := internalcluster.NewCertificatesForControlPlane(owner.Spec.ClusterConfiguration)
+	certificates := internalcluster.NewCertificatesForInitialControlPlane(owner.Spec.ClusterConfiguration)
 	if err := certificates.Generate(); err != nil {
 		t.Fatal(err)
 	}

internal/cluster/certificates.go

@@ -74,8 +74,8 @@ var (
 // Certificates are the certificates necessary to bootstrap a cluster.
 type Certificates []*Certificate
 
-// NewCertificatesForControlPlane returns a list of certificates configured for a control plane node
-func NewCertificatesForControlPlane(config *v1beta1.ClusterConfiguration) Certificates {
+// NewCertificatesForInitialControlPlane returns a list of certificates configured for a control plane node
+func NewCertificatesForInitialControlPlane(config *v1beta1.ClusterConfiguration) Certificates {
 	if config.CertificatesDir == "" {
 		config.CertificatesDir = defaultCertificatesDir
 	}
@@ -122,6 +122,32 @@ func NewCertificatesForControlPlane(config *v1beta1.ClusterConfiguration) Certif
 	return certificates
 }
 
+// NewCertificatesForJoiningControlPlane gets any certs that exist and writes them to disk
+func NewCertificatesForJoiningControlPlane() Certificates {
+	return Certificates{
+		&Certificate{
+			Purpose:  secret.ClusterCA,
+			CertFile: filepath.Join(defaultCertificatesDir, "ca.crt"),
+			KeyFile:  filepath.Join(defaultCertificatesDir, "ca.key"),
+		},
+		&Certificate{
+			Purpose:  ServiceAccount,
+			CertFile: filepath.Join(defaultCertificatesDir, "sa.pub"),
+			KeyFile:  filepath.Join(defaultCertificatesDir, "sa.key"),
+		},
+		&Certificate{
+			Purpose:  FrontProxyCA,
+			CertFile: filepath.Join(defaultCertificatesDir, "front-proxy-ca.crt"),
+			KeyFile:  filepath.Join(defaultCertificatesDir, "front-proxy-ca.key"),
+		},
+		&Certificate{
+			Purpose:  EtcdCA,
+			CertFile: filepath.Join(defaultCertificatesDir, "etcd", "ca.crt"),
+			KeyFile:  filepath.Join(defaultCertificatesDir, "etcd", "ca.key"),
+		},
+	}
+}
+
 // NewCertificatesForWorker return an initialized but empty set of CA certificates needed to bootstrap a cluster.
 func NewCertificatesForWorker(caCertPath string) Certificates {
 	if caCertPath == "" {

internal/cluster/certificates_test.go

@@ -24,7 +24,7 @@ import (
 
 func TestNewCertificatesForControlPlane_Stacked(t *testing.T) {
 	config := &v1beta1.ClusterConfiguration{}
-	certs := NewCertificatesForControlPlane(config)
+	certs := NewCertificatesForInitialControlPlane(config)
 	if certs.GetByPurpose(EtcdCA).KeyFile == "" {
 		t.Fatal("stacked control planes must define etcd CA key file")
 	}
@@ -37,7 +37,7 @@ func TestNewCertificatesForControlPlane_External(t *testing.T) {
 		},
 	}
 
-	certs := NewCertificatesForControlPlane(config)
+	certs := NewCertificatesForInitialControlPlane(config)
 	if certs.GetByPurpose(EtcdCA).KeyFile != "" {
 		t.Fatal("control planes with external etcd must *not* define the etcd key file")
 	}