[Identity] Hotfix 1.5.1: IMDS ping fixes #16878
Conversation
sadasant
requested review from
KarishmaGhiya,
chlowell,
schaabs and
ramya-rao-a
ghost
added
the
sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts
Outdated
Show resolved
Hide resolved
| clientId?: string, | ||
| options?: { | ||
| skipQuery?: boolean; | ||
| skipMetadataHeader?: boolean; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 skipDetails? It feels so constrained to some current vague definition. So, I went with being extra verbose on what’s the intention of this last optional parameter, in a way that can help us add more configuration later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
May be you can highlight some where in the ref-docs for the user when to use these skip-options and in what combinations?
There was a problem hiding this comment.
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).
sadasant
requested review from
danieljurek,
mikeharder,
mitchdenny,
praveenkuttappan and
weshaggard
as code owners
sadasant
force-pushed
the
hotfix/identity_1.5.1-changes
branch
from
aad1014 to
e980b6a
Compare
|
Tested on the k8s test environment that was presenting the issue. It works! Although there’s still some postmortem to do, this finally concludes with our investigations 😊 |
Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
|
Update: Took a bit of time to test this on other IMDS endpoints. Things are working as usual there, as far as I could tell. |
Review request for Microsoft.ContainerService to add version preview/2021-11-01-preview (Azure#17104) * Adds base for updating Microsoft.ContainerService from version stable/2021-10-01 to version 2021-11-01-preview * Updates readme * Updates API version in new specs and examples * feat: define OIDC issuer profile (Azure#16834) * feat: define OIDC issuer profile * fix: add `OIDC` to custom-wrods.txt * doc: update description * style: fix style check * fix: add type * add enableNamespaceResources in Managed Cluster (Azure#16835) * add enableNamespaceResources in Managed Cluster * update example for enableNamespaceResources * changed description of enableNamespaceresources * change description for enableNamespaceResources * feat: define currentKubernetesVersion (Azure#16878) Add property currentKubernetesVersion for feature AliasMinorVersion * add message of the day for Linux nodes (Azure#16942) * [AKS] feat: add python SDK generation for 1001/1101-preview api (Azure#16895) * feat: add python SDK generation for 1001/1101-preview api * fix: missing tag * fix: update path * fix: update path * fix: typo * feat: update readme.python.md * Rebased from main to dev branch (Azure#17081) Co-authored-by: Anumita Shenoy <anumitashenoy@gmail.com> Co-authored-by: Jianping Zeng <zjpjack@users.noreply.github.com> Co-authored-by: Ace Eldeib <alexeldeib@gmail.com> Co-authored-by: Tong Chen <45081443+tonche@users.noreply.github.com>
This PR is intended to release Identity hotfix 1.5.1, which:
Metadataheader, we skip the URL query. Both will ensure that all the known IMDS endpoints return as early as possible.AZURE_POD_IDENTITY_AUTHORITY_HOSTenvironment variable. If present, the IMDS endpoint initial verification will be skipped.It took us a while to figure out why sometimes IMDS pings couldn’t be fulfilled on time. While it was under our assumptions that requests to the IMDS endpoint would immediately fail if they were sent without a
Metadataheader, turns out that this is not the case: Some IMDS endpoints, like the one used by the Pod Identity service, won’t necessarily fail if this header is missing. More generally, these endpoints will fail if both this header and the query parameters are missing.The environment variable seems like a reasonable plus to include.
Feedback always appreciated!