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

[Service Bus] Update constructors to add overloads and use Azure Identity #5436

Merged
merged 25 commits into from
Jan 15, 2020
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions sdk/servicebus/service-bus/karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ module.exports = function(config) {
// https://www.npmjs.com/package/karma-env-preprocessor
envPreprocessor: [
"SERVICEBUS_CONNECTION_STRING_BROWSER",
"AAD_CLIENT_ID",
"AAD_CLIENT_SECRET",
"AAD_TENANT_ID"
"AZURE_CLIENT_ID",
"AZURE_CLIENT_SECRET",
"AZURE_TENANT_ID"
],

// test results reporter to use
Expand Down
4 changes: 2 additions & 2 deletions sdk/servicebus/service-bus/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
"module": "dist-esm/src/index.js",
"browser": {
"./dist/index.js": "./browser/service-bus.js",
"./dist-esm/test/utils/aadUtils.js": "./dist-esm/test/utils/aadUtils.browser.js",
"./dist-esm/src/util/crypto.js": "./dist-esm/src/util/crypto.browser.js",
"./dist-esm/src/util/parseUrl.js": "./dist-esm/src/util/parseUrl.browser.js",
"buffer": "buffer",
Expand Down Expand Up @@ -78,7 +77,7 @@
"dependencies": {
"@azure/core-amqp": "^1.0.0",
"@azure/core-http": "^1.0.0",
"@azure/ms-rest-nodeauth": "^0.9.2",
"@azure/identity": "^1.0.0",
"@opentelemetry/types": "^0.2.0",
"@types/is-buffer": "^2.0.0",
"@types/long": "^4.0.0",
Expand All @@ -93,6 +92,7 @@
},
"devDependencies": {
"@azure/eslint-plugin-azure-sdk": "^2.0.1",
"@azure/ms-rest-nodeauth": "^0.9.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that aadutils.ts file is deleted, we don't need @azure/ms-rest-nodeauth in tests either right?

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, updated

"@microsoft/api-extractor": "^7.5.4",
"@rollup/plugin-json": "^4.0.0",
"@rollup/plugin-multi-entry": "^3.0.0",
Expand Down
3 changes: 2 additions & 1 deletion sdk/servicebus/service-bus/review/service-bus.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,9 @@ export interface ServiceBusAtomManagementClientOptions {

// @public
export class ServiceBusClient {
constructor(connectionString: string, options?: ServiceBusClientOptions);
constructor(host: string, credential: TokenCredential, options?: ServiceBusClientOptions);
close(): Promise<any>;
static createFromConnectionString(connectionString: string, options?: ServiceBusClientOptions): ServiceBusClient;
createQueueClient(queueName: string): QueueClient;
createSubscriptionClient(topicName: string, subscriptionName: string): SubscriptionClient;
createTopicClient(topicName: string): TopicClient;
Expand Down
4 changes: 2 additions & 2 deletions sdk/servicebus/service-bus/src/core/batchingReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export class BatchingReceiver extends MessageReceiver {
const sessionError = context.session && context.session.error;
let error: Error | MessagingError;
if (sessionError) {
error = translate(sessionError);
error = translate(sessionError) as MessagingError;
Copy link
Contributor

Choose a reason for hiding this comment

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

This casting is not required. Same for a few lines below.

log.error(
"[%s] 'session_close' event occurred for Receiver '%s' received an error:\n%O",
this._context.namespace.connectionId,
Expand Down Expand Up @@ -265,7 +265,7 @@ export class BatchingReceiver extends MessageReceiver {
const receiverError = context.receiver && context.receiver.error;
let error: Error | MessagingError;
if (receiverError) {
error = translate(receiverError);
error = translate(receiverError) as MessagingError;
log.error(
"[%s] Receiver '%s' received an error:\n%O",
this._context.namespace.connectionId,
Expand Down
155 changes: 54 additions & 101 deletions sdk/servicebus/service-bus/src/serviceBusClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
ConnectionConfig,
DataTransformer,
TokenCredential,
SharedKeyCredential
SharedKeyCredential,
isTokenCredential
} from "@azure/core-amqp";
import { SubscriptionClient } from "./subscriptionClient";

Expand Down Expand Up @@ -56,23 +57,66 @@ export class ServiceBusClient {
*/
private _context: ConnectionContext;

/**
* Creates a ServiceBusClient for the Service Bus Namespace represented in the given connection
* string.
* @param connectionString - Connection string of the form
* 'Endpoint=sb://my-servicebus-namespace.servicebus.windows.net/;SharedAccessKeyName=my-SA-name;SharedAccessKey=my-SA-key'
* @param options Options to control ways to interact with the
* Service Bus Namespace.
* @returns ServiceBusClient
*/
constructor(connectionString: string, options?: ServiceBusClientOptions);

/**
* Instantiates a ServiceBusClient to interact with a Service Bus Namespace.
*
* @constructor
* @param {ConnectionConfig} config - The connection configuration needed to connect to the
* Service Bus Namespace.
* @param {TokenCredential} [tokenCredential] - SharedKeyCredential object or your
* credential that implements the TokenCredential interface.
* @param {ServiceBusClientOptions} - Options to control ways to interact with the Service Bus
* @param host - The host name for the Service Bus namespace. This is likely to be similar to
* <yournamespace>.servicebus.windows.net
* @param credential - credential that implements the TokenCredential interface.
* @param options - Options to control ways to interact with the Service Bus
* Namespace.
*/
private constructor(
config: ConnectionConfig,
credential: SharedKeyCredential | TokenCredential,
constructor(host: string, credential: TokenCredential, options?: ServiceBusClientOptions);

constructor(
hostOrConnectionString: string,
credentialOrServiceBusClientOptions?: TokenCredential | ServiceBusClientOptions,
options?: ServiceBusClientOptions
) {
if (!options) options = {};
let config;
let credential;

if (!isTokenCredential(credentialOrServiceBusClientOptions)) {
// connectionString and options based constructor was invoked
config = ConnectionConfig.create(hostOrConnectionString);

options = credentialOrServiceBusClientOptions as ServiceBusClientOptions;
config.webSocket = options && options.webSocket;
config.webSocketEndpointPath = "$servicebus/websocket";
config.webSocketConstructorOptions = options && options.webSocketConstructorOptions;

// Since connectionstring was passed, create a SharedKeyCredential
credential = new SharedKeyCredential(config.sharedAccessKeyName, config.sharedAccessKey);

ConnectionConfig.validate(config);
} else {
// host, credential and options based constructor was invoked
credential = credentialOrServiceBusClientOptions as TokenCredential;

hostOrConnectionString = String(hostOrConnectionString);
if (!hostOrConnectionString.endsWith("/")) {
hostOrConnectionString += "/";
}
const connectionString = `Endpoint=sb://${hostOrConnectionString};SharedAccessKeyName=defaultKeyName;SharedAccessKey=defaultKeyValue;`;
config = ConnectionConfig.create(connectionString);
}

if (!options) {
options = {};
}

this.name = config.endpoint;
this._context = ConnectionContext.create(config, credential, options);
}
Expand Down Expand Up @@ -152,95 +196,4 @@ export class ServiceBusClient {
throw errObj;
}
}

/**
* Creates a ServiceBusClient for the Service Bus Namespace represented in the given connection
* string.
* @param {string} connectionString - Connection string of the form
* 'Endpoint=sb://my-servicebus-namespace.servicebus.windows.net/;SharedAccessKeyName=my-SA-name;SharedAccessKey=my-SA-key'
* @param {ServiceBusClientOptions} [options] Options to control ways to interact with the
* Service Bus Namespace.
* @returns {ServiceBusClient}
*/
static createFromConnectionString(
connectionString: string,
options?: ServiceBusClientOptions
): ServiceBusClient {
const config = ConnectionConfig.create(connectionString);

config.webSocket = options && options.webSocket;
config.webSocketEndpointPath = "$servicebus/websocket";
config.webSocketConstructorOptions = options && options.webSocketConstructorOptions;

// Since connectionstring was passed, create a SharedKeyCredential
const credential = new SharedKeyCredential(config.sharedAccessKeyName, config.sharedAccessKey);

ConnectionConfig.validate(config);

return new ServiceBusClient(config, credential, options);
}

// /**
// * Creates a ServiceBusClient for the Service Bus Namespace represented by the given host using
// * the given TokenProvider.
// * @param {string} host - Fully qualified domain name for Servicebus. Most likely,
// * `<yournamespace>.servicebus.windows.net`.
// * @param {TokenProvider} tokenProvider - Your custom implementation of the {@link https://github.com/Azure/amqp-common-js/blob/master/lib/auth/token.ts Token Provider}
// * interface.
// * @param {ServiceBusClientOptions} options - Options to control ways to interact with the
// * Service Bus Namespace.
// * @returns {ServiceBusClient}
// */
// static createFromTokenProvider(
// host: string,
// tokenProvider: TokenProvider,
// options?: ServiceBusClientOptions
// ): ServiceBusClient {
// host = String(host);
// if (!tokenProvider) {
// throw new TypeError('Missing parameter "tokenProvider"');
// }
// if (!host.endsWith("/")) host += "/";
// const connectionString =
// `Endpoint=sb://${host};SharedAccessKeyName=defaultKeyName;` +
// `SharedAccessKey=defaultKeyValue`;
// const config = ConnectionConfig.create(connectionString);

// config.webSocket = options && options.webSocket;
// config.webSocketEndpointPath = "$servicebus/websocket";
// config.webSocketConstructorOptions = options && options.webSocketConstructorOptions;

// ConnectionConfig.validate(config);
// return new ServiceBusClient(config, tokenProvider, options);
// }

// /**
// * Creates a ServiceBusClient for the Service Bus Namespace represented by the given host using
// * the TokenCredentials generated using the `@azure/ms-rest-nodeauth` library.
// * @param {string} host - Fully qualified domain name for ServiceBus.
// * Most likely, {yournamespace}.servicebus.windows.net
// * @param {ServiceClientCredentials} credentials - The Token credentials generated by using the
// * `@azure/ms-rest-nodeauth` library. It can be one of the following:
// * - ApplicationTokenCredentials
// * - UserTokenCredentials
// * - DeviceTokenCredentials
// * - MSITokenCredentials
// * Token audience (or resource in case of MSI based credentials) to use when creating the credentials is https://servicebus.azure.net/
// * @param {ServiceBusClientOptions} options - Options to control ways to interact with the
// * Service Bus Namespace.
// * @returns {ServiceBusClient}
// */
// static createFromAadTokenCredentials(
// host: string,
// credentials:
// | ApplicationTokenCredentials
// | UserTokenCredentials
// | DeviceTokenCredentials
// | MSITokenCredentials,
// options?: ServiceBusClientOptions
// ): ServiceBusClient {
// host = String(host);
// const tokenProvider = new AadTokenProvider(credentials);
// return ServiceBusClient.createFromTokenProvider(host, tokenProvider, options);
// }
}
69 changes: 15 additions & 54 deletions sdk/servicebus/service-bus/test/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

- Run `npm i` to install all the dependencies of this project. This is a one time task.
- The tests expect a series of queues, topics and subscriptions to exist in a single Service Bus instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

After merging the feature/service-bus-atom branch, all tests now create the entities they need. So the above statement is no longer true.

Suggested change
- The tests expect a series of queues, topics and subscriptions to exist in a single Service Bus instance.
- The tests will create the required queue, topic and subscription in the Service Bus namespace you provide using environment variables.

The connection string for the service bus should be in the environment variable `SERVICEBUS_CONNECTION_STRING`.
For Node environment, the connection string for the service bus should be in the environment variable `SERVICEBUS_CONNECTION_STRING`.

For browser environment, the connection string for the service bus should be in the environment variable `SERVICEBUS_CONNECTION_STRING_BROWSER`.

Note that the tests will empty the messages in these entities to get a clean start before running each test.
Copy link
Contributor

@ramya-rao-a ramya-rao-a Jan 15, 2020

Choose a reason for hiding this comment

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

After merging the feature/service-bus-atom branch, all tests now create the entities they need. So this note and the table following it can be removed.

Similarly the sample content for the .env file should be cleaned up as well


Expand All @@ -20,39 +22,19 @@
unpartitioned-topic | Topic with partitions disabled, meant for testing subscriptions with sessions disabled | TOPIC_NAME_NO_PARTITION
partitioned-topic-sessions | Topic with partitions enabled, meant for testing subscriptions with sessions enabled | TOPIC_NAME_SESSION
unpartitioned-topic-sessions | Topic with partitions disabled, meant for testing subscriptions with sessions enabled | TOPIC_NAME_NO_PARTITION_SESSION
partitioned-topic-subscription | Subscription with sessions disabled in the Topic, `partitioned-topic` | SUBSCRIPTION_NAME
unpartitioned-topic-subscription | Subscription with sessions disabled in the Topic, `unpartitioned-topic` | SUBSCRIPTION_NAME_NO_PARTITION
partitioned-topic-sessions-subscription | Subscription with sessions enabled in the Topic, `partitioned-topic-sessions` | SUBSCRIPTION_NAME_SESSION
unpartitioned-topic-sessions-subscription | Subscription with sessions enabled in the Topic, `unpartitioned-topic-sessions` | SUBSCRIPTION_NAME_NO_PARTITION_SESSION
partitioned-topic-subscription | Subscription with sessions disabled in the partitioned topic | SUBSCRIPTION_NAME
unpartitioned-topic-subscription | Subscription with sessions disabled in the unpartitioned topic | SUBSCRIPTION_NAME_NO_PARTITION
partitioned-topic-sessions-subscription | Subscription with sessions enabled in the sessions enabled partitioned topic | SUBSCRIPTION_NAME_SESSION
unpartitioned-topic-sessions-subscription | Subscription with sessions enabled in the sessions enabled unpartitioned topic | SUBSCRIPTION_NAME_NO_PARTITION_SESSION
topic-filter | Topic for testing topic filters | TOPIC_FILTER_NAME
topic-filter-subscription | Subscription in the Topic `topic-filter` | TOPIC_FILTER_SUBSCRIPTION_NAME
topic-filter-default-subscription | Subscription in the Topic `topic-filter` | TOPIC_FILTER_DEFAULT_SUBSCRIPTION_NAME

Following are additional environment variables that will need to be defined in order to be able to run the tests in browser

Entitiy Name | Entity Description | Environment Variable if not using default names
------------- | ------------------|----------
partitioned-queue-browser | Queue with partitions enabled and sessions disabled | QUEUE_NAME_BROWSER
unpartitioned-queue-browser | Queue with partitions and sessions, both disabled | QUEUE_NAME_NO_PARTITION_BROWSER
partitioned-queue-sessions-browser | Queue with partitions and sessions, both enabled | QUEUE_NAME_SESSION_BROWSER
unpartitioned-queue-sessions-browser | Queue with partitions disabled and sessions enabled | QUEUE_NAME_NO_PARTITION_SESSION_BROWSER
partitioned-topic-browser | Topic with partitions enabled, meant for testing subscriptions with sessions disabled | TOPIC_NAME_BROWSER
unpartitioned-topic-browser | Topic with partitions disabled, meant for testing subscriptions with sessions disabled | TOPIC_NAME_NO_PARTITION_BROWSER
partitioned-topic-sessions-browser | Topic with partitions enabled, meant for testing subscriptions with sessions enabled | TOPIC_NAME_SESSION_BROWSER
unpartitioned-topic-sessions-browser | Topic with partitions disabled, meant for testing subscriptions with sessions enabled | TOPIC_NAME_NO_PARTITION_SESSION_BROWSER
partitioned-topic-subscription-browser | Subscription with sessions disabled in the Topic, `partitioned-topic-browser` | SUBSCRIPTION_NAME_BROWSER
unpartitioned-topic-subscription-browser | Subscription with sessions disabled in the Topic, `unpartitioned-topic-browser` | SUBSCRIPTION_NAME_NO_PARTITION_BROWSER
partitioned-topic-sessions-subscription-browser | Subscription with sessions enabled in the Topic, `partitioned-topic-sessions-browser` | SUBSCRIPTION_NAME_SESSION_BROWSER
unpartitioned-topic-sessions-subscription-browser | Subscription with sessions enabled in the Topic, `unpartitioned-topic-sessions-browser` | SUBSCRIPTION_NAME_NO_PARTITION_SESSION_BROWSER
topic-filter-browser | Topic for testing topic filters | TOPIC_FILTER_NAME_BROWSER
topic-filter-subscription-browser | Subscription in the Topic `topic-filter-browser` | TOPIC_FILTER_SUBSCRIPTION_NAME_BROWSER
topic-filter-default-subscription-browser | Subscription in the Topic `topic-filter-browser` | TOPIC_FILTER_DEFAULT_SUBSCRIPTION_NAME_BROWSER


The environment variables can be set by adding a file by the name `.env` in the root folder of this project.
Following is a sample .env file template that can be re-used for your environment:
```
SERVICEBUS_CONNECTION_STRING=
SERVICEBUS_CONNECTION_STRING_BROWSER=

QUEUE_NAME=partitioned-queue
QUEUE_NAME_NO_PARTITION=unpartitioned-queue
Expand All @@ -72,30 +54,10 @@
TOPIC_FILTER_SUBSCRIPTION_NAME=topic-filter-subscription
TOPIC_FILTER_DEFAULT_SUBSCRIPTION_NAME=topic-filter-default-subscription

# Resources for use by browser tests
QUEUE_NAME_BROWSER=partitioned-queue-browser
QUEUE_NAME_NO_PARTITION_BROWSER=unpartitioned-queue-browser
QUEUE_NAME_SESSION_BROWSER=partitioned-sessions-queue-browser
QUEUE_NAME_NO_PARTITION_SESSION_BROWSER=unpartitioned-sessions-queue-browser

TOPIC_NAME_BROWSER=partitioned-topic-browser
TOPIC_NAME_NO_PARTITION_BROWSER=unpartitioned-topic-browser
TOPIC_NAME_SESSION_BROWSER=partitioned-sessions-topic-browser
TOPIC_NAME_NO_PARTITION_SESSION_BROWSER=unpartitioned-sessions-topic-browser

SUBSCRIPTION_NAME_BROWSER=partitioned-subscription-browser
SUBSCRIPTION_NAME_NO_PARTITION_BROWSER=unpartitioned-subscription-browser
SUBSCRIPTION_NAME_SESSION_BROWSER=partitioned-sessions-subscription-browser
SUBSCRIPTION_NAME_NO_PARTITION_SESSION_BROWSER=unpartitioned-sessions-subscription-browser

TOPIC_FILTER_NAME_BROWSER=topic-filter-browser
TOPIC_FILTER_SUBSCRIPTION_NAME_BROWSER=topic-subscription-browser
TOPIC_FILTER_DEFAULT_SUBSCRIPTION_NAME_BROWSER=topic-subscription-default-browser

```

## Setup
Go through the following setup in order to delete and create the required servicebus-entities(just before running each test). This would need authenticating to Service Bus using `AadTokenCredentials` instead of `ConnectionString`. The below setup is also needed to run the tests that specifically uses `AadTokenCredentials` to authenticate.
Go through the following setup in order to delete and create the required servicebus-entities(just before running each test). This would need authenticating to Service Bus using AAD `TokenCredential` instead of `ConnectionString`. The below setup is also needed to run the tests that specifically uses AAD `TokenCredential` to authenticate.

**Register a new application in AAD**

Expand All @@ -114,31 +76,30 @@ Go through the following setup in order to delete and create the required servic

Populate the following variables along with the above mentioned environment variables in the `.env`.
```
AAD_CLIENT_ID=""
AAD_CLIENT_SECRET=""
AAD_TENANT_ID=""
AZURE_CLIENT_ID=""
AZURE_CLIENT_SECRET=""
AZURE_TENANT_ID=""
```

**Note:**
* `RESOURCE_GROUP` and `AZURE_SUBSCRIPTION_ID` can be found at your servicebus-namespace in the azure-portal.

_By default the deletion/creation of the required servicebus-entities(for each test) will take place in order to not reuse resources across testcases._

## Run all tests

Run `npm run unit` from your terminal to run all the tests under the `./test` folder
Run `npm run test:node` or `npm run test:browser` from your terminal to run all the tests under the `./test` folder

## Run all tests in a single test suite

Append the `.only` on the `describe` method corresponding to the test suite.

Then run `npm run unit` from your terminal.
Then run `npm run test:node` or `npm run test:browser` from your terminal.

## Run a single test

Append the `.only` on the `it` method corresponding to the test.

Then run `npm run unit` from your terminal.
Then run `npm run test:node` or `npm run test:browser` from your terminal.

## Debug tests using Visual Studio Code

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ async function RunTest(
maxConcurrentCalls: number,
messages: number
): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);

const client = ns.createQueueClient(entityPath);
const receiver = client.createReceiver(ReceiveMode.receiveAndDelete);
Expand Down
2 changes: 1 addition & 1 deletion sdk/servicebus/service-bus/test/perf/service-bus/send.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ async function RunTest(
maxInflight: number,
messages: number
): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);

const client = ns.createQueueClient(entityPath);
const sender = client.createSender();
Expand Down
Loading