-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add managed identities support to RG setup scripts #837
base: main
Are you sure you want to change the base?
Conversation
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.
Has any of this been developed with an eye towards rectifying an existing setup?
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.
@bpkroth that is a good point. Unfortunately I have only tested this with setting up from scratch.
However, in theory I think this should work for existing resource groups.
Assuming the existing RG was set up with these scripts, I think all the az cli calls (need to confirm) are idempotent.
For example, using the same ARM template and parameters should give us back the required details of the resources without changing them too much (same naming conventions)
Then the new block of logic for managed identity should then be apply to those existing resources (VMs, storage accounts, RG role)
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.
I think we might need to add end-to-end tests to validate these finally, though maybe in a separate PR.
-certName $certName | ||
|
||
# If setting up with a Managed Identity |
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.
Can you give more context in the README about this part?
When should one use a Managed Identity? How does one set it up, etc.
-controlPlaneArmParamsFile $controlPlaneArmParamsFile ` | ||
-resourceGroupName $resourceGroupName ` | ||
-resultsDbArmParamsFile $resultsDbArmParamsFile ` | ||
-managedIdentityname $managedIdentityName |
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.
-managedIdentityname $managedIdentityName | |
-managedIdentityName $managedIdentityName |
consistent camelCasing please
--controlPlaneArmParamsFile $controlPlaneArmParamsFile \ | ||
--resourceGroupName $resourceGroupName \ | ||
--resultsDbArmParamsFile $resultsDbArmParamsFile \ | ||
--managedIdentityname $managedIdentityName |
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.
--managedIdentityname $managedIdentityName | |
--managedIdentityName $managedIdentityName |
[Parameter(Mandatory=$True)] | ||
[string] $resourceGroupName, | ||
[Parameter(Mandatory=$True)] | ||
# Managed Identity params | ||
[Parameter(Mandatory=$True, ParameterSetName="ByMI")] |
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.
Are using ServicePrincipals and ManagedIdentity mutually exclusive?
Why can't we combine those still?
And if not, can the feedback to the user be improved to note that that's not the case?
TODO: