Fixing bug where variable group fails to map new variable after service endpoint secret rotates

[rdalbuquerque]

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 a key vault expansion retry logic in case there is an update to azurerm service endpoint secret.

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

[xuzhang3]

@rdalbuquerque Since this is an AzureRM service connection secret rotate issue, suggest use API(https://learn.microsoft.com/en-us/rest/api/azure/devops/serviceendpoint/endpointproxy/execute-service-endpoint-request?tabs=HTTP) to do the validation for service connection. Currently all service connections not do the validation by default which means all secrets can be dummy values. Add the retry logic in VG only resolve part of the issue as other resource may also face the same issue.

Web UI Verify:

[rdalbuquerque]

@xuzhang3 Ok, so I played around a few scenarios to make this validation possible and more generic:

func validateServiceEndpointConnection(clients *client.AggregatedClient, endpoint *serviceendpoint.ServiceEndpoint) error {
	reqArgs := serviceendpoint.ExecuteServiceEndpointRequestArgs{
		ServiceEndpointRequest: &serviceendpoint.ServiceEndpointRequest{
			DataSourceDetails: &serviceendpoint.DataSourceDetails{
				DataSourceName: converter.String("TestConnection"),
			},
			ResultTransformationDetails: &serviceendpoint.ResultTransformationDetails{},
		},
		Project:    converter.String((*endpoint.ServiceEndpointProjectReferences)[0].ProjectReference.Id.String()),
		EndpointId: converter.String(endpoint.Id.String()),
	}

	maxRetries := 15  // should probably be an input
	retryInverval := 2  // should probably be an input
	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++ {
		if reqResult, err = clients.ServiceEndpointClient.ExecuteServiceEndpointRequest(clients.Ctx, reqArgs); err != nil {
			log.Printf(fmt.Sprintf(":: serviceendpoint/commons.go :: %s :: error during endpoint validation request", *endpoint.Name))
			return err
		} else if strings.EqualFold(*reqResult.StatusCode, "ok") {
			log.Printf(fmt.Sprintf(":: serviceendpoint/commons.go :: %s :: successfully validated connection", *endpoint.Name))
			return nil
		}
		log.Printf(fmt.Sprintf(":: serviceendpoint/commons.go :: %s :: validation failed, retrying...", *endpoint.Name))
		time.Sleep(time.Duration(retryInverval) * time.Second)
	}

	return fmt.Errorf("Error validating connection: (type: %s, name: %s, code: %s, message: %s)", *endpoint.Type, *endpoint.Name, *reqResult.StatusCode, *reqResult.ErrorMessage)
}

1. Run the function above everytime any serviceendpoint is created or updated This is problematic because:

   • To run this proxy request I need the EndpointID which I only get after endpoint is successfully created and that can lead to the endpoint having to be deleted because validation didn't pass and I don't know if that's a good practice in plugin development.
   • I don't know if every endpoint has this validation functionality implemented, through the UI it looks like it's not and it even depends on the authorization method.

2. Run this validation conditioned to the endpoint type (hardcoded). there would somehow be a parameter to tell the provider whether specified endpoint type has to run the validation or not.
   problem:

   • Even if I know the service endpoint type support validation (azurerm case) I might be breaking use cases where people actually want for some reason create an invalid endpoint (maybe access is given afterwards).

3. Create a new argument on resource schema telling the provider whether or not that endpoint should be validated after creation/update. I don't see any issue here, I don't know if that's the best way to handle this

4. Add this function to helper package where it could be called from other packages (like variable group resource). I would just need to change the function inputs so it won't require the full endpoint object

Hopefully this can be solved. Thanks in advance!

[xuzhang3]

@rdalbuquerque

1. Run the function above everytime any serviceendpoint is created or updated This is problematic because:

   • Validation will not delete the service endpoint, it just verify the connection.
   • Not all the endpoint implement the validation function, only some have.

2. This could be flag, defaults to false. Only do the validation when it is true.

3. Validation is a feature of some endpoints. In this scenario the AD secret changed, the real affected resource is azuredevops_serviceendpoint_azurerm, azuredevops_serviceendpoint_azurerm should ensure it works not azuredevops_variable_group

4. Add this to the helper package/common package is helpful, any endpoint that supports this feature can use it

[rdalbuquerque]

@xuzhang3
OK so here are some toughts on the new changes I pushed:
I had a hard time isolating the changes to just resource_serviceendpoint_azurerm.go since only flatten and expand functions are delegated to specific endpoint files and each endpoint was expeted to return a specific type of function that didn't have an input to fit validate argument.

So for this first attempt I did the following:

1. changed the expand/flattenFunc types to handle a new struct serviceEndpointWithValidation instead of serviceendpoint.ServiceEndpoint so I could return if the azurerm endpoint should be validated or not on create/update operations

type serviceEndpointWithValidation struct {
	endpoint *serviceendpoint.ServiceEndpoint
	validate bool
}
type flatFunc func(d *schema.ResourceData, serviceEndpoint *serviceEndpointWithValidation, projectID *uuid.UUID)
type expandFunc func(d *schema.ResourceData) (*serviceEndpointWithValidation, *uuid.UUID, error)

2. Because of that I had to adapt all the other endpoints to handle serviceEndpointWithValidation type (and all their tests)
3. Added a conditional on genServiceEndpointCreateFunc and genServiceEndpointUpdateFunc to handle validation in case validate argument is set to true
   • on genServiceEndpointCreateFunc I can only run validation after endpoint is created so I can have the endpoint id, because of that, if validation fails I then delete the endpoint and return an error
   • on genServiceEndpointUpdateFunc I'm able to validate the new configs before applying changes, so if validation fails I return an error and service endpoint object remains unchanged
4. added the unit and acceptance tests to cover the new validate argument

One issue I couldn't figure out the best way to handle is that even if the update fails and, on the update, I'm changing the validate argument, the new validate value is still saved to state because on genServiceEndpointReadFunc doesn't have a way to know the actual validate value since it's only on state and not an object property in Azure Devops, so I get the value from config:

validate, ok := d.Get("validate").(bool)
if !ok {
  validate = false
}
flatFunc(d, &serviceEndpointWithValidation{endpoint: serviceEndpoint, validate: validate}, &projectID)

Looking forward to you feedback :)

[xuzhang3]

@rdalbuquerque currently all the service connections share the same create/update/read/delete function which is a limitation to all the service connection resources, some service connections have specifical configurations which is only available to itself. We are considering refactoring the service connection resources, every resource has its own create/update/read/delete functions but share the common functions. Each resource can customize its own configuration without affecting other resources.

[rdalbuquerque]

@xuzhang3 I see, so we wait the refactor before moving forward with this PR?

If there is anything I can do to help refactoring or any other way we can implement this validation on azurerm service endpoint that would not need the refactor, please let me know!

[xuzhang3]

@rdalbuquerque yes, wait before refactoring. More work is required if this PR is merged.

[xuzhang3]

@rdalbuquerque #863 will decouple the generic functions, you can add the validation to azurerm after that PR merged

[xuzhang3]

@rdalbuquerque #863 has been merged, you can add the validation to azurerm resoruce without affecting other resources