Added support to specify tags in the config.yaml that will be applied to all resources deployed by TRE

[jlabhard-sg]

Resolves #417

What is being addressed

We need all resources deployed by TRE (included management) to include specific tags upon creation to comply with corporate tagging policies.

Important: The current PR contains a draft implementation that solves this issue. It is not fully complete and has not been tested yet.

How is this addressed

• Tags can be specified in the config.yaml file in a new tags field.
• Must be a string representation of JSON object with the following format: '{"tag_key": "tag_value", "tag_key2": "tag_value2"}', similar to rp_bundle_values
• The tags are exported to environments variables similar to the other fields.
• Tags are added to /devops/terraform/bootstrap.sh directly in the command to create the resource group and storage account. This is done to comply with policies upon creation of resources.
• Tags are added to github actions on a best effort basis. We personally don't use them so I am less knowledgeable on their implementation.
• Tags are added as variable to the management terraform and applied to each resources. (This has been tested)
• Tags are added as variable to the core terraform and appended to the local tre_core_tags which is already applied to resources. Extension to modules is also considered.
• Similarily, for shared_services, workspace_services, user_resources, and workspaces:
  • Tags are added as terraform variable and appended to the respective locals (tre_shared_service_tags, tre_workspace_service_tags, tre_user_resources_tags, tre_workspace_tags)
  • The porter.yaml and parameters.json files have been updated for each template

[jlabhard-sg]

@microsoft-github-policy-service agree company="SOPHiA GENETICS"

[marrobi]

@jlabhard-sg approach looks good. I wonder if we can add anything to the terraform linter to ensure no services are missing the tags property.

Look forward to it being ready for review.

[github-actions]

Unit Test Results

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 5dd7784.

♻️ This comment has been updated with latest results.

[jlabhard-sg]

The PR has been tested with the deployment of a fresh TRE, shared services, base workspace and linux VM. The tags were successfully applied to all azure resources whenever possible, and complied with Azure Policies that requires tags on all resources.

I have looked into adding the tags to the terraform linter but it does not seem to support having the tags attribute, only having requirements for specific tags which is already in place.

PR is now ready for review.

[marrobi]

@jlabhard-sg thanks, can you lint the terraform - terraform fmt -recursive

[marrobi]

/test-extended

[github-actions]

🤖 pr-bot 🤖

⚠️ Cannot run tests as PR is not mergeable. Ensure that the PR is open and doesn't have any conflicts.

(in response to this comment from @marrobi)

[marrobi]

@jlabhard-sg can you resolve the conflicts please.

[marrobi]

/test-extended

[github-actions]

🤖 pr-bot 🤖

⚠️ When using /test-extended on external PRs, the SHA of the checked commit must be specified

(in response to this comment from @marrobi)

[marrobi]

/test-extended 46d6e40

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/5750094375 (with refid 17725761)

(in response to this comment from @marrobi)

[marrobi]

/test-extended 46d6e40

[github-actions]

🤖 pr-bot 🤖

⚠️ The specified SHA 46d6e40 is not the latest commit on the PR. Please validate the latest commit and re-run /test

(in response to this comment from @marrobi)

[jlabhard-sg]

@marrobi The current failure of terraform lint happens because tags of resources in the management resource group do not include "tre_id".

on devops/terraform/main.tf:

Notice: The resource is missing the following tags: "tre_id". (azurerm_resource_missing_tags)

  on main.tf line 10:
  10:   tags = merge(var.tags, {
  11:     project = "Azure Trusted Research Environment"
  12:     source  = "https://github.com/microsoft/AzureTRE/"
  13:   })

However, this is not something I have removed with this PR. The tre_id is also not a variable in the devops terraform so I'm not sure what should be done here.

[marrobi]

/test-extended 31ca5c5

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/5763811429 (with refid 17725761)

(in response to this comment from @marrobi)

[marrobi]

@jlabhard-sg thanks for getting everything green test wise. I'll try to take a look at the PR this week, but will likely hold of merging until after the next release. Thank you.

[marrobi]

Can you add this to the docs, https://microsoft.github.io/AzureTRE/v0.13.0/tre-admins/environment-variables/,

Can you also update the CHANGELOG.md file please.

Noticed this page needs updating to explain how config.yaml maps to ENV vars etc. Will create a separate issue for that.

[marrobi]

I've added this to the docs, can you verify it's correct. Thanks.

[marrobi]

@jlabhard-sg got an issue where tags aren't being updated for existing resource, I think this is due to:

lifecycle { ignore_changes = [tags] }

being set on some (not all resources)

The reason this was added was so that we didn't replace corporate tags applied by Azure policy - hence a lot of TF churn on each TF deploy.

We could remove this, but it does also make me think, should these TRE wide tags be being applied via Azure policy rather than the TRE?

How does it work in your organisation?

My thinking at the moment is remove the ignore changes (that is typically a bad thing), and orgs need to add the corporate/policy applied tags to your new variable to prevent churn.

[tamirkamara]

Removing the ignore tags is going to create issues on subscriptions where a higher level policy is used. For example, we have one that assigns an "creator" tag for each resource upon creation.

[marrobi]

Removing the ignore tags is going to create issues on subscriptions where a higher level policy is used. For example, we have one that assigns an "creator" tag for each resource upon creation.

Hmm, maybe a better solution is an Azure Policy that assigns tags based on tre_id?

Or alternatively we allow the tags to ignore to be specified in another variable.

[jlabhard-sg]

@jlabhard-sg got an issue where tags aren't being updated for existing resource, I think this is due to:

lifecycle { ignore_changes = [tags] }

being set on some (not all resources)

The reason this was added was so that we didn't replace corporate tags applied by Azure policy - hence a lot of TF churn on each TF deploy.

We could remove this, but it does also make me think, should these TRE wide tags be being applied via Azure policy rather than the TRE?

How does it work in your organisation?

My thinking at the moment is remove the ignore changes (that is typically a bad thing), and orgs need to add the corporate/policy applied tags to your new variable to prevent churn.

In our organization the Azure policy prevents the resource from being created if it does not include the necessary tags. I think this is due to the values of the tags being the responsibility of whoever creates these resources. In this case, I don't think tags automatically applied by an Azure policy would function.

In our case, the ignore changes is not an issue since the policy affects only resources on creation. If it is an issue here I am not sure what the best solution is.

[marrobi]

@jlabhard-sg got an issue where tags aren't being updated for existing resource, I think this is due to:

lifecycle { ignore_changes = [tags] }

being set on some (not all resources)
The reason this was added was so that we didn't replace corporate tags applied by Azure policy - hence a lot of TF churn on each TF deploy.
We could remove this, but it does also make me think, should these TRE wide tags be being applied via Azure policy rather than the TRE?
How does it work in your organisation?
My thinking at the moment is remove the ignore changes (that is typically a bad thing), and orgs need to add the corporate/policy applied tags to your new variable to prevent churn.

In our organization the Azure policy prevents the resource from being created if it does not include the necessary tags. I think this is due to the values of the tags being the responsibility of whoever creates these resources. In this case, I don't think tags automatically applied by an Azure policy would function.

In our case, the ignore changes is not an issue since the policy affects only resources on creation. If it is an issue here I am not sure what the best solution is.

If the tags variable is updated after initial deployment the changes will not take affect on the core deployment. I guess we could caveat that updates are not supported.

I think the more complete solution would be to have ignore_tag_changes that would add:

  lifecycle {
    ignore_changes = [
      tags["Creator"]
    ]  
  }

I'm not sure if variables can be used in lifecycle attributes though. @tamirkamara thoughts welcome.

[marrobi]

Nope, not possible. hashicorp/terraform#24188

[marrobi]

@jlabhard-sg how would you deal with the scenario "Now we need all resources to have this new tag?"

I think a policy might be abetter way forward I'm afraid, we could still add this to the TRE project to apply tags based on TRE_ID, although expect it would be relatively easy to do manually.

Modify operations are executed before Deny, so the tag would be added to the request before being blocked by the Deny.

https://learn.microsoft.com/en-us/azure/governance/policy/concepts/effects#order-of-evaluation

[jlabhard-sg]

@marrobi Would this policy be created by the TRE/terraform, or is that something that should be created manually by our organization? I am a bit worried that the scope of the project would not have the necessary permission to create new policies.

It is true that the scenario could be an issue in the future. Would it be possible to have a boolean variable determine whether the terraform should ignore tags or not?

[marrobi]

It would be created by Terraform at subscription scope. I've been having a look to see if I can make it work... this is my current WIP policy.tf file.

[marrobi]

@jlabhard-sg this seems to work for me... are you ok to give it a go in your environment with the policy that blocks without certain tags?

resource "azurerm_policy_definition" "custom_tre_tags" {
  name         = "custom_tre_tags_${var.tre_id}"
  display_name = "Azure TRE ${var.tre_id}: Custom Tags"
  description  = "Add custom tags to all resources on TRE ${var.tre_id}"
  policy_type  = "Custom"
  mode         = "Indexed"

  metadata = <<METADATA
  {
    "category": "TRE_Tags",
    "version": "1.0.0"
  }
  METADATA

  policy_rule = jsonencode({
    "if" : {
      "allOf" : [
        {
          "field" : "tags['tre_id']",
          "equals" : var.tre_id
        }
      ]
    },
    "then" : {
      "effect" : "modify",
      "details" : {
        "roleDefinitionIds" : [
          "/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c"
        ],
        "operations" : [
          for tag_key, tag_value in var.tags : {
            "operation" : "addOrReplace",
            "field" : "tags['${tag_key}']",
            "value" : tag_value
          }
        ]
      }
    }
  })
}

resource "azurerm_subscription_policy_assignment" "custom_tre_tags" {
  name                 = "custom_tre_tags_${var.tre_id}_assignment"
  policy_definition_id = azurerm_policy_definition.custom_tre_tags.id
  subscription_id      = data.azurerm_subscription.current.id
  location             = var.location

  identity {
    type = "SystemAssigned"
  }
}


resource "azurerm_role_assignment" "subscription_contributor" {
  scope                = data.azurerm_subscription.current.id
  role_definition_name = "Contributor"
  principal_id         = azurerm_subscription_policy_assignment.custom_tre_tags.identity[0].principal_id
}

resource "azurerm_subscription_policy_remediation" "custom_tre_tags" {
  name                 = "custom_tre_tags_${var.tre_id}_remediation"
  policy_assignment_id = azurerm_subscription_policy_assignment.custom_tre_tags.id
  subscription_id      = data.azurerm_subscription.current.id
}

I reverted the change in local.tf

  tre_core_tags = {
      tre_id              = var.tre_id
      tre_core_service_id = var.tre_id
  }

and added a dependency to the core RG in main.tf

   depends_on = [
    azurerm_subscription_policy_assignment.custom_tre_tags
  ]

Let me know if you can give it a go, and also the results. Lets leave the PR here until we know. Thanks.

[marrobi]

I also note we haven't got ignore changes on all resources with tags defined. :/

[jlabhard-sg]

@marrobi Yes I will try to test the deployment tomorrow using this policy file instead of directly adding tags

[marrobi]

@jlabhard-sg @tamirkamara I've created to PR here #3670 to add the missing lifecycle changes blocks (with a bit of help from GitHub copilot!)

[jlabhard-sg]

@marrobi I've tested the deployment with the azure policy in the terraform and it seems to work, policy is complied with and did not encounter any permission issue. I think this the the way to go.

However, in our case, it is necessary to have the policy in the management resources terraform (/devops folder), and for this reason, also add tre_id to the variables and tags of the management resources. Would that be an issue for the TRE repo?

Specifically what I did:

• Add your code snippet to /devops/terraform/tags_policy.tf
• Reverted the tags changes and added tre_id to the variables and tags
• Added the depends_on on the management resources group and the tredev_purge acr task
• Added the lifecycle ignore changes on the tredev_purge acr task
• Tested deployment of management resources and deployment of a VM without tags

If there is no issue, I can adapt the PR with these changes, adding the policy to the management resources, and removing the custom tags to all resources terraform.

[marrobi]

@jlabhard-sg why do you need it in the /devops folder?

Initially the idea was multiple environments might share the same ACR and storage account, but in reality not sure that is the case anywhere.

I don't see any real reason why policies couldn't go there, have you tagged the management resources with the tre_id too?

The other option is we do nothing, and say tagging policy is outside the remit of the repository, but we could provide an example in the docs on how to do it.

[jlabhard-sg]

@marrobi Because the policy that requires the tags also applies on the management resources. The resource group and storage account could be created manually as they are then imported to the state, but the ACR is not.
I did tag the management resources with the tre_id.

Doing nothing is also an option. the azure policies as they are defined here is not something that is necessary in either the core or devops terraform since it is at subscription scope. We could match it to something other than the tre_id to apply it to the management resources as well.

Let me know what you think is the best approach. I actually didn't consider using policies to apply tags but it seems to solve the issue without needing changes to the repository.

[marrobi]

Thanks, its actually similar to this one: #50

The management resources should probably have tags. @tamirkamara can you think of any reason why they shouldn;t be tagged with tre_id? To be honest they could sit in the main TRE resource group, but that's more effort from a migration perspective and is a chicken and egg situation with the state storage.

I'm in two minds about including the policies. It would make it easier for organisations to get up and running - and people to want these things to work "out of the box", but is policies might also need tweaking to meet an organisations needs.

[tamirkamara]

The management resources should probably have tags. tamirkamara can you think of any reason why they shouldn;t be tagged with tre_id? To be honest they could sit in the main TRE resource group, but that's more effort from a migration perspective and is a chicken and egg situation with the state storage.

We don't use tre_id when creating the management resource group since at the time there was a notion that several TREs will use the same management RG.

[marrobi]

I think there are two separate issues in play here. "how should we handle tagging", and "does the management resource group need a rethink". The latter needs a separate issue and longer discussion - for now you could tag in a policy filtering on the management resource group name.

Given you have a workaround, as much as I would like to get the PR in as you have put in effort, I think adding notes to the original issue might be the easiest next step, then if want to revisit later we can.

We do need to get the ignore changes to tags PR in #3670 , otherwise TF is going to keep trying to remove any tags applied by policy.