adding validation feature to azuredevops_serviceendpoint_azurerm reso… #865

rdalbuquerque · 2023-08-24T02:17:23Z

All Submissions:

Have you added an explanation of what your changes do and why you'd like us to include them?
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
My code follows the code style of this project.
I ran lint checks locally prior to submission.
Have you checked to ensure there aren't other open PRs for the same update/change?

What about the current behavior has changed?

Adding validation argument to verify if connection with azure is valid.
currently if you rotate the service principal credential backing a azurerm service endpoint it takes a little bit of time before the connection is valid again. That makes other dependent resources, like variable groups linked with Azure, return an error if they are also doing an operation that depends on said connection.

With validate argument set to true, the provider will verify the connection with a retry logic before completing the modification.

Issue Number: #825

Also fixing error messages typos
Issue Number: #303

Does this introduce a change to `go.mod`, `go.sum` or `vendor/`?

Yes
No

Does this introduce a breaking change?

Yes
No

Any relevant logs, error output, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Other information

The tradeoff is that if the secret is in fact wrong it will take 30 seconds to return an error

…urce

rdalbuquerque · 2023-08-24T02:21:11Z

@xuzhang3
I closed my last PR #826 by mistake, so I'm reopening.
my bad :(

xuzhang3

@rdalbuquerque thanks for submit PR. A few changes requested and you need document the new feature in serviceendpoint_azurerm.html.markdown

xuzhang3 · 2023-09-01T05:13:20Z

azuredevops/internal/service/serviceendpoint/commons.go

+	var reqResult *serviceendpoint.ServiceEndpointRequestResult
+	var err error
+	log.Printf(fmt.Sprintf(":: serviceendpoint/commons.go :: %s :: Initiating validation", *endpoint.Name))
+	for i := 0; i < maxRetries; i++ {


can we replace the retry code with resource.RetryContext(), a simple retry util tool

xuzhang3 · 2023-09-01T05:17:27Z

azuredevops/internal/service/serviceendpoint/data_serviceendpoint_azurerm.go

@@ -23,7 +23,7 @@ func dataSourceServiceEndpointAzureRMRead(d *schema.ResourceData, m interface{})
 	if serviceEndpoint != nil {
 		(*serviceEndpoint.Data)["creationMode"] = ""
 		d.Set("service_endpoint_id", serviceEndpoint.Id.String())
-		flattenServiceEndpointAzureRM(d, serviceEndpoint, projectID)
+		flattenServiceEndpointAzureRM(d, serviceEndpoint, projectID, false)


The validation flag is a feature of the resource, data source can ignore it and the data source schema not defined this property. There is also no validation in the API response, true or false does no matter here.

xuzhang3 · 2023-09-01T05:20:51Z

azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_azurerm.go

@@ -161,6 +170,22 @@ func resourceServiceEndpointAzureRMCreate(d *schema.ResourceData, m interface{})
 		return err
 	}

+	if validate {


can we directly get the validate with d.Get("validate") instead of expandServiceEndpointAzureRM.
flatten/expand focuses on the core properties of flattening/expanding resources

xuzhang3 · 2023-09-01T05:24:12Z

azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_azurerm.go

@@ -181,30 +206,41 @@ func resourceServiceEndpointAzureRMRead(d *schema.ResourceData, m interface{}) e
 		return fmt.Errorf(" looking up service endpoint given ID (%v) and project ID (%v): %v", getArgs.EndpointId, getArgs.Project, err)
 	}

-	flattenServiceEndpointAzureRM(d, serviceEndpoint, (*serviceEndpoint.ServiceEndpointProjectReferences)[0].ProjectReference.Id)
+	validate, ok := d.Get("validate").(bool)


You can set validation directly in read instead of flattenServiceEndpointAzureRM

Suggested change

validate, ok := d.Get("validate").(bool)

d.Set("validate", d.Get("validate").(bool))

xuzhang3 · 2023-09-01T09:00:35Z

azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_azurerm.go

@@ -146,12 +149,18 @@ func ResourceServiceEndpointAzureRM() *schema.Resource {
 			Version: 1,
 		},
 	}
+
+	r.Schema["validate"] = &schema.Schema{


Can we wrapper validate into a features block like build definition features ? This property is a feature not part of the API.

rdalbuquerque · 2023-09-14T12:18:36Z

Thanks for the review @xuzhang3 , will start working on it now

…es block

rdalbuquerque · 2023-09-17T16:38:28Z

@xuzhang3
Ok, So I guess all suggested changes have been implemented, docs are updated and tests are passing :)

Only issue I caught is that even though the validation passes the variable group expansion might still fail. Upon further investigation I found that even though the validation passes once it might fail again right after (this issue is also reproducible through the UI). Looks like some kind of load balancing/replication issue after azuread_application_password rotation. Might also be an issue related to a weekend maintenence or Brazil South region.

Although variable group extension might still fail after validation passes it drastically decreases the probability of said issue.

this is the script I used to test this scenarios:

$secretvars = @{
    secretvars = @{}
}

$errCount = 0
for ($i = 0; $i -lt 5; $i++) {
    $secretvars.secretvars["secret$i"] = "secretvalue"
    $secretvars | ConvertTo-Json | Set-Content -Path secretvars.auto.tfvars.json
    terraform apply -replace azuread_application_password.this -auto-approve | Tee-Object -Variable result
    if (-not ($result -like '*Apply complete*')) {
        $errCount++
        Write-Warning "error occurred on run $i"
    }
    Start-Sleep -Seconds 10
}

Write-Host
Write-Warning "Total errors: $errCount"

And the terraform resources:

resource "azuread_application" "this" {
  display_name = "test-serviceendpoint"
}

resource "azuread_service_principal" "this" {
  application_id = azuread_application.this.application_id
}

resource "azuread_application_password" "this" {
  depends_on = [ azurerm_key_vault_secret.this ]
  application_object_id = azuread_application.this.object_id
}

resource "azuredevops_serviceendpoint_azurerm" "this" {
  project_id                             = data.azuredevops_project.this.id
  service_endpoint_name                  = "Example AzureRM"
  description                            = "Managed by Terraform"
  service_endpoint_authentication_scheme = "ServicePrincipal"
  credentials {
    serviceprincipalid  = azuread_service_principal.this.application_id
    serviceprincipalkey = azuread_application_password.this.value
  }
  azurerm_spn_tenantid      = data.azurerm_client_config.current.tenant_id
  azurerm_subscription_id   = data.azurerm_client_config.current.subscription_id
  azurerm_subscription_name = "prd"
  features {
    validate = true
  }
}

resource "azurerm_resource_group" "this" {
  name     = "test-azuredevops-conn"
  location = "eastus"
}

resource "azurerm_key_vault" "this" {
  name                      = "test-azdo-conn"
  resource_group_name       = azurerm_resource_group.this.name
  location                  = "eastus"
  sku_name                  = "standard"
  tenant_id                 = data.azurerm_client_config.current.tenant_id
  enable_rbac_authorization = true
}

resource "azurerm_role_assignment" "data_admin_2_current_client" {
  scope                = azurerm_key_vault.this.id
  role_definition_name = "Key Vault Administrator"
  principal_id         = data.azurerm_client_config.current.object_id
}

resource "azurerm_role_assignment" "secret_reader_2_conn" {
  scope                = azurerm_key_vault.this.id
  role_definition_name = "Key Vault Secrets User"
  principal_id         = azuread_service_principal.this.object_id
}

## variable group linked to vault using the connection
variable "secretvars" {
  type = map(string)
}

resource "azurerm_key_vault_secret" "this" {
  for_each     = var.secretvars
  key_vault_id = azurerm_key_vault.this.id
  name         = each.key
  value        = each.value
}

resource "azuredevops_variable_group" "this" {
  project_id = data.azuredevops_project.this.id
  name       = "Example Variable Group"
  key_vault {
    name                = azurerm_key_vault.this.name
    service_endpoint_id = azuredevops_serviceendpoint_azurerm.this.id
  }
  dynamic "variable" {
    for_each = azurerm_key_vault_secret.this
    content {
      name = variable.value.name
    }
  }
}

xuzhang3 · 2023-11-09T06:24:28Z

@rdalbuquerque can you help resolve the conflict?

rdalbuquerque · 2023-11-10T03:51:30Z

@rdalbuquerque can you help resolve the conflict?

done :)

xuzhang3 · 2023-11-14T09:27:08Z

=== RUN   TestAccServiceEndpointAzureRm_CreateAndUpdate
=== PAUSE TestAccServiceEndpointAzureRm_CreateAndUpdate
=== RUN   TestAccServiceEndpointAzureRm_CreateAndUpdate_WithValidate
=== PAUSE TestAccServiceEndpointAzureRm_CreateAndUpdate_WithValidate
=== RUN   TestAccServiceEndpointAzureRm_Create_WithValidate
=== PAUSE TestAccServiceEndpointAzureRm_Create_WithValidate
=== RUN   TestAccServiceEndpointAzureRm_MgmtGrpCreateAndUpdate
=== PAUSE TestAccServiceEndpointAzureRm_MgmtGrpCreateAndUpdate
=== RUN   TestAccServiceEndpointAzureRm_AutomaticCreateAndUpdate
=== PAUSE TestAccServiceEndpointAzureRm_AutomaticCreateAndUpdate
=== RUN   TestAccServiceEndpointAzureRm_WorkloadFederation_Manual_CreateAndUpdate
=== PAUSE TestAccServiceEndpointAzureRm_WorkloadFederation_Manual_CreateAndUpdate
=== RUN   TestAccServiceEndpointAzureRm_WorkloadFederation_Automatic_CreateAndUpdate
=== PAUSE TestAccServiceEndpointAzureRm_WorkloadFederation_Automatic_CreateAndUpdate
=== RUN   TestAccServiceEndpointAzureRm_ManagedServiceIdentity_CreateAndUpdate
=== PAUSE TestAccServiceEndpointAzureRm_ManagedServiceIdentity_CreateAndUpdate
=== CONT  TestAccServiceEndpointAzureRm_ManagedServiceIdentity_CreateAndUpdate
=== CONT  TestAccServiceEndpointAzureRm_CreateAndUpdate_WithValidate
=== CONT  TestAccServiceEndpointAzureRm_CreateAndUpdate
=== CONT  TestAccServiceEndpointAzureRm_WorkloadFederation_Automatic_CreateAndUpdate
=== CONT  TestAccServiceEndpointAzureRm_WorkloadFederation_Manual_CreateAndUpdate
=== CONT  TestAccServiceEndpointAzureRm_AutomaticCreateAndUpdate
=== CONT  TestAccServiceEndpointAzureRm_MgmtGrpCreateAndUpdate
=== CONT  TestAccServiceEndpointAzureRm_Create_WithValidate
--- PASS: TestAccServiceEndpointAzureRm_MgmtGrpCreateAndUpdate (106.75s)
--- PASS: TestAccServiceEndpointAzureRm_Create_WithValidate (136.56s)
--- PASS: TestAccServiceEndpointAzureRm_AutomaticCreateAndUpdate (162.94s)
--- PASS: TestAccServiceEndpointAzureRm_CreateAndUpdate (168.45s)
--- PASS: TestAccServiceEndpointAzureRm_WorkloadFederation_Manual_CreateAndUpdate (168.57s)
--- PASS: TestAccServiceEndpointAzureRm_ManagedServiceIdentity_CreateAndUpdate (168.92s)
--- PASS: TestAccServiceEndpointAzureRm_CreateAndUpdate_WithValidate (188.89s)
--- PASS: TestAccServiceEndpointAzureRm_WorkloadFederation_Automatic_CreateAndUpdate (195.06s)
PASS
ok      github.com/microsoft/terraform-provider-azuredevops/azuredevops/internal/acceptancetests        201.072s

xuzhang3 · 2023-11-14T09:27:17Z

@rdalbuquerque LGTM

adding validation feature to azuredevops_serviceendpoint_azurerm reso…

ec15568

…urce

xuzhang3 added enhancement service connection labels Aug 24, 2023

xuzhang3 requested changes Sep 1, 2023

View reviewed changes

xuzhang3 reviewed Sep 1, 2023

View reviewed changes

rdalbuquerque added 5 commits September 14, 2023 10:01

rewrite retry func with resource.RetryContext()

06dcedc

remove validate from flatten/expand logic and wrap argument on featur…

563f55d

…es block

implementing tests

f8d45a3

change azurerm service endpoint validation timeout to 45 seconds

efe5508

increate timeout for unit tests

e10fa1b

rdalbuquerque requested a review from xuzhang3 October 5, 2023 01:22

xuzhang3 mentioned this pull request Oct 16, 2023

Support for federated authentication instead of PAT? #903

Closed

rdalbuquerque mentioned this pull request Nov 8, 2023

error message with typos, unable to expand variable group #920

Open

rdalbuquerque added 2 commits November 9, 2023 23:54

Merge remote-tracking branch 'upstream/main'

b9cb7d3

merge upstream

2705e39

xuzhang3 approved these changes Nov 14, 2023

