Merge pull request #211 from chuckha/more-test-cleanup

🐛 Test and logic fixes

Author: k8s-ci-robot

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
 	}

controllers/kubeadmconfig_controller_test.go

@@ -281,6 +281,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
@@ -340,6 +341,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
@@ -375,7 +378,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
@@ -551,7 +553,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,
@@ -564,13 +567,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
@@ -592,8 +595,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")
@@ -689,28 +692,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
@@ -726,24 +729,24 @@ 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")
 			}
 		})
 	}
 }
 
-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
@@ -816,39 +819,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)
@@ -859,34 +868,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)
+			}
+		})
 	}
 }