vault_azure_access_credentials: fix validate_creds bug (hashicorp#2086)

* vault_azure_access_credentials: fix validate_creds bug

* add changelog

* rename var

Author: fairclothjm

CHANGELOG.md

@@ -1,8 +1,8 @@
 ## Unreleased
 
 BUGS:
-
 * Fix `vault_identity_group` loses externally managed policies on updates when `external_policies = true` ([#2084](https://github.com/hashicorp/terraform-provider-vault/pull/2084))
+* Fix regression in `vault_azure_access_credentials` where we returned prematurely on 401 responses:([#2086](https://github.com/hashicorp/terraform-provider-vault/pull/2086))
 
 ## 3.22.0 (Nov 1, 2023)
 

testutil/testutil.go

@@ -127,7 +127,7 @@ func GetTestAWSRegion(t *testing.T) string {
 }
 
 type AzureTestConf struct {
-	SubscriptionID, TenantID, ClientID, ClientSecret, Scope string
+	SubscriptionID, TenantID, ClientID, ClientSecret, Scope, AppObjectID string
 }
 
 func GetTestAzureConf(t *testing.T) *AzureTestConf {
@@ -147,6 +147,23 @@ func GetTestAzureConf(t *testing.T) *AzureTestConf {
 	}
 }
 
+func GetTestAzureConfExistingSP(t *testing.T) *AzureTestConf {
+	v := SkipTestEnvUnset(t,
+		"AZURE_SUBSCRIPTION_ID",
+		"AZURE_TENANT_ID",
+		"AZURE_CLIENT_ID",
+		"AZURE_CLIENT_SECRET",
+		"AZURE_APPLICATION_OBJECT_ID")
+
+	return &AzureTestConf{
+		SubscriptionID: v[0],
+		TenantID:       v[1],
+		ClientID:       v[2],
+		ClientSecret:   v[3],
+		AppObjectID:    v[4],
+	}
+}
+
 func GetTestGCPCreds(t *testing.T) (string, string) {
 	t.Helper()
 

vault/data_source_azure_access_credentials.go

@@ -238,49 +238,52 @@ func azureAccessCredentialsDataSourceRead(ctx context.Context, d *schema.Resourc
 		return diag.Errorf("failed to create providers client: %s", err)
 	}
 	delay := time.Duration(d.Get("num_seconds_between_tests").(int)) * time.Second
-	endTime := time.Now().Add(
-		time.Duration(d.Get("max_cred_validation_seconds").(int)) * time.Second)
+	maxValidationSeconds := d.Get("max_cred_validation_seconds").(int)
+	endTime := time.Now().Add(time.Duration(maxValidationSeconds) * time.Second)
 	wantSuccessCount := d.Get("num_sequential_successes").(int)
 	var successCount int
+	// begin validate_creds retry loop
 	for {
 		pager := providerClient.NewListPager(&armresources.ProvidersClientListOptions{
 			Expand: pointerutil.StringPtr("metadata"),
 		})
 
-		var providers []*armresources.Provider
+		hasError := false
 		for pager.More() {
 			// capture raw response so we can get the status code
 			var rawResponse *http.Response
 			ctxWithResp := runtime.WithCaptureResponse(ctx, &rawResponse)
 
-			nextResult, err := pager.NextPage(ctxWithResp)
+			_, err := pager.NextPage(ctxWithResp)
 			if err != nil {
+				hasError = true
 				log.Printf("[WARN] Provider Client List request failed err=%s", err)
-			}
-			if rawResponse.StatusCode == http.StatusUnauthorized {
-				return diag.Errorf("validation failed, unauthorized credentials from Vault, err=%s", err)
+				// ensure we don't loop forever
+				break
 			}
 
+			// log the response status code and headers
 			log.Printf("[DEBUG] Provider Client List response %+v", rawResponse)
-
-			if nextResult.Value != nil {
-				providers = append(providers, nextResult.Value...)
-			}
 		}
 
-		if len(providers) != 0 {
+		if !hasError {
 			successCount++
 			log.Printf("[DEBUG] Credential validation succeeded on try %d/%d", successCount, wantSuccessCount)
 			if successCount >= wantSuccessCount {
 				break
 			}
 		} else {
-			log.Printf("[WARN] Credential validation failed with %s, retrying in %s", err, delay)
+			log.Printf("[WARN] Credential validation failed, retrying in %s", delay)
 			successCount = 0
 		}
 
 		if time.Now().After(endTime) {
-			return diag.Errorf("validation failed after max_cred_validation_seconds, giving up; now=%s, endTime=%s", time.Now().String(), endTime.String())
+			return diag.Errorf(
+				"validation failed after max_cred_validation_seconds of %d, giving up; now=%s, endTime=%s",
+				maxValidationSeconds,
+				time.Now().String(),
+				endTime.String(),
+			)
 		}
 
 		time.Sleep(delay)

vault/data_source_azure_access_credentials_test.go

@@ -14,6 +14,8 @@ import (
 	"github.com/hashicorp/terraform-provider-vault/testutil"
 )
 
+// TestAccDataSourceAzureAccessCredentials_basic tests the creation of dynamic
+// service principals
 func TestAccDataSourceAzureAccessCredentials_basic(t *testing.T) {
 	// This test takes a while because it's testing a loop that
 	// retries real credentials until they're eventually consistent.
@@ -38,6 +40,68 @@ func TestAccDataSourceAzureAccessCredentials_basic(t *testing.T) {
 	})
 }
 
+// TestAccDataSourceAzureAccessCredentials_basic tests the credential
+// generation for existing service principals
+func TestAccDataSourceAzureAccessCredentials_ExistingSP(t *testing.T) {
+	// This test takes a while because it's testing a loop that
+	// retries real credentials until they're eventually consistent.
+	if testing.Short() {
+		t.SkipNow()
+	}
+	mountPath := acctest.RandomWithPrefix("tf-test-azure")
+	conf := testutil.GetTestAzureConfExistingSP(t)
+	resource.Test(t, resource.TestCase{
+		ProviderFactories: providerFactories,
+		PreCheck:          func() { testutil.TestAccPreCheck(t) },
+		Steps: []resource.TestStep{
+			{
+				Config: testAccDataSourceAzureAccessCredentialsConfig_existingSP(mountPath, conf, 60),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttrSet("data.vault_azure_access_credentials.test", "client_id"),
+					resource.TestCheckResourceAttrSet("data.vault_azure_access_credentials.test", "client_secret"),
+					resource.TestCheckResourceAttrSet("data.vault_azure_access_credentials.test", "lease_id"),
+				),
+			},
+		},
+	})
+}
+
+func testAccDataSourceAzureAccessCredentialsConfig_existingSP(mountPath string, conf *testutil.AzureTestConf, maxSecs int) string {
+	template := `
+resource "vault_azure_secret_backend" "test" {
+	path = "{{mountPath}}"
+	subscription_id = "{{subscriptionID}}"
+	tenant_id = "{{tenantID}}"
+	client_id = "{{clientID}}"
+	client_secret = "{{clientSecret}}"
+}
+
+resource "vault_azure_secret_backend_role" "test" {
+	backend = vault_azure_secret_backend.test.path
+	role = "my-role"
+	application_object_id = "{{appObjectID}}"
+	ttl = 300
+	max_ttl = 600
+}
+
+data "vault_azure_access_credentials" "test" {
+    backend = vault_azure_secret_backend.test.path
+    role = vault_azure_secret_backend_role.test.role
+    validate_creds = true
+	num_seconds_between_tests = 1
+	max_cred_validation_seconds = {{maxCredValidationSeconds}}
+}`
+
+	parsed := strings.Replace(template, "{{mountPath}}", mountPath, -1)
+	parsed = strings.Replace(parsed, "{{subscriptionID}}", conf.SubscriptionID, -1)
+	parsed = strings.Replace(parsed, "{{tenantID}}", conf.TenantID, -1)
+	parsed = strings.Replace(parsed, "{{clientID}}", conf.ClientID, -1)
+	parsed = strings.Replace(parsed, "{{clientSecret}}", conf.ClientSecret, -1)
+	parsed = strings.Replace(parsed, "{{appObjectID}}", conf.AppObjectID, -1)
+	parsed = strings.Replace(parsed, "{{maxCredValidationSeconds}}", strconv.Itoa(maxSecs), -1)
+	return parsed
+}
+
 func testAccDataSourceAzureAccessCredentialsConfigBasic(mountPath string, conf *testutil.AzureTestConf, maxSecs int) string {
 	template := `
 resource "vault_azure_secret_backend" "test" {