Skip to content

Fix serviceendpoint_generic_v2 to let the api throw the failure and properly update the authentication parameters. - #1544

Open
Levente Laszlo Papp (aklakina) wants to merge 5 commits into
microsoft:mainfrom
aklakina:fix/serviceendpoint-generic-v2
Open

Fix serviceendpoint_generic_v2 to let the api throw the failure and properly update the authentication parameters.#1544
Levente Laszlo Papp (aklakina) wants to merge 5 commits into
microsoft:mainfrom
aklakina:fix/serviceendpoint-generic-v2

Conversation

@aklakina

Copy link
Copy Markdown
Contributor

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.
  • Have you checked to ensure there aren't other open PRs for the same update/change?

Description

The apply time input validation was causing a lot of issues as the code did not reimplement the conditional logic from the service endpoint validation. I have added a toggle that defaults to turn this feature off. This validation is still useful for small service connections but it is unusable for azure service connections for example.

Also there was a bug that skipped the authentication parameters from the update. Fixed that too.

PR 2/2 from #1505

Does this introduce a breaking change?

  • Yes
  • No

Test Result

Related Issue(s)

Fix #0000

Other information

@aklakina

Copy link
Copy Markdown
Contributor Author

Hi magodo (@magodo)

This PR is ready for review.

@magodo

Copy link
Copy Markdown
Member

I think this is more proper to be moved to CustomizeDiff so that this can happen during plan time instead of apply time, WDYT?

@aklakina

Copy link
Copy Markdown
Contributor Author

It runs both at plan time and apply time. The problem with plan time run is that the input parameters can be undeterministic in which case terraform provides a null object. This breaks plan time validation.

@magodo

Copy link
Copy Markdown
Member

These attributes are less likely to be unknown during plan time. You shall skip the validation if there is any unknown value. Whilst the CustomizeDiff also runs at apply time, where all the attributes will be known.

@aklakina

Copy link
Copy Markdown
Contributor Author

It is part of costumizediff

But the main problem is not that the values are unkown but that the api retruns a validation logic for each data field and we would need to implement the same logic parser as it is already implemented in the azure devops. So for example for azure service connection you would not be able to create it in the current state because both service principal and other login method inputs are present in the data field but the conditional logic is not parsed or processed so the diff just dies because it expects all the auth data which are conflicting.

@magodo

Copy link
Copy Markdown
Member

Could you pass the acctests and paste the run result here then I'm happy to merge this PR? Thx!

@aklakina

Copy link
Copy Markdown
Contributor Author

Hi magodo (@magodo), I finally had time to run the acc tests. Sadly they are not passing and seemingly the azure devops client is causing the error.

Here is the output:
image
and here is the stdout:
acctests.log

I tracked down the failure to the following line in the code:
https://github.com/aklakina/terraform-provider-azuredevops/blob/e5598111ada403e84fb117b8485d4885fc67281e/azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_generic_v2.go#L126
Which calls the client to get all the types. This was working when the resource got added as the acctests all passed then and as you can see, this pr does not touch that code part.

Can you check if the ADO go client dev team knows about this?

@aklakina

Copy link
Copy Markdown
Contributor Author

hi magodo (@magodo)

Any news here?

@magodo

Copy link
Copy Markdown
Member

Sorry, I've no idea why the ADO service fails.

@aklakina

Copy link
Copy Markdown
Contributor Author

Technical Investigation: JSON Unmarshalling Issue

I've done some digging into the JSON unmarshalling error we've been seeing in the test runs, and I wanted to share what I found:

Error: error initializing service endpoint types: failed to retrieve service endpoint types: 
json: cannot unmarshal number into Go struct field InputValidation.Value.authenticationSchemes.inputDescriptors.validation.minValue of type *big.Float

What's Actually Happening

The issue is in the Azure DevOps Go SDK (vendor/github.com/microsoft/azure-devops-go-api/azuredevops/v7/forminput/models.go). The InputValidation struct defines its min/max value fields like this:

type InputValidation struct {
    MinValue *big.Float `json:"minValue,omitempty"`
    MaxValue *big.Float `json:"maxValue,omitempty"`
}

The problem is that Go's standard json.Unmarshal() can't convert JSON numbers directly into *big.Float types. It only works with basic types (string, int, float64, bool) or custom types that implement UnmarshalJSON() themselves. *big.Float doesn't have that built-in.

So when we call GetServiceEndpointTypes() and the API returns those numeric validation values, the JSON decoder just throws up its hands and fails.

Why This PR Solves It

By adding the validate_input toggle and defaulting it to false, we sidestep this issue entirely. When validation is disabled, we never try to unmarshal those problematic fields. Instead, we let Azure DevOps do the validation server-side, which is honestly more reliable anyway since it's the source of truth.

I think this is actually a better design than trying to replicate all of Azure DevOps' validation logic on the client side. The API will always have the most up-to-date rules, and users get clear error messages directly from Azure DevOps if their config is invalid.

Next Steps

If we wanted to fully enable client-side validation in the future, someone would need to file an issue with the Azure DevOps Go SDK to add proper JSON unmarshalling support for *big.Float. But for now, letting the API handle it is the pragmatic solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants