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

New Resource: azurerm_kusto_cluster_customer_managed_key #7520

Conversation

jrauschenbusch
Copy link
Contributor

@jrauschenbusch jrauschenbusch commented Jun 29, 2020

Provides Kusto Cluster support for a customer managed encryption key

@jrauschenbusch jrauschenbusch changed the title New Resource: kusto_cluster_customer_managed_key New Resource: azurerm_kusto_cluster_customer_managed_key Jun 29, 2020
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @jrauschenbusch, taking a look through this resource, I have to say it seems better suited to just be used as an attribute on the existing cluster resource. I probably missing something so do you mind talking through a little of your thoughts around why this is split out into its own resource?

@jrauschenbusch
Copy link
Contributor Author

Hey @mbfrahry,

there was a discussion with @tombuildsstuff that first the cluster needs to be created with an identity type SystemAssigned. After the cluster is created the managed identity needs to get access to the key vault. Hence this feature needs it's own resource. For further details see #7420.

@jrauschenbusch
Copy link
Contributor Author

@mbfrahry @tombuildsstuff If we could get this merged today, release 2.18.0 would contain all major Kusto Cluster features currently provided by the SDK. Especially for enterprise users custom encryption keys for encryption at rest are a must.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @jrauschenbusch, I'm seeing errors when running the tests

TestAccAzureRMKustoClusterCustomerManagedKey_basic: testing.go:684: Step 0 error: errors during apply:
              
              Error: Error retrieving Kusto Cluster "acctestkcun8z6" (Resource Group "acctestRG-200709182537441411"): `properties` was nil
              

I'm investigating more on my side but any help from your end would be appreciated too

@jrauschenbusch
Copy link
Contributor Author

Hey @mbfrahry This commit should do the trick. I'll test it within my own subscription and give you feedback if tests are passing.

@jrauschenbusch
Copy link
Contributor Author

@mbfrahry I'm currently facing the following problems when running the acc tests:

--- FAIL: TestAccAzureRMKustoClusterCustomerManagedKey_requiresImport (803.10s)
    testing.go:684: Step 0 error: errors during apply:
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to azurerm_kusto_cluster_customer_managed_key.test,
        provider "azurerm" produced an unexpected new value for was present, but now
        absent.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
        
    testing.go:745: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: config is invalid: 4 problems:
        
        - Reference to undeclared resource: A managed resource "azurerm_storage_account_customer_managed_key" "test" has not been declared in the root module.
        - Reference to undeclared resource: A managed resource "azurerm_storage_account_customer_managed_key" "test" has not been declared in the root module.
        - Reference to undeclared resource: A managed resource "azurerm_storage_account_customer_managed_key" "test" has not been declared in the root module.
        - Reference to undeclared resource: A managed resource "azurerm_storage_account_customer_managed_key" "test" has not been declared in the root module.
...
--- FAIL: TestAccAzureRMKustoClusterCustomerManagedKey_basic (815.34s)
    testing.go:684: Step 0 error: errors during apply:
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to azurerm_kusto_cluster_customer_managed_key.test,
        provider "azurerm" produced an unexpected new value for was present, but now
        absent.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
        
    testing.go:745: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during apply: Error deleting Kusto Cluster "acctestXXXXX" (Resource Group "acctestRG-XXXXX"): kusto.ClustersClient#Delete: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="RequestConflict" Message="Cannot modify resource with id '/subscriptions/XXXXXXXXXXXXXXX/resourceGroups/acctestRG-XXXXX/providers/Microsoft.Kusto/Clusters/acctestXXXXX' because the resource entity provisioning state is not terminal. Please wait for the provisioning state to become terminal and then retry the request."
--- FAIL: TestAccAzureRMKustoClusterCustomerManagedKey_updateKey (1464.67s)
    testing.go:684: Step 0 error: errors during apply:
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to azurerm_kusto_cluster_customer_managed_key.test,
        provider "azurerm" produced an unexpected new value for was present, but now
        absent.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

Maybe you have a quick idea of where they come from?

@mbfrahry
Copy link
Member

mbfrahry commented Jul 9, 2020

So it looks like the import test is failing because we're calling azurerm_storage_account_customer_managed_key instead of azurerm_kusto_cluster_customer_managed_key

@mbfrahry
Copy link
Member

mbfrahry commented Jul 9, 2020

The inconsistent results failure normally happens when what is in the config differs from what is being returned by the api. That one will require more digging into. Your best bet might be to see what is being returned by the api and compare it to what's in the config

@mbfrahry
Copy link
Member

mbfrahry commented Jul 9, 2020

I'm building it locally now to see if I can help get to the bottom of it

@mbfrahry
Copy link
Member

mbfrahry commented Jul 9, 2020

Hmmm, so I have a bit more information. We're definitely sending the key vault properties to the api but we're not getting them back and then looking in the portal they aren't there either.

This is what we're sending with values removed:

"keyVaultProperties": {
	"keyName": "test",
	"keyVersion": "asdfqwer",
	"keyVaultUri": "hasdqweqwe"
},

And this is what we get back:

"keyVaultProperties": null,

@mbfrahry
Copy link
Member

mbfrahry commented Jul 9, 2020

Another lead! So it looks like clusterClient.Update(ctx, clusterID.ResourceGroup, clusterID.Name, props) returns a future. I added a future wait for completion but am now seeing

Error: Code="Failed" Message="Internal Server Error"

Doing this in the portal returns the same error message

@jrauschenbusch
Copy link
Contributor Author

Thanks for giving the hints. I'm going to fix it.

@jrauschenbusch
Copy link
Contributor Author

Hey @mbfrahry

Regarding the Error: Code="Failed" Message="Internal Server Error":

I've now tested a lot with the resource itself and with the Azure portal, and it seems that there is currently an issue on the Azure API. I'll file a bug ticket for it.

@jrauschenbusch
Copy link
Contributor Author

Hey @mbfrahry Finally the Kusto product team has fixed the bug related to the Internal Server Error message. I've finally fixed some minor bugs within the resource implementation and acc tests and now it seems to look good.

@jrauschenbusch
Copy link
Contributor Author

Acceptance tests succeed now 👍

make acctests SERVICE='kusto' TESTARGS='-run=TestAccAzureRMKustoClusterCustomerManagedKey' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/kusto/tests/ -run=TestAccAzureRMKustoClusterCustomerManagedKey -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMKustoClusterCustomerManagedKey_basic
=== PAUSE TestAccAzureRMKustoClusterCustomerManagedKey_basic
=== RUN   TestAccAzureRMKustoClusterCustomerManagedKey_requiresImport
=== PAUSE TestAccAzureRMKustoClusterCustomerManagedKey_requiresImport
=== RUN   TestAccAzureRMKustoClusterCustomerManagedKey_updateKey
=== PAUSE TestAccAzureRMKustoClusterCustomerManagedKey_updateKey
=== CONT  TestAccAzureRMKustoClusterCustomerManagedKey_basic
=== CONT  TestAccAzureRMKustoClusterCustomerManagedKey_updateKey
=== CONT  TestAccAzureRMKustoClusterCustomerManagedKey_requiresImport
--- PASS: TestAccAzureRMKustoClusterCustomerManagedKey_basic (1584.25s)
--- PASS: TestAccAzureRMKustoClusterCustomerManagedKey_updateKey (1651.15s)
--- PASS: TestAccAzureRMKustoClusterCustomerManagedKey_requiresImport (1692.00s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/kusto/tests 1693.087s

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @jrauschenbusch!

@mbfrahry mbfrahry merged commit a87636f into hashicorp:master Jul 17, 2020
mbfrahry added a commit that referenced this pull request Jul 17, 2020
@jrauschenbusch jrauschenbusch deleted the azurerm_kusto_cluster_customer_managed_key branch July 18, 2020 09:46
@ghost
Copy link

ghost commented Aug 17, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Aug 17, 2020
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.

None yet

2 participants