View reviewed changes

xuzhang3 merged commit 857b30e into microsoft:main Nov 14, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding validation feature to azuredevops_serviceendpoint_azurerm reso… #865

adding validation feature to azuredevops_serviceendpoint_azurerm reso… #865

rdalbuquerque commented Aug 24, 2023 •

edited

Loading

rdalbuquerque commented Aug 24, 2023

xuzhang3 left a comment

xuzhang3 Sep 1, 2023

xuzhang3 Sep 1, 2023

xuzhang3 Sep 1, 2023

xuzhang3 Sep 1, 2023

xuzhang3 Sep 1, 2023

rdalbuquerque commented Sep 14, 2023

rdalbuquerque commented Sep 17, 2023 •

edited

Loading

xuzhang3 commented Nov 9, 2023

rdalbuquerque commented Nov 10, 2023

xuzhang3 commented Nov 14, 2023

xuzhang3 commented Nov 14, 2023

	validate, ok := d.Get("validate").(bool)
	d.Set("validate", d.Get("validate").(bool))

adding validation feature to azuredevops_serviceendpoint_azurerm reso… #865

adding validation feature to azuredevops_serviceendpoint_azurerm reso… #865

Conversation

rdalbuquerque commented Aug 24, 2023 • edited Loading

All Submissions:

What about the current behavior has changed?

Does this introduce a change to go.mod, go.sum or vendor/?

Does this introduce a breaking change?

Any relevant logs, error output, etc?

Other information

rdalbuquerque commented Aug 24, 2023

xuzhang3 left a comment

Choose a reason for hiding this comment

xuzhang3 Sep 1, 2023

Choose a reason for hiding this comment

xuzhang3 Sep 1, 2023

Choose a reason for hiding this comment

xuzhang3 Sep 1, 2023

Choose a reason for hiding this comment

xuzhang3 Sep 1, 2023

Choose a reason for hiding this comment

xuzhang3 Sep 1, 2023

Choose a reason for hiding this comment

rdalbuquerque commented Sep 14, 2023

rdalbuquerque commented Sep 17, 2023 • edited Loading

xuzhang3 commented Nov 9, 2023

rdalbuquerque commented Nov 10, 2023

xuzhang3 commented Nov 14, 2023

xuzhang3 commented Nov 14, 2023

rdalbuquerque commented Aug 24, 2023 •

edited

Loading

Does this introduce a change to `go.mod`, `go.sum` or `vendor/`?

rdalbuquerque commented Sep 17, 2023 •

edited

Loading