From 21c486408c9b3a3020d3d04c6673deacf285d6cf Mon Sep 17 00:00:00 2001 From: Dominik Giger Date: Fri, 20 Sep 2024 16:10:11 +0200 Subject: [PATCH 1/3] Elasticsearch trust settings: Only add them to the update payload if there are changes. The trust setting should only be added to the payload when they change. If they are left out of the payload, no changes to the trust will be made. --- .../v2/elasticsearch_keystore_contents.go | 12 +- .../elasticsearch/v2/elasticsearch_payload.go | 23 +++- .../v2/elasticsearch_payload_test.go | 121 ++++++++++++++++++ 3 files changed, 141 insertions(+), 15 deletions(-) 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..b68266e32 100644 --- a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go +++ b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go @@ -65,9 +65,17 @@ func ElasticsearchPayload(ctx context.Context, plan types.Object, state *types.O return nil, nil } + var esState *ElasticsearchTF + if state != nil { + esState, diags = objectToElasticsearch(ctx, *state) + if diags.HasError() { + return nil, diags + } + } + 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() { @@ -160,11 +168,14 @@ func (es *ElasticsearchTF) payload(ctx context.Context, res *models.Elasticsearc res.Plan.AutoscalingEnabled = ec.Bool(es.Autoscale.ValueBool()) } - res.Settings, ds = elasticsearchTrustAccountPayload(ctx, es.TrustAccount, res.Settings) - diags.Append(ds...) + // Only add trust settings to update payload if trust has changed + if state == nil || !es.TrustAccount.Equal(state.TrustAccount) || !es.TrustExternal.Equal(state.TrustExternal) { + res.Settings, ds = elasticsearchTrustAccountPayload(ctx, es.TrustAccount, res.Settings) + diags.Append(ds...) - res.Settings, ds = elasticsearchTrustExternalPayload(ctx, es.TrustExternal, res.Settings) - diags.Append(ds...) + res.Settings, ds = elasticsearchTrustExternalPayload(ctx, es.TrustExternal, res.Settings) + diags.Append(ds...) + } res.Settings, ds = elasticsearchKeystoreContentsPayload(ctx, es.KeystoreContents, res.Settings, state) diags.Append(ds...) diff --git a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go index 2c99f44f7..524f93c2f 100644 --- a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go +++ b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload_test.go @@ -2575,6 +2575,127 @@ func Test_writeElasticsearch(t *testing.T) { }, }), }, + { + name: "don't put trust settings into the payload if they haven't changed", + args: args{ + esPlan: Elasticsearch{ + RefId: ec.String("main-elasticsearch"), + ResourceId: ec.String(mock.ValidClusterID), + Region: ec.String("some-region"), + TrustAccount: ElasticsearchTrustAccounts{ + { + AccountId: ec.String("id1"), + TrustAllowlist: []string{"a", "b"}, + }, + { + AccountId: ec.String("id2"), + TrustAll: ec.Bool(true), + }, + }, + TrustExternal: ElasticsearchTrustExternals{ + { + RelationshipId: ec.String("id3"), + TrustAll: ec.Bool(true), + }, + { + RelationshipId: ec.String("id4"), + TrustAllowlist: []string{"c", "d"}, + }, + }, + Strategy: ec.String("rolling_all"), + HotTier: &ElasticsearchTopology{ + id: "hot_content", + Size: ec.String("2g"), + ZoneCount: 1, + }, + }, + esState: &Elasticsearch{ + RefId: ec.String("main-elasticsearch"), + ResourceId: ec.String(mock.ValidClusterID), + Region: ec.String("some-region"), + TrustAccount: ElasticsearchTrustAccounts{ + { + AccountId: ec.String("id1"), + TrustAllowlist: []string{"a", "b"}, + }, + { + AccountId: ec.String("id2"), + TrustAll: ec.Bool(true), + }, + }, + TrustExternal: ElasticsearchTrustExternals{ + { + RelationshipId: ec.String("id3"), + TrustAll: ec.Bool(true), + }, + { + RelationshipId: ec.String("id4"), + TrustAllowlist: []string{"c", "d"}, + }, + }, + Strategy: ec.String("rolling_all"), + 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, + }, + Plan: &models.ElasticsearchClusterPlan{ + AutoscalingEnabled: ec.Bool(false), + Elasticsearch: &models.ElasticsearchConfiguration{ + Version: "7.7.0", + }, + DeploymentTemplate: &models.DeploymentTemplateReference{ + ID: ec.String("aws-io-optimized-v2"), + }, + Transient: &models.TransientElasticsearchPlanConfiguration{ + Strategy: &models.PlanStrategy{ + Rolling: &models.RollingStrategyConfig{GroupBy: "__all__"}, + }, + }, + ClusterTopology: []*models.ElasticsearchClusterTopologyElement{ + { + ID: "hot_content", + ZoneCount: 1, + InstanceConfigurationID: "aws.data.highio.i3", + Size: &models.TopologySize{ + Resource: ec.String("memory"), + Value: ec.Int32(2048), + }, + 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"), + }, + }, + }, + }, + }), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 724e9e3146baaccccc6d27c044456dd2dfa36c13 Mon Sep 17 00:00:00 2001 From: Dominik Giger Date: Mon, 23 Sep 2024 11:25:41 +0200 Subject: [PATCH 2/3] Add changelog --- .changelog/859.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/859.txt diff --git a/.changelog/859.txt b/.changelog/859.txt new file mode 100644 index 000000000..b755065f1 --- /dev/null +++ b/.changelog/859.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/deployment: Avoid sending an update for trust settings if they have not changed. +``` \ No newline at end of file From fb9c9517ce95401deb3c7b04a304007df3fe29b7 Mon Sep 17 00:00:00 2001 From: Dominik Giger Date: Mon, 23 Sep 2024 18:26:14 +0200 Subject: [PATCH 3/3] Update elasticsearch_payload.go --- .../elasticsearch/v2/elasticsearch_payload.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go index bcdbc1790..5c8a544c9 100644 --- a/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go +++ b/ec/ecresource/deploymentresource/elasticsearch/v2/elasticsearch_payload.go @@ -61,14 +61,6 @@ 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 }