New Data Source/Resource: azurerm_elasticsearch

[utkarshjain1508]

Adding elastic plugin for azure resource provider

[ms-henglu]

This function is used in both create and update, so we need to know whether this is a new resource by d.IsNewResource(), because if it's a new resource, it must not exist otherwise we'll throw ImportAsExistsError.

       if d.IsNewResource() {
		existing, err := client.Get(ctx, id.ResourceGroup, id.MonitorName, id.RuleSetName)
		if err != nil {
			if !utils.ResponseWasNotFound(existing.Response) {
				return fmt.Errorf("checking for existing %q: %+v", id, err)
			}
		}
		if !utils.ResponseWasNotFound(existing.Response) {
			return tf.ImportAsExistsError("azurerm_elastic_tag_rule", id.ID())
		}
	}

[ms-henglu]

@utkarshjain1508 , thanks for the PR! I took a loot at it, overall looks good but I have some minor comments to address that I've left inline.

[utkarshjain1508]

@tombuildsstuff can you please trigger the workflows ?

[utkarshjain1508]

@tombuildsstuff can you review and approve the PR ? All checks have passed and review comments resolved.

[ms-henglu]

@utkarshjain1508 , would you please change the title to be more descriptive like "new resource azurerm_elastic_monitor and azurerm_elastic_tag_rule"

[ms-henglu]

Thanks @utkarshjain1508 for the PR. Most part looks good to me, though there still are some minor changes need to address.

[ms-henglu]

No need to set it nil here since we're removing this resource.

[utkarshjain1508]

will be updated in the next commit.

[utkarshjain1508]

@ms-henglu I checked our RP and it seems we do not support delete request explicitly for this resource type. We set the resource to null and update it as a way of deletion. Let me know if this is acceptable.

[ms-henglu]

I think we should use client.Delete?

[utkarshjain1508]

will be updated in the next commit

[utkarshjain1508]

Reference previous conversation https://github.com/hashicorp/terraform-provider-azurerm/pull/14821#discussion_r795040966

[utkarshjain1508]

@tombuildsstuff this PR has been open for a very long time. Can we prioritize this ?

[tombuildsstuff]

hey @utkarshjain1508

Thanks for this PR - apologies for the delayed review here.

Due to the size of this PR with the Embedded (Legacy) SDK, I ended up pulling this down locally yesterday to review it and understand the use-case a little more fully.

Having spent some time investigating this, I believe there's a number of design changes we need to make here:

1. Whilst the Azure API defines these as two separate resources (Monitor and TagRule) - there's a 1:1 relationship between the two, as such it's worth combining these into a single resource.
2. The names of these resources in the Azure API (again Monitor and TagRule) differs substantially from the terminology used in the Azure Portal (Azure Elastic Stack) and marketing documentation (Elastic on Azure) - as such we feel that naming this single resource azurerm_elastic_stack would make more sense to end-users.
3. Moving the properties to the top-level, since these are all related to the resource (regardless if they're user or deployment level) - meaning that the nesting is unnecessary.
4. The SKU being used for testing (staging_Monthly) appears unavailable outside of the (staging?) environment - having deployed one in the Portal it appears the (only?) possible value at this time is ess-monthly-consumption_Monthly, so I've updated the tests for now. However since this appears to be the only possible value, and the size of the cluster is managed separately within the Elastic Portal (and doesn't appear to be updatable via ARM) - I'm wondering if this is the only fixed value so we should be defaulting this/if there's a fixed set of values we should be checking for here?

Since these are some larger changes I hope you don't mind but I've gone ahead and made those changes in this branch - and at the same time introduced a Data Source for the Elastic Stack cluster.

Running the tests for those unfortunately it appears there's an issue in either the Elastic ARM API or the Elastic Cloud API (which presumably ARM is calling to tear-down the cluster) - where during the deletion of the Elastic Stack the Resource Group is also being deleted. I didn't check whether this behaviour also happens if the Resource Group contains other items, however this behaviour causes an issue in Terraform and as such needs to be fixed in either the Azure / Elastic API to be able to get this supported in Terraform.

As this is a bug in the upstream API, I've opened this issue on the Azure Rest API Specs repository to track this - where once this is fixed we should be able to run the tests and get this merged.

Thanks!

[utkarshjain1508]

@tombuildsstuff can you send us list of subscriptions you were using while testing ?

[utkarshjain1508]

@koikonom @tombuildsstuff We have discussed the issue for the resource name with elasticsearch team and PMs. We have decided to go aheadwith the name azurerm_elasticsearch. I have made the changes and pushed. Please have a look.

[tombuildsstuff]

@utkarshjain1508 as mentioned above unfortunately this name would be misleading.. are there any other suggestions from the team? azurerm_elastic_cloud_elasticsearch would explain what this is actually deploying, for example.

[utkarshjain1508]

@karansinghkushwah Your thoughts on this ?

[karansinghkushwah]

@utkarshjain1508 as mentioned above unfortunately this name would be misleading.. are there any other suggestions from the team? azurerm_elastic_cloud_elasticsearch would explain what this is actually deploying, for example#16531

@osmanis, @rotata - Can you please share your perspective here?

// cc: @hashah-msft

[hemantmalik]

@karansinghkushwah IMO azurerm_elastic_cloud_elasticsearch does make sense since it aligns with Marketplace naming and the user experience in Azure Portal.

[tombuildsstuff]

@hemantmalik @utkarshjain1508 thanks for confirming - let's go with azurerm_elastic_cloud_elasticsearch then 👍

I think the only other blocker here is that the API failed when the email contained the word test - has that been fixed?

[utkarshjain1508]

@tombuildsstuff That failure was not due to email containing "test" but due to some other issue. That has been fixed. You can see that current acceptance tests have the email with word test.

[koikonom]

Hello @utkarshjain1508

Unfortunately the acceptance tests are still not passing. The first issue is this:

------- Stdout: -------
=== RUN   TestAccElasticsearchDataSource_basic
=== PAUSE TestAccElasticsearchDataSource_basic
=== CONT  TestAccElasticsearchDataSource_basic
=== CONT  TestAccElasticsearchDataSource_basic
testing_new_config.go:111: no "id" found in attributes
testing_new.go:53: no "id" found in attributes
--- FAIL: TestAccElasticsearchDataSource_basic (378.18s)

This is happening because no ID is being set in the data source's Read() method. I fixed it (see 6d98ca9 ) but now the tests are failing because vairous checks fail too.

For example:

testcase.go:110: Step 1/1 error: Check failed: Check 5/12 error: azurerm_elasticsearch.test: Attribute 'logs' expected to be set

Please revisit this test.

[utkarshjain1508]

@koikonom let mecheck that.

[utkarshjain1508]

@koikonom I have made changes to the resource name and fixed the acceptance tests. They seem to be passing on my local now.

[github-actions]

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.