chore: Updates cluster resource to use cluster APIs to support certain advanced configuration attributes #1344

maastha · 2025-05-02T14:08:42Z

Proposed changes

Updates cluster resource to consume some of the processArgs fields from the createCluster/updateCluster instead of /processArgs API

This PR needed some changes (new AdvancedConfiguration attribute in ClusterDescription) backported to the SDK v20231115014. This is done by vendoring a local version of the SDK. This involves adding all files of this SDK version in the repo & using those. This would also avoid automatic dependabot updates if a new tag is created for that version.

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as
expected)
This change requires a documentation update
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.

Manual QA performed:

cfn invoke for each of CRUDL/cfn test
Updated resource in example
Published to AWS private registry
Used the template in example to create and update a stack in AWS
Deleted stack to ensure resources are deleted
Created multiple resources in same stack
Validated in Atlas UI
Included screenshots

Required Checklist:

I have signed the MongoDB CLA
I have added tests that prove my fix is effective or that my feature works
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
For CFN Resources: I have released by changes in the private registry and proved by change
works in Atlas

Further comments

marcosuma

@AgustinBettati to have a look whenever possible, but this change LGTM - I think we're still pre-ISS with the schema so there is no risk since Aastha is linking to the custom build

lantoli · 2025-05-06T06:33:59Z

cfn-resources/cluster/cmd/resource/mappings.go

+	if advConfig := cluster.AdvancedConfiguration; advConfig != nil {
+		res.MinimumEnabledTLSProtocol = advConfig.MinimumEnabledTlsProtocol
+		res.TlsCipherConfigMode = advConfig.TlsCipherConfigMode
+		res.CustomOpensslCipherConfigTls12 = *advConfig.CustomOpensslCipherConfigTls12


is advConfig.CustomOpensslCipherConfigTls12 never nil? it could panic if not

CustomOpensslCipherConfigTls12 is never nil. I have updated it to use the Get method though to be safe. Also, one of the tests does cover not using this field so we should be good

lantoli · 2025-05-06T06:34:43Z

cfn-resources/cluster/cmd/resource/mappings.go

 		DefaultReadConcern:               p.DefaultReadConcern,
 		DefaultWriteConcern:              p.DefaultWriteConcern,
 		FailIndexKeyTooLong:              p.FailIndexKeyTooLong,
 		JavascriptEnabled:                p.JavascriptEnabled,
-		MinimumEnabledTLSProtocol:        p.MinimumEnabledTlsProtocol,


shoudn't we keep reading MinimumEnabledTLSProtocol from here in case advConfig is nil?

I see flattenProcessArgs is called in read only if advancedConfig is defined by the user

I see flattenProcessArgs is called in read only if advancedConfig is defined by the user

right

shoudn't we keep reading MinimumEnabledTLSProtocol from here in case advConfig is nil?

no I think for consistency we should stick to one API

lantoli · 2025-05-06T06:35:40Z

cfn-resources/cluster/cmd/resource/mappings.go

+	if processArgs.TlsCipherConfigMode != nil {
+		args.TlsCipherConfigMode = processArgs.TlsCipherConfigMode
+	}
+	args.CustomOpensslCipherConfigTls12 = &processArgs.CustomOpensslCipherConfigTls12


similar to before, do we want to check if processArgs.CustomOpensslCipherConfigTls12 is nil? or maybe we can have if len(processArgs.CustomOpensslCipherConfigTls12) > 0 ...

lantoli · 2025-05-06T06:36:52Z

cfn-resources/go.mod

@@ -19,7 +19,7 @@ require (
 	github.com/stretchr/testify v1.10.0
 	github.com/tidwall/pretty v1.2.1
 	go.mongodb.org/atlas-sdk/v20231115002 v20231115002.1.0
-	go.mongodb.org/atlas-sdk/v20231115014 v20231115014.0.0
+	go.mongodb.org/atlas-sdk/v20231115014 v20231115014.0.1 // This is a tag on Atlas SDK v20231115014 used for internal purposes only & not officially supported


can we also say WHY we need this in the comment?

have added above replace

AgustinBettati

changes LGTM, lets double check on Leo comments associated to conversion logic

maastha · 2025-05-15T08:20:23Z

cfn-resources/go.mod

@@ -2,6 +2,9 @@ module github.com/mongodb/mongodbatlas-cloudformation-resources

 go 1.23.1

+// Replacing with local copy of Atlas SDK v20231115014 to support new AdvancedConfiguration in *admin.AdvancedClusterDescription
+replace go.mongodb.org/atlas-sdk/v20231115014 => ../atlas-sdk-local-v20231115014/go.mongodb.org/atlas-sdk/v20231115014


using local SDK

AgustinBettati · 2025-05-16T09:09:45Z

cfn-resources/go.mod

+// Replacing with local copy of Atlas SDK v20231115014 to support new AdvancedConfiguration in *admin.AdvancedClusterDescription
+replace go.mongodb.org/atlas-sdk/v20231115014 => ../atlas-sdk-local-v20231115014/go.mongodb.org/atlas-sdk/v20231115014


Is there a significant drawback to the branch approach? Asking in case we need a followup for adjusting our terraform repo: https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/go.mod#L36

One drawback I came across is dependabot updates, if a new version/tag is created for that major version, that will be automatically updated.
The other is of course if the branch/tag is unintentionally modified somehow, this is more like a static copy in that case.

Asking in case we need a followup for adjusting our terraform repo: https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/go.mod#L36

For TF, can you confirm if v20240805005 will be removed in 2.0 release? If yes, then I'd leave it as is but if not then yes, I'd change that as well. Tagging @lantoli since Agustin is out.

AgustinBettati · 2025-05-16T09:12:53Z

atlas-sdk-local-v20231115014/go.mongodb.org/atlas-sdk/v20231115014/go.mod

@@ -0,0 +1,5 @@
+module go.mongodb.org/atlas-sdk/v20231115014
+


Is there any directory convention when making use of vendoring? Looking into some examples I do see a root ./vendor directory (example). Not sure if go mod vendor helps with this directly.

yes I think that makes sense, renamed to vendor

.gitignore

maastha added 3 commits May 2, 2025 15:06

use cluster APIs to support certain processArgs

9e1b2ad

min

9562dd9

use tag

8cb0f08

maastha marked this pull request as ready for review May 2, 2025 22:49

maastha requested a review from a team as a code owner May 2, 2025 22:49

marcosuma approved these changes May 5, 2025

View reviewed changes

lantoli reviewed May 6, 2025

View reviewed changes

AgustinBettati reviewed May 6, 2025

View reviewed changes

maastha added 6 commits May 14, 2025 13:51

use SDK local version for v20231115014

319bf3e

use SDK local version for v20231115014

762217d

Merge branch 'master' into CLOUDP-296223-processArgs-from-cluster-api

f4707bd

add comment

a340cc1

add copyright

fdddae0

min

9f02f86

maastha commented May 15, 2025

View reviewed changes

AgustinBettati reviewed May 16, 2025

View reviewed changes

maastha added 3 commits May 19, 2025 17:46

rename vendor folder

52c9d18

rename vendor folder

1520f09

rename vendor folder

b1f387b

lantoli reviewed May 19, 2025

View reviewed changes

.gitignore Show resolved Hide resolved

lantoli approved these changes May 19, 2025

View reviewed changes

maastha added this pull request to the merge queue May 20, 2025

Merged via the queue into master with commit bedbda0 May 20, 2025
35 checks passed

maastha deleted the CLOUDP-296223-processArgs-from-cluster-api branch May 20, 2025 12:49

lantoli mentioned this pull request Jun 2, 2025

fix: Fix nil pointer in Cluster #1358

Merged

19 tasks

		// Replacing with local copy of Atlas SDK v20231115014 to support new AdvancedConfiguration in *admin.AdvancedClusterDescription
		replace go.mongodb.org/atlas-sdk/v20231115014 => ../atlas-sdk-local-v20231115014/go.mongodb.org/atlas-sdk/v20231115014

chore: Updates cluster resource to use cluster APIs to support certain advanced configuration attributes #1344

chore: Updates cluster resource to use cluster APIs to support certain advanced configuration attributes #1344

Uh oh!

Conversation

maastha commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Type of change:

Manual QA performed:

Required Checklist:

Further comments

Uh oh!

marcosuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lantoli May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AgustinBettati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maastha commented May 2, 2025 •

edited

Loading

lantoli May 6, 2025 •

edited

Loading