Move NewManagedIdentityCredential's id parameter into ManagedIdentityCredentialOptions

[chlowell]

Per our last API review, this moves NewManagedIdentityCredential's id parameter into its options bag, so it isn't necessary to pass an empty string in the common case of wanting to use the hosting environment's default identity.

[jhendrixMSFT]

Is it possible for us to reliably detect that the ID is a resource ID so we don't need this option?

[chlowell]

Most of the time, yes, a client ID is a GUID and a resource ID is a URI path like "/subscriptions/...", so we could use a simple heuristic like "anything starting with /subscriptions is a resource ID" or even "anything containing / is a resource ID". However, I don't know whether this is always reliable. For example, Azure Stack diverges in that its client IDs aren't always GUIDs. I don't know whether, or how, resource and client IDs can overlap there, which makes me cautious. Perhaps too cautious though--the docs I've seen show Stack resource IDs having the same form I've seen everywhere else.

Still, if we drop the IDKind option, we may discover a case in which we can't reliably discriminate IDs by inspection, and have to add the option or something like it. I do like this idea though, I'd love to get rid of the option. How do you feel about the risk?

[jhendrixMSFT]

You bring up some good points. Do you think it's worth asking folks on the MSI side of things to see if they could give us a more definitive answer? What I'm thinking is that we can probably detect if it's a resource ID as those should have a deterministic format (I believe this format is standard for ARM). Would it be enough to say that if the string isn't a resource ID then we know it's a client ID?

The other angle, we can always add this later if needed. Taking it away post-GA is much harder.

[RickWinter]

I don't see the IDKind used in other language impls. I only see the clientID in the options.

[jhendrixMSFT]

The other languages are behind the curve here (maybe they never had the need for this). AKS requires this feature.

[JeffreyRichter]

I think it would be best if @schaabs would give us prescriptive guidance as to what to do here as he knows how this works best and what the future holds.

[schaabs]

I agree that being explicit is probably better in this area as we don't know what the universe possible client ids and resource ids are, and can't guarantee they won't overlap. Given that, I think the two options @chlowell outlined are above are probably the best options:

Option 1: Add properties ID and IDKind to the ManagedIdentityCredentialOptions as it is in this PR currently.

options := new ManagedIdentityCredentialOptions()
options.ID = "resourceid"
options.IDKind = ResourceID

Option 2: Add properties ClientID and ResourceID to the options bag and return a runtime error if both are set when the credential is constructed.

options := new ManagedIdentityCredentialOptions()
options.ResourceID = "resourceid"

Both of these have trade-offs but I might lean towards option 2, as it only requires me to set one property and we add one less "type" to the library. That being said I don't feel that strongly for one or the other.

Alternative Wacky Option 3:

We also discussed this a bit offline and came up with a 3rd slightly zany option. We could have an ID interface with a public string Value private member idtype. Then two structs ResourceID and ClientID which implement it specifying the correct id type internally. The usage of this would then look like

options := new ManagedIdentityCredentialOptions()
options.ID = new ClientID("clientid")

OR

options := new ManagedIdentityCredentialOptions()
options.ID = new ResourceID("resourceid")

I like this because it maintains the compile time consistency without having two properties I have to set on the options. However, it does introduce more types than any of the approaches so far. Also this would be something we could only do in Go, so we wouldn't be able to be consistent with other languages if we choose this option.

In the end I think this choice is really more of question of API style and convention, not so much about the workings of managed identity, so I would defer to @JeffreyRichter.

[jhendrixMSFT]

My vote is option 3, and this works quite well.

type ManagedIDKind interface {
	fmt.Stringer
	idKind() managedIdentityIDKind
}

type UserAssignedID string

func (UserAssignedID) idKind() managedIdentityIDKind {
	return miClientID
}

func (u UserAssignedID) String() string {
	return string(u)
}

type ResourceAssignedID string

func (ResourceAssignedID) idKind() managedIdentityIDKind {
	return miResourceID
}

func (r ResourceAssignedID) String() string {
	return string(r)
}

func NewManagedIdentityCredential(id ManagedIDKind, options *ManagedIdentityCredentialOptions)

[chlowell]

Works for me, though I'd still move id into the options bag because not specifying an ID will be very common. I also like Scott's option 2, having mutually exclusive ClientID and ResourceID in the options, because I don't think having both will create confusion, and I like that users would only have to set one option in all cases.

Your thoughts, @JeffreyRichter?

[JeffreyRichter]

I'm fine with option #3 and moving to the options struct.
The docs have to mention the types that implement the ManagedIDKind interface so customers have a clue as to what they can use here.