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 userAssignedIdentity to AAD group #858

Closed

Conversation

TheRSP
Copy link
Contributor

@TheRSP TheRSP commented Dec 22, 2021

This PR closes #857

The changes in this PR are as follows:

  • Added add_to_ad_group(s) operators to userAssignedIdentity builder
  • Added IPostDeploy action to userAssignedIdentity

I have read the contributing guidelines and have completed the following:

  • Tested my code end-to-end against a live Azure subscription.
  • Updated the documentation in the docs folder for the affected changes.
  • Written unit tests against the modified code that I have made.
  • Updated the release notes with a new entry for this PR.
  • Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

If I haven't completed any of the tasks above, I include the reasons why here:

  • I can't find any other PostDeploy tests and am not sure how to unit test it due to the dependency on Az.

Below is a minimal example configuration that includes the new features, which can be used to deploy to Azure:

    userAssignedIdentity {
        name "my-test-identity"
        add_to_ad_group "my-apps" // This AAD group must exist before the deployment is run
    }

@TheRSP
Copy link
Contributor Author

TheRSP commented Dec 22, 2021

Manual test results using userassignedidentity.fsx by modifying the add_to_group line

(Control) No groups ``` Compatible version of Azure CLI 2.29.1 detected Checking Azure CLI logged in status... you are already logged in, nothing to do. Using subscription '***' (***). Creating resource group aad-identity-group (1/1)... Deploying ARM template (please be patient, this can take a while)... All done, now parsing ARM response to get any outputs... Press any key to continue . . . ```
Add to group ``` Compatible version of Azure CLI 2.29.1 detected Checking Azure CLI logged in status... you are already logged in, nothing to do. Using subscription '***' (***). Creating resource group aad-identity-group (1/1)... Deploying ARM template (please be patient, this can take a while)... Getting object id for identity 'my-test-identity' Adding identity 'my-test-identity' (***) to group '***'...OK: my-test-identity added to group '***' All done, now parsing ARM response to get any outputs... Press any key to continue . . . ```
Add to group already containing member ``` Compatible version of Azure CLI 2.29.1 detected Checking Azure CLI logged in status... you are already logged in, nothing to do. Using subscription '***' (***). Creating resource group aad-identity-group (1/1)... Deploying ARM template (please be patient, this can take a while)... Getting object id for identity 'my-test-identity' Adding identity 'my-test-identity' (***) to group '***'...OK: my-test-identity already in group '***' All done, now parsing ARM response to get any outputs... Press any key to continue . . . ```
Non-existent AAD group ``` Compatible version of Azure CLI 2.29.1 detected Checking Azure CLI logged in status... you are already logged in, nothing to do. Using subscription '***' (***). Creating resource group aad-identity-group (1/1)... Deploying ARM template (please be patient, this can take a while)... Getting object id for identity 'my-test-identity' Adding identity 'my-test-identity' (***) to group 'my-aad-group'...Farmer.FarmerException: ERROR: No group matches the name of 'my-aad-group'

at Farmer.Exceptions.raiseFarmer[a](String msg) in C:\dev\farmer\src\Farmer\Result.fs:line 7
at Farmer.Deploy.execute(String resourceGroupName, FSharpList`1 parameters, IDeploymentSource deployment) in C:\dev\farmer\src\Farmer\Deploy.fs:line 285 at <StartupCode$FSI_0001>.$FSI_0001.main@()
Stopped due to error

</details>

@ninjarobot
Copy link
Collaborator

Is there some way to assign these groups with an ARM deployment?

@TheRSP
Copy link
Contributor Author

TheRSP commented Dec 22, 2021 via email

@ninjarobot
Copy link
Collaborator

Not that I could find. There is also this question which implies it's not

possible:

https://docs.microsoft.com/en-us/answers/questions/214227/can-we-add-users-group-azure-ad-using-arm-template.html

We shouldn't put it in the same builder, then. There is no way to tell that this isn't going to be in the resulting template.

@ninjarobot
Copy link
Collaborator

Maybe this should be a CLI wrapper only and go in the AzHelpers module. What do you think @isaacabraham? Most of the CLI wrappers are specifically for ARM deployments and this is starting to become more general purpose Azure CLI, so I'm not sure where it goes.

@TheRSP
Copy link
Contributor Author

TheRSP commented Dec 23, 2021

I thought the whole point of PostDeploy actions was to do things exactly like this from the builder? Similar to webapp's zip_deploy or storageAccount's use_static_website. Is the plan not to control PostDeploy tasks on the builders anymore?

Or is the concern the location of the logic rather than the addition of an operator in the builder?

@ninjarobot
Copy link
Collaborator

Those two are rolling out your application code to those resources.

@isaacabraham
Copy link
Member

I'm in two minds here. I'm not specifically opposed to this as a post-deploy action personally - it's not the end of the world if some bits go into the CLI as well as ARM - as long as everything that can go into ARM, does. Then again, for the AD side of things, we already have some CLI helpers - could this not be one of them?

I'm not sure I have the answer here. I think maybe it's about the declarative nature of "I want to create this identity, it's part of these groups" as a declarative statement - in which case this might be fine. This is the approach we took for the upload app stuff.

@ninjarobot
Copy link
Collaborator

It may make sense to have some CLI wrappers around AD group assignments, but I don't think it's a good idea to tie them to user assigned identities. You have to have much greater permissions to manage AD group assignments than to simply create an identity for a resource, so this would be a deployment that would partially fail in a lot of cases. Also you can't really even do them through ARM deployments, and this limitation isn't really clear to a user that's creating the resource.

@TheRSP
Copy link
Contributor Author

TheRSP commented Jan 5, 2022

I was hoping to achieve the declarative statement as mentioned by isaac but I do see that this would be confusing for users who only use Farmer as an ARM generator (rather than a deployment tool).
If we were to make this an Az Helper, I was wondering if adding a keyword such as post_deploy might be acceptable to allow us to maintain this declarative nature without the confusion? Maybe something like the following?

userAssignedIdentity {
    name "my-test-identity"
    post_deploy (AzHelpers.addIdentityToActiveDirectoryGroup "my-apps")
}

I'm assuming that post_deploy will take a UserAssignedIdentityConfig -> unit so that the called function would still have access to the configured identity object.

@martinbryant
Copy link
Contributor

Hi @TheRSP - can we look at getting the conflicts dealt with? Thanks

@TheRSP TheRSP closed this Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Add user-assigned-identity to AAD group
4 participants