-
Notifications
You must be signed in to change notification settings - Fork 385
Bearer Requests should Fallback to IMDS in Preview #5562
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
Conversation
| [DataTestMethod] | ||
| [DataRow(UserAssignedIdentityId.None, null)] | ||
| [DataRow(UserAssignedIdentityId.ClientId, TestConstants.ClientId)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with all other test, can you also add cases for ResourceId and ObjectId?
| using (new EnvVariableContext()) | ||
| using (var httpManager = new MockHttpManager()) | ||
| { | ||
| ManagedIdentityClient.ResetSourceForTest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have something that runs before each test? Shouldn't this be put there?
Robbie-Microsoft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this functionality temporary? If so, can you add comments in the code stating that this will be reversed when this hits GA?
| if ((source == ManagedIdentitySource.DefaultToImds) && isMtlsPopRequested) | ||
| // If the source is determined to be ImdsV1 and mTLS PoP was requested, | ||
| // throw an exception since ImdsV1 does not support mTLS PoP | ||
| if (source == ManagedIdentitySource.DefaultToImds && isMtlsPopRequested) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be elif? You want both if statements to run?
Fixes - fallback to IMDS in Preview
We already fallback to IMDS in PROD. But when the MSI v2 endpoints exist, we do not fallback.
Changes proposed in this request
Testing
unit, integration and e2e
Performance impact
none
Documentation