Prevent Service Principal password from being reset to "" for updates (#296)

* Prevent Service Principal password from being reset to "" for updates

This change fixes a bug that was caused when there was an update applied
to a service connection. The bug would result in the service principal
secret being reset to the empty string.

This change fixes the problem by catching this case and setting the
value to "null", which indicates to the AzDO services that the
password should not be updated.

Author: Nicholas M. Iodice

azuredevops/resource_serviceendpoint_azurerm.go

@@ -89,7 +89,7 @@ func expandServiceEndpointAzureRM(d *schema.ResourceData) (*serviceendpoint.Serv
 	if _, ok := d.GetOk("credentials"); ok {
 		credentials := d.Get("credentials").([]interface{})[0].(map[string]interface{})
 		(*serviceEndpoint.Authorization.Parameters)["serviceprincipalid"] = credentials["serviceprincipalid"].(string)
-		(*serviceEndpoint.Authorization.Parameters)["serviceprincipalkey"] = credentials["serviceprincipalkey"].(string)
+		(*serviceEndpoint.Authorization.Parameters)["serviceprincipalkey"] = expandSpnKey(credentials)
 		(*serviceEndpoint.Data)["creationMode"] = "Manual"
 	}
 
@@ -98,6 +98,22 @@ func expandServiceEndpointAzureRM(d *schema.ResourceData) (*serviceendpoint.Serv
 	return serviceEndpoint, projectID
 }
 
+func expandSpnKey(credentials map[string]interface{}) string {
+	// Note: if this is an update for a field other than `serviceprincipalkey`, the `serviceprincipalkey` will be
+	// set to `""`. Without catching this case and setting the value to `"null"`, the `serviceprincipalkey` will
+	// actually be set to `""` by the Azure DevOps service.
+	//
+	// This step is critical in order to ensure that the service connection can update without loosing its password!
+	//
+	// This behavior is unfortunately not documented in the API documentation.
+	spnKey, ok := credentials["serviceprincipalkey"]
+	if !ok || spnKey.(string) == "" {
+		return "null"
+	}
+
+	return spnKey.(string)
+}
+
 func flattenCredentials(serviceEndpoint *serviceendpoint.ServiceEndpoint, hashKey string, hashValue string) interface{} {
 	return []map[string]interface{}{{
 		"serviceprincipalid":  (*serviceEndpoint.Authorization.Parameters)["serviceprincipalid"],

azuredevops/resource_serviceendpoint_azurerm_test.go

@@ -26,19 +26,19 @@ var azurermTestServiceEndpointAzureRMID = uuid.New()
 var azurermRandomServiceEndpointAzureRMProjectID = uuid.New().String()
 var azurermTestServiceEndpointAzureRMProjectID = &azurermRandomServiceEndpointAzureRMProjectID
 
-var azurermTestServiceEndpointsAzureRM = []serviceendpoint.ServiceEndpoint{
-	{
+func getManualAuthServiceEndpoint() serviceendpoint.ServiceEndpoint {
+	return serviceendpoint.ServiceEndpoint{
 		Authorization: &serviceendpoint.EndpointAuthorization{
 			Parameters: &map[string]string{
 				"authenticationType":  "spnKey",
-				"serviceprincipalid":  "",
-				"serviceprincipalkey": "",
+				"serviceprincipalid":  "e31eaaac-47da-4156-b433-9b0538c94b7e", //fake value
+				"serviceprincipalkey": "d96d8515-20b2-4413-8879-27c5d040cbc2", //fake value
 				"tenantid":            "aba07645-051c-44b4-b806-c34d33f3dcd1", //fake value
 			},
 			Scheme: converter.String("ServicePrincipal"),
 		},
 		Data: &map[string]string{
-			"creationMode":     "Automatic",
+			"creationMode":     "Manual",
 			"environment":      "AzureCloud",
 			"scopeLevel":       "Subscription",
 			"subscriptionId":   "42125daf-72fd-417c-9ea7-080690625ad3", //fake value
@@ -50,19 +50,23 @@ var azurermTestServiceEndpointsAzureRM = []serviceendpoint.ServiceEndpoint{
 		Owner:       converter.String("library"), // Supported values are "library", "agentcloud"
 		Type:        converter.String("azurerm"),
 		Url:         converter.String("https://management.azure.com/"),
-	},
+	}
+}
+
+var azurermTestServiceEndpointsAzureRM = []serviceendpoint.ServiceEndpoint{
+	getManualAuthServiceEndpoint(),
 	{
 		Authorization: &serviceendpoint.EndpointAuthorization{
 			Parameters: &map[string]string{
 				"authenticationType":  "spnKey",
-				"serviceprincipalid":  "e31eaaac-47da-4156-b433-9b0538c94b7e", //fake value
-				"serviceprincipalkey": "d96d8515-20b2-4413-8879-27c5d040cbc2", //fake value
+				"serviceprincipalid":  "",
+				"serviceprincipalkey": "",
 				"tenantid":            "aba07645-051c-44b4-b806-c34d33f3dcd1", //fake value
 			},
 			Scheme: converter.String("ServicePrincipal"),
 		},
 		Data: &map[string]string{
-			"creationMode":     "Manual",
+			"creationMode":     "Automatic",
 			"environment":      "AzureCloud",
 			"scopeLevel":       "Subscription",
 			"subscriptionId":   "42125daf-72fd-417c-9ea7-080690625ad3", //fake value
@@ -224,6 +228,41 @@ func TestAzureDevOpsServiceEndpointAzureRM_Update_DoesNotSwallowError(t *testing
 	}
 }
 
+func TestAzureDevOpsServiceEndpointAzureRM_ExpandCredentials(t *testing.T) {
+	spnKeyExistsWithValue := map[string]interface{}{"serviceprincipalkey": "fake-spn-key"}
+	spnKeyExistsWithEmptyValue := map[string]interface{}{"serviceprincipalkey": ""}
+	spnKeyDoesNotExists := map[string]interface{}{}
+
+	require.Equal(t, expandSpnKey(spnKeyExistsWithValue), "fake-spn-key")
+	require.Equal(t, expandSpnKey(spnKeyExistsWithEmptyValue), "null")
+	require.Equal(t, expandSpnKey(spnKeyDoesNotExists), "null")
+}
+
+// This is a little different than most. The steps done, along with the motivation behind each, are as follows:
+//	(1) The service endpoint is configured. The `serviceprincipalkey` is set to `""`, which matches
+//		the Azure DevOps API behavior. The service will intentionally hide the value of
+//		`serviceprincipalkey` because it is a secret value
+//	(2) The resource is flattened/expanded
+//	(3) The `serviceprincipalkey` field is inspected and asserted to equal `"null"`. This special
+//		value, which is unfortunately not documented in the REST API, will be interpreted by the
+//		Azure DevOps API as an indicator to "not update" the field. The resulting behavior is that
+//		this Terraform Resource will be able to update the Service Endpoint without needing to
+//		pass the password along in each request.
+func TestAzureDevOpsServiceEndpointAzureRM_ExpandHandlesMissingSpnKeyInAPIResponse(t *testing.T) {
+	// step (1)
+	endpoint := getManualAuthServiceEndpoint()
+	resourceData := getResourceData(t, endpoint)
+	(*endpoint.Authorization.Parameters)["serviceprincipalkey"] = ""
+
+	// step (2)
+	flattenServiceEndpointAzureRM(resourceData, &endpoint, azurermTestServiceEndpointAzureRMProjectID)
+	expandedEndpoint, _ := expandServiceEndpointAzureRM(resourceData)
+
+	// step (3)
+	spnKeyProperty := (*expandedEndpoint.Authorization.Parameters)["serviceprincipalkey"]
+	require.Equal(t, "null", spnKeyProperty)
+}
+
 /**
  * Begin acceptance tests
  */