diff --git a/controllers/kubeadmconfig_controller.go b/controllers/kubeadmconfig_controller.go index e58b8ec..d9bd8af 100644 --- a/controllers/kubeadmconfig_controller.go +++ b/controllers/kubeadmconfig_controller.go @@ -267,10 +267,15 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re } if certificates != nil { hashes, err := certs.CertificateHashes(certificates.ClusterCA.Cert) - if err == nil { - config.Spec.JoinConfiguration.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{ - CACertHashes: hashes, + if err != nil { + log.Error(err, "Unable to generate certificate hashes") + return ctrl.Result{}, err + } + if hashes != nil { + if config.Spec.JoinConfiguration.Discovery.BootstrapToken == nil { + config.Spec.JoinConfiguration.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{} } + config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes = hashes } } @@ -432,9 +437,8 @@ func (r *KubeadmConfigReconciler) reconcileDiscovery(cluster *clusterv1.Cluster, log.Info("Altering JoinConfiguration.Discovery.BootstrapToken", "Token", token) } - // if BootstrapToken already contains a CACertHashes or UnsafeSkipCAVerification, respect it; otherwise set for UnsafeSkipCAVerification - // TODO: set CACertHashes instead of UnsafeSkipCAVerification - if len(config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes) == 0 && !config.Spec.JoinConfiguration.Discovery.BootstrapToken.UnsafeSkipCAVerification { + // If the BootstrapToken does not contain any CACertHashes then force skip CA Verification + if len(config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes) == 0 { log.Info("No CAs were provided. Falling back to insecure discover method by skipping CA Cert validation") config.Spec.JoinConfiguration.Discovery.BootstrapToken.UnsafeSkipCAVerification = true } diff --git a/controllers/kubeadmconfig_controller_test.go b/controllers/kubeadmconfig_controller_test.go index 81e8388..cf740aa 100644 --- a/controllers/kubeadmconfig_controller_test.go +++ b/controllers/kubeadmconfig_controller_test.go @@ -280,6 +280,7 @@ func TestKubeadmConfigReconciler_Reconcile_RequeueJoiningNodesIfControlPlaneNotI } } +// This generates cloud-config data but does not test the validity of it. func TestKubeadmConfigReconciler_Reconcile_GenerateCloudConfigData(t *testing.T) { cluster := newCluster("cluster") cluster.Status.InfrastructureReady = true @@ -339,6 +340,8 @@ func TestKubeadmConfigReconciler_Reconcile_GenerateCloudConfigData(t *testing.T) } } +// Return an error if a worker has no JoinConfiguration defined +// TODO: This logic should not error in this case. A JoinConfiguration should be autogenerated func TestKubeadmConfigReconciler_Reconcile_ErrorIfAWorkerHasNoJoinConfigurationAndTheControlPlaneIsInitialized(t *testing.T) { cluster := newCluster("cluster") cluster.Status.InfrastructureReady = true @@ -374,7 +377,6 @@ func TestKubeadmConfigReconciler_Reconcile_ErrorIfAWorkerHasNoJoinConfigurationA } // If a controlplane has an invalid JoinConfiguration then user intervention is required. -// TODO: Could potentially requeue instead of error. func TestKubeadmConfigReconciler_Reconcile_ErrorIfJoiningControlPlaneHasInvalidConfiguration(t *testing.T) { cluster := newCluster("cluster") cluster.Status.InfrastructureReady = true @@ -550,7 +552,8 @@ func TestReconcileIfJoinNodesAndControlPlaneIsReady(t *testing.T) { } } -func TestReconcileDiscoverySuccces(t *testing.T) { +// Ensure the discovery portion of the JoinConfiguration gets generated correctly. +func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testing.T) { k := &KubeadmConfigReconciler{ Log: log.Log, Client: nil, @@ -563,13 +566,13 @@ func TestReconcileDiscoverySuccces(t *testing.T) { Status: clusterv1.ClusterStatus{ APIEndpoints: []clusterv1.APIEndpoint{ { - Host: "foo.com", + Host: "example.com", Port: 6443, }, }, }, } - var useCases = []struct { + testcases := []struct { name string cluster *clusterv1.Cluster config *bootstrapv1.KubeadmConfig @@ -591,8 +594,8 @@ func TestReconcileDiscoverySuccces(t *testing.T) { if d.BootstrapToken.Token == "" { return errors.Errorf(("BootstrapToken.Token expected, got empty string")) } - if d.BootstrapToken.APIServerEndpoint != "foo.com:6443" { - return errors.Errorf("BootstrapToken.APIServerEndpoint=foo.com:6443 expected, got %q", d.BootstrapToken.APIServerEndpoint) + 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") @@ -688,28 +691,28 @@ func TestReconcileDiscoverySuccces(t *testing.T) { }, } - for _, rt := range useCases { - rt := rt - t.Run(rt.name, func(t *testing.T) { - err := k.reconcileDiscovery(rt.cluster, rt.config) + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := k.reconcileDiscovery(tc.cluster, tc.config) if err != nil { t.Errorf("expected nil, got error %v", err) } - if err := rt.validateDiscovery(rt.config); err != nil { + if err := tc.validateDiscovery(tc.config); err != nil { t.Fatal(err) } }) } } -func TestReconcileDiscoveryErrors(t *testing.T) { +// Test failure cases for the discovery reconcile function. +func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileFailureBehaviors(t *testing.T) { k := &KubeadmConfigReconciler{ Log: log.Log, Client: nil, } - var useCases = []struct { + testcases := []struct { name string cluster *clusterv1.Cluster config *bootstrapv1.KubeadmConfig @@ -725,10 +728,9 @@ func TestReconcileDiscoveryErrors(t *testing.T) { }, } - for _, rt := range useCases { - rt := rt - t.Run(rt.name, func(t *testing.T) { - err := k.reconcileDiscovery(rt.cluster, rt.config) + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := k.reconcileDiscovery(tc.cluster, tc.config) if err == nil { t.Error("expected error, got nil") } @@ -736,13 +738,14 @@ func TestReconcileDiscoveryErrors(t *testing.T) { } } -func TestReconcileTopLevelObjectSettings(t *testing.T) { +// Set cluster configuration defaults based on dynamic values from the cluster object. +func TestKubeadmConfigReconciler_Reconcile_DynamicDefaultsForClusterConfiguration(t *testing.T) { k := &KubeadmConfigReconciler{ Log: log.Log, Client: nil, } - var useCases = []struct { + testcases := []struct { name string cluster *clusterv1.Cluster machine *clusterv1.Machine @@ -815,39 +818,45 @@ func TestReconcileTopLevelObjectSettings(t *testing.T) { }, } - for _, rt := range useCases { - rt := rt - t.Run(rt.name, func(t *testing.T) { - k.reconcileTopLevelObjectSettings(rt.cluster, rt.machine, rt.config) + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + k.reconcileTopLevelObjectSettings(tc.cluster, tc.machine, tc.config) - if rt.config.Spec.ClusterConfiguration.ControlPlaneEndpoint != "myControlPlaneEndpoint:6443" { - t.Fatalf("expected ClusterConfiguration.ControlPlaneEndpoint %q, got %q", "myControlPlaneEndpoint:6443", rt.config.Spec.ClusterConfiguration.ControlPlaneEndpoint) + if tc.config.Spec.ClusterConfiguration.ControlPlaneEndpoint != "myControlPlaneEndpoint:6443" { + t.Errorf("expected ClusterConfiguration.ControlPlaneEndpoint %q, got %q", "myControlPlaneEndpoint:6443", tc.config.Spec.ClusterConfiguration.ControlPlaneEndpoint) } - if rt.config.Spec.ClusterConfiguration.ClusterName != "mycluster" { - t.Fatalf("expected ClusterConfiguration.ClusterName %q, got %q", "mycluster", rt.config.Spec.ClusterConfiguration.ClusterName) + if tc.config.Spec.ClusterConfiguration.ClusterName != "mycluster" { + t.Errorf("expected ClusterConfiguration.ClusterName %q, got %q", "mycluster", tc.config.Spec.ClusterConfiguration.ClusterName) } - if rt.config.Spec.ClusterConfiguration.Networking.PodSubnet != "myPodSubnet" { - t.Fatalf("expected ClusterConfiguration.Networking.PodSubnet %q, got %q", "myPodSubnet", rt.config.Spec.ClusterConfiguration.Networking.PodSubnet) + if tc.config.Spec.ClusterConfiguration.Networking.PodSubnet != "myPodSubnet" { + t.Errorf("expected ClusterConfiguration.Networking.PodSubnet %q, got %q", "myPodSubnet", tc.config.Spec.ClusterConfiguration.Networking.PodSubnet) } - if rt.config.Spec.ClusterConfiguration.Networking.ServiceSubnet != "myServiceSubnet" { - t.Fatalf("expected ClusterConfiguration.Networking.ServiceSubnet %q, got %q", "myServiceSubnet", rt.config.Spec.ClusterConfiguration.Networking.ServiceSubnet) + if tc.config.Spec.ClusterConfiguration.Networking.ServiceSubnet != "myServiceSubnet" { + t.Errorf("expected ClusterConfiguration.Networking.ServiceSubnet %q, got %q", "myServiceSubnet", tc.config.Spec.ClusterConfiguration.Networking.ServiceSubnet) } - if rt.config.Spec.ClusterConfiguration.Networking.DNSDomain != "myDNSDomain" { - t.Fatalf("expected ClusterConfiguration.Networking.DNSDomain %q, got %q", "myDNSDomain", rt.config.Spec.ClusterConfiguration.Networking.DNSDomain) + if tc.config.Spec.ClusterConfiguration.Networking.DNSDomain != "myDNSDomain" { + t.Errorf("expected ClusterConfiguration.Networking.DNSDomain %q, got %q", "myDNSDomain", tc.config.Spec.ClusterConfiguration.Networking.DNSDomain) } - if rt.config.Spec.ClusterConfiguration.KubernetesVersion != "myversion" { - t.Fatalf("expected ClusterConfiguration.KubernetesVersion %q, got %q", "myversion", rt.config.Spec.ClusterConfiguration.KubernetesVersion) + if tc.config.Spec.ClusterConfiguration.KubernetesVersion != "myversion" { + t.Errorf("expected ClusterConfiguration.KubernetesVersion %q, got %q", "myversion", tc.config.Spec.ClusterConfiguration.KubernetesVersion) } }) } } -func TestCACertHashesAndUnsafeCAVerifySkip(t *testing.T) { - namespace := "default" // hardcoded in the new* functions +// Allow users to skip CA Verification if they *really* want to. +func TestKubeadmConfigReconciler_Reconcile_AlwaysCheckCAVerificationUnlessRequestedToSkip(t *testing.T) { + // Setup work for an initialized cluster clusterName := "my-cluster" cluster := newCluster(clusterName) cluster.Status.ControlPlaneInitialized = true cluster.Status.InfrastructureReady = true + cluster.Status.APIEndpoints = []clusterv1.APIEndpoint{ + { + Host: "example.com", + Port: 6443, + }, + } controlPlaneMachineName := "my-machine" machine := newMachine(cluster, controlPlaneMachineName) @@ -858,34 +867,66 @@ func TestCACertHashesAndUnsafeCAVerifySkip(t *testing.T) { controlPlaneConfigName := "my-config" config := newKubeadmConfig(machine, controlPlaneConfigName) - workerConfigName := "worker-join-cfg" - workerConfig := newWorkerJoinKubeadmConfig(workerMachine) - objects := []runtime.Object{ - cluster, machine, workerMachine, config, workerConfig, + cluster, machine, workerMachine, config, } objects = append(objects, createSecrets(t, cluster)...) - myclient := fake.NewFakeClientWithScheme(setupScheme(), objects...) - reconciler := KubeadmConfigReconciler{ - Client: myclient, - SecretsClientFactory: newFakeSecretFactory(), - KubeadmInitLock: &myInitLocker{}, - Log: klogr.New(), + testcases := []struct { + name string + discovery *kubeadmv1beta1.BootstrapTokenDiscovery + skipCAVerification bool + }{ + { + name: "Do not skip CA verification by default", + discovery: &kubeadmv1beta1.BootstrapTokenDiscovery{}, + skipCAVerification: false, + }, + { + name: "Skip CA verification if requested by the user", + discovery: &kubeadmv1beta1.BootstrapTokenDiscovery{ + UnsafeSkipCAVerification: true, + }, + skipCAVerification: true, + }, + { + // skipCAVerification should be true since no Cert Hashes are provided, but reconcile will *always* get or create certs. + // TODO: Certificate get/create behavior needs to be mocked to enable this test. + name: "cannot test for defaulting behavior through the reconcile function", + discovery: &kubeadmv1beta1.BootstrapTokenDiscovery{ + CACertHashes: []string{""}, + }, + skipCAVerification: false, + }, } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + myclient := fake.NewFakeClientWithScheme(setupScheme(), objects...) + reconciler := KubeadmConfigReconciler{ + Client: myclient, + SecretsClientFactory: newFakeSecretFactory(), + KubeadmInitLock: &myInitLocker{}, + Log: klogr.New(), + } - req := ctrl.Request{ - NamespacedName: types.NamespacedName{Name: workerConfigName, Namespace: namespace}, - } - if _, err := reconciler.Reconcile(req); err != nil { - t.Fatalf("reconciled an error: %v", err) - } - cfg := &bootstrapv1.KubeadmConfig{} - if err := myclient.Get(context.Background(), req.NamespacedName, cfg); err != nil { - t.Fatal(err) - } - if cfg.Spec.JoinConfiguration.Discovery.BootstrapToken.UnsafeSkipCAVerification == true { - t.Fatal("Should not skip unsafe") + wc := newWorkerJoinKubeadmConfig(workerMachine) + wc.Spec.JoinConfiguration.Discovery.BootstrapToken = tc.discovery + key := types.NamespacedName{Namespace: wc.Namespace, Name: wc.Name} + if err := myclient.Create(context.Background(), wc); err != nil { + t.Fatal(err) + } + req := ctrl.Request{NamespacedName: key} + if _, err := reconciler.Reconcile(req); err != nil { + t.Fatalf("reconciled an error: %v", err) + } + cfg := &bootstrapv1.KubeadmConfig{} + if err := myclient.Get(context.Background(), key, cfg); err != nil { + t.Fatal(err) + } + if cfg.Spec.JoinConfiguration.Discovery.BootstrapToken.UnsafeSkipCAVerification != tc.skipCAVerification { + t.Fatalf("Expected skip CA verification: %v but was %v", tc.skipCAVerification, !tc.skipCAVerification) + } + }) } }