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

Move credential and header policies after retry policy #14629

Merged
merged 8 commits into from
Oct 26, 2020

Conversation

tasherif-msft
Copy link
Contributor

@tasherif-msft tasherif-msft commented Oct 20, 2020

This PR resolves #14701 and resolves #14067.

Case 1:
This case is related to the StorageHeadersPolicy. In the header policy the current_time is set. This policy is currently ran before the retry policy. This means that if the retry policy was retrying and the time period has exceeded 15 min difference between the pre-established current_time it would fail as it would be outdated.

Solution Move the headers policy after RetryPolicy, i.e, current time would get reset with every retry.

Case 2:
This case is related to the StorageCredentialPolicy, moving it after the retry policy means that the TokenCredential can be refreshed if it has expired while retrying.

@ghost ghost added the Storage Storage Service (Queues, Blobs, Files) label Oct 20, 2020
@tasherif-msft
Copy link
Contributor Author

/azp run python - storage - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tasherif-msft
Copy link
Contributor Author

/azp run python - storage - tests

@tasherif-msft tasherif-msft marked this pull request as ready for review October 20, 2020 05:53
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tasherif-msft tasherif-msft changed the title Move credential policy after retry policy Move credential and header policies after retry policy Oct 20, 2020
rakshith91
rakshith91 previously approved these changes Oct 20, 2020
Copy link
Contributor

@rakshith91 rakshith91 left a comment

Choose a reason for hiding this comment

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

lgtm - can you please associate this with an issue

@kasobol-msft
Copy link
Contributor

Could you please add a test for that change?

@tasherif-msft
Copy link
Contributor Author

Could you please add a test for that change?

It's going to be a little difficult to test these changes, I think the best way to test it is running the live tests pipeline

@tasherif-msft
Copy link
Contributor Author

/azp run python - storage - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Azure Azure deleted a comment from azure-pipelines bot Oct 20, 2020
@kasobol-msft
Copy link
Contributor

kasobol-msft commented Oct 20, 2020

Could you please add a test for that change?

It's going to be a little difficult to test these changes, I think the best way to test it is running the live tests pipeline

One idea is to inject a pipeline policy that simulates 5xx error. I know we do it in .NET and Java, in Python the closest thing I recall is this

(for different purpose but I hope it gives some idea).
Then if any of our auth policies supports credential rotation then we could rotate credential and assert that retry went with new cred.

Example in .NET https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Common/tests/Shared/FaultyDownloadPipelinePolicy.cs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
3 participants