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

Upgrade Kusto SDK to 2020-02-15 from 2019-05-15 #6838

Merged
merged 2 commits into from Jun 16, 2020

Conversation

jrauschenbusch
Copy link
Contributor

No description provided.

Copy link
Member

@tombuildsstuff tombuildsstuff 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

Thanks for this PR :)

Taking a look through this LGTM - I'll kick off the tests now 👍

Thanks!

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.

👋 @jrauschenbusch,

it looks like there is a potential crash with the AsDatabase call:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x44a518b]

goroutine 3312 [running]:
github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/kusto.resourceArmKustoDatabaseCreateUpdate(0xc0008f5960, 0x4aa1ec0, 0xc0011fa500, 0x0, 0x0)
	/Users/kt/hashi/tf/azure/azurerm/azurerm/internal/services/kusto/kusto_database_resource.go:97 +0xdab

@katbyte katbyte added this to the v2.10.0 milestone May 12, 2020
@jrauschenbusch
Copy link
Contributor Author

👋 @jrauschenbusch,

it looks like there is a potential crash with the AsDatabase call:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x44a518b]

goroutine 3312 [running]:
github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/kusto.resourceArmKustoDatabaseCreateUpdate(0xc0008f5960, 0x4aa1ec0, 0xc0011fa500, 0x0, 0x0)
	/Users/kt/hashi/tf/azure/azurerm/azurerm/internal/services/kusto/kusto_database_resource.go:97 +0xdab

@katbyte You're right. Shame on me 🤦 Did not tested it before... The domain model changed from API 2010-05-15 to API 2020-02-15, so i had to adjust it. I'll fix and test it and update this PR afterwards.

@jrauschenbusch
Copy link
Contributor Author

👋 @jrauschenbusch,
it looks like there is a potential crash with the AsDatabase call:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x44a518b]

goroutine 3312 [running]:
github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/kusto.resourceArmKustoDatabaseCreateUpdate(0xc0008f5960, 0x4aa1ec0, 0xc0011fa500, 0x0, 0x0)
	/Users/kt/hashi/tf/azure/azurerm/azurerm/internal/services/kusto/kusto_database_resource.go:97 +0xdab

@katbyte You're right. Shame on me 🤦 Did not tested it before... The domain model changed from API 2010-05-15 to API 2020-02-15, so i had to adjust it. I'll fix and test it and update this PR afterwards.

While fixing the related issues i've found a bug within the unmarshalling logic in the azure sdk:
Azure/azure-sdk-for-go#9339. I think this PR is blocked until this is solved.

@katbyte katbyte added sdk/requires-upgrade This is dependent upon upgrading an SDK upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR labels May 13, 2020
@katbyte katbyte modified the milestones: v2.10.0, Blocked May 13, 2020
@jrauschenbusch
Copy link
Contributor Author

@katbyte I can proceed with this PR as soon as Azure/azure-sdk-for-go#9756 is merged. But before, a module update for the azure-sdk-for-go dependency must take place.

@katbyte
Copy link
Collaborator

katbyte commented Jun 1, 2020

@jrauschenbusch - happy to bump us up to 43 when its released :)

@jrauschenbusch
Copy link
Contributor Author

@tombuildsstuff
Copy link
Member

Dependent on #7188

@jrauschenbusch jrauschenbusch force-pushed the upgrade-kusto-sdk branch 2 times, most recently from 99bf011 to 37e1dbf Compare June 12, 2020 12:00
@jrauschenbusch
Copy link
Contributor Author

@tombuildsstuff All Kusto resources are now updated and tested (ID parsers extracted, Unit tests available, resources adapted to 2020-02-15 API). As there are now 2 types of databases (read/write and read only) i had to make some tests upfront to check out whether we need to support the direct creation of such read only databases, too. But it seems that only read/write DBs can be created via the CreateUpdate method available in the client used inside the azurerm_kusto_database resource. Hence the database resource for now is working as we always assume that we need to create a read/write database:

