Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eujing
Copy link
Contributor

@eujing eujing commented Aug 8, 2024

  • Creates the managed identity (MI) in the specified RG. If existing, operation is idempotent.
  • Assigns the MI as a user-assigned MI to the created control VM
  • Grants the MI required roles for usage with the storage account file shares.

TODO:

  • Add support to the powershell script too.
  • Consider MI access to the results DB too (part of incoming security wave requirements).

@eujing eujing requested a review from a team as a code owner August 8, 2024 22:25
Copy link
Contributor

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?

Copy link
Contributor Author

@eujing eujing Aug 13, 2024

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)

Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-managedIdentityname $managedIdentityName
-managedIdentityName $managedIdentityName

consistent camelCasing please

--controlPlaneArmParamsFile $controlPlaneArmParamsFile \
--resourceGroupName $resourceGroupName \
--resultsDbArmParamsFile $resultsDbArmParamsFile \
--managedIdentityname $managedIdentityName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
--managedIdentityname $managedIdentityName
--managedIdentityName $managedIdentityName

[Parameter(Mandatory=$True)]
[string] $resourceGroupName,
[Parameter(Mandatory=$True)]
# Managed Identity params
[Parameter(Mandatory=$True, ParameterSetName="ByMI")]
Copy link
Contributor

@bpkroth bpkroth Sep 23, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants