go-autorest: Validation is overly strict and causing problems

See here: https://github.com/Azure/acs-engine/issues/194#issuecomment-274654088

We got new validation code in the latest version of the SDK. When we upgraded in Kubernetes, it broke a core scenario - being able to create load balancers.

Error message: servicecontroller.go:760] Failed to process service. Retrying in 1m20s: Failed to create load balancer for service default/nginx: network.SecurityGroupsClient#CreateOrUpdate: Invalid input: autorest/validation: validation failed: parameter=parameters.SecurityGroupPropertiesFormat.Subnets constraint=ReadOnly value=[]network.Subnet{network.Subnet{Response:autorest.Response{Response:(*http.Response)(nil)}, ID:(*string)(0xc4228be160), SubnetPropertiesFormat:(*network.SubnetPropertiesFormat)(nil), Name:(*string)(nil), Etag:(*string)(nil)}} details: readonly parameter; must send as nil or empty in request

Not sure what the recommended course of action here is. I’m not 100% up on what these validations do… but I know they’re blocking a scenario that was perfectly functional before.

Further, discussion of PUT semantics today imply that such fields must be submitted, but simply shouldn’t be changed (which is not something client SDK validation should be checking).

cc: @brendandburns

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 17 (17 by maintainers)

Commits related to this issue

Merge pull request #40405 from vwfs/azure_lb_readonly_fix Automatic merge from submit-queue Set NetworkInterfaces and Subnets to nil before updating Azure security groups **What this PR does / why ... — committed to kubernetes/kubernetes by deleted user 7 years ago

Most upvoted comments

This has been mitigated in GO-SDK 8.0.0-beta

salameer on Feb 8, 2017

As explained to me on Monday:

PUT is the full body of the object, missing fields indicate deletes

PATCH is a partial update of the object. missing fields are treated as “leave me the same”. fields that are changed overwrite the existing field

I think this might be into the territory of “everyone treats REST differently” though.

colemickens on Jan 25, 2017

For instance, the crux of this issue is that it is bad REST manners to send values that don’t need to be updated.

I don’t agree with this interpretation. My understanding (after being convinced by Ryan and Brendan) was that PUT semantics say that missing fields are removals. PATCH semantics are different, but aren’t at play here anyway.

Further, to me, “readonly” means unchanged, not “must be absent from request”.

We’ll tinker around with what an updated API that behaved itself better would look like. In your opinion, @colemickens, would that be worthy of a breaking change in the future?

Unclear. Not to be too brash, but I’m still of the mind that this isn’t hugely value-adding, plus I’m not sure why it would need to be a breaking change. Obviously if azure-sdk-for-go introduced it, we would adopt/adapt to it.

colemickens on Jan 25, 2017