Fix Azure Batch netcore header signing#3206
Conversation
|
This fixes issue #3170 |
|
Please DO NOT MERGE until I get signoff from my team members: @itowlson @darylmsft @xingwu1 @kiwidev I will respond once we are ready to merge. |
|
@matthchr please remove "Do not merge" label and add need-review, so that I know it is ready to be reviewed. |
|
@shahabhijeet Thanks, didn't realize we had those labels. In the future I can add them myself as well. I'll let you know when we're ready to merge. |
There was a problem hiding this comment.
are these methods usually named async Task XxxYyyAsync()?
There was a problem hiding this comment.
Normal convention is not to add Async suffix to tests - they are leaf methods and never called by other places. I'm unsure if there is other prior art in this project though.
There was a problem hiding this comment.
+1 - the Async suffix is a hint to human callers of a method, which is not an issue for tests
There was a problem hiding this comment.
The tests in these projects don't mention Async (even if they are async). Leaving this as is.
kiwidev
left a comment
kiwidev
left a comment
There was a problem hiding this comment.
I think the logic is ok, comments are related to the tests.
There was a problem hiding this comment.
Do we need to iterate through ports until we find a free one?
There was a problem hiding this comment.
While these tests are good, they test an aspect of the underlying behavior, but don't seem to actually test our shared key algorithm. What we're wanting to do is enforce that the StringToSign that we computed (made up of headers etc) matches the StringToSign that would be computed by the server (based on the headers sent).
So we'd either need a way of extracting the StringToSign from our BatchSharedKeyCredential (do we expose this?) or else compute the hash again on the server side and validate that they match.
I think we should keep these tests too as a way of exploring the underlying functionality / confirming that different platforms don't change behavior, however the "SharedKeyAuth" doesn't seem to belong inside this test as it isn't being asserted against.
There was a problem hiding this comment.
Agree with basically all you said.
So we'd either need a way of extracting the StringToSign from our BatchSharedKeyCredential (do we expose this?) or else compute the hash again on the server side and validate that they match.
Yes, I have another item to enable the end to end integration tests to run in both netcore and netframework, but in order to do that there is more work required. So instead of doing that for this issue (since it's customer impacting), I added a targeted unit test. We will circle back around and enable the end to end tests to run against both, at which point we should get the validation we seek (since the E2E tests use all the different verbs)
There was a problem hiding this comment.
I'll fix the name of the test class/test to more clearly describe what it is testing
There was a problem hiding this comment.
Daryl brought this up too... even though we don't use Options anywhere internally, I guess I'll add it.
There was a problem hiding this comment.
I'd still prefer that the #if FullNetFx was outside of the if statement so we're not generating an empty if for dotNetCore.
There was a problem hiding this comment.
Alright - I've made that change
There was a problem hiding this comment.
Test name does not describe what is being asserted/tested. Reading the code this one seems to contain a number of assertions, not all of which relate to content length; and the content length assertions vary from HTTP method to HTTP method. So I'm not sure what to suggest as a better name except that it should be a statement which can be true or false. (You might want to consider breaking out the different HTTP methods so you could have more specific test names for each, e.g. HttpDelete_WithSharedKeySigning_DoesNotIncludeContentLength.)
There was a problem hiding this comment.
The issue with a line in the sand name like "DoesNot" or "Does" with respect to including content length is that the behavior varies across target framework, so unless I conditionally compile the whole test out, it's going to be wrong for one of the frameworks when it comes to DELETE
I could add a bunch of tests clearly spelling out each case (either does or does not), but they would all just call into a common helper function, which is why I just used Theory instead.
I've changed this to be: HttpClient_IncludesContentLengthHeaderOnExpectedHttpVerbs. Obviously what is "Expected" is still nebulous (you have to read the test code)...
Let me know if you don't think this is sufficient.
There was a problem hiding this comment.
This function name doesn't mean a whole lot to me. 'Lambda' seems to describe more its intended usage than its actual semantics...?
There was a problem hiding this comment.
Putting this long local function in the middle of test impacts readability. Consider restructuring to keep the test readable top-to-bottom.
There was a problem hiding this comment.
Is HttpMethod an enum? If so consider using a switch instead of a series of if-clauses.
- This is due to a change in HttpClient where DELETE no longer
always includes a Content-Length 0 header. This is a change from
net452 where DELETE always has that header.
- Also includes a test for this behavior.
matthchr
force-pushed
the
bugfix/fix-netcore-headersigning
branch
from
454ac79 to
dcdb02c
Compare
|
@shahabhijeet - Okay, this needs review from an SDK team member and merge now. |
6 participants
Description
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.