-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[Identity] Hotfix 1.5.1: IMDS ping fixes #16878
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
Changes from all commits
b829447
020bcbe
e980b6a
3848c5b
004618f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,14 @@ import { | |
| createHttpHeaders, | ||
| PipelineRequestOptions, | ||
| createPipelineRequest, | ||
| RawHttpHeaders, | ||
| RestError | ||
| } from "@azure/core-rest-pipeline"; | ||
| import { SpanStatusCode } from "@azure/core-tracing"; | ||
| import { IdentityClient } from "../../client/identityClient"; | ||
| import { credentialLogger } from "../../util/logging"; | ||
| import { createSpan } from "../../util/tracing"; | ||
| import { imdsApiVersion, imdsEndpoint } from "./constants"; | ||
| import { imdsApiVersion, imdsHost, imdsEndpointPath } from "./constants"; | ||
| import { MSI } from "./models"; | ||
| import { msiGenericGetToken } from "./utils"; | ||
|
|
||
|
|
@@ -33,7 +34,14 @@ function expiresInParser(requestBody: any): number { | |
| } | ||
| } | ||
|
|
||
| function prepareRequestOptions(resource?: string, clientId?: string): PipelineRequestOptions { | ||
| function prepareRequestOptions( | ||
| resource?: string, | ||
| clientId?: string, | ||
| options?: { | ||
| skipQuery?: boolean; | ||
| skipMetadataHeader?: boolean; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows to skip both or only one of the two. From my understanding of the PR description, we are skipping both to verify the endpoint. We keep both when getting the token. What is the scenario where we would need to skip only one of the two?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now we don’t have a scenario in which we might want to skip only one, but it felt worse for me to have some arbitrary parameter that would do more than one thing. Like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why I consider this a “concurrent definition” is because we assumed all IMDS endpoints would behave this way (rejecting requests without the Metadata header) up to this point. So, I want to make it easier for us to adapt to future findings, if they happen.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although I made this hotfix, and I’m intending to release it asap, I’m in the process of communicating this unexpected behavior to the team. Charles confirmed to me that, given that this header is not as we thought it would be, a similar change seems necessary in other languages. It could lead to something interesting. But the code change seems an improvement regardless of future inquiries.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be you can highlight some where in the ref-docs for the user when to use these skip-options and in what combinations?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not part of the public API though. This is invisible to users. Well, except when their authentication sometimes fails (current scenario in k8s). |
||
| } | ||
| ): PipelineRequestOptions { | ||
| const queryParameters: any = { | ||
| resource, | ||
| "api-version": imdsApiVersion | ||
|
|
@@ -43,15 +51,30 @@ function prepareRequestOptions(resource?: string, clientId?: string): PipelineRe | |
| queryParameters.client_id = clientId; | ||
| } | ||
|
|
||
| const query = qs.stringify(queryParameters); | ||
| const url = new URL(imdsEndpointPath, process.env.AZURE_POD_IDENTITY_AUTHORITY_HOST ?? imdsHost); | ||
|
|
||
| const { skipQuery, skipMetadataHeader } = options || {}; | ||
|
|
||
| // Pod Identity will try to process this request even if the Metadata header is missing. | ||
| // We can exclude the request query to ensure no IMDS endpoint tries to process the ping request. | ||
| let query = ""; | ||
| if (!skipQuery) { | ||
| query = `?${qs.stringify(queryParameters)}`; | ||
| } | ||
|
|
||
| const headersSource: RawHttpHeaders = { | ||
| Accept: "application/json", | ||
| Metadata: "true" | ||
| }; | ||
| // Remove the Metadata header to invoke a request error from some IMDS endpoints. | ||
| if (skipMetadataHeader) { | ||
| delete headersSource.Metadata; | ||
| } | ||
|
|
||
| return { | ||
| url: `${imdsEndpoint}?${query}`, | ||
| url: `${url}${query}`, | ||
| method: "GET", | ||
| headers: createHttpHeaders({ | ||
| Accept: "application/json", | ||
| Metadata: "true" | ||
| }) | ||
| headers: createHttpHeaders(headersSource) | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -62,29 +85,29 @@ export const imdsMsi: MSI = { | |
| clientId?: string, | ||
| getTokenOptions?: GetTokenOptions | ||
| ): Promise<boolean> { | ||
| // if the PodIdenityEndpoint environment variable was set no need to probe the endpoint, it can be assumed to exist | ||
| if (process.env.AZURE_POD_IDENTITY_AUTHORITY_HOST) { | ||
| return true; | ||
| } | ||
|
|
||
| const { span, updatedOptions: options } = createSpan( | ||
| "ManagedIdentityCredential-pingImdsEndpoint", | ||
| getTokenOptions | ||
| ); | ||
|
|
||
| const requestOptions = prepareRequestOptions(resource, clientId); | ||
|
|
||
| // This will always be populated, but let's make TypeScript happy | ||
| if (requestOptions.headers) { | ||
| // Remove the Metadata header to invoke a request error from | ||
| // IMDS endpoint | ||
| requestOptions.headers.delete("Metadata"); | ||
| } | ||
|
|
||
| requestOptions.tracingOptions = { | ||
| spanOptions: options.tracingOptions && options.tracingOptions.spanOptions, | ||
| tracingContext: options.tracingOptions && options.tracingOptions.tracingContext | ||
| }; | ||
|
|
||
| try { | ||
| // Create a request with a timeout since we expect that | ||
| // not having a "Metadata" header should cause an error to be | ||
| // returned quickly from the endpoint, proving its availability. | ||
| // Later we found that skipping the query parameters is also necessary in some cases. | ||
| const requestOptions = prepareRequestOptions(resource, clientId, { | ||
| skipMetadataHeader: true, | ||
| skipQuery: true | ||
| }); | ||
| requestOptions.tracingOptions = { | ||
| spanOptions: options.tracingOptions && options.tracingOptions.spanOptions, | ||
| tracingContext: options.tracingOptions && options.tracingOptions.tracingContext | ||
| }; | ||
| const request = createPipelineRequest(requestOptions); | ||
|
|
||
| request.timeout = options.requestOptions?.timeout ?? 300; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.