fix(deploy): fix SP cred handling in deploy (#1052)

Author: colemickens

cmd/deploy.go

@@ -180,37 +180,40 @@ func autofillApimodel(dc *deployCmd) {
 
 	if !useManagedIdentity {
 		spp := dc.containerService.Properties.ServicePrincipalProfile
-		if spp != nil && !(spp.ClientID == "" && spp.Secret == "") {
-			log.Fatal("apimodel invalid: ServicePrincipalProfile is missing either the clientid or secret.")
-		}
-
-		log.Warnln("apimodel: ServicePrincipalProfile was empty, creating application...")
+		if spp != nil && (spp.ClientID == "" || spp.Secret == "") {
+			// they didn't specify one or the other
+			if spp != nil && !(spp.ClientID == "" && spp.Secret == "") {
+				// they didn't specify just one
+				log.Fatal("apimodel invalid: ServicePrincipalProfile is missing either the clientid or secret.")
+			}
 
-		// TODO: consider caching the creds here so they persist between subsequent runs of 'deploy'
-		appName := dc.containerService.Properties.MasterProfile.DNSPrefix
-		appURL := fmt.Sprintf("https://%s/", appName)
-		applicationID, servicePrincipalObjectID, secret, err := dc.client.CreateApp(appName, appURL)
-		if err != nil {
-			log.Fatalf("apimodel invalid: ServicePrincipalProfile was empty, and we failed to create valid credentials: %q", err)
-		}
-		log.Warnf("created application with applicationID (%s) and servicePrincipalObjectID (%s).", applicationID, servicePrincipalObjectID)
+			log.Warnln("apimodel: ServicePrincipalProfile was missing or empty, creating application...")
 
-		log.Warnln("apimodel: ServicePrincipalProfile was empty, assigning role to application...")
-		for {
-			err = dc.client.CreateRoleAssignmentSimple(dc.resourceGroup, servicePrincipalObjectID)
+			// TODO: consider caching the creds here so they persist between subsequent runs of 'deploy'
+			appName := dc.containerService.Properties.MasterProfile.DNSPrefix
+			appURL := fmt.Sprintf("https://%s/", appName)
+			applicationID, servicePrincipalObjectID, secret, err := dc.client.CreateApp(appName, appURL)
 			if err != nil {
-				log.Warnf("Failed to create role assignment (will retry): %q", err)
-				time.Sleep(3 * time.Second)
-				continue
+				log.Fatalf("apimodel invalid: ServicePrincipalProfile was empty, and we failed to create valid credentials: %q", err)
+			}
+			log.Warnf("created application with applicationID (%s) and servicePrincipalObjectID (%s).", applicationID, servicePrincipalObjectID)
+
+			log.Warnln("apimodel: ServicePrincipalProfile was empty, assigning role to application...")
+			for {
+				err = dc.client.CreateRoleAssignmentSimple(dc.resourceGroup, servicePrincipalObjectID)
+				if err != nil {
+					log.Warnf("Failed to create role assignment (will retry): %q", err)
+					time.Sleep(3 * time.Second)
+					continue
+				}
+				break
 			}
-			break
-		}
 
-		dc.containerService.Properties.ServicePrincipalProfile = &api.ServicePrincipalProfile{
-			ClientID: applicationID,
-			Secret:   secret,
+			dc.containerService.Properties.ServicePrincipalProfile = &api.ServicePrincipalProfile{
+				ClientID: applicationID,
+				Secret:   secret,
+			}
 		}
-
 	}
 }
 

cmd/deploy_test.go

@@ -21,25 +21,33 @@ const ExampleAPIModel = `{
     "windowsProfile": { "adminUsername": "azureuser", "adminPassword": "replacepassword1234$" },
     "linuxProfile": { "adminUsername": "azureuser", "ssh": { "publicKeys": [ { "keyData": "" } ] }
     },
-    "servicePrincipalProfile": { "servicePrincipalClientID": "", "servicePrincipalClientSecret": "" }
+    "servicePrincipalProfile": { "servicePrincipalClientID": "%s", "servicePrincipalClientSecret": "%s" }
   }
 }
 `
 
-func getExampleAPIModel(useManagedIdentity bool) string {
-	return fmt.Sprintf(ExampleAPIModel, strconv.FormatBool(useManagedIdentity))
+func getExampleAPIModel(useManagedIdentity bool, clientID, clientSecret string) string {
+	return fmt.Sprintf(
+		ExampleAPIModel,
+		strconv.FormatBool(useManagedIdentity),
+		clientID,
+		clientSecret)
 }
 
 func TestAutofillApimodelWithoutManagedIdentityCreatesCreds(t *testing.T) {
-	testMSIPopulatedInternal(t, false)
+	testAutodeployCredentialHandling(t, false, "", "")
 }
 
 func TestAutofillApimodelWithManagedIdentitySkipsCreds(t *testing.T) {
-	testMSIPopulatedInternal(t, true)
+	testAutodeployCredentialHandling(t, true, "", "")
 }
 
-func testMSIPopulatedInternal(t *testing.T, useManagedIdentity bool) {
-	apimodel := getExampleAPIModel(useManagedIdentity)
+func TestAutofillApimodelAllowsPrespecifiedCreds(t *testing.T) {
+	testAutodeployCredentialHandling(t, false, "clientID", "clientSecret")
+}
+
+func testAutodeployCredentialHandling(t *testing.T, useManagedIdentity bool, clientID, clientSecret string) {
+	apimodel := getExampleAPIModel(useManagedIdentity, clientID, clientSecret)
 	cs, ver, err := api.DeserializeContainerService([]byte(apimodel), false)
 	if err != nil {
 		t.Fatalf("unexpected error deserializing the example apimodel: %s", err)
@@ -51,7 +59,7 @@ func testMSIPopulatedInternal(t *testing.T, useManagedIdentity bool) {
 	deployCmd := &deployCmd{
 		apimodelPath:    "./this/is/unused.json",
 		dnsPrefix:       "dnsPrefix1",
-		outputDirectory: "dummy/path/",
+		outputDirectory: "_test_output",
 		location:        "westus",
 
 		containerService: cs,
@@ -62,6 +70,9 @@ func testMSIPopulatedInternal(t *testing.T, useManagedIdentity bool) {
 
 	autofillApimodel(deployCmd)
 
+	// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
+	defer os.RemoveAll(deployCmd.outputDirectory)
+
 	cs, ver, err = revalidateApimodel(cs, ver)
 	if err != nil {
 		log.Fatalf("unexpected error validating apimodel after populating defaults: %s", err)
@@ -78,7 +89,4 @@ func testMSIPopulatedInternal(t *testing.T, useManagedIdentity bool) {
 			log.Fatalf("Credentials were missing even though MSI was not active.")
 		}
 	}
-
-	// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
-	os.RemoveAll(deployCmd.outputDirectory)
 }