From 3aceccd92336d1d0064e367952e69958a88fc9e3 Mon Sep 17 00:00:00 2001 From: Chuck Ha Date: Wed, 2 Oct 2019 09:38:36 -0600 Subject: [PATCH] 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 --- controllers/kubeadmconfig_controller.go | 108 +++++++++++-------- controllers/kubeadmconfig_controller_test.go | 37 +++++-- internal/cluster/certificates.go | 30 +++++- internal/cluster/certificates_test.go | 4 +- 4 files changed, 120 insertions(+), 59 deletions(-) diff --git a/controllers/kubeadmconfig_controller.go b/controllers/kubeadmconfig_controller.go index 1c9a0634b088..f218c195313b 100644 --- a/controllers/kubeadmconfig_controller.go +++ b/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 == "" { diff --git a/controllers/kubeadmconfig_controller_test.go b/controllers/kubeadmconfig_controller_test.go index 6966539ca13f..00a2455e4990 100644 --- a/controllers/kubeadmconfig_controller_test.go +++ b/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,7 +767,13 @@ 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"}, + }, + }, + }, }, }, }, @@ -766,7 +781,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileFailureBehaviors(t 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) } diff --git a/internal/cluster/certificates.go b/internal/cluster/certificates.go index 8ca62d115436..51b4da5dfbfa 100644 --- a/internal/cluster/certificates.go +++ b/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 == "" { diff --git a/internal/cluster/certificates_test.go b/internal/cluster/certificates_test.go index b4efe6f8a714..d1070b91f104 100644 --- a/internal/cluster/certificates_test.go +++ b/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") }