Skip to content

[Storage] Support SAS Connection String - storage-queue#4372

Merged
HarshaNalluru merged 26 commits intoAzure:feature/storagefrom
HarshaNalluru:SASConnStringQueue
Aug 1, 2019
Merged

[Storage] Support SAS Connection String - storage-queue#4372
HarshaNalluru merged 26 commits intoAzure:feature/storagefrom
HarshaNalluru:SASConnStringQueue

Conversation

@HarshaNalluru
Copy link
Contributor

@HarshaNalluru HarshaNalluru commented Jul 20, 2019

Adds a new environment variable - "STORAGE_SAS_CONNECTION_STRING"

Storage service provides two types of connection strings, one with the accountname and key and the other one SAS conn. string with SAS string and endpoints.

This PR adds the SAS connection string support to the existing connection string static methods and constructors.

Handled SAS connection string in the recorder and added tests and recordings accordingly.

Changes in this PR

@HarshaNalluru HarshaNalluru self-assigned this Jul 20, 2019
@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jul 20, 2019
@HarshaNalluru HarshaNalluru changed the title [Storage] Support SAS Connection String [Storage] Support SAS Connection String - storage-queue Jul 20, 2019
@ramya-rao-a ramya-rao-a requested a review from bterlson July 21, 2019 04:20
connectionString.search("DefaultEndpointsProtocol=") !== -1 &&
connectionString.search("AccountKey=") !== -1
) {
const matchCredentials = connectionString.match(
Copy link
Member

Choose a reason for hiding this comment

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

const matchCredentials = connectionString.match [](start = 4, length = 47)

May not related to this CR about SAS connection string. But seems this kind of connection string cannot be successfully parsed:

DefaultEndpointsProtocol=https;
BlobEndpoint=[http://www.mydomain.com](http://www.mydomain.com);
AccountName=storagesample;
AccountKey=<account-key>

https://docs.microsoft.com/en-us/azure/storage/common/storage-configure-connection-string#create-a-connection-string-for-an-explicit-storage-endpoint

Copy link
Contributor Author

@HarshaNalluru HarshaNalluru Jul 30, 2019

Choose a reason for hiding this comment

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

I'm under the impression that connection string can only be of the following type.

DefaultEndpointsProtocol=[http|https];
AccountName=myAccountName;
AccountKey=myAccountKey;
EndpointSuffix=mySuffix;

And this is what the portal provides from what I know.
Keeping this in mind, we had the logic of parsing the connection strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can definitely add support to cover a wider range of connection strings.
You want me to do it in the same PR or create a new issue for it?

Copy link
Member

Choose a reason for hiding this comment

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

Both fine to me. Just what you prefer : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a new issue - #4578


let newMessage = "New Message";
let messageIdClient = new MessageIdClient(
getSASConnectionStringFromEnvironment(),
Copy link
Member

Choose a reason for hiding this comment

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

getSASConnectionStringFromEnvironment [](start = 6, length = 37)

Can we generate a SAS connection string dynamically using storage account & key in Node.js runtime?

Account Name + Account Key => SAS

So we can reuse current account_name and account_key environment variable, instead of importing a new environment variable.

SAS aims for a short term period access, and can be oudated when storage key is rotated. Also a SAS has a valid period, after that, SAS will be outdated.

In browser runtime, we can use the environment variable account_name with account_sas to generate the SAS connection string too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we can do that. I'll update.

}

return connectionString;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we generate a SAS connection string dynamically using storage account & key in Node.js runtime?

Account Name + Account Key => SAS

So we can reuse current account_name and account_key environment variable, instead of importing a new REQUIRED environment variable.

SAS aims for a short term period access, and can be oudated when storage key is rotated. Also a SAS has a valid period, after that, SAS will be outdated.

In browser runtime, we can reuse existing environment variables account_name with account_sas to generate the SAS connection string too.

Copy link
Member

@XiaoningLiu XiaoningLiu left a comment

Choose a reason for hiding this comment

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

Looks good. My major concern is about the new introduced REQUIRED environment variable for SAS connection string. Basically we can generate a SAS connection string based on existing environment variabless like account name and account key in node.js runtime, or account_name and account_sas in browsers.

Copy link
Member

@bterlson bterlson left a comment

Choose a reason for hiding this comment

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

Looks good though I agree with removing the new environment variable if possible (if it isn't, let me know).

ACCOUNT_KEY=<storage-account-key>
ACCOUNT_SAS=<shared-access-signature> No newline at end of file
ACCOUNT_SAS=<shared-access-signature>
STORAGE_CONNECTION_STRING=<storage-account-connection-string>
Copy link
Member

Choose a reason for hiding this comment

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

Where does this get referred?

Copy link
Contributor Author

@HarshaNalluru HarshaNalluru Aug 1, 2019

Choose a reason for hiding this comment

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

sample.env is just a file to show how the environment(or .env file) should look like.

STORAGE_CONNECTION_STRING environment variable is being used in some of the tests where we test the connection string static methods and constructors. [Unrelated to this PR]

@HarshaNalluru HarshaNalluru merged commit 3666c3a into Azure:feature/storage Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants