chore: Updates `mongodbatlas_advanced_cluster` (SDKv2 & TPF) to consume selected processArgs fields from the createCluster/updateCluster APIs #3231

maastha · 2025-03-31T17:25:19Z

Description

This PR updates Atlas SDK v20240805 generated from api-bot-update-v20240805-backport-cluster branch (mongodb/atlas-sdk-go#576)

Link to any related issue(s): CLOUDP-296222

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contributing guides
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals I have added appropriate changelog entries.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

maastha · 2025-04-02T14:33:34Z

internal/service/advancedcluster/data_source_advanced_cluster.go

@@ -348,7 +348,7 @@ func dataSourceRead(ctx context.Context, d *schema.ResourceData, meta any) diag.
 		return diag.FromErr(fmt.Errorf(ErrorAdvancedConfRead, "", clusterName, err))
 	}

-	if err := d.Set("advanced_configuration", flattenProcessArgs(processArgs20240530, processArgs)); err != nil {
+	if err := d.Set("advanced_configuration", flattenProcessArgs(processArgs20240530, processArgs, clusterDesc.AdvancedConfiguration)); err != nil {


although not an absolute requirement for Read, but still fetching these from cluster response (instead of /processArgs) for consistency.

maastha · 2025-04-02T14:49:56Z

internal/service/advancedcluster/resource_advanced_cluster.go

@@ -939,7 +945,7 @@ func resourceUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.
 			}
 			waitOnUpdate = true
 		}
-		if d.HasChange("replica_set_scaling_strategy") || d.HasChange("redact_client_log_data") || d.HasChange("config_server_management_mode") {
+		if d.HasChange("replica_set_scaling_strategy") || d.HasChange("redact_client_log_data") || d.HasChange("config_server_management_mode") || d.HasChange("advanced_configuration") {


@AgustinBettati can you confirm if this should be okay or we should update this in the above call (connV220240530.ClustersApi.UpdateCluster #L943)? In that case, will need the SDK for that to be updated as well to support the new AdvancedConfiguration attribute.

I don't see a concern here, as we are still respecting what the comment below mentions when calling the latest API version.

can call latest API (2024-10-23 or newer) as replications specs (with nested autoscaling property) is not specified

In this case I understand we only add the advancedConfiguration property but no replicationSpecs, meaning no risk.

maastha · 2025-04-02T18:03:39Z

internal/service/advancedclustertpf/resource.go

 	if diags.HasError() {
 		return
 	}
-	AddAdvancedConfig(ctx, modelOut, advConfig, legacyAdvConfig, diags)


updateModelAdvancedConfig already calls AddAdvancedConfig(), so this is not required

marcosuma · 2025-04-03T09:58:16Z

internal/service/advancedclustertpf/model_ClusterDescriptionProcessArgs20240805.go

 			NoTableScan:                      types.BoolValue(conversion.SafeValue(input.NoTableScan)),
 			OplogMinRetentionHours:           types.Float64Value(conversion.SafeValue(input.OplogMinRetentionHours)),
 			OplogSizeMb:                      types.Int64Value(conversion.SafeValue(conversion.IntPtrToInt64Ptr(input.OplogSizeMB))),
 			SampleSizeBiconnector:            types.Int64Value(conversion.SafeValue(conversion.IntPtrToInt64Ptr(input.SampleSizeBIConnector))),
 			SampleRefreshIntervalBiconnector: types.Int64Value(conversion.SafeValue(conversion.IntPtrToInt64Ptr(input.SampleRefreshIntervalBIConnector))),
 			TransactionLifetimeLimitSeconds:  types.Int64Value(conversion.SafeValue(input.TransactionLifetimeLimitSeconds)),
 			DefaultMaxTimeMS:                 types.Int64PointerValue(conversion.IntPtrToInt64Ptr(input.DefaultMaxTimeMS)),
-			TlsCipherConfigMode:              types.StringValue(conversion.SafeValue(input.TlsCipherConfigMode)),
+			MinimumEnabledTlsProtocol:        types.StringValue(conversion.SafeValue(clusterAdvConfig.MinimumEnabledTlsProtocol)),


nit: you moved this from line 42 intentionally?

didn't remove anything, just moved MinimumEnabledTlsProtocol here

yup, I said moved :) just wanted to check if you moved it intentionally and if there was a reason

lol I misread, yes moved intentionally just to keep these together for readability since they are both fetched from clusterAdvConfig

marcosuma

from a first look, change LGTM - are you running all cluster tests? I remember we disabled some from regular PR execution.

maastha · 2025-04-03T10:18:44Z

from a first look, change LGTM - are you running all cluster tests? I remember we disabled some from regular PR execution.

Yes I'm running them separately for this branch, will add links soon

oarbusi · 2025-04-03T13:10:47Z

internal/service/advancedcluster/model_advanced_cluster.go

+			p := aclist[0].(map[string]any)
+
+			if _, ok := d.GetOkExists("advanced_configuration.0.minimum_enabled_tls_protocol"); ok {
+				res.MinimumEnabledTlsProtocol = conversion.StringPtr(cast.ToString(p["minimum_enabled_tls_protocol"]))


nit: cast p["minimum_enabled_tls_protocol"].(string) instead of using the current way

oarbusi · 2025-04-03T13:13:07Z

internal/service/advancedcluster/resource_advanced_cluster.go

@@ -1148,6 +1150,11 @@ func updateRequest(ctx context.Context, d *schema.ResourceData, projectID, clust
 	if d.HasChange("config_server_management_mode") {
 		cluster.ConfigServerManagementMode = conversion.StringPtr(d.Get("config_server_management_mode").(string))
 	}
+	if d.HasChange("advanced_configuration") {
+		if aclist, ok := d.Get("advanced_configuration").([]any); ok && len(aclist) > 0 {


This means that removing advanced_configuration from the config will never update?

That's the behavior today as well right? since it's O/C, or did you mean something else?

oarbusi

Changes LGMT overall :)

maastha · 2025-04-03T21:10:00Z

All acceptance test runs succeed:
advanced_cluster: https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/14248157843
advanced_cluster_tpf: https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/14248151341

lantoli · 2025-04-04T05:49:11Z

internal/service/advancedclustertpf/common_admin_sdk.go

@@ -100,6 +100,11 @@ func UpdateAdvancedConfiguration(ctx context.Context, diags *diag.Diagnostics, c
 			return nil, nil, false
 		}
 	}
+	// clusterAdvConfig is managed through create/updateCluster APIs instead of processArgs APIs but since corresponding TF attributes


I think it can be confusing that UpdateAdvancedConfiguration doesn't update clusterAdvConfig, can we move this comment to be in the func description? although not ideal but I think it's clearer

also func is having many params, consider using a request struct

moved comment & updated to use struct, ty!

lantoli · 2025-04-04T05:53:06Z

internal/service/advancedcluster/model_advanced_cluster.go

+	if clusterAdvConfig != nil {
+		flattenedProcessArgs[0]["tls_cipher_config_mode"] = clusterAdvConfig.GetTlsCipherConfigMode()
+		flattenedProcessArgs[0]["custom_openssl_cipher_config_tls12"] = clusterAdvConfig.GetCustomOpensslCipherConfigTls12()
+		flattenedProcessArgs[0]["minimum_enabled_tls_protocol"] = clusterAdvConfig.GetMinimumEnabledTlsProtocol()


can it happen that minimum_enabled_tls_protocol exists in p20240530 but not in clusterAdvConfig? in case it makes sense to keep reading it from p20240530, and override if present at clusterAdvConfig

lantoli · 2025-04-04T05:57:07Z

internal/service/advancedclustertpf/model_ClusterDescriptionProcessArgs20240805.go

 	var advancedConfig TFAdvancedConfigurationModel
-	if input != nil && inputLegacy != nil {
+	if input != nil && inputLegacy != nil && clusterAdvConfig != nil {


can it happen that clusterAdvConfig is nil and we want at least to see the other params that don't depend on it?

yes, recently discovered for TENANT cluster type this may be the case, updated.

lantoli · 2025-04-04T06:00:05Z

go.mod

@@ -33,7 +33,7 @@ require (
 	github.com/zclconf/go-cty v1.16.2
 	go.mongodb.org/atlas v0.37.0
 	go.mongodb.org/atlas-sdk/v20240530005 v20240530005.0.0
-	go.mongodb.org/atlas-sdk/v20240805005 v20240805005.0.0
+	go.mongodb.org/atlas-sdk/v20240805005 v20240805005.0.1-0.20250402112219-2468c5354718 // uses api-bot-update-v20240805-backport-cluster to support AdvancedConfiguration in create/updateCluster APIs


I think there are no tests changes, can we add or modify tests to check these new attributes are set correctly? for instance in some tests first steps to check in the data source the attribute values

existing tests do check all these values to be set as expected

terraform-provider-mongodbatlas/internal/service/advancedcluster/resource_advanced_cluster_test.go

Line 2140 in 1335955

func checkAdvanced(usePreviewProvider bool, name, tls string, processArgs *admin.ClusterDescriptionProcessArgs20240805) resource.TestCheckFunc {

AgustinBettati

LGTM

AgustinBettati · 2025-04-04T09:10:58Z

internal/service/advancedclustertpf/common_admin_sdk.go

+	if !update.IsZeroValues(clusterAdvConfig) {
+		changed = true
+	}


q: If clusterAdvConfig made some changes, is there a need to flag the changed here? If changes are made only in clusterAdvConfig I would expect once cluster is IDLE there is not need to detect changes here as well.

changed is required to be flagged here so that the advanced_configuration in the TF model is updated (which is required to be done if any of the params change)

maastha · 2025-04-08T13:11:06Z

Will re-run complete acc test group advanced_cluster & TPF in sometime before merge as dev is unstable right now when creating new projects

lantoli

LGTM , thanks for the follow-ups

EspenAlbert · 2025-04-09T07:00:40Z

internal/service/advancedclustertpf/common.go

@@ -20,6 +23,12 @@ var (
 	ErrLegacyIgnoreLabel = fmt.Errorf("label `%s` is not supported as it is reserved for internal purposes", LegacyIgnoredLabelKey)
 )

+type ProcessArgs struct {


Nice refactoring

EspenAlbert

Great job!

… fields from the createCluster/updateCluster APIs (#3250)

maastha · 2025-04-10T12:43:11Z

only 1 cluster test failing due to OUT_OF_CAPACITY, merging

init

6a7b365

maastha changed the title ~~chore: init~~ chore: Updates mongodbatlas_advanced_cluster (SDKv2) to consume selected processArgs fields from the createCluster/updateCluster Mar 31, 2025

maastha added 10 commits March 31, 2025 18:31

lint

e26f2f1

TPF

be7e3ab

TPF read

265b6c0

clean

fbb5a9e

update go lib

a8435cd

Merge branch 'master' into CLOUDP-296222-adv_conf_cluster_api

5a8fd9f

minor

1b48237

lint

3990691

ut fix

d836ff2

mock tests

2c17d0e

maastha commented Apr 2, 2025

View reviewed changes

cleanup

444778f

maastha commented Apr 2, 2025

View reviewed changes

maastha marked this pull request as ready for review April 3, 2025 08:49

maastha requested a review from a team as a code owner April 3, 2025 08:49

maastha and others added 2 commits April 3, 2025 10:21

revert

18523bf

nit

8e9ab05

marcosuma reviewed Apr 3, 2025

View reviewed changes

oarbusi reviewed Apr 3, 2025

View reviewed changes

oarbusi approved these changes Apr 3, 2025

View reviewed changes

adv config change

9f85896

cleanup

ff69fc8

marcosuma approved these changes Apr 4, 2025

View reviewed changes

lantoli reviewed Apr 4, 2025

View reviewed changes

lantoli approved these changes Apr 4, 2025

View reviewed changes

AgustinBettati approved these changes Apr 4, 2025

View reviewed changes

maastha added 5 commits April 7, 2025 23:07

address PR comments

dcf2c23

Merge branch 'master' into CLOUDP-296222-adv_conf_cluster_api

1cd04a4

min

8dc6f53

min

5262ec8

nit

1335955

lantoli approved these changes Apr 8, 2025

View reviewed changes

nit

092c0f3

EspenAlbert reviewed Apr 9, 2025

View reviewed changes

EspenAlbert approved these changes Apr 9, 2025

View reviewed changes

chore: Updates mongodbatlas_cluster to consume selected processArgs…

5e9889b

… fields from the createCluster/updateCluster APIs (#3250)

maastha merged commit 96d18c0 into master Apr 10, 2025
37 of 40 checks passed

maastha deleted the CLOUDP-296222-adv_conf_cluster_api branch April 10, 2025 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Updates `mongodbatlas_advanced_cluster` (SDKv2 & TPF) to consume selected processArgs fields from the createCluster/updateCluster APIs #3231

chore: Updates `mongodbatlas_advanced_cluster` (SDKv2 & TPF) to consume selected processArgs fields from the createCluster/updateCluster APIs #3231

maastha commented Mar 31, 2025 •

edited

Loading

maastha Apr 2, 2025 •

edited

Loading

maastha Apr 2, 2025

AgustinBettati Apr 2, 2025

maastha Apr 2, 2025

marcosuma Apr 3, 2025

maastha Apr 3, 2025

marcosuma Apr 3, 2025

maastha Apr 3, 2025

marcosuma left a comment

maastha commented Apr 3, 2025

oarbusi Apr 3, 2025 •

edited

Loading

oarbusi Apr 3, 2025

maastha Apr 8, 2025

oarbusi left a comment

maastha commented Apr 3, 2025

lantoli Apr 4, 2025

lantoli Apr 4, 2025

maastha Apr 8, 2025

lantoli Apr 4, 2025

maastha Apr 8, 2025

lantoli Apr 4, 2025

maastha Apr 8, 2025

lantoli Apr 4, 2025

maastha Apr 8, 2025

AgustinBettati left a comment

AgustinBettati Apr 4, 2025

maastha Apr 8, 2025

maastha commented Apr 8, 2025 •

edited

Loading

lantoli left a comment

EspenAlbert Apr 9, 2025

EspenAlbert left a comment

maastha commented Apr 10, 2025

chore: Updates mongodbatlas_advanced_cluster (SDKv2 & TPF) to consume selected processArgs fields from the createCluster/updateCluster APIs #3231

chore: Updates mongodbatlas_advanced_cluster (SDKv2 & TPF) to consume selected processArgs fields from the createCluster/updateCluster APIs #3231

Conversation

maastha commented Mar 31, 2025 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

maastha Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosuma left a comment

Choose a reason for hiding this comment

maastha commented Apr 3, 2025

oarbusi Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oarbusi left a comment

Choose a reason for hiding this comment

maastha commented Apr 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgustinBettati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maastha commented Apr 8, 2025 • edited Loading

lantoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EspenAlbert left a comment

Choose a reason for hiding this comment

maastha commented Apr 10, 2025

chore: Updates `mongodbatlas_advanced_cluster` (SDKv2 & TPF) to consume selected processArgs fields from the createCluster/updateCluster APIs #3231

chore: Updates `mongodbatlas_advanced_cluster` (SDKv2 & TPF) to consume selected processArgs fields from the createCluster/updateCluster APIs #3231

maastha commented Mar 31, 2025 •

edited

Loading

maastha Apr 2, 2025 •

edited

Loading

oarbusi Apr 3, 2025 •

edited

Loading

maastha commented Apr 8, 2025 •

edited

Loading