[DO NOT REVIEW] MSI V2 #5001
[DO NOT REVIEW] MSI V2 #5001
Conversation
| /// <summary> | ||
| /// Resets the cached managed identity source. Used only for testing purposes. | ||
| /// </summary> | ||
| internal static void ResetManagedIdentitySourceCache() |
There was a problem hiding this comment.
I think this will need to be made public for Azure SDK to test. And it'd be better to have a "ResetStaticCaches" general API, like our tests currently do, which reset everything.
There was a problem hiding this comment.
i.e. see the logic in TestCommon.ResetInternalStaticCaches();
| @@ -30,6 +30,9 @@ public class ServiceFabricTests | |||
| public void TestInitialize() | |||
| { | |||
| TestCommon.ResetInternalStaticCaches(); | |||
There was a problem hiding this comment.
The TestBase class already calls TestCommon.ResetInternalStaticCaches();
| @@ -37,6 +37,13 @@ public class ManagedIdentityTests : TestBase | |||
| internal const string ExpectedErrorCode = "ErrorCode"; | |||
| internal const string ExpectedCorrelationId = "Some GUID"; | |||
|
|
|||
| [TestInitialize] | |||
There was a problem hiding this comment.
See TestCommon.ResetInternalStaticCaches() - just include it it there.
|
|
||
| namespace Microsoft.Identity.Client.ManagedIdentity | ||
| { | ||
| internal interface IProbe |
There was a problem hiding this comment.
The only reason to introduce an interface is if you're going to mock it, but it doesn't seem to be the case here. You don't need to mock this object, as you can rely on HTTP mocking.
| } | ||
| } | ||
|
|
||
| internal class ProbeResult |
There was a problem hiding this comment.
Seems to be more like a general purpose class "SuccessAndErrorMessage". I would also not use a class here, but a struct, to avoid heap allocations.
| } | ||
|
|
||
| _logger.Verbose(() => "[Probe] Credential endpoint not supported."); | ||
| return ProbeResult.Failure("Credential endpoint not supported."); |
There was a problem hiding this comment.
If you're just using static messages, I don't see the point of "ProbeResult" at all.
| return ProbeResult.Success(); | ||
| } | ||
|
|
||
| _logger.Verbose(() => "[Probe] Credential endpoint not supported."); |
There was a problem hiding this comment.
You will need a lot more logging here, to find out why the probe failed.
| private const string ProbeBody = "."; | ||
| private const string ImdsHeader = "IMDS/"; | ||
| private static readonly SemaphoreSlim s_lock = new(1); | ||
| private static ProbeResult s_cachedResult; |
There was a problem hiding this comment.
We had a lot of problems with static region auto-discovery, which uses a similar concept.
- Should the HTTP call fail after some time? (it does in region ... 2 seconds)
- Are you sure that probe failure really means endpoint not available? Could the probe fail for other reasons?
- should the probe (HTTP call) be retried? How often / for how long?
- should we "default" to credential endpoint? it seems more widespread?
[DO NOT REVIEW] MSI V2