Skip to content

[Identity] [Hotfix] 1.4.0 increased the IMDS MSI default timeout #16055

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.

Sign up for GitHub

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

Merged
sadasant merged 8 commits into from
Jul 9, 2021
Merged

[Identity] [Hotfix] 1.4.0 increased the IMDS MSI default timeout #16055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5260452
[Identity] [Hotfix] 1.3.1 increased the IMDS MSI default timeout
sadasant Jun 28, 2021
9c287b3
pnpm-lock, formatting and fixing tests
sadasant Jul 7, 2021
0c49e9d
Apply suggestions from code review
sadasant Jul 8, 2021
83b727c
Feedback from Ramya
sadasant Jul 8, 2021
70a8dcf
grammar on changelog
sadasant Jul 8, 2021
89b5d42
v1.4.0
sadasant Jul 8, 2021
72a3bfc
upgraded to 1.0.0-preview.12
sadasant Jul 8, 2021
254eec3
removed core-http 2.0.0 from common-versions
sadasant Jul 9, 2021
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
51 changes: 22 additions & 29 deletions common/config/rush/pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions sdk/identity/identity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Release History

## 1.4.0 (2021-07-09)

- With this release, we drop support for Node.js versions that have reached the end of life, like Node.js 8. Read our [support policy](https://github.com/Azure/azure-sdk-for-js/blob/main/SUPPORT.md) for more details.
- Updated the default timeout of the first request of the IMDS MSI from half a second to three seconds to compensate for the slowness caused by `node-fetch` for initial requests in specific environments, like Kubernetes pods.
- Upgraded `@azure/core-http` to version `^2.0.0`, and `@azure/core-tracing` to version `1.0.0-preview.12`.

## 1.3.0 (2021-04-05)

### Breaking Changes
Expand Down
6 changes: 6 additions & 0 deletions sdk/identity/identity/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ You can find examples for these various credentials in [Azure Identity Examples

## Getting started

### Currently supported environments

- [LTS versions of Node.js](https://nodejs.org/about/releases/)
- Latest versions of Safari, Chrome, Edge, and Firefox.
- Note: Among the different credentials exported in this library, `InteractiveBrowserCredential` is the only one that is supported in the browser.

### Install the package

Install Azure Identity with `npm`:
Expand Down
11 changes: 4 additions & 7 deletions sdk/identity/identity/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@azure/identity",
"sdk-type": "client",
"version": "1.3.0",
"version": "1.4.0",
"description": "Provides credential implementations for Azure SDK libraries that can authenticate with Azure Active Directory",
"main": "dist/index.js",
"module": "dist-esm/src/index.js",
Expand Down Expand Up @@ -55,7 +55,7 @@
"LICENSE"
],
"engines": {
"node": ">=8.0.0"
"node": ">=12.0.0"
},
"repository": "github:Azure/azure-sdk-for-js",
"keywords": [
Expand All @@ -74,14 +74,11 @@
"bugs": {
"url": "https://github.com/Azure/azure-sdk-for-js/issues"
},
"engine": {
"node": ">=8.0.0"
},
"homepage": "https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/identity/identity/README.md",
"sideEffects": false,
"dependencies": {
"@azure/core-http": "^1.2.4",
"@azure/core-tracing": "1.0.0-preview.11",
"@azure/core-http": "^2.0.0",
Copy link
Member

@maorleger maorleger Jun 28, 2021

Choose a reason for hiding this comment

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

Do you have to update the version of core-http? Seems like an unnecessary risk for a hotfix unless we have to take it

Copy link
Contributor

@ramya-rao-a ramya-rao-a Jun 29, 2021

Choose a reason for hiding this comment

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

I think its because he needs the fix in #15906

Copy link
Contributor

@ramya-rao-a ramya-rao-a Jun 29, 2021

Choose a reason for hiding this comment

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

On that note, should this hotfix branch take the update to core-tracing as well?

Copy link
Contributor

@ramya-rao-a ramya-rao-a Jun 29, 2021

Choose a reason for hiding this comment

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

Nah, we can do that when core-tracing goes GA.

Copy link
Member

@maorleger maorleger Jun 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I still don't think this is the right thing to do. I worry about surprising someone with a whole host of changes unintentionally especially a major version change in core-http. Instead (and I know it's more painful, so sorry!), I would recommend releasing a hotfix to core-http with just the fix you need, then using that as the core-http version to release the hotfix to identity

Copy link
Contributor Author

@sadasant sadasant Jul 8, 2021

Choose a reason for hiding this comment

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

After investigating about the new types, doing some simple tests and discussing this with @chradek , it seems safe for us to upgrade to core-http 2.0.0 on this Identity release.

Copy link
Contributor

@chradek chradek Jul 8, 2021

Choose a reason for hiding this comment

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

The reason I believe it is safe is because

  • @azure/identity only re-exports PipelineOptions for @azure/core-http
  • PipelineOptions doesn't include any enums (doesn't appear to include them directly or transitively)
  • Other than transpiling core-http to ES2017, there weren't any breaking changes to the interfaces.
  • We're bumping identity to 1.4.0 because we're dropping node 8 support, which means ES2017 should be safe to use.

@maorleger is there anything in particular that worries you? Any testing we could do to assuage your concerns?

Copy link
Member

@maorleger maorleger Jul 8, 2021

Choose a reason for hiding this comment

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

I'm fine with going to 1.4.0 with this, but was concerned when going to 1.3.1 👍 thanks for verifying the changes are safe

"@azure/core-tracing": "1.0.0-preview.12",
"@azure/logger": "^1.0.0",
"@azure/msal-node": "1.0.0-beta.6",
"@types/stoppable": "^1.1.0",
Expand Down
5 changes: 4 additions & 1 deletion sdk/identity/identity/src/client/identityClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ export class IdentityClient extends ServiceClient implements INetworkModule {
`IdentityClient: refreshing access token with client ID: ${clientId}, scopes: ${scopes} started`
);

const { span, updatedOptions: newOptions } = createSpan("IdentityClient-refreshAccessToken", options);
const { span, updatedOptions: newOptions } = createSpan(
"IdentityClient-refreshAccessToken",
options
);
Copy link
Contributor Author

@sadasant sadasant Jul 8, 2021

Choose a reason for hiding this comment

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

I ran rushx format and it changed a bunch of files. I rather have these formatting changes than not have them. It will make it easier to do further hotfixes if necessary.


const refreshParams = {
grant_type: "refresh_token",
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/src/credentials/authorizationCodeCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ export class AuthorizationCodeCredential implements TokenCredential {
},
abortSignal: options && options.abortSignal,
spanOptions: newOptions.tracingOptions && newOptions.tracingOptions.spanOptions,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext
});

tokenResponse = await this.identityClient.sendTokenRequest(webResource);
Expand Down
5 changes: 4 additions & 1 deletion sdk/identity/identity/src/credentials/chainedTokenCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ export class ChainedTokenCredential implements TokenCredential {
let token = null;
const errors = [];

const { span, updatedOptions: newOptions } = createSpan("ChainedTokenCredential-getToken", options);
const { span, updatedOptions: newOptions } = createSpan(
"ChainedTokenCredential-getToken",
options
);

for (let i = 0; i < this._sources.length && token === null; i++) {
try {
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/src/credentials/clientCertificateCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ export class ClientCertificateCredential implements TokenCredential {
},
abortSignal: options && options.abortSignal,
spanOptions: newOptions.tracingOptions && newOptions.tracingOptions.spanOptions,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext
});

const tokenResponse = await this.identityClient.sendTokenRequest(webResource);
Expand Down
7 changes: 5 additions & 2 deletions sdk/identity/identity/src/credentials/clientSecretCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ export class ClientSecretCredential implements TokenCredential {
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
const { span, updatedOptions: newOptions } = createSpan("ClientSecretCredential-getToken", options);
const { span, updatedOptions: newOptions } = createSpan(
"ClientSecretCredential-getToken",
options
);
try {
const urlSuffix = getIdentityTokenEndpointSuffix(this.tenantId);
const webResource = this.identityClient.createWebResource({
Expand All @@ -82,7 +85,7 @@ export class ClientSecretCredential implements TokenCredential {
},
abortSignal: options && options.abortSignal,
spanOptions: newOptions.tracingOptions && newOptions.tracingOptions.spanOptions,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext
});

const tokenResponse = await this.identityClient.sendTokenRequest(webResource);
Expand Down
10 changes: 5 additions & 5 deletions sdk/identity/identity/src/credentials/environmentCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ import { AccessToken, TokenCredential, GetTokenOptions } from "@azure/core-http"
import { TokenCredentialOptions } from "../client/identityClient";
import { ClientSecretCredential } from "./clientSecretCredential";
import { createSpan } from "../util/tracing";
import {
AuthenticationError,
CredentialUnavailable
} from "../client/errors";
import { AuthenticationError, CredentialUnavailable } from "../client/errors";
import { SpanStatusCode } from "@azure/core-tracing";
import { ClientCertificateCredential } from "./clientCertificateCredential";
import { UsernamePasswordCredential } from "./usernamePasswordCredential";
Expand Down Expand Up @@ -121,7 +118,10 @@ export class EnvironmentCredential implements TokenCredential {
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
const { span, updatedOptions: newOptions } = createSpan("EnvironmentCredential-getToken", options);
const { span, updatedOptions: newOptions } = createSpan(
"EnvironmentCredential-getToken",
options
);
if (this._credential) {
try {
const result = await this._credential.getToken(scopes, newOptions);
Expand Down
3 changes: 2 additions & 1 deletion sdk/identity/identity/src/credentials/managedIdentityCredential/arcMsi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ export const arcMsi: MSI = {
deserializationMapper: undefined,
abortSignal: getTokenOptions.abortSignal,
spanOptions: getTokenOptions.tracingOptions && getTokenOptions.tracingOptions.spanOptions,
tracingContext: getTokenOptions.tracingOptions && getTokenOptions.tracingOptions.tracingContext,
tracingContext:
getTokenOptions.tracingOptions && getTokenOptions.tracingOptions.tracingContext,
...prepareRequestOptions(resource)
};

Expand Down
6 changes: 5 additions & 1 deletion sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ export const imdsMsi: MSI = {
// not having a "Metadata" header should cause an error to be
// returned quickly from the endpoint, proving its availability.
const webResource = identityClient.createWebResource(request);
webResource.timeout = (options.requestOptions && options.requestOptions.timeout) || 500;

// In Kubernetes pods, node-fetch (used by core-http) takes longer than 2 seconds to begin sending the network request,
// So smaller timeouts will cause this credential to be immediately aborted.
// This won't be a problem once we move Identity to core-rest-pipeline.
webResource.timeout = (options.requestOptions && options.requestOptions.timeout) || 3000;

try {
logger.info(`Pinging IMDS endpoint`);
Expand Down
10 changes: 5 additions & 5 deletions sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
import { AccessToken, GetTokenOptions, TokenCredential } from "@azure/core-http";
import { IdentityClient, TokenCredentialOptions } from "../../client/identityClient";
import { createSpan } from "../../util/tracing";
import {
AuthenticationError,
CredentialUnavailable
} from "../../client/errors";
import { AuthenticationError, CredentialUnavailable } from "../../client/errors";
import { SpanStatusCode } from "@azure/core-tracing";
import { credentialLogger, formatSuccess, formatError } from "../../util/logging";
import { mapScopesToResource } from "./utils";
Expand Down Expand Up @@ -133,7 +130,10 @@ export class ManagedIdentityCredential implements TokenCredential {
): Promise<AccessToken | null> {
let result: AccessToken | null = null;

const { span, updatedOptions: newOptions } = createSpan("ManagedIdentityCredential-getToken", options);
const { span, updatedOptions: newOptions } = createSpan(
"ManagedIdentityCredential-getToken",
options
);

try {
// isEndpointAvailable can be true, false, or null,
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/src/credentials/usernamePasswordCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class UsernamePasswordCredential implements TokenCredential {
},
abortSignal: options && options.abortSignal,
spanOptions: newOptions.tracingOptions && newOptions.tracingOptions.spanOptions,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext,
tracingContext: newOptions.tracingOptions && newOptions.tracingOptions.tracingContext
});

const tokenResponse = await this.identityClient.sendTokenRequest(webResource);
Expand Down
Loading