Azure / azure-rest-api-specs

The source for REST API specifications for Microsoft Azure.
MIT License
2.68k stars 5.1k forks source link

Storage Account: `PUT` doesn't failed as expected when setting a misconfigured subnet to `.networkAcls.virtualNetworkRules` #18844

Open magodo opened 2 years ago

magodo commented 2 years ago

The creation (PUT) of storage account doesn't failed as expected when setting a subnet that didn't have service endpoint configured, to the .networkAcls.virtualNetworkRules property. The response is of status code 200, but the response body is as below:

{"status":"Failed","error":{"code":"NetworkAclsValidationFailure","message":"Validation of network acls failure: SubnetsHaveNoServiceEndpointsConfigured:Subnets acctestsubnet220427085102717465 of virtual network /subscriptions/67a9759d-d099-4aa8-8675-e6cfd669c3f4/resourceGroups/acctestRG-storage-220427085102717465/providers/Microsoft.Network/virtualNetworks/acctestvirtnet220427085102717465 do not have ServiceEndpoints for Microsoft.Storage resources configured. Add Microsoft.Storage to subnet's ServiceEndpoints collection before trying to ACL Microsoft.Storage resources to these subnets.."}}

The request identifier is as below:

ghost commented 2 years ago

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

Issue Details
The creation (`PUT`) of storage account doesn't failed as expected when setting a subnet that didn't have service endpoint configured, to the `.networkAcls.virtualNetworkRules` property. The response is of status code 200, but the response body is as below: ```json {"status":"Failed","error":{"code":"NetworkAclsValidationFailure","message":"Validation of network acls failure: SubnetsHaveNoServiceEndpointsConfigured:Subnets acctestsubnet220427085102717465 of virtual network /subscriptions/67a9759d-d099-4aa8-8675-e6cfd669c3f4/resourceGroups/acctestRG-storage-220427085102717465/providers/Microsoft.Network/virtualNetworks/acctestvirtnet220427085102717465 do not have ServiceEndpoints for Microsoft.Storage resources configured. Add Microsoft.Storage to subnet's ServiceEndpoints collection before trying to ACL Microsoft.Storage resources to these subnets.."}} ``` The request identifier is as below: - x-ms-request-id: 0bb562f4-c582-48dd-9e79-d8fd6a6663f0 - x-ms-correlation-request-id: eceeede5-f103-2ddb-17da-74945fd90256
Author: magodo
Assignees: -
Labels: `Service Attention`, `Network - Virtual Network`, `needs-triage`
Milestone: -
blueww commented 2 years ago

@magodo

I have checked with server team, and get the current server behavior is correct and aligned with Microsoft Azure REST API Guidelines

PUT storage account is a long running operation. Per Microsoft Azure REST API Guidelines , it should "returns a 200-OK response with the current state of the status monitor". And you can find the request success/Fail from the "status" in the responds.

magodo commented 2 years ago

@blueww What you described above is the behavior of the azure-asyncoperation, as is defined here, whilst the current RP is using the location header, whose flow is defined here, that it doesn't use "status".

Also see the reply from the Go SDK team: https://github.com/Azure/go-autorest/pull/705#issuecomment-1190754589

blueww commented 2 years ago

@magodo

The API for this issue is "PUT" StorageAccount, and it is Long-running operation (see swagger definition here.)

As the request is Long-running operation, it must follow up the Microsoft Azure REST API Guidelines. From this doc, you can see :

  1. For PUT: "YOU SHOULD include an Operation-Location header in the response with the absolute URL of the status monitor for the operation.", this is why we use "Location" header to return the URL to retrieve the operation status
  2. "DO support the GET method on the status monitor endpoint that returns a 200-OK response with the current state of the status monitor." So the final responds should have 200 status code.

From the doc you provide, I see “If the operation is complete, return the exact same response that would have been returned had the operation been executed synchronously.” The 2 doc seems not so aligned since for the failed scenarios, Microsoft Azure REST API Guidelines still return 200 with status = Failed. And this doc says "return the exact same response that would have been returned had the operation been executed synchronously".

@ArcturusZhang, @qiaozha, @jianyexi Do you have any idea for the 2 docs seems not so align?

magodo commented 2 years ago

I've heard from the Swagger team that the RPC repo is authoritive than the API guideline.

magodo commented 1 year ago

Ping @ArcturusZhang and @jianyexi, any update on this?