Correct IMDS resource ID query parameter #22650
Conversation
chlowell
requested review from
jhendrixMSFT,
RickWinter and
a team
as code owners
chlowell
requested review from
maorleger,
billwert,
ahsonkhan,
KarishmaGhiya,
schaabs,
mpodwysocki and
xiangyan99
|
Does this need a changelog entry? |
| @@ -34,14 +34,14 @@ const ( | |||
| identityServerThumbprint = "IDENTITY_SERVER_THUMBPRINT" | |||
| headerMetadata = "Metadata" | |||
| imdsEndpoint = "http://169.254.169.254/metadata/identity/oauth2/token" | |||
| miResID = "mi_res_id" | |||
There was a problem hiding this comment.
How come this is still needed (in what scenarios)?
I can't easily tell from the logic below where we'd use this one vs the MSI one.
Both seem to be wrapped around if id.idKind() == miResourceID
There was a problem hiding this comment.
We still need this for App Service. Each platform we support has a different API, so the query parameter we need to set depends on the platform. id in the line you pasted is the value of the parameter i.e. the resource ID.
| @@ -437,7 +437,7 @@ func (c *managedIdentityClient) createCloudShellAuthRequest(ctx context.Context, | |||
| log.Write(EventAuthentication, "WARNING: Cloud Shell doesn't support user-assigned managed identities") | |||
| q := request.Raw().URL.Query() | |||
| if id.idKind() == miResourceID { | |||
| q.Add(qpResID, id.String()) | |||
| q.Add(miResID, id.String()) | |||
There was a problem hiding this comment.
Most places seem to keep using the mi string, instead of msi, but the PR description is implying we should convege and only use msi.
There was a problem hiding this comment.
We need both. We should use "msi_res_id" for IMDS (and implicitly, anything resembling IMDS). For other platforms such as App Service, "mi_res_id" is correct. Edit: whoops, not according to the docs. I'll investigate App Service next 🕵️
There was a problem hiding this comment.
Well, this was interesting. The App Service doc was changed after we implemented this feature because a reader saw that other APIs use msi_res_id and concluded mi_res_id was a typo. It isn't: mi_res_id is correct and App Service ignores msi_res_id. I opened MicrosoftDocs/azure-docs#121176 to fix the doc.
IMDS observes both
msi_res_idandmi_res_idas the query parameter specifying an identity by its resource ID. We should prefermsi_res_idbecause it's the documented parameter and because Azure Container Instances, which has a different implementation based on the IMDS API, observes onlymsi_res_id.