Skip to content
This repository was archived by the owner on Jul 30, 2021. It is now read-only.

🐛 Test and logic fixes #211

Merged
merged 1 commit into from
Sep 9, 2019
Merged

🐛 Test and logic fixes #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions controllers/kubeadmconfig_controller.go
View file
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this piece of code should be moved into reconcileDiscovery , so all the logics impaction JoinConfiguration.Discovery are in the same place.
Nb. the bit of code for creating BootstrapToken if nil is already implemented there as well

Copy link
Contributor Author

@chuckha chuckha Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense to add this check to reconcileDiscovery unless you are also suggesting to add certificates into the reconcileDiscovery method. We look up certificates to see if we have a CACertHash to use. If we do then reconcileDiscovery does something different than if it didn't have a CACertHash. Therefore reconcileDiscovery depends on looking up certificates. Either certificates becomes part of reconcileDiscovery or we will have to live with not all defaults ending up in reconcileDiscovery

If we want to refactor it that's fine but I think that's out of scope for this PR.

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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand All @@ -725,24 +728,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 @@ -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)
Expand All @@ -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)
}
})
}
}

Expand Down