Fixes a bug and adds tests for kubeadm defaults

pkg/cloud/aws/actuators/machine_scope.go

@@ -97,6 +97,16 @@ func (m *MachineScope) Region() string {
 	return m.Scope.Region()
 }
 
+// GetMachine returns the machine wrapped in the scope.
+func (m *MachineScope) GetMachine() *clusterv1.Machine {
+	return m.Machine
+}
+
+// GetScope() returns the scope that is wrapping the machine.
+func (m *MachineScope) GetScope() *Scope {
+	return m.Scope
+}
+
 func (m *MachineScope) storeMachineSpec(machine *clusterv1.Machine) (*clusterv1.Machine, error) {
 	ext, err := v1alpha1.EncodeMachineSpec(m.MachineConfig)
 	if err != nil {

pkg/cloud/aws/services/ec2/instances.go

@@ -171,7 +171,7 @@ func (s *Service) createInstance(machine *actuators.MachineScope, bootstrapToken
 		if bootstrapToken != "" {
 			klog.V(2).Infof("Allowing machine %q to join control plane for cluster %q", machine.Name(), s.scope.Name())
 
-			kubeadm.SetJoinNodeConfigurationOverrides(caCertHash, bootstrapToken, machine, &machine.MachineConfig.KubeadmConfiguration.Join)
+			machine.MachineConfig.KubeadmConfiguration.Join = kubeadm.SetJoinNodeConfigurationOverrides(caCertHash, bootstrapToken, machine, &machine.MachineConfig.KubeadmConfiguration.Join)
 			kubeadm.SetControlPlaneJoinConfigurationOverrides(&machine.MachineConfig.KubeadmConfiguration.Join)
 			joinConfigurationYAML, err := kubeadm.ConfigurationToYAML(&machine.MachineConfig.KubeadmConfiguration.Join)
 			if err != nil {

pkg/cloud/aws/services/kubeadm/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -16,7 +16,20 @@ go_library(
         "//vendor/k8s.io/klog:go_default_library",
         "//vendor/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1:go_default_library",
         "//vendor/k8s.io/kubernetes/cmd/kubeadm/app/util:go_default_library",
+        "//vendor/sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1:go_default_library",
         "//vendor/sigs.k8s.io/cluster-api/pkg/util:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/runtime/scheme:go_default_library",
     ],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["aws_defaults_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//pkg/apis/awsprovider/v1alpha1:go_default_library",
+        "//pkg/cloud/aws/actuators:go_default_library",
+        "//vendor/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1:go_default_library",
+        "//vendor/sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1:go_default_library",
+    ],
+)

pkg/cloud/aws/services/kubeadm/aws_defaults.go

@@ -19,6 +19,7 @@ package kubeadm
 import (
 	"fmt"
 
+	"sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	"sigs.k8s.io/cluster-api/pkg/util"
 
 	"k8s.io/klog"
@@ -31,16 +32,18 @@ const (
 	// See https://cloudinit.readthedocs.io/en/latest/topics/instancedata.html
 	localIPV4Lookup = "{{ ds.meta_data.local_ipv4 }}"
 
-	// hostname lookup uses the instance metadata service to lookup its own hostname.
-	hostnameLookup = "{{ ds.meta_data.hostname }}"
+	// HostnameLookup uses the instance metadata service to lookup its own hostname.
+	HostnameLookup = "{{ ds.meta_data.hostname }}"
 
-	// containerdSocket is the expected path to containerd socket.
-	containerdSocket = "/var/run/containerd/containerd.sock"
+	// ContainerdSocket is the expected path to containerd socket.
+	ContainerdSocket = "/var/run/containerd/containerd.sock"
 
-	// apiServerBindPort is the default port for the kube-apiserver to bind to.
-	apiServerBindPort = 6443
+	// APIServerBindPort is the default port for the kube-apiserver to bind to.
+	APIServerBindPort = 6443
 
-	cloudProvider = "aws"
+	// CloudProvider is the name of the cloud provider passed to various
+	// kubernetes components.
+	CloudProvider = "aws"
 
 	nodeRole = "node-role.kubernetes.io/node="
 )
@@ -55,7 +58,7 @@ func SetDefaultClusterConfiguration(machine *actuators.MachineScope, base *kubea
 
 	// Only set the control plane endpoint if the user hasn't specified one.
 	if base.ControlPlaneEndpoint == "" {
-		base.ControlPlaneEndpoint = fmt.Sprintf("%s:%d", s.Network().APIServerELB.DNSName, apiServerBindPort)
+		base.ControlPlaneEndpoint = fmt.Sprintf("%s:%d", s.Network().APIServerELB.DNSName, APIServerBindPort)
 	}
 	// Add the control plane endpoint to the list of cert SAN
 	base.APIServer.CertSANs = append(base.APIServer.CertSANs, localIPV4Lookup, s.Network().APIServerELB.DNSName)
@@ -75,18 +78,18 @@ func SetClusterConfigurationOverrides(machine *actuators.MachineScope, base *kub
 	if base.APIServer.ControlPlaneComponent.ExtraArgs == nil {
 		base.APIServer.ControlPlaneComponent.ExtraArgs = map[string]string{}
 	}
-	if cp, ok := base.APIServer.ControlPlaneComponent.ExtraArgs["cloud-provider"]; ok && cp != cloudProvider {
-		klog.Infof("Overriding cloud provider %q with required value %q", cp, cloudProvider)
+	if cp, ok := base.APIServer.ControlPlaneComponent.ExtraArgs["cloud-provider"]; ok && cp != CloudProvider {
+		klog.Infof("Overriding cloud provider %q with required value %q", cp, CloudProvider)
 	}
-	base.APIServer.ControlPlaneComponent.ExtraArgs["cloud-provider"] = cloudProvider
+	base.APIServer.ControlPlaneComponent.ExtraArgs["cloud-provider"] = CloudProvider
 
 	if base.ControllerManager.ExtraArgs == nil {
 		base.ControllerManager.ExtraArgs = map[string]string{}
 	}
-	if cp, ok := base.ControllerManager.ExtraArgs["cloud-provider"]; ok && cp != cloudProvider {
-		klog.Infof("Overriding cloud provider %q with required value %q", cp, cloudProvider)
+	if cp, ok := base.ControllerManager.ExtraArgs["cloud-provider"]; ok && cp != CloudProvider {
+		klog.Infof("Overriding cloud provider %q with required value %q", cp, CloudProvider)
 	}
-	base.ControllerManager.ExtraArgs["cloud-provider"] = cloudProvider
+	base.ControllerManager.ExtraArgs["cloud-provider"] = CloudProvider
 
 	// The kubeadm config clustername must match the provided name of the cluster.
 	if base.ClusterName != "" && base.ClusterName != s.Name() {
@@ -112,63 +115,70 @@ func SetInitConfigurationOverrides(base *kubeadmv1beta1.InitConfiguration) {
 		base = &kubeadmv1beta1.InitConfiguration{}
 	}
 
-	if base.NodeRegistration.Name != "" && base.NodeRegistration.Name != hostnameLookup {
-		klog.Infof("Overriding NodeRegistration name from %q to %q. The node registration needs to be dynamically generated in aws.", base.NodeRegistration.Name, hostnameLookup)
+	if base.NodeRegistration.Name != "" && base.NodeRegistration.Name != HostnameLookup {
+		klog.Infof("Overriding NodeRegistration name from %q to %q. The node registration needs to be dynamically generated in aws.", base.NodeRegistration.Name, HostnameLookup)
 	}
-	base.NodeRegistration.Name = hostnameLookup
+	base.NodeRegistration.Name = HostnameLookup
 
 	// TODO(chuckha): This may become a default instead of an override.
-	if base.NodeRegistration.CRISocket != "" && base.NodeRegistration.CRISocket != containerdSocket {
-		klog.Infof("Overriding CRISocket from %q to %q. Containerd is only supported container runtime.", base.NodeRegistration.CRISocket, containerdSocket)
+	if base.NodeRegistration.CRISocket != "" && base.NodeRegistration.CRISocket != ContainerdSocket {
+		klog.Infof("Overriding CRISocket from %q to %q. Containerd is only supported container runtime.", base.NodeRegistration.CRISocket, ContainerdSocket)
 	}
-	base.NodeRegistration.CRISocket = containerdSocket
+	base.NodeRegistration.CRISocket = ContainerdSocket
 
 	if base.NodeRegistration.KubeletExtraArgs == nil {
 		base.NodeRegistration.KubeletExtraArgs = map[string]string{}
 	}
-	if cp, ok := base.NodeRegistration.KubeletExtraArgs["cloud-provider"]; ok && cp != cloudProvider {
-		klog.Infof("Overriding node's cloud-provider to the required value of %q.", cloudProvider)
+	if cp, ok := base.NodeRegistration.KubeletExtraArgs["cloud-provider"]; ok && cp != CloudProvider {
+		klog.Infof("Overriding node's cloud-provider to the required value of %q.", CloudProvider)
 	}
-	base.NodeRegistration.KubeletExtraArgs["cloud-provider"] = cloudProvider
+	base.NodeRegistration.KubeletExtraArgs["cloud-provider"] = CloudProvider
+}
+
+// joinMachine is a local interface scoping down exactly what SetJoinNodeConfigurationOverrides needs
+type joinMachine interface {
+	GetScope() *actuators.Scope
+	GetMachine() *v1alpha1.Machine
 }
 
 // SetJoinNodeConfigurationOverrides overrides user input for certain fields of
 // the kubeadm JoinConfiguration during a worker node join.
-func SetJoinNodeConfigurationOverrides(caCertHash, bootstrapToken string, machine *actuators.MachineScope, base *kubeadmv1beta1.JoinConfiguration) {
+func SetJoinNodeConfigurationOverrides(caCertHash, bootstrapToken string, machine joinMachine, base *kubeadmv1beta1.JoinConfiguration) kubeadmv1beta1.JoinConfiguration {
 	if base == nil {
 		base = &kubeadmv1beta1.JoinConfiguration{}
 	}
-	s := machine.Scope
+	out := base.DeepCopy()
 
-	if base.Discovery.BootstrapToken == nil {
-		base.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{}
+	if out.Discovery.BootstrapToken == nil {
+		out.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{}
 	}
 	// TODO: should this actually be the cluster's ContolPlaneEndpoint?
-	base.Discovery.BootstrapToken.APIServerEndpoint = fmt.Sprintf("%s:%d", s.Network().APIServerELB.DNSName, apiServerBindPort)
-	base.Discovery.BootstrapToken.Token = bootstrapToken
-	base.Discovery.BootstrapToken.CACertHashes = append(base.Discovery.BootstrapToken.CACertHashes, caCertHash)
+	out.Discovery.BootstrapToken.APIServerEndpoint = fmt.Sprintf("%s:%d", machine.GetScope().Network().APIServerELB.DNSName, APIServerBindPort)
+	out.Discovery.BootstrapToken.Token = bootstrapToken
+	out.Discovery.BootstrapToken.CACertHashes = append(out.Discovery.BootstrapToken.CACertHashes, caCertHash)
 
-	if base.NodeRegistration.Name != "" && base.NodeRegistration.Name != hostnameLookup {
-		klog.Infof("Overriding NodeRegistration name from %q to %q. The node registration needs to be dynamically generated in aws.", base.NodeRegistration.Name, hostnameLookup)
+	if out.NodeRegistration.Name != "" && out.NodeRegistration.Name != HostnameLookup {
+		klog.Infof("Overriding NodeRegistration name from %q to %q. The node registration needs to be dynamically generated in aws.", out.NodeRegistration.Name, HostnameLookup)
 	}
-	base.NodeRegistration.Name = hostnameLookup
+	out.NodeRegistration.Name = HostnameLookup
 
 	// TODO(chuckha): This may become a default instead of an override.
-	if base.NodeRegistration.CRISocket != "" && base.NodeRegistration.CRISocket != containerdSocket {
-		klog.Infof("Overriding CRISocket from %q to %q. Containerd is only supported container runtime.", base.NodeRegistration.CRISocket, containerdSocket)
+	if out.NodeRegistration.CRISocket != "" && out.NodeRegistration.CRISocket != ContainerdSocket {
+		klog.Infof("Overriding CRISocket from %q to %q. Containerd is only supported container runtime.", out.NodeRegistration.CRISocket, ContainerdSocket)
 	}
-	base.NodeRegistration.CRISocket = containerdSocket
+	out.NodeRegistration.CRISocket = ContainerdSocket
 
-	if base.NodeRegistration.KubeletExtraArgs == nil {
-		base.NodeRegistration.KubeletExtraArgs = map[string]string{}
+	if out.NodeRegistration.KubeletExtraArgs == nil {
+		out.NodeRegistration.KubeletExtraArgs = map[string]string{}
 	}
-	if cp, ok := base.NodeRegistration.KubeletExtraArgs["cloud-provider"]; ok && cp != cloudProvider {
-		klog.Infof("Overriding node's cloud-provider to the required value of %q.", cloudProvider)
+	if cp, ok := out.NodeRegistration.KubeletExtraArgs["cloud-provider"]; ok && cp != CloudProvider {
+		klog.Infof("Overriding node's cloud-provider to the required value of %q.", CloudProvider)
 	}
-	base.NodeRegistration.KubeletExtraArgs["cloud-provider"] = cloudProvider
-	if !util.IsControlPlaneMachine(machine.Machine) {
-		base.NodeRegistration.KubeletExtraArgs["node-labels"] = nodeRole
+	out.NodeRegistration.KubeletExtraArgs["cloud-provider"] = CloudProvider
+	if !util.IsControlPlaneMachine(machine.GetMachine()) {
+		out.NodeRegistration.KubeletExtraArgs["node-labels"] = nodeRole
 	}
+	return *out
 }
 
 // SetControlPlaneJoinConfigurationOverrides user input for kubeadm join
@@ -181,5 +191,5 @@ func SetControlPlaneJoinConfigurationOverrides(base *kubeadmv1beta1.JoinConfigur
 		base.ControlPlane = &kubeadmv1beta1.JoinControlPlane{}
 	}
 	base.ControlPlane.LocalAPIEndpoint.AdvertiseAddress = localIPV4Lookup
-	base.ControlPlane.LocalAPIEndpoint.BindPort = apiServerBindPort
+	base.ControlPlane.LocalAPIEndpoint.BindPort = APIServerBindPort
 }

pkg/cloud/aws/services/kubeadm/aws_defaults_test.go

@@ -0,0 +1,163 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubeadm_test
+
+import (
+	"fmt"
+	"testing"
+
+	kubeadmv1beta1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsprovider/v1alpha1"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/actuators"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/services/kubeadm"
+	clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
+)
+
+type jm struct {
+	*actuators.Scope
+	*clusterv1.Machine
+}
+
+func (j *jm) GetScope() *actuators.Scope {
+	return j.Scope
+}
+func (j *jm) GetMachine() *clusterv1.Machine {
+	return j.Machine
+}
+
+func TestSetJoinNodeConfigurationBootstrapSettings(t *testing.T) {
+	testcases := []struct {
+		name              string
+		caCertHash        string
+		bootstrapToken    string
+		joinMachine       *jm
+		joinConfiguration *kubeadmv1beta1.JoinConfiguration
+	}{
+		{
+			name: "test default values",
+			joinMachine: &jm{
+				Scope: &actuators.Scope{
+					ClusterStatus: &v1alpha1.AWSClusterProviderStatus{
+						Network: v1alpha1.Network{
+							APIServerELB: v1alpha1.ClassicELB{
+								DNSName: "test.test",
+							},
+						},
+					},
+				},
+				Machine: &clusterv1.Machine{},
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			out := kubeadm.SetJoinNodeConfigurationOverrides(tc.caCertHash, tc.bootstrapToken, tc.joinMachine, tc.joinConfiguration)
+
+			// The apiserver endpoint should always match the incoming machine's network
+			expected := fmt.Sprintf("%v:%v", tc.joinMachine.GetScope().Network().APIServerELB.DNSName, kubeadm.APIServerBindPort)
+			if out.Discovery.BootstrapToken.APIServerEndpoint != expected {
+				t.Fatalf("join configuration apiserver endpoint: %q but expected %q", out.Discovery.BootstrapToken.APIServerEndpoint, expected)
+			}
+
+			// The bootstrap token on the new join node configuration should be
+			// the same that is passed in
+			if out.Discovery.BootstrapToken.Token != tc.bootstrapToken {
+				t.Fatalf("bootstrap tokens did  not match: got %q but expected %q", out.Discovery.BootstrapToken.Token, tc.bootstrapToken)
+			}
+
+			// The passed in caCertHash should be appended to the existing hashes
+			if !contains(out.Discovery.BootstrapToken.CACertHashes, tc.caCertHash) {
+				t.Fatalf("did not find %q in %v", tc.caCertHash, out.Discovery.BootstrapToken.CACertHashes)
+			}
+		})
+	}
+}
+
+func TestSetJoinNodeConfigurationOverrides(t *testing.T) {
+	testcases := []struct {
+		name              string
+		caCertHash        string
+		bootstrapToken    string
+		joinMachine       *jm
+		joinConfiguration *kubeadmv1beta1.JoinConfiguration
+	}{
+		{
+			name: "test node registration override values",
+			joinMachine: &jm{
+				Scope: &actuators.Scope{
+					ClusterStatus: &v1alpha1.AWSClusterProviderStatus{
+						Network: v1alpha1.Network{
+							APIServerELB: v1alpha1.ClassicELB{
+								DNSName: "test.test",
+							},
+						},
+					},
+				},
+				Machine: &clusterv1.Machine{},
+			},
+			joinConfiguration: &kubeadmv1beta1.JoinConfiguration{
+				NodeRegistration: kubeadmv1beta1.NodeRegistrationOptions{
+					Name:      "cat",
+					CRISocket: "unix:///var/run/docker/docker.sock",
+					KubeletExtraArgs: map[string]string{
+						"cloud-provider": "gcp",
+					},
+				},
+			},
+		},
+		{
+			name: "should set the cloud-provider extra args even if args are nil",
+			joinMachine: &jm{
+				Scope: &actuators.Scope{
+					ClusterStatus: &v1alpha1.AWSClusterProviderStatus{},
+				},
+				Machine: &clusterv1.Machine{},
+			},
+			joinConfiguration: &kubeadmv1beta1.JoinConfiguration{},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			out := kubeadm.SetJoinNodeConfigurationOverrides(tc.caCertHash, tc.bootstrapToken, tc.joinMachine, tc.joinConfiguration)
+
+			if tc.joinConfiguration.NodeRegistration.Name != kubeadm.HostnameLookup &&
+				tc.joinConfiguration.NodeRegistration.Name == out.NodeRegistration.Name {
+				t.Fatal("did not properly override the NodeRegistration.Name")
+			}
+
+			if tc.joinConfiguration.NodeRegistration.CRISocket != kubeadm.ContainerdSocket &&
+				tc.joinConfiguration.NodeRegistration.CRISocket == out.NodeRegistration.CRISocket {
+				t.Fatal("did not properly override the CRISocket")
+			}
+
+			if out.NodeRegistration.KubeletExtraArgs["cloud-provider"] != kubeadm.CloudProvider {
+				t.Fatal("did not properly set the cloud-provider on the kubelet extra args")
+			}
+		})
+	}
+}
+
+func contains(haystack []string, needle string) bool {
+	for _, hay := range haystack {
+		if needle == hay {
+			return true
+		}
+	}
+	return false
+}