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

Look up existing resources by properties other than name #4917

Open
WhitWaldo opened this issue Oct 19, 2021 · 26 comments
Open

Look up existing resources by properties other than name #4917

WhitWaldo opened this issue Oct 19, 2021 · 26 comments
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered

Comments

@WhitWaldo
Copy link

Is your feature request related to a problem? Please describe.
If one attempts to deploy a role assignment that already exists, the deployment will fail instead of gracefully acknowledging that the resource already exists and skipping it. If there were an "exists" method as covered in this issue, I would still have a hard time applying it to role assignments.

A role assignment has the following properties:

  • RoleAssignmentId
  • Scope
  • DisplayName
  • SignInName
  • RoleDefinitionName
  • RoleDefinitionId
  • ObjectId
  • ObjectType
  • CanDelegate
  • Description
  • ConditionVersion
  • Condition

Unfortunately, none of those are the name property that must be used to describe an existing resource in Bicep.

Describe the solution you'd like
I'd like to have some way to describe such an assignment nonetheless using any of the other properties, as in the following:

resource assignment 'Microsoft.Authorization/roleAssignments@<version>' existing = {
  DisplayName: 'MyName'
  RoleDefinitionName: 'MyRoleName'
}

Of course, this is no use until there is an exists method available, but following that, it would be an ideal to have it on the roadmap afterwards.

@WhitWaldo WhitWaldo added the enhancement New feature or request label Oct 19, 2021
@ghost ghost added the Needs: Triage 🔍 label Oct 19, 2021
@brwilkinson
Copy link
Collaborator

I would recommend to do your reporting with powershell.

You can query the role assignments efficiently and even delete them all at a particular scope that you are deploying, then redeploy them all out new.

Once you have your naming standard in place, future deployments will work without error.

It's also helpful to output your role assignments as a deployment output with all of the information that you need to resolve conflicts.

https://github.com/brwilkinson/AzureDeploymentFramework/blob/6fe4fd4fd0a4ac07bae6cb1404542b369b00e555/ADF/bicep/sub-RBAC-ALL.bicep#L95

@brwilkinson
Copy link
Collaborator

brwilkinson commented Oct 19, 2021

Another thing to look out for is orphaned role assignments.

They will show up with the following:

Get-AzRoleAssignment | where objectType -eq 'Unknown'

You can remove them with:

Get-AzRoleAssignment | where objectType -eq 'Unknown' | Remove-AzRoleAssignment -confirm

@WhitWaldo
Copy link
Author

In your ADF framework, you have a global object that's passed into each module partially derived from these large JSON parameter blobs that are specific to each environment. Given the current lack of interface and subsequent strong-typing in the editor for imported objects, I was trying to avoid requiring this global object and instead insert minimal information into each module in order to retain the greatest flexibility in the modules, eliminate typos/errors, and limit each to the core resources and their children all being in the same place (though using external modules for children where looping became necessary per your previous guidance).

As such, all my role assignments are handled in the modules themselves and generically applied, again per your previous guidance. If I don't deploy a CosmosDB instance, it never assigns any managed identity roles for Cosmos.

However, without some means for checking of the role assignment exists prior to attempting to recreate it, it means I'm limited to using this script only for greenfield deployments. While I'm somewhat limited to doing so anyway because of vnet/subnet issues, it seems that could be similarly solved with an exists() function.

But ideally, I'd love for all things deployment-related to live in one place and avoid having all manner of different scripts/templates lying around for edge cases. It'd be far more convenient if I could have one script that can be used for either greenfield or brownfield deployments with existing resources being handled on a case-by-case basis in the template itself instead of doing outside checks to do the same.

@alex-frankel
Copy link
Collaborator

At least one challenge with this is that ARM (and the deployments service) only has one way to look up resources -- resource IDs. We construct those IDs with the scope the resource is deployed to, the type, and the name of the resource. While I understand the desire for an alternative way to look up a resource (particularly for role definitions and assignments), I don't see a way that we could implement it.

