Validation is overly strict and causing problems

[colemickens]

See here: Azure/acs-engine#194 (comment)

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

[colemickens]

cc: @mcardosos for help routing this to someone who can weigh in. thanks.

[mcardosos]

Hello @colemickens
I had already added an issue to AutoRest. I think AutoRest should generate some lines inside the operation function in which those parameters are turned to nil.
AFAIK, Azure will simply ignore this readonly params, but I may be wrong. If this is correct, we could get rid of the validation lines for readonly params.

[codablock]

Regarding the 2 specific fields (Subnets and Interfaces) that brought this up...are they really ReadOnly? In the Azrue Portal there are buttons to modify these. For example, the "Network Interfaces" view has an "associate" button and each interface can be disassociated when clicking on the "...". The same seems to apply to subnets.

[colemickens]

@mcardosos I'm not really sure of the right thing to do here. Is there a way to blanket disable validation?

More over, what is the point of the validation? Is it just to save on quota or to get the error a few seconds faster than making the call over the network? Just trying to frame how I'm thinking about this validation framework.

[colemickens]

For example, @codablock is suggesting that we blank out those fields to nil. Per my discussion with @brendandburns and @rjmax... I'd expect that to be "bad guidance" for a PUT. Instead, the same body should be PUT with only new/altered fields being changed.

To me, a PUT with those fields nild out is telling Azure to remove the SecurityGroup for the reference subnet... which is certainly not what we want.

Hence why I'm confused what this validation is meant to test...

[mcardosos]

@colemickens I think the best thing to do is remove the lines that have to do with ReadOnly parameters from the validation part in the function. Validation is useful to know what might be wrong with the payload before sending the request.
@codablock I don't know the network API that well. Please open an issue asking if those fields are really readonly in the azure-rest-api-specs repo. @amarzavery Do you know who is the right person to contact about the network API?

[colemickens]

@colemickens I think the best thing to do is remove the lines that have to do with ReadOnly parameters from the validation part in the function.

I don't think we have an option of monkey-patching the version of go-autorest used in upstream projects. At least, I don't feel comfortable with it (it probably will fail builds and validations anyway...)

I don't know the network API that well. Please open an issue asking if those fields are really readonly in the azure-rest-api-specs repo. @amarzavery Do you know who is the right person to contact about the network API?

The point is not whether or not the fields are really read-only. The question is why are we even checking this?

Putting mutable or immutable fields in a PUT request is valid... and removing those fields has different semantic meaning.

Also:

Validation is useful to know what might be wrong with the payload before sending the request.

doesn't really answer my query. I don't understand how these client-side validations provide any value other than giving me a response a few tens of milliseconds faster.

Meanwhile, now we're in a case where we're being blocked client side on calls that are perfectly valid...

Meaning so far, the validation has only caused breakage, and nothing helpful. The things that this validation seems to be catching... are things that are likely to be coding errors and will be caught at dev-time and not at run-time anyway. (Outside of user input scenarios I guess)

[colemickens]

One more comment: If the validation were exposed in per request functions (PrepareBlah, SendBlah... if there were a ValidateBlah)... then I could write tests in Kubernetes to prove that my models in my test cases would pass validation.

[marstr]

This is a really interesting thread, because it brings to light a sort of larger question which is, "why is validation done in the runtime?" @jhendrixMSFT and I just thought through this a little, and agreed that as much as possible we'd rather use the language to enforce these rules. For instance, the crux of this issue is that it is bad REST manners to send values that don't need to be updated. If it is ReadOnly, it doesn't ever make sense to include it in a PUT once created. @mcardosos has pointed out to me that Azure seems to do the right thing and just ignore it, but ideally we wouldn't send it at all. This is why all of our types are pointers, so that we can differentiate between a) setting values to empty and b) not sending updates. So really, moving to a world where the API handles nil'ing out any ReadOnly fields automatically before a PUT request would be best.

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? It would mean that all of this validation could would be unnecessary.

[colemickens]

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.

[mcardosos]

I am curious, how would PATCH affect all this? And which are the best REST manners?

[colemickens]

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.

[mcardosos]

Good explanation. Thanks :D

[salameer]

colemickens can you please review the PR:

Azure/azure-sdk-for-go#526

[colemickens]

Not really, I've never read/touched/used the storage code, sorry.

I am testing the proposed fix for strict validation this afternoon and will report back soon.

[mcardosos]

I think it is the wrong link O_O
I'm sure Samer meant the validation fix.

[salameer]

This has been mitigated in GO-SDK 8.0.0-beta