Merge pull request #263 from chuckha/control-plane-join-certs

🐛 Separate certificate logic for joins

Author: k8s-ci-robot

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")
 	}