Azure / azure-cli

Azure Command-Line Interface
MIT License
4.03k stars 3.01k forks source link

SignalR CORS add/remove not atomic #17210

Closed Arithmomaniac closed 10 months ago

Arithmomaniac commented 3 years ago

https://github.com/Azure/azure-cli/blob/e015d5bcba0c2d21dc42189daa43dc1eb82d2485/src/azure-cli/azure/cli/command_modules/signalr/cors.py#L16-L35

While appreciating that the underlying API does not support atomic operations here, someone not knowing that and working on a system where CORS is programmatically enabled for new clients could potentially lose writes in a race condition. (I'm actually working on such a system now.

Suggestion: the non-atomicity should (a) be clearly documented in docs, and/or (b) the methods should be deprecated and replaced with a signalr cors set that operates on the whole list.

ghost commented 3 years ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @sffamily, @chenkennt.

Issue Details
https://github.com/Azure/azure-cli/blob/e015d5bcba0c2d21dc42189daa43dc1eb82d2485/src/azure-cli/azure/cli/command_modules/signalr/cors.py#L16-L35 While appreciating that the underlying API does not support atomic operations here, someone not knowing that and working on a system where CORS is programmatically enabled for new clients could potentially lose writes in a race condition. (I'm actually working on such a system now. Suggestion: the non-atomicity should (a) be clearly documented in docs, and/or (b) the methods should be deprecated and replaced with a `signalr cors set` that operates on the whole list.
Author: Arithmomaniac
Assignees: -
Labels: `Service Attention`, `SignalR`, `question`
Milestone: -
yonzhan commented 3 years ago

route to SignalR service team to help with.

chenkennt commented 3 years ago

@zackliu please help take a look.

zackliu commented 3 years ago

@Arithmomaniac We will add a set operation and update the document to make it clear. As cors add/remove operation are common in cli, I think we can keep it.

holtalanm commented 3 years ago

this is also not an idempotent command, which results in needing to check the list of CORs before adding.