Add thread safe public set_storage_credentials to cloud_client

[will-tong]

This is useful/needed to swap between primary and secondary shared access keys without recreating the client, in conjunction with retries and retry handling in azure::storage::operation_context::set_response_received. Currently, access key expiry during operation would result in need to recreate client.

Adding thread safety on top of PR #303

[scharan]

lgtm from client interface perspective; please get it reviewed by the SDK team.

[Jinming-Hu]

Not thread safe.

In the first place, why do you want to change the credential? Do you want to refresh the bearer token? If so, it shouldn't be done in this way.

[will-tong]

We have a service that talks to BlobStore using wasstorage.dll. Following some initial checks, the service creates a storage client object and setup a bunch of parameters on this client object, which we then use to talk to wasstorage.

When creating the client, we use account keys. The case we are specifically trying to address here is the scenario when the key is rotated. We rely on the retry mechanism etc., that are built into the wasstorage library, so this is an attempt to reuse the same client and switch its credentials and throw azure::storage::storage_exception(reason, true /*retryable*/); so that the wasstorage retry mechanism will do the trick.

Adding retries at the top level would be expensive in terms of new code and is wasteful to throw away a fully setup client. The capability to switch the client credentials would simplify the service code.

[will-tong]

Jinming suggested adding a setter to account_key inside service_credential, instead of recreating service_credential with a new account key. As such, I used a shared pointer similar to the existing bearer token credentials that allow for a threadsafe update.

[Jinming-Hu]

Jinming suggested adding a setter to account_key inside service_credential, instead of recreating service_credential with a new account key. As such, I used a shared pointer similar to the existing bearer token credentials that allow for a threadsafe update.

@will-tong Thank you very much for your contribution, we'll take some time to review it.

[Jinming-Hu]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[Jinming-Hu]

request some small changes, see comments.

[Jinming-Hu]

@will-tong Can you please rebase this PR on top of dev branch to get CI passed?

[will-tong]

Changes were made. Note that method is_account_key() is a breaking change in this instance since empty() is no longer valid on an account_key, this is seen in the cloud_storage_account_test file change.

[Jinming-Hu]

Changes were made. Note that method is_account_key() is a breaking change in this instance since empty() is no longer valid on an account_key, this is seen in the cloud_storage_account_test file change.

I think this is fine. people aren't supposed to call account_key() if it's empty.

[Jinming-Hu]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[Jinming-Hu]

Seems there's a test case failed. I'll look into it when i'm available.

[Jinming-Hu]

@will-tong The way you rebased is wrong. My commit is listed in this PR. This will lead to conflicts when merging.

[will-tong]

Sorry for the rebasing issue, I ended up just doing a force push from my forked dev to forked master with the proper commits, so things seem to be properly mergeable now.

[Jinming-Hu]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

Commenter does not have sufficient privileges for PR 353 in repo Azure/azure-storage-cpp

[Jinming-Hu]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.