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

azurerm_synapse_workspace - support for the customer_managed_key_versionless_id property #11328

Merged
merged 5 commits into from
Apr 27, 2021

Conversation

allantargino
Copy link
Contributor

This PR adds support for CMK on Synapse.

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@allantargino, thank you so much for the PR. It's looking good so far but I left a few minor comments and questions. Once we get those addressed we can get this merged and shipped in the next release! 🚀

Requested Change:
Can we actually pull this out of the azurerm_synapse_workspace resource and make it its own separate azurerm_synapse_workspace_customer_managed_key resource to be more consistent with the already existing implementations of this functionality? (e.g. azurerm_storage_account_customer_managed_key, azurerm_log_analytics_cluster_customer_managed_key, etc.)

Comment on lines +353 to +355
key_vault {
purge_soft_delete_on_destroy = false
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for disabling the purge_soft_delete_on_destroy feature?

Copy link
Contributor Author

@allantargino allantargino Apr 22, 2021

Choose a reason for hiding this comment

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

I added it because I keep receive the following error message:

The user, group or application 'appid=...;oid=...;iss=https://sts.windows.net/.../' does not have keys purge permission on key vault 'acckv210422144256592239;location=...'

Even though I set the permissions on the KV terraform code:

  access_policy {
    tenant_id = data.azurerm_client_config.current.tenant_id
    object_id = data.azurerm_client_config.current.object_id
    key_permissions = [
      "create",
      "get",
      "delete",
      "purge"
    ]
  }

website/docs/r/synapse_workspace.html.markdown Outdated Show resolved Hide resolved
@WodansSon
Copy link
Collaborator

@allantargino, to get rid of the CI error you may need to pull the origin/master branch into your fork.

@allantargino
Copy link
Contributor Author

allantargino commented Apr 22, 2021

Hi @WodansSon, thanks for reviewing the PR!

I first was trying to implement azurerm_synapse_workspace_customer_managed_key to be consistent with the other implementations as you said, but seems Synapse has a different behavior than the other resources when it comes to double encryption.

You can only specify whether you need double encryption or not when you are creating the resource. If you opted for it, you need to pass a key vault key on the body of the request. Otherwise, you won't be able to update Synapse to use double encryption after the workspace has been created. So that's why I chose to embed it :(

Please check this documentation.

Workspaces can be configured to enable double encryption with a customer-managed key at the time of workspace creation.
[...]
Important: The configuration setting for double encryption cannot be changed after the workspace is created.

Also, check how the portal shows it up:
image

How about using the current interface and, if/when the Synapse team updates this behavior, we deprecate it in favor of azurerm_synapse_workspace_customer_managed_key?

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @allantargino - LGTM 👍

@katbyte katbyte added this to the v2.57.0 milestone Apr 27, 2021
@katbyte katbyte merged commit 801cf7b into hashicorp:master Apr 27, 2021
@katbyte katbyte changed the title Adding CMK to Synapse azurerm_synapse_workspace - support for the customer_managed_key_versionless_id property Apr 27, 2021
katbyte added a commit that referenced this pull request Apr 27, 2021
@allantargino allantargino deleted the synapse-double-encryption branch April 27, 2021 20:18
@ghost
Copy link

ghost commented Apr 30, 2021

This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.57.0"
}
# ... other configuration ...

@sehgalnamit
Copy link
Contributor

It doe snot work as it needs grant to be done to managed identity to the AKV

2021-05-05 18:25:10
2021-05-05 18:25:10 Error: updating Synapse Workspace "syn-sgrtss-dev-az1-mwrgy7" Sql Admin (Resource Group "rsg-sgrtss-dev-az1-mwrgy7"): synapse.WorkspaceAadAdminsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="WorkspaceActivationRequired" Message="Workspace needs to be activated, by adding the managed identity in the KeyVault containing the customer managed key and activating the workspace through the keys subresource."

Azure Synapse Encryption can only be enabled during creation time for some unknown reason.
During creation of Azure Synapse it fails as managed identity of Synapse getting created is not granted access to AKV.

MS document says activation can only be done manually:-
https://docs.microsoft.com/en-us/azure/synapse-analytics/security/workspaces-encryption

So this feature doe snot work with terraform and need to be relloked.

I can grant MSI access in my module

resource "azurerm_key_vault_access_policy" "instance" {

key_vault_id = var.keyvault_id
tenant_id = data.azurerm_client_config.current.tenant_id
object_id = azurerm_synapse_workspace.synapse.identity[0].principal_id
key_permissions = ["Get", "List", "Delete", "Create", "Encrypt", "Recover", "Purge", "Verify", "UnwrapKey", "WrapKey"]
secret_permissions = ["Get", "List", "Set", "Delete"]
depends_on = [
azurerm_synapse_workspace.synapse,
]
}

But issue is Synapse is still getting created and it just fails and there is not az command to activate it.

Unlike other service Azure SQL, Postgres etc.. which can enable TDE after instance creation, Azure Synapse doe snot allow this.

@sehgalnamit
Copy link
Contributor

sehgalnamit commented May 7, 2021

This is the Bug which MS has to fix in its ARM template and also in terraform
#aad_admin, can not use with Azure Synapse, with customer managed key

• Create workspace, with CMK set
• Grant workspace identity permission to the key vault key
• Active the workspace
• Create the workspace's AAD admin ( this one not sure how to dit as we can just add roles for synapse admin, sql admin and spark pool admin).

@DesaCh01
Copy link

Hello,

I too am facing the same issue as @sehgalnamit.

Error received while trying to create a synapse workspace using customer_managed_key_versionless_id property.

**_Error: updating Synapse Workspace "rsgue2fidodasa02" Sql Admin (Resource Group "shared-fido-dev-eastus2"): synapse.WorkspaceAadAdminsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="WorkspaceActivationRequired" Message="Workspace needs to be activated, by adding the managed identity in the KeyVault containing the customer managed key and activating the workspace through the keys subresource."

on azure-synapse.tf line 6, in resource "azurerm_synapse_workspace" "synapse":
6: resource "azurerm_synapse_workspace" "synapse"_**

I can assign the key permissions to Managed Identity only once the workspace is deployed and I get the identity's principal ID. But, currently it erroring out while synapse workspace creation.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants