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 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..1b9dbebb8 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, @@ -603,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 @@ -900,37 +903,37 @@ 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{}) - 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 { + 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{}) + // 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(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)) } - } 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..6cb54e56c 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" @@ -140,6 +141,22 @@ 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), ExpectNonEmptyPlan: true}, + {Config: testAccResourceIndexRemovingFieldPostUpdate(indexName), ExpectNonEmptyPlan: true}, + }, + }) +} + func testAccResourceIndexCreate(name string) string { return fmt.Sprintf(` provider "elasticstack" { @@ -340,6 +357,67 @@ resource "elasticstack_elasticsearch_index" "test_settings_conflict" { `, name) } +func testAccResourceIndexRemovingFieldCreate(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" } + field2 = { type = "text" } + } + }) + lifecycle { + prevent_destroy = true + } +} + `, name) +} + +func testAccResourceIndexRemovingFieldUpdate(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" } + } + }) + lifecycle { + prevent_destroy = true + } +} + `, 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 { @@ -405,14 +483,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 +535,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) } })