Also, in your example above, it would be possible to have more than one resource returned. What would you expect to do with the existing resource or resources in this case? It might be helpful to see a complete pseudo-code sample of both the lookup and the use of the existing resource(s).

@WhitWaldo
Copy link
Author

WhitWaldo commented Oct 19, 2021

Yes, that's a fair challenge right there. In my example above though, I'd like to have the means of working out what to do with it in the template so I can handle it based on expectations and known quirky ARM behaviors (e.g. vnets/subnets).

I imagine that in most cases, it'd be as easy as:

  1. Create the reference to the resource
  2. Does anything exist?
    2a) If so, skip provisioning of that resource/child resources
    2b) If not, provision that resource/child resources

Whether there's one or more resources returned, the fact that anything was returned would be reason to simply skip it (as in the examples in the linked ticket of storing secrets in the Key Vault - if it already exists, leave it alone).

I'm thinking of something like:

resource existingAssignment 'Microsoft.Authorization/roleAssignments@<version>' existing = {
  DisplayName: 'MyName'
  RoleDefinitionName: 'MyRoleName'
}

resource roleAssignment 'Microsoft.Authorization/roleAssignments@<version>' = if (exists(existingAssignment) == false) {
  name: guid(subscription().id, resourceGroup().location, roleDefinitionId, principalId)
  scope: myResource
  properties: {
    //Properties
  }
}

@alex-frankel
Copy link
Collaborator

So the goal state in this code sample is: "I want a role assignment with this role definition and display name". Doesn't this leave open the possibility that the role assignment has the wrong principal ID?

Separately, with this code, you are conditionally setting the properties of a resource based on it's existence, which violates a core principal of declarative deployments. In Bicep, you should not need to be concerned with it's existence or not, you only want to provide the final state of the desired resources -- "I want a role assignment with this name and these properties". The fact that you need to write this logic yourself means that something is wrong with the resource provider's implementation IMO.

For most resources, it's not as frustrating if a resource that cannot be updated already exists (i.e. if a storage account or web site name is taken), because it's a more or less friendly name. The extra challenge is that you need to generate a GUID which may or may not be taken, so you basically have to guess. To me, this means that the right fix is to relax the GUID requirement. @WhitWaldo if you could set the name to be more friendly, how much of the problem do you think is solved?

The concern is that the second we allow an existence check, the code becomes much more imperative and less repeatable/reliable.

@brwilkinson
Copy link
Collaborator

brwilkinson commented Oct 19, 2021

I definitely see your challenge here, especially related to role assignments and that fact that the name of the role assignment is a guid, so it makes reporting more difficult.

However, without some means for checking of the role assignment exists prior to attempting to recreate it, it means I'm limited to using this script only for greenfield deployments.

That is not totally true.

Here is a standard format role assignment that I currently use for ALL role assignments, this particular example is taken from the User assigned Identity. Given that the focus with my customers nearly always involved standing up new infrastructure to be able to iterate faster over standing up multiples of new environments, this is what I use.

          {
            "name": "StorageAccountOperatorGlobal",
            "RBAC": [
              {
                "Name": "Storage Account Key Operator Service Role",
                "RG": "G1",
                "Prefix": "ACU1",
                "Tenant": "HAA"
              }
            ]
          }

However I only need to add 1 extra property to create an override for this. Then I can easily export out the guid name for the role assignment and add it to this lookup table for existing role assignments. I can very easily generate this with just a few lines of powershell via get-azroleassignment.

e.g.

          {
            "name": "StorageAccountOperatorGlobal",
            "RBAC": [
              {
                "Current": "ac302733-4340-4405-b6c7-382bdfebd390",  # previously randomly generated and now exported
                "Name": "Storage Account Key Operator Service Role",
                "RG": "G1",
                "Prefix": "ACU1",
                "Tenant": "HAA"
              }
            ]
          }

Now in my role assignment code, if the property Current exists on that object, I will assign the role assignment with that guid, if not I will generate the guid based on the deterministic algorithm that I always use.

GUID: contains(rbac, 'Current') ? rbac.Current : whatEverIdidtogeneratedeterministic

In this regards, you are treating the role assignment more like Network ip ranges, since you never want them to overlap you need to maintain the information of network ranges in some external table or in this case your parameter file and pass them in.

All new role assignments will be deterministic, while you maintain the ability to deploy over Brownfields environments.

UPDATE - Adding example of querying a role assignment

get-azroleassignment | where displayname -match StorageAccountOperatorGlobal


RoleAssignmentId   : /subscriptions/855c22ce-7a6c-468b-ac72-1d1ef4355acf/resourcegroups/ACU1-BRW-HAA-RG-G1/providers/Microsoft.Authorization/roleAssignments/1285e57a-8a8e-558b-b5a1-17c1f4344b29
Scope              : /subscriptions/855c22ce-7a6c-468b-ac72-1d1ef4355acf/resourcegroups/ACU1-BRW-HAA-RG-G1
DisplayName        : ACU1-BRW-HAA-S1-uaiStorageAccountOperatorGlobal
SignInName         : 
RoleDefinitionName : Storage Account Key Operator Service Role
RoleDefinitionId   : 81a9662b-bebf-436f-a333-f67b29880f12
ObjectId           : 15a8fe45-1704-4caa-801a-35034abccbb9
ObjectType         : ServicePrincipal
CanDelegate        : False
Description        : 
ConditionVersion   : 
Condition          :

@brwilkinson
Copy link
Collaborator

@WhitWaldo if you are interested to pursue the export option, I would be happy to work with you on the process.

You should be able to generate the exact object you need for your parameters via PowerShell.

This thread shows an example, exporting management groups/subscriptions, which is a similar concept: #4715 (comment)

@ggirard07
Copy link

Not sure how out of scope I am, but I was looking for the same kind of feature requested by OP in issue title.

Use-case is a resource deployed from another template that I have then to look-up to retrieve information from (connection string, keys, ...). Right now I have to use existing keywork, but need to pas in multiple parameters for this as name AND scope are dynamics based on environment we are in. So this is multiple parameters I have to pass in per resource I need to lookup.

Having the possibility to resolve existing resource from id instead would greatly simplify the numbers of required parameters in that case.
Also I am aware of all the work being done to pass resource directly as parameter, but this won't help in my case as resources are being deployed from their own pipeline runs.

@brwilkinson
Copy link
Collaborator

brwilkinson commented Oct 21, 2021

@ggiradro07

just to clarify the ask here, is below what you are after?

param storageResourceId string

resource storageAccount 'Microsoft.Storage/storageAccounts@2021-06-01' existing = {
  id: storageResourceId
}

@ggirard07
Copy link

exactly, in order to reduce the number of parameters to pass between templates

@anthony-c-martin
Copy link
Member

anthony-c-martin commented Oct 21, 2021

Also I am aware of all the work being done to pass resource directly as parameter, but this won't help in my case as resources are being deployed from their own pipeline runs.

For #2246 (passing resource references as parameters) - given a Bicep file like:

param storageAccount resource 'Microsoft.Storage/storageAccounts@2021-06-01'

...

We haven't closed yet on what the parameters file / CLI arguments would look like if you were to deploy this file directly (vs including it as a module), but I imagine it'll be translated into some form of resourceId - e.g.:

az group deployment create ... --parameters storageAccount="/subscriptions/<subId>/resourcegroups/<rgName>/providers/Microsoft.Storage/storageAccounts/<name>"

Would that work for your scenario?

/cc @rynowak

@brwilkinson
Copy link
Collaborator

@ggirard07 I am keeping a list of personal requests myself, waiting for the right time to ask for each of them.

This is on my list...

Still wondering how that will play out based on what @anthony-c-martin mentioned.

@ggirard07
Copy link

it will be even easier as it will save the extra resource ... existing = {} in that case, with the same number of parameters to pass-in!

@stewartadam
Copy link

Throwing in another voice that referencing resources by their ID is necessary, having only references name causes parameter bloat prevents easy interop as parameters values for resources often accept/require resource IDs, or output resource IDs. So now you're stuck converting between resource ID string and resource anmes, and passing that info as a bunch of parameters.

@alex-frankel
Copy link
Collaborator

alex-frankel commented Feb 1, 2022

With #2245/#2246 we will introduce some of this additional flexibility, but a workaround you can do today to avoid passing the individual segments as params is by using the split() function like so (assuming the resource type is static):

param myResourceId string

var segments = split(myResourceId, '/')

resource stg 'Microsoft.Network/networkSecurityGroups@2021-05-01' existing = {
  scope: resourceGroup(segments[2], segments[4]) // not required if existing resource is in the same scope as the current module
  name: segments[8]
}

@WhitWaldo
Copy link
Author

Personally, I can hold out for now as I'd rather have the editor-support coming with #2245 and #2246 rather than adding to what are already some confusing templates with some arbitrary split variables.

@ghost
Copy link

ghost commented May 24, 2023

Hi WhitWaldo, this issue has been marked as stale because it was labeled as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thanks for contributing to bicep! 😄 🦾

@stewartadam
Copy link

Commenting to avoid staleness.

Perhaps a 4-day timer on all issues is a bit too aggressive?

@alex-frankel
Copy link
Collaborator

Thanks. We have switched the time to respond to 7 days.

@ghost
Copy link

ghost commented Jun 1, 2023

Hi WhitWaldo, this issue has been marked as stale because it was labeled as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thanks for contributing to bicep! 😄 🦾

@alex-frankel alex-frankel added Needs: Upvote This issue requires more votes to be considered and removed awaiting response Status: No Recent Activity labels Jun 1, 2023
@stewartadam
Copy link

Avoiding staleness

@ivarne
Copy link

ivarne commented Jul 6, 2023

Another thing to look out for is orphaned role assignments.

...

You can remove them with:

Get-AzRoleAssignment | where objectType -eq 'unknown' | Remove-AzRoleAssignment

@brwilkinson This didn't quite work as expected! Remove-AzRoleAssignment does not seem to support piping in lists of roles to remove, but has a default to remove all role assignments in a resource group. That was quite bad for me, as i locked my team out of our resource group, as no one has access anymore.

@brwilkinson
Copy link
Collaborator

To be fair, the recommendation was to confirm the existence of the orphaned role assignments first via:

Get-AzRoleAssignment | where objectType -eq 'Unknown'

@ivarne
Copy link

ivarne commented Jul 6, 2023

I did run that part first, and the filter worked fine, and I got only one role back (the one I wanted to delete). The issue seems to have happened when I ran the code in a azure devops pipeline (my personal account dosen't have Owner role, so I need it to run through the service account).

      - task: AzurePowerShell@5
        inputs:
          azureSubscription: '$(azureServiceConnection)'
          ScriptType: 'InlineScript'
          Inline: 'Get-AzRoleAssignment | where ObjectType -eq ''Unknown'' | Remove-AzRoleAssignment'
          azurePowerShellVersion: 'LatestVersion'

Edited to add
The double '' was added automatically by the pipeline editor and is a YAML artifact. My current theory is that somehow the Get-AzRoleAssignment failed to lookup the other object types when runing in the pipeline, so all the role assignments came with ObjectType -eq 'Unknown' and thus got deleted. My error seems to be in the same category as a DELETE sql query with a broken WHERE clause

@brwilkinson
Copy link
Collaborator

it appears there is a syntax error above, based on using two sets of single quotes on ''Unknown''

Normally above would be a syntax error, I am not sure what scenario would make the code work in the pipeline.

I would recommend to use the -whatif switch if testing code you are unfamiliar with or in a new environment etc.

Moving from local testing to testing in the pipeline; it would be useful to first test the code with -whatif.

https://techcommunity.microsoft.com/t5/itops-talk-blog/powershell-basics-don-t-fear-hitting-enter-with-whatif/ba-p/353579

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered
Projects
Status: Todo
Development

No branches or pull requests

7 participants