Error: Error creating or updating Kusto Database "ajo8fe-kusto-database" (Resource Group "ajo8fe-rg-2", Cluster "ajo8fecluster2"): kusto.DatabasesClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="BadRequest" Message="Creating readonly following database directly is not supported, create attached database configuration instead."

  on main.tf line 276, in resource "azurerm_kusto_database" "test":
 276: resource "azurerm_kusto_database" "test" {

The attached database configuration should be another TF resource.

The only thing is that one could try to add principals to an existing follower database having a principal_modification_kind= "Union" || "Replace".
https://docs.microsoft.com/de-de/azure/data-explorer/follower#manage-principals

BUT:

  1. This would currently only be possible if one would create a follower DB by using ARM templates or Code snippets (C#, Python). Hence as for now there is no native Terraform AzureRM support not Azure CLI support for creating follower databases.
    NOTE: I have a branch ready to add this feature in the near future, as soon as this PR is done.
  2. There are now 2 new Azure REST resources to support CRUD operations for assigning database and cluster principals. As the current azurerm_kusto_database_principal resource was a workaround we could add new resources which would us allow to have a much cleaner resource design for those features.

@jrauschenbusch
Copy link
Contributor Author

@katbyte Test results are ok now.

make acctests SERVICE='kusto' TESTTIMEOUT='120m'==> 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/  -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceAzureRMKustoCluster_basic
=== PAUSE TestAccDataSourceAzureRMKustoCluster_basic
=== RUN   TestAccAzureRMKustoCluster_basic
=== PAUSE TestAccAzureRMKustoCluster_basic
=== RUN   TestAccAzureRMKustoCluster_update
=== PAUSE TestAccAzureRMKustoCluster_update
=== RUN   TestAccAzureRMKustoCluster_withTags
=== PAUSE TestAccAzureRMKustoCluster_withTags
=== RUN   TestAccAzureRMKustoCluster_sku
=== PAUSE TestAccAzureRMKustoCluster_sku
=== RUN   TestAccAzureRMKustoDatabasePrincipal_basic
=== PAUSE TestAccAzureRMKustoDatabasePrincipal_basic
=== RUN   TestAccAzureRMKustoDatabase_basic
=== PAUSE TestAccAzureRMKustoDatabase_basic
=== RUN   TestAccAzureRMKustoDatabase_softDeletePeriod
=== PAUSE TestAccAzureRMKustoDatabase_softDeletePeriod
=== RUN   TestAccAzureRMKustoDatabase_hotCachePeriod
=== PAUSE TestAccAzureRMKustoDatabase_hotCachePeriod
=== RUN   TestAccAzureRMKustoEventHubDataConnection_basic
=== PAUSE TestAccAzureRMKustoEventHubDataConnection_basic
=== CONT  TestAccDataSourceAzureRMKustoCluster_basic
=== CONT  TestAccAzureRMKustoDatabase_basic
=== CONT  TestAccAzureRMKustoCluster_basic
=== CONT  TestAccAzureRMKustoCluster_withTags
=== CONT  TestAccAzureRMKustoDatabasePrincipal_basic
=== CONT  TestAccAzureRMKustoEventHubDataConnection_basic
=== CONT  TestAccAzureRMKustoCluster_sku
=== CONT  TestAccAzureRMKustoCluster_update
=== CONT  TestAccAzureRMKustoDatabase_hotCachePeriod
=== CONT  TestAccAzureRMKustoDatabase_softDeletePeriod
--- PASS: TestAccDataSourceAzureRMKustoCluster_basic (1341.51s)
--- PASS: TestAccAzureRMKustoCluster_basic (1521.29s)
--- PASS: TestAccAzureRMKustoCluster_withTags (1532.44s)
--- PASS: TestAccAzureRMKustoDatabase_softDeletePeriod (1596.07s)
--- PASS: TestAccAzureRMKustoDatabase_hotCachePeriod (1596.12s)
--- PASS: TestAccAzureRMKustoDatabase_basic (1643.31s)
--- PASS: TestAccAzureRMKustoDatabasePrincipal_basic (1654.12s)
--- PASS: TestAccAzureRMKustoEventHubDataConnection_basic (1717.40s)
--- PASS: TestAccAzureRMKustoCluster_sku (2509.78s)
--- PASS: TestAccAzureRMKustoCluster_update (2857.73s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/kusto/tests 2858.935s

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 @jrauschenbusch - LGTM now! 👍

@katbyte katbyte removed sdk/requires-upgrade This is dependent upon upgrading an SDK upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR labels Jun 16, 2020
@katbyte katbyte modified the milestones: Blocked, v2.15.0 Jun 16, 2020
@katbyte katbyte merged commit eec0ed6 into hashicorp:master Jun 16, 2020
katbyte added a commit that referenced this pull request Jun 16, 2020
@ghost
Copy link

ghost commented Jun 19, 2020

This has been released in version 2.15.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.15.0"
}
# ... other configuration ...

@jrauschenbusch jrauschenbusch deleted the upgrade-kusto-sdk branch July 9, 2020 05:15
@ghost
Copy link

ghost commented Jul 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 Jul 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

3 participants