Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Implements extra API Calls for AdvancedCluster TPF #2852

Merged
merged 10 commits into from
Dec 4, 2024
7 changes: 4 additions & 3 deletions internal/common/constant/cloud_provider.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package constant

const (
AWS = "AWS"
AZURE = "AZURE"
GCP = "GCP"
AWS = "AWS"
AZURE = "AZURE"
GCP = "GCP"
TENANT = "TENANT"
)
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,15 @@ type LegacySchemaInfo struct {
RootDiskSize *float64
}

func NewTFModel(ctx context.Context, input *admin.ClusterDescription20240805, timeout timeouts.Value, diags *diag.Diagnostics, legacyInfo *LegacySchemaInfo) *TFModel {
type ExtraAPIInfo struct {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although we could define only containerIDs map[string]string as a parameter, I think this is more "future-proof".
(Couldn't use LegacySchemaInfo, as that is only meant for numShards > 1 configurations)

Copy link
Member

@lantoli lantoli Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have mixed feelings about anticipating future needs as it can create overcomplexity but it's ok here.

ContainerIDs map[string]string
}

func NewTFModel(ctx context.Context, input *admin.ClusterDescription20240805, timeout timeouts.Value, diags *diag.Diagnostics, legacyInfo *LegacySchemaInfo, apiInfo ExtraAPIInfo) *TFModel {
biConnector := NewBiConnectorConfigObjType(ctx, input.BiConnector, diags)
connectionStrings := NewConnectionStringsObjType(ctx, input.ConnectionStrings, diags)
labels := NewLabelsObjType(ctx, input.Labels, diags)
replicationSpecs := NewReplicationSpecsObjType(ctx, input.ReplicationSpecs, diags, legacyInfo)
replicationSpecs := NewReplicationSpecsObjType(ctx, input.ReplicationSpecs, diags, legacyInfo, apiInfo)
tags := NewTagsObjType(ctx, input.Tags, diags)
if diags.HasError() {
return nil
Expand Down Expand Up @@ -112,15 +116,19 @@ func NewLabelsObjType(ctx context.Context, input *[]admin.ComponentLabel, diags
return setType
}

func NewReplicationSpecsObjType(ctx context.Context, input *[]admin.ReplicationSpec20240805, diags *diag.Diagnostics, legacyInfo *LegacySchemaInfo) types.List {
func NewReplicationSpecsObjType(ctx context.Context, input *[]admin.ReplicationSpec20240805, diags *diag.Diagnostics, legacyInfo *LegacySchemaInfo, apiInfo ExtraAPIInfo) types.List {
if input == nil {
return types.ListNull(ReplicationSpecsObjType)
}
var tfModels *[]TFReplicationSpecsModel
if len(apiInfo.ContainerIDs) == 0 {
diags.AddError("containerIDs", "containerIDs not set in ExtraAPIInfo")
return types.ListNull(ReplicationSpecsObjType)
}
if legacyInfo == nil {
tfModels = convertReplicationSpecs(ctx, input, diags)
tfModels = convertReplicationSpecs(ctx, input, diags, apiInfo)
} else {
tfModels = convertReplicationSpecsLegacy(ctx, input, diags, legacyInfo)
tfModels = convertReplicationSpecsLegacy(ctx, input, diags, legacyInfo, apiInfo)
}
if diags.HasError() {
return types.ListNull(ReplicationSpecsObjType)
Expand All @@ -130,15 +138,15 @@ func NewReplicationSpecsObjType(ctx context.Context, input *[]admin.ReplicationS
return listType
}

func convertReplicationSpecs(ctx context.Context, input *[]admin.ReplicationSpec20240805, diags *diag.Diagnostics) *[]TFReplicationSpecsModel {
func convertReplicationSpecs(ctx context.Context, input *[]admin.ReplicationSpec20240805, diags *diag.Diagnostics, apiInfo ExtraAPIInfo) *[]TFReplicationSpecsModel {
tfModels := make([]TFReplicationSpecsModel, len(*input))
for i, item := range *input {
regionConfigs := NewRegionConfigsObjType(ctx, item.RegionConfigs, diags)
tfModels[i] = TFReplicationSpecsModel{
Id: types.StringPointerValue(item.Id),
ExternalId: types.StringPointerValue(item.Id),
NumShards: types.Int64Value(1), // TODO: Static
ContainerId: conversion.ToTFMapOfString(ctx, diags, nil), // TODO: Static
NumShards: types.Int64Value(1), // TODO: Static
EspenAlbert marked this conversation as resolved.
Show resolved Hide resolved
ContainerId: conversion.ToTFMapOfString(ctx, diags, &apiInfo.ContainerIDs),
RegionConfigs: regionConfigs,
ZoneId: types.StringPointerValue(item.ZoneId),
ZoneName: types.StringPointerValue(item.ZoneName),
Expand All @@ -147,7 +155,7 @@ func convertReplicationSpecs(ctx context.Context, input *[]admin.ReplicationSpec
return &tfModels
}

func convertReplicationSpecsLegacy(ctx context.Context, input *[]admin.ReplicationSpec20240805, diags *diag.Diagnostics, legacyInfo *LegacySchemaInfo) *[]TFReplicationSpecsModel {
func convertReplicationSpecsLegacy(ctx context.Context, input *[]admin.ReplicationSpec20240805, diags *diag.Diagnostics, legacyInfo *LegacySchemaInfo, apiInfo ExtraAPIInfo) *[]TFReplicationSpecsModel {
tfModels := []TFReplicationSpecsModel{}
tfModelsSkipIndexes := []int{}
for i, item := range *input {
Expand Down Expand Up @@ -179,7 +187,7 @@ func convertReplicationSpecsLegacy(ctx context.Context, input *[]admin.Replicati
}
}
tfModels = append(tfModels, TFReplicationSpecsModel{
ContainerId: conversion.ToTFMapOfString(ctx, diags, nil), // TODO: Static
ContainerId: conversion.ToTFMapOfString(ctx, diags, &apiInfo.ContainerIDs),
ExternalId: types.StringPointerValue(item.Id),
Id: types.StringValue(legacyID),
RegionConfigs: regionConfigs,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"go.mongodb.org/atlas-sdk/v20241113001/admin"
)

var defaultZoneName = "ZoneName managed by Terraform"
EspenAlbert marked this conversation as resolved.
Show resolved Hide resolved

func NewAtlasReq(ctx context.Context, input *TFModel, diags *diag.Diagnostics) *admin.ClusterDescription20240805 {
acceptDataRisksAndForceReplicaSetReconfig, ok := conversion.StringPtrToTimePtr(input.AcceptDataRisksAndForceReplicaSetReconfig.ValueStringPointer())
if !ok {
Expand Down Expand Up @@ -93,11 +95,19 @@ func newReplicationSpec20240805(ctx context.Context, input types.List, diags *di
Id: conversion.NilForUnknown(item.Id, item.Id.ValueStringPointer()),
ZoneId: conversion.NilForUnknown(item.ZoneId, item.ZoneId.ValueStringPointer()),
RegionConfigs: newCloudRegionConfig20240805(ctx, item.RegionConfigs, diags),
ZoneName: item.ZoneName.ValueStringPointer(),
ZoneName: conversion.StringPtr(resolveZoneNameOrUseDefault(item)),
}
}
return &resp
}

func resolveZoneNameOrUseDefault(item *TFReplicationSpecsModel) string {
zoneName := conversion.NilForUnknown(item.ZoneName, item.ZoneName.ValueStringPointer())
if zoneName == nil {
return defaultZoneName
}
return *zoneName
}
func newResourceTag(ctx context.Context, input types.Set, diags *diag.Diagnostics) *[]admin.ResourceTag {
if input.IsUnknown() || input.IsNull() {
return nil
Expand Down
2 changes: 1 addition & 1 deletion internal/service/advancedclustertpf/move_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func setMoveStateResponse(ctx context.Context, projectID, clusterName string, re
model := NewTFModel(ctx, &admin.ClusterDescription20240805{
GroupId: conversion.StringPtr(projectID),
Name: conversion.StringPtr(clusterName),
}, validTimeout, &resp.Diagnostics, nil)
}, validTimeout, &resp.Diagnostics, nil, ExtraAPIInfo{})
if resp.Diagnostics.HasError() {
return
}
Expand Down
52 changes: 38 additions & 14 deletions internal/service/advancedclustertpf/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,24 @@ func (r *rs) Update(ctx context.Context, req resource.UpdateRequest, resp *resou
if diags.HasError() {
return
}
cluster := r.applyClusterChanges(ctx, diags, &state, &plan)
stateReq := normalizeFromTFModel(ctx, &state, diags, false)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored some applyClusterChanges logic here, to have the distinction between tenantUpgrade/ClusterChanges more clear

planReq := normalizeFromTFModel(ctx, &plan, diags, false)
if diags.HasError() {
return
}
normalizePatchState(stateReq)
patchReq, err := update.PatchPayload(stateReq, planReq)
if err != nil {
diags.AddError("errorPatchPayload", err.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more human-friendly summary

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will handle as part of next ticket

return
}
upgradeRequest := tenantUpgrade(stateReq, patchReq)
var cluster *admin.ClusterDescription20240805
if upgradeRequest != nil {
cluster = r.applyTenantUpgrade(ctx, &plan, upgradeRequest, diags)
} else {
cluster = r.applyClusterChanges(ctx, diags, &state, &plan, patchReq)
}
if diags.HasError() {
return
}
Expand Down Expand Up @@ -233,18 +250,7 @@ func (r *rs) applyAdvancedConfigurationChanges(ctx context.Context, diags *diag.
return legacyAdvConfig, advConfig
}

func (r *rs) applyClusterChanges(ctx context.Context, diags *diag.Diagnostics, state, plan *TFModel) *admin.ClusterDescription20240805 {
stateReq := normalizeFromTFModel(ctx, state, diags, false)
planReq := normalizeFromTFModel(ctx, plan, diags, false)
if diags.HasError() {
return nil
}
normalizePatchState(stateReq)
patchReq, err := update.PatchPayload(stateReq, planReq)
if err != nil {
diags.AddError("errorPatchPayload", err.Error())
return nil
}
func (r *rs) applyClusterChanges(ctx context.Context, diags *diag.Diagnostics, state, plan *TFModel, patchReq *admin.ClusterDescription20240805) *admin.ClusterDescription20240805 {
var cluster *admin.ClusterDescription20240805
if usingLegacySchema(ctx, plan.ReplicationSpecs, diags) {
// Only updates of replication specs will be done with legacy API
Expand Down Expand Up @@ -344,6 +350,19 @@ func (r *rs) updateAndWaitLegacy(ctx context.Context, patchReq *admin20240805.Cl
return AwaitChanges(ctx, r.Client.AtlasV2.ClustersApi, &tfModel.Timeouts, diags, projectID, clusterName, changeReasonUpdate)
}

func (r *rs) applyTenantUpgrade(ctx context.Context, plan *TFModel, upgradeRequest *admin.LegacyAtlasTenantClusterUpgradeRequest, diags *diag.Diagnostics) *admin.ClusterDescription20240805 {
api := r.Client.AtlasV2.ClustersApi
oarbusi marked this conversation as resolved.
Show resolved Hide resolved
projectID := plan.ProjectID.ValueString()
clusterName := plan.Name.ValueString()
upgradeRequest.Name = clusterName
_, _, err := api.UpgradeSharedCluster(ctx, projectID, upgradeRequest).Execute()
if err != nil {
diags.AddError("errorTenantUpgrade", fmt.Sprintf(errorUpdate, clusterName, err.Error()))
return nil
}
return AwaitChanges(ctx, api, &plan.Timeouts, diags, projectID, clusterName, changeReasonUpdate)
}

func (r *rs) convertClusterAddAdvConfig(ctx context.Context, legacyAdvConfig *admin20240530.ClusterDescriptionProcessArgs, advConfig *admin.ClusterDescriptionProcessArgs20240805, cluster *admin.ClusterDescription20240805, modelIn *TFModel, diags *diag.Diagnostics) *TFModel {
api := r.Client.AtlasV2.ClustersApi
api20240530 := r.Client.AtlasV220240530.ClustersApi
Expand All @@ -365,7 +384,12 @@ func (r *rs) convertClusterAddAdvConfig(ctx context.Context, legacyAdvConfig *ad
}
}
legacyInfo := resolveLegacyInfo(ctx, modelIn, diags, cluster, api20240530)
modelOut := NewTFModel(ctx, cluster, modelIn.Timeouts, diags, legacyInfo)
apiInfo, err := resolveExtraAPIInfo(ctx, projectID, cluster, r.Client.AtlasV2.NetworkPeeringApi)
if err != nil {
diags.AddError("errorExtraApiInfo", err.Error())
return nil
}
modelOut := NewTFModel(ctx, cluster, modelIn.Timeouts, diags, legacyInfo, *apiInfo)
if diags.HasError() {
return nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"fmt"
"strings"

"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/constant"
"github.com/spf13/cast"
admin20240530 "go.mongodb.org/atlas-sdk/v20240530005/admin"
"go.mongodb.org/atlas-sdk/v20241113001/admin"
Expand All @@ -17,6 +18,52 @@
return fmt.Sprintf("%.1f", cast.ToFloat32(version))
}

func resolveExtraAPIInfo(ctx context.Context, projectID string, cluster *admin.ClusterDescription20240805, api admin.NetworkPeeringApi) (*ExtraAPIInfo, error) {
containerIDs := map[string]string{}
for _, spec := range cluster.GetReplicationSpecs() {
for _, regionConfig := range spec.GetRegionConfigs() {
providerName := regionConfig.GetProviderName()
if providerName == constant.TENANT {
continue
}
params := &admin.ListPeeringContainerByCloudProviderApiParams{
GroupId: projectID,
ProviderName: &providerName,
}
containerIDKey := fmt.Sprintf("%s:%s", providerName, regionConfig.GetRegionName())
if _, ok := containerIDs[containerIDKey]; ok {
continue
}
containers, _, err := api.ListPeeringContainerByCloudProviderWithParams(ctx, params).Execute()
if err != nil {
return nil, err
}
if results := getAdvancedClusterContainerID(containers.GetResults(), &regionConfig); results != "" {
EspenAlbert marked this conversation as resolved.
Show resolved Hide resolved
containerIDs[containerIDKey] = results
} else {
return nil, fmt.Errorf("container id not found for %s", containerIDKey)
}
}
}
return &ExtraAPIInfo{
ContainerIDs: containerIDs,
}, nil
}

// copied from model_advanced_cluster.go
func getAdvancedClusterContainerID(containers []admin.CloudProviderContainer, cluster *admin.CloudRegionConfig20240805) string {
for i := range containers {
gpc := cluster.GetProviderName() == constant.GCP
azure := containers[i].GetProviderName() == cluster.GetProviderName() && containers[i].GetRegion() == cluster.GetRegionName()
aws := containers[i].GetRegionName() == cluster.GetRegionName()

if check gpc || azure || aws {

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / tflint

expected ';', found gpc

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / tf-validate

expected ';', found gpc

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / tf-validate

syntax error: unexpected name gpc, expected {

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / build

expected ';', found gpc

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / lint

syntax error: unexpected name gpc, expected { (typecheck)

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / lint

expected ';', found gpc (typecheck)

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / lint

syntax error: unexpected name gpc, expected {) (typecheck)

Check failure on line 60 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / lint

syntax error: unexpected name gpc, expected {) (typecheck)
EspenAlbert marked this conversation as resolved.
Show resolved Hide resolved
return containers[i].GetId()

Check failure on line 61 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / tflint

expected '{', found 'return'

Check failure on line 61 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / tf-validate

expected '{', found 'return'

Check failure on line 61 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / build

expected '{', found 'return'

Check failure on line 61 in internal/service/advancedclustertpf/resource_compatibility_reuse.go

View workflow job for this annotation

GitHub Actions / lint

expected '{', found 'return' (typecheck)
}
}
return ""
}

func getReplicationSpecIDsFromOldAPI(ctx context.Context, projectID, clusterName string, api admin20240530.ClustersApi) (map[string]string, error) {
clusterOldAPI, _, err := api.GetCluster(ctx, projectID, clusterName).Execute()
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ func numShardsMap(ctx context.Context, input types.List, diags *diag.Diagnostics
counts := map[string]int64{}
for i := range elements {
e := elements[i]
counts[e.ZoneName.ValueString()] = e.NumShards.ValueInt64()
zoneName := resolveZoneNameOrUseDefault(&e)
counts[zoneName] = e.NumShards.ValueInt64()
}
return counts
}
Expand Down
18 changes: 7 additions & 11 deletions internal/service/advancedclustertpf/resource_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/hashicorp/terraform-plugin-framework/resource/schema/int64default"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/setdefault"
Copy link
Collaborator Author

@EspenAlbert EspenAlbert Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to the resource_schema.go are due to TPF strictness and using the /tenantUpgrade endpoint, which would raise errors:

  | Error: Provider produced inconsistent result after apply
  | 
  | When applying changes to mongodbatlas_advanced_cluster.test, provider
  | "provider[\"registry.terraform.io/hashicorp/mongodbatlas\"]" produced an
  | unexpected new value: .create_date: was
  | cty.StringVal("2024-12-03T16:54:26Z"), but now
  | cty.StringVal("2024-12-03T16:57:44Z").
  | 
  | This is a bug in the provider, which should be reported in the provider's own
  | issue tracker.
  | 
  | Error: Provider produced inconsistent result after apply
  | 
  | When applying changes to mongodbatlas_advanced_cluster.test, provider
  | "provider[\"registry.terraform.io/hashicorp/mongodbatlas\"]" produced an
  | unexpected new value: .replication_specs[0].zone_name: was
  | cty.StringVal("ZoneName managed by Terraform"), but now cty.StringVal("Zone
  | 1").
  | 
  | This is a bug in the provider, which should be reported in the provider's own
  | issue tracker.
  | 
  | Error: Provider produced inconsistent result after apply
  | 
  | When applying changes to mongodbatlas_advanced_cluster.test, provider
  | "provider[\"registry.terraform.io/hashicorp/mongodbatlas\"]" produced an
  | unexpected new value: .cluster_id: was
  | cty.StringVal("674f37c288864967c85f6b47"), but now
  | cty.StringVal("674f388843ca6e2ab9803e84").

"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringdefault"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/schemafunc"
Expand Down Expand Up @@ -128,10 +127,7 @@ func ResourceSchema(ctx context.Context) schema.Schema {
},
},
"create_date": schema.StringAttribute{
Computed: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes sense to remove UseStateForUnknown as this is intended to use only when we're sure that value won't change

},
Computed: true,
MarkdownDescription: "Date and time when MongoDB Cloud created this cluster. This parameter expresses its value in ISO 8601 format in UTC.",
},
"encryption_at_rest_provider": schema.StringAttribute{
Expand All @@ -150,9 +146,9 @@ func ResourceSchema(ctx context.Context) schema.Schema {
},
"cluster_id": schema.StringAttribute{ // TODO: was generated as id
EspenAlbert marked this conversation as resolved.
Show resolved Hide resolved
Computed: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
},
// PlanModifiers: []planmodifier.String{
// stringplanmodifier.UseStateForUnknown(),
EspenAlbert marked this conversation as resolved.
Show resolved Hide resolved
// },
MarkdownDescription: "Unique 24-hexadecimal digit string that identifies the cluster.",
},
"labels": schema.SetNestedAttribute{
Expand Down Expand Up @@ -276,9 +272,9 @@ func ResourceSchema(ctx context.Context) schema.Schema {
MarkdownDescription: "Unique 24-hexadecimal digit string that identifies the zone in a Global Cluster. This value can be used to configure Global Cluster backup policies.",
},
"zone_name": schema.StringAttribute{
Computed: true, // must be computed to have a Default
Optional: true,
Default: stringdefault.StaticString("ZoneName managed by Terraform"), // TODO: as in current resource
Computed: true, // must be computed to have a Default
EspenAlbert marked this conversation as resolved.
Show resolved Hide resolved
Optional: true,
// Default: stringdefault.StaticString("ZoneName managed by Terraform"), // TODO: as in current resource
MarkdownDescription: "Human-readable label that describes the zone this shard belongs to in a Global Cluster. Provide this value only if \"clusterType\" : \"GEOSHARDED\" but not \"selfManagedSharding\" : true.",
},
},
Expand Down
Loading
Loading