Skip to content

Commit

Permalink
Merge pull request kubernetes-sigs#211 from chuckha/more-test-cleanup
Browse files Browse the repository at this point in the history 
🐛 Test and logic fixes
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Sep 9, 2019
2 parents 06ae0a0 + 5d369d4 commit 08364d5
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 65 deletions.
16 changes: 10 additions & 6 deletions controllers/kubeadmconfig_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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
}
Expand Down
159 changes: 100 additions & 59 deletions controllers/kubeadmconfig_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
})
}
}

Expand Down

0 comments on commit 08364d5

Please sign in to comment.