Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update api version of LB to 2020-05-01 #8910

Closed
wants to merge 2 commits into from

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Oct 16, 2020

The LB api version is rolled back to 2020-03-01 in #8006 because of issue #7691. Now that the issue has been addressed in Azure/azure-rest-api-specs#10131 and the change has been involved in the Go SDK currently under used. So we shall upgrade the API back to 2020-05-01 so that all network resources are using the same one.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for this @magodo

@tombuildsstuff
Copy link
Contributor

@magodo how does Azure/azure-rest-api-specs#10131 fix this? That Swagger fix appears to be just re-ordering two fields in the Swagger, rather than doing anything else - IIRC this was an API bug where these fields were being incorrectly mapped?

@magodo
Copy link
Collaborator Author

magodo commented Oct 19, 2020

@tombuildsstuff The reason is that the service somehow can't tolerate the first properties to be read only, after swapping the order, the service can correctly decode the request.

Quote from service team:

We took a look at the issue. The problem is that payload does not match what service expects:
In json payload we see both backendIPConfigurations and loadBalancerBackendAddressesProperties in backendAddressPool child resource.
It is not expected in service side to get the backendIPConfigurations property located first in JSON. That is causing the error on server side when deserialization happens.

@tombuildsstuff
Copy link
Contributor

@magodo given the JSON spec states that the keys inside a map are can be in any order (and I can't think of a JSON Serialisation library which supports that?) - that sounds like an API bug which still needs to be resolved? Unfortunately we'd be unable to ship this until this is fixed, due to the design of/monthly churn from the Networking API's

@tombuildsstuff tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Oct 19, 2020
@tombuildsstuff tombuildsstuff added this to the Blocked milestone Oct 19, 2020
@magodo
Copy link
Collaborator Author

magodo commented Oct 20, 2020

@tombuildsstuff Yes, you are right. So the reason why it now works seems because we do not send the other properties in the API request in Go SDK any more due to Azure/autorest.go#446.

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Oct 20, 2020

@magdoo unfortunately that's not something we can depend/reply on, due to the large churn within the Azure SDK - so it's possible for this to subtly break at any time, in a way that's incredibly hard to diagnose?

As such I think this PR is blocked for the moment - can we open a Swagger issue and reach out to the Service Team to get this bug fixed? Closing this temporarily since this is blocked - but we can re-open this once the API's fixed.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants