Validate and encode query parameters

[jhendrixMSFT]

If the endpoint passed to runtime.NewRequest contains query parameters they must be validated and encoded. The typical case is when a paged operation's nextLink value contains query parameters.

Fixes #21291

[chlowell]

I get that we can ship this much faster than a codegen update, but it's odd that callers would be responsible for encoding everything except query params. Shouldn't azcore encode everything or nothing?

[jhendrixMSFT]

I was (am) struggling with that too. I agree that ideally it should be all or nothing. These are the points that swayed me to go down this path.

• I believe we don't have enough context at this point to know how properly encode the path (e.g. a / should be URL encoded within a path segment, but obviously not the ones that separate segments). Please correct me if I'm wrong on this one.
• In the presence of query params, we could split, encode, and reconstruct the endpoint. It just seems a bit redundant though given that this already happens in http.NewRequestWithContext.
• Since endpoint can accept query params, if we don't validate/encode them, the resultant request is malformed.
• Having a centralized fix is appealing, though not a requirement (in hindsight, I should have tried to move more of this logic into azcore).

Totally open to other suggestions.

[jhendrixMSFT]

In some ways, you could make the argument that this is how the net/url APIs work (although they are a bit clunky).

https://play.golang.com/p/4d17WGIC452

Here, RawQuery contains the unencoded query parameters which is strange considering the docs state that it should contain the "encoded query values" and u.RequestURI() doesn't encode them even though its docs state that "RequestURI returns the encoded path?query or opaque?query string that would be used in an HTTP request for u."

[chlowell]

If we need to support path segments containing /, I agree azcore can't encode the whole endpoint. But we could (continue to) expect callers to encode it. Considering the documented and actual behavior of NewRequest, isn't any bug here in the calling code?

[jhendrixMSFT]

You've convinced me. I will retool this such that the caller encodes the query parameters (just like we do for path segments).

[jhendrixMSFT]

I've moved this to be specific to pagers by adding a helper when creating a PagingHandler[T].Fetcher. The generated code would be updated as per below. I like this as it keeps the changes targeted to handling of next links and consolidates the fetcher so if we can centralize future updates. It also reduces the LOC for the generated fetcher by 50%.

Fetcher: func(ctx context.Context, page *PagingClientGetMultiplePagesResponse) (PagingClientGetMultiplePagesResponse, error) {
	ctx = context.WithValue(ctx, runtime.CtxAPINameKey{}, "PagingClient.NewGetMultiplePagesPager")
	nextLink := ""
	if page != nil {
		nextLink = *page.NextLink
	}
	resp, err := runtime.FetcherHelper(ctx, client.internal.Pipeline(), nextLink, func(ctx context.Context) (*policy.Request, error) {
		return client.getMultiplePagesCreateRequest(ctx, options)
	})
	if err != nil {
		return PagingClientGetMultiplePagesResponse{}, err
	}
	return client.getMultiplePagesHandleResponse(resp)
},

[jhendrixMSFT]

Unfortunately this falls apart for paged methods that use a next link operation instead of calling runtime.NewRequest. It might just be better to fix this on the codegen side.

[jhendrixMSFT]

Thinking more on the next link problem, we can relegate to solving that part via codegen and keep the consolidated helper.

[yumingqiao]

Hi @jhendrixMSFT, it seems the fix was made in azcore v1.8.0, though we still experience the same issue with azcore v1.11.1. I see the server returned nextLink was not escaped in the second page's request. In the example below. The first response is truncated for brevity, at the end, you can see it returns "nextLink", which is a unescaped URL, i.e. $filter='virtualMachineScaleSet/id' eq '/subscriptions/'. Then in the request of the next page, the SDK send the nextLink as it without escaping, as opposed to the bug fix, see Request URL: https://management.azure.com:443/subscriptions/xxxxx/resourceGroups/xxxx/providers/Microsoft.Compute/virtualMachines?api-version=2022-11-01&$skiptoken=83afce64-955b-47b3-8188-db0a7cf9c530!/Subscriptions/xxxxx/ResourceGroups/xxxx/VMs/C3D2001D933B-789C_AADFBC6A&$filter='virtualMachineScaleSet/id' eq '/subscriptions/xxxxx/resourceGroups/dev-azure-wu-nephos-0-yvjzkx/providers/Microsoft.Compute/virtualMachineScaleSets/c3d2001d933b-789c'

For the VM client, I am still using the armcompute v4.2.0, same as when the issue was first reported. I have the assumption that the bug fix is in azcore package.

Could you let us know if this is expected? We have many VMs in a resource group, if we are not able to query by VMSS, we will have to get all VMs of the entire resource group and filter by VMSS ourselves, this will consume much more API quota. Thank you.

        "diagnosticsProfile": {
          "bootDiagnostics": {
            "enabled": true
          }
        },
        "priority": "Regular",
        "timeCreated": "2024-07-19T05:05:33.4935443+00:00"
      }
    }
  ],
  "nextLink": "https://management.azure.com:443/subscriptions/xxxxx/resourceGroups/xxxx/providers/Microsoft.Compute/virtualMachines?api-version=2022-11-01&$skiptoken=83afce64-955b-47b3-8188-db0a7cf9c530!/Subscriptions/xxxxx/ResourceGroups/xxxx/VMs/C3D2001D933B-789C_AADFBC6A&$filter='virtualMachineScaleSet/id' eq '/subscriptions/xxxxx/resourceGroups/xxxx/providers/Microsoft.Compute/virtualMachineScaleSets/c3d2001d933b-789c'"
}
I0719 05:12:07.979935       1 http_client_utils.go:71] Request URL: https://management.azure.com:443/subscriptions/xxxxx/resourceGroups/xxxx/providers/Microsoft.Compute/virtualMachines?api-version=2022-11-01&$skiptoken=83afce64-955b-47b3-8188-db0a7cf9c530!/Subscriptions/xxxxx/ResourceGroups/xxxx/VMs/C3D2001D933B-789C_AADFBC6A&$filter='virtualMachineScaleSet/id' eq '/subscriptions/xxxxx/resourceGroups/dev-azure-wu-nephos-0-yvjzkx/providers/Microsoft.Compute/virtualMachineScaleSets/c3d2001d933b-789c'
I0719 05:12:07.979952       1 http_client_utils.go:72] Request Method: GET
I0719 05:12:07.979955       1 http_client_utils.go:73] Request Headers: map[User-Agent:[azsdk-go-armcompute.VirtualMachinesClient/v4.2.0 (go1.22.4 X:boringcrypto,nocoverageredesign; linux)]]
I0719 05:12:07.980568       1 http_client_utils.go:93] Response Status: 400 Bad Request
I0719 05:12:07.980581       1 http_client_utils.go:94] Response Headers: map[Content-Length:[311] Content-Type:[text/html; charset=us-ascii] Date:[Fri, 19 Jul 2024 05:12:07 GMT]]
I0719 05:12:07.980594       1 http_client_utils.go:101] Response Body: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN""http://www.w3.org/TR/html4/strict.dtd">
<HTML><HEAD><TITLE>Bad Request</TITLE>
<META HTTP-EQUIV="Content-Type" Content="text/html; charset=us-ascii"></HEAD>
<BODY><h2>Bad Request</h2>
<hr><p>HTTP Error 400. The request is badly formed.</p>
</BODY></HTML>
E0719 05:12:07.980655       1 compute_client.go:112] "Failed to list virtual machines with vmss filter via VM API." err=<
	GET https://management.azure.com:443/subscriptions/xxxxx/resourceGroups/xxxx/providers/Microsoft.Compute/virtualMachines
	--------------------------------------------------------------------------------
	RESPONSE 400: 400 Bad Request
	ERROR CODE UNAVAILABLE
	--------------------------------------------------------------------------------
	<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN""http://www.w3.org/TR/html4/strict.dtd">
	<HTML><HEAD><TITLE>Bad Request</TITLE>
	<META HTTP-EQUIV="Content-Type" Content="text/html; charset=us-ascii"></HEAD>
	<BODY><h2>Bad Request</h2>
	<hr><p>HTTP Error 400. The request is badly formed.</p>
	</BODY></HTML>
	
	--------------------------------------------------------------------------------
 >

[jhendrixMSFT]

The fix is comprised of a helper in azcore/runtime for escaping the next link URL and some changes to the generated SDKs to use it. So, it will not be enough to just update to the latest azcore to get the fix. You must also upgrade your SDKs to a later version.

[yumingqiao]

@jhendrixMSFT the latest VM client armconpute is v4.2.1, which is released in 2023 April before your fix. Do you have another version I can try?

[jhendrixMSFT]

Latest version is v5.7.0 https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5