From 98dfc0611b9f4b7d80f4b1ade5883d1676479902 Mon Sep 17 00:00:00 2001 From: Dominik Giger Date: Mon, 23 Sep 2024 18:25:22 +0200 Subject: [PATCH] Bugfix: Only pass snapshot settings into the update plan if there are changes (#858) - This ensures the settings are only put into the update request if there are actual changes in the config - Otherwise the settings are left out of the payload - This solves a bug where every apply would reset the policy back to the default (because putting the default settings in the plan will cause it to reset the policy). - When using the elasticstack provider to manage the policy, we want the ec provider to not touch it. --- .changelog/858.txt | 3 + .../v2/elasticsearch_keystore_contents.go | 12 +-- .../elasticsearch/v2/elasticsearch_payload.go | 14 ++- .../v2/elasticsearch_payload_test.go | 91 +++++++++++++++++++ .../v2/elasticsearch_snapshot.go | 8 +- 5 files changed, 115 insertions(+), 13 deletions(-) create mode 100644 .changelog/858.txt diff --git a/.changelog/858.txt b/.changelog/858.txt new file mode 100644 index 000000000..79267a025 --- /dev/null +++ b/.changelog/858.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/deployment: Avoid overriding snapshot settings with every update. The snapshot settings are now only updated if they are actually set in the terraform config. This allows managing the snapshot lifecycle policy with the elasticstack provider instead of the ec provider. +``` diff --git a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_keystore_contents.go b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_keystore_contents.go index b8a03f33c..9dc275e2d 100644 --- a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_keystore_contents.go +++ b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_keystore_contents.go @@ -37,10 +37,10 @@ type ElasticsearchKeystoreContents struct { AsFile *bool `tfsdk:"as_file"` } -func elasticsearchKeystoreContentsPayload(ctx context.Context, keystoreContentsTF types.Map, model *models.ElasticsearchClusterSettings, esStateObj *types.Object) (*models.ElasticsearchClusterSettings, diag.Diagnostics) { +func elasticsearchKeystoreContentsPayload(ctx context.Context, keystoreContentsTF types.Map, model *models.ElasticsearchClusterSettings, esState *ElasticsearchTF) (*models.ElasticsearchClusterSettings, diag.Diagnostics) { var diags diag.Diagnostics - if (keystoreContentsTF.IsNull() || len(keystoreContentsTF.Elements()) == 0) && esStateObj == nil { + if (keystoreContentsTF.IsNull() || len(keystoreContentsTF.Elements()) == 0) && esState == nil { return model, nil } @@ -69,13 +69,7 @@ func elasticsearchKeystoreContentsPayload(ctx context.Context, keystoreContentsT } // remove secrets that were in state but are removed from plan - if esStateObj != nil && !esStateObj.IsNull() { - var esState ElasticsearchTF - - if diags := tfsdk.ValueAs(ctx, esStateObj, &esState); diags.HasError() { - return nil, diags - } - + if esState != nil { if !esState.KeystoreContents.IsNull() { for k := range esState.KeystoreContents.Elements() { if _, ok := secrets[k]; !ok { diff --git a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go index 9c569b9d2..fc6d95229 100644 --- a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go +++ b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go @@ -61,13 +61,21 @@ func ElasticsearchPayload(ctx context.Context, plan types.Object, state *types.O return nil, diags } + var esState *ElasticsearchTF + if state != nil { + esState, diags = objectToElasticsearch(ctx, *state) + if diags.HasError() { + return nil, diags + } + } + if es == nil { return nil, nil } templatePayload := EnrichElasticsearchTemplate(payloadFromUpdate(updateResources), dtID, version, useNodeRoles) - payload, diags := es.payload(ctx, templatePayload, state) + payload, diags := es.payload(ctx, templatePayload, esState) if diags.HasError() { return nil, diags } @@ -123,7 +131,7 @@ func CheckAvailableMigration(ctx context.Context, plan types.Object, state types return false, nil } -func (es *ElasticsearchTF) payload(ctx context.Context, res *models.ElasticsearchPayload, state *types.Object) (*models.ElasticsearchPayload, diag.Diagnostics) { +func (es *ElasticsearchTF) payload(ctx context.Context, res *models.ElasticsearchPayload, state *ElasticsearchTF) (*models.ElasticsearchPayload, diag.Diagnostics) { var diags diag.Diagnostics if !es.RefId.IsNull() { @@ -149,7 +157,7 @@ func (es *ElasticsearchTF) payload(ctx context.Context, res *models.Elasticsearc res.Plan.Elasticsearch, ds = elasticsearchConfigPayload(ctx, es.Config, res.Plan.Elasticsearch) diags.Append(ds...) - res.Settings, ds = elasticsearchSnapshotPayload(ctx, es.Snapshot, res.Settings) + res.Settings, ds = elasticsearchSnapshotPayload(ctx, es.Snapshot, res.Settings, state) diags.Append(ds...) diags.Append(elasticsearchSnapshotSourcePayload(ctx, es.SnapshotSource, res.Plan)...) diff --git a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go index 2c99f44f7..b843e6912 100644 --- a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go +++ b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go @@ -2103,6 +2103,97 @@ func Test_writeElasticsearch(t *testing.T) { }, }), }, + { + name: "don't put snapshot settings into payload if there is no change between plan and state", + args: args{ + esPlan: Elasticsearch{ + RefId: ec.String("main-elasticsearch"), + ResourceId: ec.String(mock.ValidClusterID), + Region: ec.String("some-region"), + Snapshot: &ElasticsearchSnapshot{ + Enabled: true, + Repository: &ElasticsearchSnapshotRepositoryInfo{ + Reference: &ElasticsearchSnapshotRepositoryReference{ + RepositoryName: "my-snapshot-repository", + }, + }, + }, + HotTier: &ElasticsearchTopology{ + id: "hot_content", + Size: ec.String("4g"), + ZoneCount: 1, + }, + }, + esState: &Elasticsearch{ + RefId: ec.String("main-elasticsearch"), + ResourceId: ec.String(mock.ValidClusterID), + Region: ec.String("some-region"), + Snapshot: &ElasticsearchSnapshot{ + Enabled: true, + Repository: &ElasticsearchSnapshotRepositoryInfo{ + Reference: &ElasticsearchSnapshotRepositoryReference{ + RepositoryName: "my-snapshot-repository", + }, + }, + }, + HotTier: &ElasticsearchTopology{ + id: "hot_content", + Size: ec.String("2g"), + ZoneCount: 1, + }, + }, + updatePayloads: testutil.UpdatePayloadsFromTemplate(t, "../../testdata/template-aws-io-optimized-v2.json"), + templateID: "aws-io-optimized-v2", + version: "7.7.0", + useNodeRoles: false, + }, + want: EnrichWithEmptyTopologies(tp770(), &models.ElasticsearchPayload{ + Region: ec.String("some-region"), + RefID: ec.String("main-elasticsearch"), + Settings: &models.ElasticsearchClusterSettings{ + DedicatedMastersThreshold: 6, + Snapshot: nil, + }, + Plan: &models.ElasticsearchClusterPlan{ + AutoscalingEnabled: ec.Bool(false), + Elasticsearch: &models.ElasticsearchConfiguration{ + Version: "7.7.0", + }, + DeploymentTemplate: &models.DeploymentTemplateReference{ + ID: ec.String("aws-io-optimized-v2"), + }, + ClusterTopology: []*models.ElasticsearchClusterTopologyElement{ + { + ID: "hot_content", + ZoneCount: 1, + InstanceConfigurationID: "aws.data.highio.i3", + Size: &models.TopologySize{ + Resource: ec.String("memory"), + Value: ec.Int32(4096), + }, + NodeType: &models.ElasticsearchNodeType{ + Data: ec.Bool(true), + Ingest: ec.Bool(true), + Master: ec.Bool(true), + }, + Elasticsearch: &models.ElasticsearchConfiguration{ + NodeAttributes: map[string]string{"data": "hot"}, + }, + TopologyElementControl: &models.TopologyElementControl{ + Min: &models.TopologySize{ + Resource: ec.String("memory"), + Value: ec.Int32(1024), + }, + }, + AutoscalingMax: &models.TopologySize{ + Value: ec.Int32(118784), + Resource: ec.String("memory"), + }, + }, + }, + }, + }), + }, { name: "parse autodetect configuration strategy", args: args{ diff --git a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_snapshot.go b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_snapshot.go index 8b068d31d..d40583afb 100644 --- a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_snapshot.go +++ b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_snapshot.go @@ -78,12 +78,18 @@ func readElasticsearchSnapshot(in *models.ElasticsearchClusterSettings) (*Elasti return &snapshot, nil } -func elasticsearchSnapshotPayload(ctx context.Context, srcObj attr.Value, model *models.ElasticsearchClusterSettings) (*models.ElasticsearchClusterSettings, diag.Diagnostics) { +func elasticsearchSnapshotPayload(ctx context.Context, srcObj attr.Value, model *models.ElasticsearchClusterSettings, state *ElasticsearchTF) (*models.ElasticsearchClusterSettings, diag.Diagnostics) { var snapshot ElasticsearchSnapshotTF if srcObj.IsNull() || srcObj.IsUnknown() { return model, nil } + // Only put snapshot updates into the payload, if the plan is making changes to the snapshot settings + // (To avoid overwriting changes made outside, i.e. with the elasticstack provider) + if state != nil && state.Snapshot.Equal(srcObj) { + return model, nil + } + if diags := tfsdk.ValueAs(ctx, srcObj, &snapshot); diags.HasError() { return model, diags }