Azure / azure-storage-cpp

Microsoft Azure Storage Client Library for C++
http://azure.github.io/azure-storage-cpp
Apache License 2.0
132 stars 147 forks source link

Add thread safe public set_storage_credentials to cloud_client #353

Closed will-tong closed 4 years ago

will-tong commented 4 years ago

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

will-tong commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

/azp run

azure-pipelines[bot] commented 4 years ago
No pipelines are associated with this pull request.
Jinming-Hu commented 4 years ago

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

will-tong commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

/azp run

azure-pipelines[bot] commented 4 years ago
No pipelines are associated with this pull request.
Jinming-Hu commented 4 years ago

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

Jinming-Hu commented 4 years ago

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

will-tong commented 4 years ago

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 commented 4 years ago

/azp run

azure-pipelines[bot] commented 4 years ago
No pipelines are associated with this pull request.
azure-pipelines[bot] commented 4 years ago
Commenter does not have sufficient privileges for PR 353 in repo Azure/azure-storage-cpp
Jinming-Hu commented 4 years ago

/azp run

azure-pipelines[bot] commented 4 years ago
No pipelines are associated with this pull request.