Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queues: specifying a path parameter #4

Merged
merged 3 commits into from
Aug 21, 2019
Merged

Queues: specifying a path parameter #4

merged 3 commits into from
Aug 21, 2019

Conversation

tombuildsstuff
Copy link
Owner

@tombuildsstuff tombuildsstuff commented Aug 21, 2019

Turns out that https://github.com/Azure/go-autorest doesn't automatically specify a path of / if one isn't specified - which means the strings computed for the authentication header can be wrong - since we were producing:

GET

application/xml; charset=utf-8

x-ms-date:Wed, 21 Aug 2019 11:53:48 GMT
x-ms-version:2018-11-09
/unlikely23exst2accti1t0?comp=properties

when Azure was expecting:

GET

application/xml; charset=utf-8

x-ms-date:Wed, 21 Aug 2019 11:53:48 GMT
x-ms-version:2018-11-09
/unlikely23exst2accti1t0/?comp=properties

(note the missing /)

This PR adds a default path of / to the GetServiceProperties method in the QueuesClient so that this works as expected

This bug only affects when using a SharedKeyLite to authenticate, which is why we didn't see this before

@tombuildsstuff tombuildsstuff requested a review from katbyte August 21, 2019 12:12
@tombuildsstuff
Copy link
Owner Author

Confirmed this fixes hashicorp/terraform-provider-azurerm#3925 and enables fixing hashicorp/terraform-provider-azurerm#3939

Copy link

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me, given the limited context I have.

@tombuildsstuff tombuildsstuff merged commit f4eca0c into master Aug 21, 2019
@tombuildsstuff tombuildsstuff deleted the b/queues branch August 21, 2019 13:18
@invidian
Copy link

@tombuildsstuff thanks for fixing that, I was debugging it today and trying to fix it myself :)

Regarding fixing hashicorp/terraform-provider-azurerm#3925, there seems to be some other issue related to decoding the error message, which needs to be addressed as well I believe.

@tombuildsstuff
Copy link
Owner Author

@invidian thanks for debugging that.

With regards to parsing the error message is a bug, that's a bug in Azure/go-autorest which assumes the response is JSON rather than XML (or trying both), however since this is functional (as the error is displayed, albeit it's not overly clear) we've not gotten around to sending a PR for it just yet.

Ultimately I believe this should be fixed in hashicorp/terraform-provider-azurerm#4122 - which we're working on at the moment :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants