chore: Adds equivalent CRUD logic for Advanced Cluster TPF

[EspenAlbert]

Description

Out of Scope

[TF] Adv Cluster TPF Update docs related fields

• Improve error summaries
• Add schema attribute descriptions

[TF] Adv Cluster TPF Test Coverage and Cleanup

• Run All AdvancedCluster Acceptance tests
• Cleanup tc directory
• Add advanced_cluster tests
  • TenantUpgrade
  • Replicaset (updates to advanced_config for the same cluster)

In Scope

• All logic in CRUD operations. Summarized changes:
• Removed UseStateForUnknown (see discussion below)
• Adds advanced_configuration.0.oplog_size_mb validation similar to legacy
• Retain backups enabled sent in delete
• Fail on projectID & clusterName updates
• Fix replication_spec.*.id/external_id
• Support caching results of a providerName -> ContainerIds

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

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 defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.
• 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

[EspenAlbert]

TODOs, decisions made:

• TODO: StateFunc: FormatMongoDBMajorVersion, current implementation supports both 8 and 8.0 in config
• TODO: MaxItems: 1 Will be handled in the next minimize breaking changes milestone
• TODO: Validation changes
  • num_shards Decide to remove validation validation.IntBetween(1, 50)
  • region_name Decide to remove validation validate.StringIsUppercase()
  • provider_name Decide to remove validation validate.StringIsUppercase()
  • version_release_system Decide to remove validation validation.StringInSlice([]string{"LTS", "CONTINUOUS"}, false)
• TODO: We want to avoid breaking changes even though it is incompatible with flex cluster and project resource
  • labels|tags Choose to avoid breaking changes by keeping schema.SetNestedAttribute
• TODO: Generation todos
  • cluster_id was generated as id
  • mongo_db_major_version was generated as mongo_dbmajor_version
  • mongo_db_version was generated as mongo_dbversion
  • sample_refresh_interval_bi_connector was sample_refresh_interval_biconnector
  • sample_size_bi_connector was sample_size_biconnector
  • advanced_configuration generated from processArgs endpoint
    • Extra attributes added manually
      • num_shards
      • external_id

[EspenAlbert]

As we need to read the LegacyInfo to populate the old replication_spec.*.id it didn't make sense to keep two structs. Now a single ExtraAPIInfo is used.

[EspenAlbert]

Only updating the changed fields doesn't work, we will get errors similar to below snippet when trying to only update AdvancedConfiguration and leaving the rest empty:
(In the error below it complains about timeouts, but it is also all other empty values)

  | Error: Value Conversion Error
  | 
  |   with mongodbatlas_advanced_cluster.test,
  |   on terraform_plugin_test.tf line 13, in resource "mongodbatlas_advanced_cluster" "test":
  |   13: \t\t\ttimeouts = {
  |   14: \t\t\t\tcreate = "20s"
  |   15: \t\t\t}
  | 
  | An unexpected error was encountered while verifying an attribute value
  | matched its expected type to prevent unexpected behavior or panics. This is
  | always an error in the provider. Please report the following to the provider
  | developer:
  | 
  | Expected framework type from provider logic: timeouts.Type / underlying type:
  | tftypes.Object["create":tftypes.String, "delete":tftypes.String,
  | "update":tftypes.String]
  | Received framework type from provider logic: timeouts.Type / underlying type:
  | tftypes.Object[] # NOTICE HOW THE EMPTY Object is used, using correct AttributeTypes but empty values would RESET the state.
  | Path: timeouts

However, we can workaround this by using the values from the state.
In this PR we support using state values when:

1. No changes to advanced_configuration, use value from state instead of re-reading

• As state will contain latest values from the API, we can be sure any computed attributes are updated. Thank you @lantoli for the demo, changed back in 71e3119

2. No changes to cluster, only update advanced config, see the new updateAdvConfig method

[EspenAlbert]

This approach differs slightly from legacy implementation but I argue it is better:

1. Supports raising errors during planning instead of during apply
2. Reusable
3. Easy to test, see resource_schema_test.go

[EspenAlbert]

All UseStateForUnknown are removed.
Although they simplify the plan output, they can also lead to:
Error: Provider produced inconsistent result after apply

I think a better option is using the resource_plan modifier to support simplifying the plan when we are absolutely sure there will be no inconsistent result after apply

[lantoli]

arithmetic and equal comparison with floats can be tricky because roundings, i assume it's working fine here

[EspenAlbert]

I tried with both "8" and "8.0" format and seems to work well enough

[lantoli]

when the func caller uses ! IsMajorVersionHigherGreaterOrEqual it will get true when parse fails, maybe it's not a problem

[EspenAlbert]

Updated and using a warning when that is the case

[AgustinBettati]

leaving a suggestion and one doubt

[AgustinBettati]

in auto scaling dev branch (CLOUDP-256272-auto-scaling-per-shard) we use compressAPIObjectList to remove duplicate zone names, can definitely see we can unify logic over to tpf implementation as well. I believe this is currently done with tfModelsSkipIndexes logic right?

[EspenAlbert]

Correct

[AgustinBettati]

q: In the case of update for legacy replication specs, are we expecting that the state has external_id defined?

[EspenAlbert]

Yes. Maybe we would be safer re-reading from old API and using zone_name -> legacy_id?
Adding a check to TPF Test Coverage Ticket

[AgustinBettati]

In case an old sharding configuration is used the expectation from the read is that external_id is not populated. This is mainly because if we have a zone with 2 shards (e.g. 1 replication spec with num_shards = 2) there would be multiple external_id values associated to that replication spec.
As you mention using zone_name -> legacy_id would be the safe way to have this mapping.