From 416c9ace70e2555dc16d48502d677e5b375aa0a2 Mon Sep 17 00:00:00 2001 From: k-yomo Date: Sat, 17 Dec 2022 03:49:04 +0900 Subject: [PATCH 1/8] Don't recreate index when field is removed --- docs/resources/elasticsearch_index.md | 4 +- internal/elasticsearch/index/index.go | 44 +++++++++++----------- internal/elasticsearch/index/index_test.go | 25 ++++++++++-- 3 files changed, 47 insertions(+), 26 deletions(-) diff --git a/docs/resources/elasticsearch_index.md b/docs/resources/elasticsearch_index.md index 5140ed127..c3ef1601c 100644 --- a/docs/resources/elasticsearch_index.md +++ b/docs/resources/elasticsearch_index.md @@ -88,7 +88,9 @@ resource "elasticstack_elasticsearch_index" "my_index" { - `load_fixed_bitset_filters_eagerly` (Boolean) Indicates whether cached filters are pre-loaded for nested queries. This can be set only on creation. - `mappings` (String) Mapping for fields in the index. If specified, this mapping can include: field names, [field data types](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html), [mapping parameters](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-params.html). -**NOTE:** changing datatypes in the existing _mappings_ will force index to be re-created. +**NOTE:** +- Changing datatypes in the existing _mappings_ will force index to be re-created. +- Removing field will be ignored by default same as elasticsearch. You need to recreate the index to remove field completely. - `max_docvalue_fields_search` (Number) The maximum number of `docvalue_fields` that are allowed in a query. - `max_inner_result_window` (Number) The maximum value of `from + size` for inner hits definition and top hits aggregations to this index. - `max_ngram_diff` (Number) The maximum allowed difference between min_gram and max_gram for NGramTokenizer and NGramTokenFilter. diff --git a/internal/elasticsearch/index/index.go b/internal/elasticsearch/index/index.go index 42f2cd3ba..5301dc5bc 100644 --- a/internal/elasticsearch/index/index.go +++ b/internal/elasticsearch/index/index.go @@ -471,7 +471,10 @@ func ResourceIndex() *schema.Resource { "mappings": { Description: `Mapping for fields in the index. If specified, this mapping can include: field names, [field data types](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html), [mapping parameters](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-params.html). -**NOTE:** changing datatypes in the existing _mappings_ will force index to be re-created.`, +**NOTE:** +- Changing datatypes in the existing _mappings_ will force index to be re-created. +- Removing field will be ignored by default same as elasticsearch. You need to recreate the index to remove field completely. +`, Type: schema.TypeString, Optional: true, DiffSuppressFunc: utils.DiffJsonSuppress, @@ -903,34 +906,31 @@ func resourceIndexDelete(ctx context.Context, d *schema.ResourceData, meta inter func IsMappingForceNewRequired(old map[string]interface{}, new map[string]interface{}) bool { for k, v := range old { oldFieldSettings := v.(map[string]interface{}) - if newFieldSettings, ok := new[k]; ok { - newSettings := newFieldSettings.(map[string]interface{}) - // check if the "type" field exists and match with new one - if s, ok := oldFieldSettings["type"]; ok { - if ns, ok := newSettings["type"]; ok { - if !reflect.DeepEqual(s, ns) { - return true - } - continue - } else { + newFieldSettings, ok := new[k] + // When field is removed, it'll be ignored in elasticsearch + if !ok { + continue + } + newSettings := newFieldSettings.(map[string]interface{}) + // check if the "type" field exists and match with new one + if s, ok := oldFieldSettings["type"]; ok { + if ns, ok := newSettings["type"]; ok { + if !reflect.DeepEqual(s, ns) { return true } + continue + } else { + return true } + } - // if we have "mapping" field, let's call ourself to check again - if s, ok := oldFieldSettings["properties"]; ok { - if ns, ok := newSettings["properties"]; ok { - if IsMappingForceNewRequired(s.(map[string]interface{}), ns.(map[string]interface{})) { - return true - } - continue - } else { + // if we have "mapping" field, let's call ourself to check again + if s, ok := oldFieldSettings["properties"]; ok { + if ns, ok := newSettings["properties"]; ok { + if IsMappingForceNewRequired(s.(map[string]interface{}), ns.(map[string]interface{})) { return true } } - } else { - // if the key not found in the new props, force new resource - return true } } return false diff --git a/internal/elasticsearch/index/index_test.go b/internal/elasticsearch/index/index_test.go index 82ce6eb4f..7d02ea363 100644 --- a/internal/elasticsearch/index/index_test.go +++ b/internal/elasticsearch/index/index_test.go @@ -1,6 +1,7 @@ package index_test import ( + "context" "fmt" "regexp" "testing" @@ -405,14 +406,32 @@ func Test_IsMappingForceNewRequired(t *testing.T) { want: true, }, { - name: "return true when field is removed", + name: "return false when field is removed", old: map[string]interface{}{ "field1": map[string]interface{}{ "type": "text", }, }, new: map[string]interface{}{}, - want: true, + want: false, + }, + { + name: "return false when dynamically added child property is removed", + old: map[string]interface{}{ + "parent": map[string]interface{}{ + "properties": map[string]interface{}{ + "child": map[string]interface{}{ + "type": "keyword", + }, + }, + }, + }, + new: map[string]interface{}{ + "parent": map[string]interface{}{ + "type": "object", + }, + }, + want: false, }, { name: "return true when child property's type changes", @@ -439,7 +458,7 @@ func Test_IsMappingForceNewRequired(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := index.IsMappingForceNewRequired(tt.old, tt.new); got != tt.want { + if got := index.IsMappingForceNewRequired(context.Background(), tt.old, tt.new); got != tt.want { t.Errorf("IsMappingForceNewRequired() = %v, want %v", got, tt.want) } }) From 30c1ba7c68654fa5ef2c9c0b076e4ec02e661703 Mon Sep 17 00:00:00 2001 From: k-yomo Date: Sat, 17 Dec 2022 03:55:34 +0900 Subject: [PATCH 2/8] Add warn log --- internal/elasticsearch/index/index.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/elasticsearch/index/index.go b/internal/elasticsearch/index/index.go index 5301dc5bc..c109ba63c 100644 --- a/internal/elasticsearch/index/index.go +++ b/internal/elasticsearch/index/index.go @@ -606,7 +606,7 @@ If specified, this mapping can include: field names, [field data types](https:// if !ok { return true } - return IsMappingForceNewRequired(oldProps.(map[string]interface{}), newProps.(map[string]interface{})) + return IsMappingForceNewRequired(ctx, oldProps.(map[string]interface{}), newProps.(map[string]interface{})) } // if all check passed, we can update the map @@ -903,12 +903,13 @@ func resourceIndexDelete(ctx context.Context, d *schema.ResourceData, meta inter return diags } -func IsMappingForceNewRequired(old map[string]interface{}, new map[string]interface{}) bool { +func IsMappingForceNewRequired(ctx context.Context, old map[string]interface{}, new map[string]interface{}) bool { for k, v := range old { oldFieldSettings := v.(map[string]interface{}) newFieldSettings, ok := new[k] // When field is removed, it'll be ignored in elasticsearch if !ok { + tflog.Warn(ctx, fmt.Sprintf("removing %s field in mappings is ignored, if you neeed to remove it completely, please recreate the index", k)) continue } newSettings := newFieldSettings.(map[string]interface{}) @@ -927,9 +928,11 @@ func IsMappingForceNewRequired(old map[string]interface{}, new map[string]interf // if we have "mapping" field, let's call ourself to check again if s, ok := oldFieldSettings["properties"]; ok { if ns, ok := newSettings["properties"]; ok { - if IsMappingForceNewRequired(s.(map[string]interface{}), ns.(map[string]interface{})) { + if IsMappingForceNewRequired(context.Background(), s.(map[string]interface{}), ns.(map[string]interface{})) { return true } + } else { + tflog.Warn(ctx, fmt.Sprintf("removing %s propeties in mappings is ignored, if you neeed to remove it completely, please recreate the index", k)) } } } From 5d0742e84a73b4ded45f97eb644480751159ba5c Mon Sep 17 00:00:00 2001 From: Kanji Yomoda Date: Mon, 19 Dec 2022 18:08:23 +0900 Subject: [PATCH 3/8] Update internal/elasticsearch/index/index.go Co-authored-by: Toby Brain --- internal/elasticsearch/index/index.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/elasticsearch/index/index.go b/internal/elasticsearch/index/index.go index c109ba63c..1b9dbebb8 100644 --- a/internal/elasticsearch/index/index.go +++ b/internal/elasticsearch/index/index.go @@ -909,7 +909,7 @@ func IsMappingForceNewRequired(ctx context.Context, old map[string]interface{}, newFieldSettings, ok := new[k] // When field is removed, it'll be ignored in elasticsearch if !ok { - tflog.Warn(ctx, fmt.Sprintf("removing %s field in mappings is ignored, if you neeed to remove it completely, please recreate the index", k)) + tflog.Warn(ctx, fmt.Sprintf("removing %s field in mappings is ignored. Re-index to remove the field completely.", k)) continue } newSettings := newFieldSettings.(map[string]interface{}) From 4efd5ef87a6a857ceeceb818f4e842b0806598d5 Mon Sep 17 00:00:00 2001 From: k-yomo Date: Mon, 19 Dec 2022 21:09:46 +0900 Subject: [PATCH 4/8] Add acceptance test for removing field --- internal/elasticsearch/index/index_test.go | 50 ++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/internal/elasticsearch/index/index_test.go b/internal/elasticsearch/index/index_test.go index 7d02ea363..6fbf1a4af 100644 --- a/internal/elasticsearch/index/index_test.go +++ b/internal/elasticsearch/index/index_test.go @@ -141,6 +141,21 @@ func TestAccResourceIndexSettingsConflict(t *testing.T) { }) } +func TestAccResourceIndexRemovingField(t *testing.T) { + indexName := sdkacctest.RandStringFromCharSet(22, sdkacctest.CharSetAlphaNum) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + CheckDestroy: checkResourceIndexDestroy, + ProtoV5ProviderFactories: acctest.Providers, + Steps: []resource.TestStep{ + // Confirm removing field doesn't produce recreate by using prevent_destroy + {Config: testAccResourceIndexRemovingFieldCreate(indexName)}, + {Config: testAccResourceIndexRemovingFieldUpdate(indexName)}, + }, + }) +} + func testAccResourceIndexCreate(name string) string { return fmt.Sprintf(` provider "elasticstack" { @@ -341,6 +356,41 @@ resource "elasticstack_elasticsearch_index" "test_settings_conflict" { `, name) } +func testAccResourceIndexRemovingFieldCreate(name string) string { + return fmt.Sprintf(` +resource "elasticstack_elasticsearch_index" "test_settings_removing_field" { + name = "%s" + + mappings = jsonencode({ + properties = { + field1 = { type = "text" } + field2 = { type = "text" } + } + }) + lifecycle { + prevent_destroy = true + } +} + `, name) +} + +func testAccResourceIndexRemovingFieldUpdate(name string) string { + return fmt.Sprintf(` +resource "elasticstack_elasticsearch_index" "test_settings_removing_field" { + name = "%s" + + mappings = jsonencode({ + properties = { + field1 = { type = "text" } + } + }) + lifecycle { + prevent_destroy = true + } +} + `, name) +} + func checkResourceIndexDestroy(s *terraform.State) error { client, err := clients.NewAcceptanceTestingClient() if err != nil { From eb06fbcd6dd5bb1623aa86917442349d5ba1242e Mon Sep 17 00:00:00 2001 From: k-yomo Date: Mon, 19 Dec 2022 21:15:13 +0900 Subject: [PATCH 5/8] Fix test --- internal/elasticsearch/index/index_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/elasticsearch/index/index_test.go b/internal/elasticsearch/index/index_test.go index 6fbf1a4af..6bb8b5778 100644 --- a/internal/elasticsearch/index/index_test.go +++ b/internal/elasticsearch/index/index_test.go @@ -358,6 +358,10 @@ resource "elasticstack_elasticsearch_index" "test_settings_conflict" { func testAccResourceIndexRemovingFieldCreate(name string) string { return fmt.Sprintf(` +provider "elasticstack" { + elasticsearch {} +} + resource "elasticstack_elasticsearch_index" "test_settings_removing_field" { name = "%s" @@ -376,6 +380,10 @@ resource "elasticstack_elasticsearch_index" "test_settings_removing_field" { func testAccResourceIndexRemovingFieldUpdate(name string) string { return fmt.Sprintf(` +provider "elasticstack" { + elasticsearch {} +} + resource "elasticstack_elasticsearch_index" "test_settings_removing_field" { name = "%s" From 29d1b9319d154bed1aa40fd092a757b4725c9022 Mon Sep 17 00:00:00 2001 From: k-yomo Date: Mon, 19 Dec 2022 21:21:33 +0900 Subject: [PATCH 6/8] Fix failing destroy in test --- internal/elasticsearch/index/index_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/internal/elasticsearch/index/index_test.go b/internal/elasticsearch/index/index_test.go index 6bb8b5778..a919bf90a 100644 --- a/internal/elasticsearch/index/index_test.go +++ b/internal/elasticsearch/index/index_test.go @@ -152,6 +152,7 @@ func TestAccResourceIndexRemovingField(t *testing.T) { // Confirm removing field doesn't produce recreate by using prevent_destroy {Config: testAccResourceIndexRemovingFieldCreate(indexName)}, {Config: testAccResourceIndexRemovingFieldUpdate(indexName)}, + {Config: testAccResourceIndexRemovingFieldPostUpdate(indexName)}, }, }) } @@ -399,6 +400,24 @@ resource "elasticstack_elasticsearch_index" "test_settings_removing_field" { `, name) } +func testAccResourceIndexRemovingFieldPostUpdate(name string) string { + return fmt.Sprintf(` +provider "elasticstack" { + elasticsearch {} +} + +resource "elasticstack_elasticsearch_index" "test_settings_removing_field" { + name = "%s" + + mappings = jsonencode({ + properties = { + field1 = { type = "text" } + } + }) +} + `, name) +} + func checkResourceIndexDestroy(s *terraform.State) error { client, err := clients.NewAcceptanceTestingClient() if err != nil { From ba3227883e2d069cc71aab4521ebb61d7002ea04 Mon Sep 17 00:00:00 2001 From: k-yomo Date: Mon, 19 Dec 2022 21:29:54 +0900 Subject: [PATCH 7/8] Fix non-empty plan failure --- internal/elasticsearch/index/index_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/elasticsearch/index/index_test.go b/internal/elasticsearch/index/index_test.go index a919bf90a..6cb54e56c 100644 --- a/internal/elasticsearch/index/index_test.go +++ b/internal/elasticsearch/index/index_test.go @@ -151,8 +151,8 @@ func TestAccResourceIndexRemovingField(t *testing.T) { Steps: []resource.TestStep{ // Confirm removing field doesn't produce recreate by using prevent_destroy {Config: testAccResourceIndexRemovingFieldCreate(indexName)}, - {Config: testAccResourceIndexRemovingFieldUpdate(indexName)}, - {Config: testAccResourceIndexRemovingFieldPostUpdate(indexName)}, + {Config: testAccResourceIndexRemovingFieldUpdate(indexName), ExpectNonEmptyPlan: true}, + {Config: testAccResourceIndexRemovingFieldPostUpdate(indexName), ExpectNonEmptyPlan: true}, }, }) } From dad2f8e5b32095be183f3b461c8f9a40e65a866c Mon Sep 17 00:00:00 2001 From: k-yomo Date: Mon, 19 Dec 2022 21:35:19 +0900 Subject: [PATCH 8/8] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20c33eb9b..2f3b9b781 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### Fixed - Respect `ignore_unavailable` and `include_global_state` values when configuring SLM policies ([#224](https://github.com/elastic/terraform-provider-elasticstack/pull/224)) - Refactor API client functions and return diagnostics ([#220](https://github.com/elastic/terraform-provider-elasticstack/pull/220)) +- Fix not to recreate index when field is removed from mapping ([#232](https://github.com/elastic/terraform-provider-elasticstack/pull/232)) ## [0.5.0] - 2022-12-07