Merge pull request #306 from twiest/add_clusterid_to_ssh_key_name

Avoid cloud ssh public key name collision

cmd/cluster-operator-apiserver/app/testing/server_test.go

@@ -54,6 +54,9 @@ func TestRun(t *testing.T) {
 					},
 				},
 			},
+			Hardware: clusteroperator.ClusterHardwareSpec{
+				AWS: &clusteroperator.AWSClusterSpec{},
+			},
 		},
 	})
 	if err != nil {

contrib/ansible/create-cluster-playbook.yml

@@ -31,9 +31,6 @@
     # SSH private key used for access to all VMs CO creates:
     ssh_priv_key: '~/.ssh/libra.pem'
 
-    # AWS keypair name to use for cluster machines
-    aws_ssh_keypair: "libra"
-
     # Force regeneration of cluster cert. Will happen regardless if one does not exist.
     redeploy_cluster_cert: False
 
@@ -136,7 +133,6 @@
         AWS_SECRET_ACCESS_KEY: "{{ l_aws_secret_access_key }}"
         SSH_PRIVATE_KEY: "{{ l_aws_ssh_private_key }}"
         SSH_PUBLIC_KEY: "{{ l_aws_ssh_public_key }}"
-        SSH_KEYPAIR_NAME: "{{ aws_ssh_keypair }}"
     register: cluster_reg
 
   - name: create/update cluster

contrib/examples/cluster-deployment-template.yaml

@@ -40,8 +40,7 @@ parameters:
   required: true
   description: Base64 encoded SSH public key that can be used to access the provisioned servers.
 - name: SSH_KEYPAIR_NAME
-  required: true
-  value: libra
+  required: false
   description: Name of AWS keypair to use for machines in this cluster.
 
 objects:

pkg/ansible/generate.go

@@ -222,6 +222,9 @@ all:
     openshift_aws_users:
     - key_name: [[ .SSHKeyName ]]
       pub_key: "{{ lookup('file', '/ansible/ssh/publickey.pub') }}"
+
+    # This will remove the .SSHKeyName keypair from AWS
+    openshift_aws_enable_uninstall_shared_objects: [[ .UninstallSSHKeyPair ]]
     
     # -- #
     # S3 #
@@ -323,6 +326,7 @@ type clusterParams struct {
 	Region                           string
 	SSHKeyName                       string
 	SSHUser                          string
+	UninstallSSHKeyPair              bool
 	VPCDefaults                      string
 	ELBMasterExternalName            string
 	ELBMasterInternalName            string
@@ -368,6 +372,7 @@ func GenerateClusterWideVars(
 		Region:                hardwareSpec.Region,
 		SSHKeyName:            hardwareSpec.KeyPairName,
 		SSHUser:               hardwareSpec.SSHUser,
+		UninstallSSHKeyPair:   hardwareSpec.KeyPairName == clusterID, // only uninstall the cloud ssh keypair when it's unique to the cluster
 		ELBMasterExternalName: controller.ELBMasterExternalName(clusterID),
 		ELBMasterInternalName: controller.ELBMasterInternalName(clusterID),
 		ELBInfraName:          controller.ELBInfraName(clusterID),

pkg/ansible/generate_test.go

@@ -29,12 +29,12 @@ const (
 	imageFormat = "openshift/origin-${component}:v3.10.0"
 )
 
-func testClusterSpec() *coapi.ClusterDeploymentSpec {
+func testClusterSpec(sshKeyPairName string) *coapi.ClusterDeploymentSpec {
 	return &coapi.ClusterDeploymentSpec{
 		Hardware: coapi.ClusterHardwareSpec{
 			AWS: &coapi.AWSClusterSpec{
 				Region:      "us-east-1",
-				KeyPairName: "mykey",
+				KeyPairName: sshKeyPairName,
 				SSHUser:     "centos",
 			},
 		},
@@ -67,7 +67,7 @@ func TestGenerateClusterWideVars(t *testing.T) {
 		{
 			name:           "cluster",
 			clusterID:      "testcluster",
-			clusterSpec:    testClusterSpec(),
+			clusterSpec:    testClusterSpec("testcluster"),
 			infraSize:      2,
 			clusterVersion: testClusterVersion(),
 			sdnPluginName:  "fakeplugin",
@@ -79,14 +79,15 @@ func TestGenerateClusterWideVars(t *testing.T) {
 				"openshift_aws_elb_master_external_name: testcluster-cp-ext",
 				"openshift_aws_elb_master_internal_name: testcluster-cp-int",
 				"openshift_aws_elb_infra_name: testcluster-infra",
-				"openshift_aws_ssh_key_name: mykey",
+				"openshift_aws_ssh_key_name: testcluster",
 				"openshift_aws_region: us-east-1",
 				"ansible_ssh_user: centos",
 				"openshift_deployment_type: origin",
 				"openshift_hosted_registry_replicas: 1",
 				"openshift_portal_net: 172.30.0.0/16",
 				"osm_cluster_network_cidr: 10.128.0.0/14",
 				"os_sdn_network_plugin_name: \"fakeplugin\"",
+				"openshift_aws_enable_uninstall_shared_objects: true",
 			},
 			shouldNotInclude: []string{
 				"openshift_release",
@@ -100,7 +101,7 @@ func TestGenerateClusterWideVars(t *testing.T) {
 		{
 			name:           "long clusterID",
 			clusterID:      "012345678901234567890123456789-abcde",
-			clusterSpec:    testClusterSpec(),
+			clusterSpec:    testClusterSpec("not_same_as_clusterid"),
 			infraSize:      2,
 			clusterVersion: testClusterVersion(),
 			serviceCIDRs:   capiv1.NetworkRanges{CIDRBlocks: []string{"172.30.0.0/16"}},
@@ -110,6 +111,8 @@ func TestGenerateClusterWideVars(t *testing.T) {
 				"openshift_aws_elb_master_external_name: 0123456789012345678-abcde-cp-ext",
 				"openshift_aws_elb_master_internal_name: 0123456789012345678-abcde-cp-int",
 				"openshift_aws_elb_infra_name: 0123456789012345678-abcde-infra",
+				"openshift_aws_ssh_key_name: not_same_as_clusterid",
+				"openshift_aws_enable_uninstall_shared_objects: false",
 			},
 		},
 	}

pkg/registry/clusteroperator/clusterdeployment/storage_test.go

@@ -71,6 +71,9 @@ func validNewClusterDeployment(name string) *clusteroperatorapi.ClusterDeploymen
 				Pods:          capiv1.NetworkRanges{CIDRBlocks: []string{"10.128.0.0/14"}},
 				ServiceDomain: "svc.cluster.local",
 			},
+			Hardware: clusteroperatorapi.ClusterHardwareSpec{
+				AWS: &clusteroperatorapi.AWSClusterSpec{},
+			},
 		},
 	}
 }
@@ -92,6 +95,11 @@ func TestCreate(t *testing.T) {
 		// invalid
 		&clusteroperatorapi.ClusterDeployment{
 			ObjectMeta: metav1.ObjectMeta{Name: "*BadName!"},
+			Spec: clusteroperatorapi.ClusterDeploymentSpec{
+				Hardware: clusteroperatorapi.ClusterHardwareSpec{
+					AWS: &clusteroperatorapi.AWSClusterSpec{},
+				},
+			},
 		},
 	)
 }

pkg/registry/clusteroperator/clusterdeployment/strategy.go

@@ -91,6 +91,11 @@ func (clusterDeploymentRESTStrategy) PrepareForCreate(ctx genericapirequest.Cont
 
 	clusterDeployment.Spec.ClusterName = clusterDeployment.Name + "-" + utilrand.String(5)
 
+	if clusterDeployment.Spec.Hardware.AWS.KeyPairName == "" {
+		// This is how the KeyPairName is generated.
+		clusterDeployment.Spec.Hardware.AWS.KeyPairName = clusterDeployment.Spec.ClusterName
+	}
+
 	// Creating a brand new object, thus it must have no
 	// status. We can't fail here if they passed a status in, so
 	// we just wipe it clean.

pkg/registry/clusteroperator/clusterdeployment/strategy_test.go

@@ -19,6 +19,8 @@ package clusterdeployment
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	clusteroperator "github.com/openshift/cluster-operator/pkg/apis/clusteroperator"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -50,13 +52,58 @@ func TestClusterDeploymentStrategyTrivial(t *testing.T) {
 	}
 }
 
-// TestClusterDeploymentCreate
-func TestClusterDeploymentCreate(t *testing.T) {
-	// Create a clusterdeployment or clusterdeployments
-	clusterDeployment := &clusteroperator.ClusterDeployment{}
+// TestClusterDeploymentCreateWithSshKeyName
+func TestClusterDeploymentCreateWithSshKeyName(t *testing.T) {
+	cases := []struct {
+		name                          string
+		clusterDeploymentName         string
+		keyPairName                   string
+		keyPairNameGenerationExpected bool
+	}{
+		{
+			name: "specify ssh keypair name",
+			clusterDeploymentName:         "testcluster",
+			keyPairName:                   "TEST KEYPAIR NAME",
+			keyPairNameGenerationExpected: false,
+		},
+		{
+			name: "generate ssh keypair name",
+			clusterDeploymentName:         "testcluster",
+			keyPairNameGenerationExpected: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Arrange
+			// Create a clusterdeployment or clusterdeployments
+			clusterDeployment := &clusteroperator.ClusterDeployment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: tc.clusterDeploymentName,
+				},
+				Spec: clusteroperator.ClusterDeploymentSpec{
+					Hardware: clusteroperator.ClusterHardwareSpec{
+						AWS: &clusteroperator.AWSClusterSpec{
+							KeyPairName: tc.keyPairName,
+						},
+					},
+				},
+			}
+
+			// Act
+			// Canonicalize the cluster
+			clusterDeploymentRESTStrategies.PrepareForCreate(nil, clusterDeployment)
 
-	// Canonicalize the cluster
-	clusterDeploymentRESTStrategies.PrepareForCreate(nil, clusterDeployment)
+			// Assert
+			assert.Regexp(t, "^testcluster-\\w\\w\\w\\w\\w$", clusterDeployment.Spec.ClusterName)
+
+			if tc.keyPairNameGenerationExpected {
+				assert.Equal(t, clusterDeployment.Spec.ClusterName, clusterDeployment.Spec.Hardware.AWS.KeyPairName)
+			} else {
+				assert.Equal(t, tc.keyPairName, clusterDeployment.Spec.Hardware.AWS.KeyPairName)
+			}
+		})
+	}
 }
 
 // TestClusterDeploymentUpdate tests that generation is incremented correctly when the