INTMDB 186 - Added authorization resource to split the cloud access provider config #420

leofigy · 2021-03-17T14:23:57Z

Description

Introducing the authorization resource for cloud provider access, the expected behavior is to have the original

cloud provider access resource - for the users that want to have a unique resource,
cloud provider access setup - just creates the mongodbatlas ids
cloud provider access authorization - based on the role_id it performs an auth for the given cloud provicer access

Link to any related issue(s):

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
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the Terraform contribution guidelines
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

Further comments

leofigy · 2021-03-24T01:04:43Z

Hi @themantissa explaining a little bit about the behavior

the cloud_provider_access_setup performs the following operations, this means you can do the setup, is
create, delete, but not update it (perform PATCH)

	Read:   resourceMongoDBAtlasCloudProviderAccessSetupRead,
	Create: resourceMongoDBAtlasCloudProviderAccessSetupCreate,
	Update: resourceMongoDBAtlasCloudProviderAccessAuthorizationPlaceHolder,
	Delete: resourceMongoDBAtlasCloudProviderAccessSetupDelete,

The authorization resource, is tricky in terraform sense you can create, delete, and update, but mapping to the API.
The PATCH request is actually done in the CREATE and Update, and the delete is just removing in it from terraform, but not calling the API, the reason behind this, it's because if you do a DELETE here , the upper resource won't be aware of.

	Read:   resourceMongoDBAtlasCloudProviderAccessAuthorizationRead,
	Create: resourceMongoDBAtlasCloudProviderAccessAuthorizationCreate,
	Update: resourceMongoDBAtlasCloudProviderAccessAuthorizationUpdate,
	Delete: resourceMongoDBAtlasCloudProviderAccessAuthorizationPlaceHolder,

leofigy · 2021-03-24T16:13:35Z

mongodbatlas/resource_mongodbatlas_cloud_provider_access_setup_test.go

+	)
+}
+
+func TestAccResourceMongoDBAtlasCloudProviderAccessSetup_importBasic(t *testing.T) {


Hi @pitakill just fyi

themantissa

Overall it looks great. Sorry for the delay but had to check some things with others first. Mostly just a few questions/comments and doc edits. Thanks!

integration-testing/README.md

mongodbatlas/resource_mongodbatlas_cloud_provider_access_setup.go

themantissa · 2021-04-02T22:15:59Z

mongodbatlas/resource_mongodbatlas_cloud_provider_access_setup.go

+				Type:     schema.TypeString,
+				Required: true,
+			},
+			// Note: when new providers will be added, this will trigger a recreate


By recreate do you mean that if someone had AWS already and submitted as is for Azure, for example, it would recreate, that is destroy the existing AWS? Or do you mean if a new cloud provider is added it would create a new one in the terraform state? I kinda get what you are saying I think - the way the API is written now it would only really allow one cloud provider at a time to be in use for the project.

right, if the cloud provider is change from aws to azure, it will hit a force new, that means to delete the existing one (aws) and send a create for azure

Is there any good way to prepare for this to have more than one later? Or should we just punt that to when (if) it actually comes up?

Hi Melissa, when it comes we can just add in the Update request, something like if d.HasChanges("provider_name"), get the old and new value (new accepted providers), in case we need extra logic in case it's needed in an update process.

Makes sense. Thanks for the clarification.

website/docs/r/cloud_provider_access.markdown

coderGo93 · 2021-04-08T22:29:53Z

examples/atlas-cloud-provider-access/aws/aws-roles.tf

+  ]
+}
+EOF
+}


Appears a linter error about not newline at the end of file

coderGo93 · 2021-04-08T22:30:09Z

examples/atlas-cloud-provider-access/aws/providers.tf

+  access_key = var.access_key
+  secret_key = var.secret_key
+  region     = var.aws_region
+}


Same as before about linter error

coderGo93

Just fix the linter code from github, if the tests has passed and the code LGTM

themantissa

One very tiny nit and follow up question. Then I think it's ready!

website/docs/r/cloud_provider_access.markdown

themantissa

LGTM! Thank you! @shum you feel good where this is at? If you don't have the time to review just give it a quick once over and let us know if any questions.

leofigy · 2021-04-13T17:06:39Z

Hi @shum , if you don't have any concern, I will merge this today EOD :)

shum

LGTM, minor typos but logic makes sense to me

shum · 2021-04-13T18:50:28Z

integration-testing/README.md

+    MONGODB_ATLAS_PRIVATE_KEY
+```
+
+For especific aws related interactions 


Suggested change

For especific aws related interactions

For specific aws related interactions

shum · 2021-04-13T18:52:06Z

integration-testing/README.md

+    AWS_SECRET_ACCESS_KEY
+    AWS_REGION
+
+    AWS_CUSTOMER_MASTER_KEY_ID (only cloud at rest)


Suggested change

AWS_CUSTOMER_MASTER_KEY_ID (only cloud at rest)

AWS_CUSTOMER_MASTER_KEY_ID (only required for encryption at rest with customer managed keys)

shum · 2021-04-13T18:56:40Z

mongodbatlas/data_source_mongodbatlas_cloud_provider_access_setup.go

+	} else {
+		// planning for the future multiple providers
+		return fmt.Errorf(errorGetRead,
+			fmt.Sprintf("unsopported provider type %s", providerName))


Suggested change

fmt.Sprintf("unsopported provider type %s", providerName))

fmt.Sprintf("unsupported provider type %s", providerName))

leofigy added 2 commits March 17, 2021 00:47

splitting schema into two resources

28be2a1

added read for second resource

8b208a0

leofigy requested a review from coderGo93 as a code owner March 17, 2021 14:23

leofigy added the enhancement label Mar 17, 2021

auth resource

bf63d3b

leofigy requested a review from pitakill March 17, 2021 14:28

leofigy marked this pull request as draft March 17, 2021 14:28

leofigy self-assigned this Mar 17, 2021

leofigy added 9 commits March 17, 2021 08:44

fixing linter

a61dc48

added placeholder foos

cc0343f

importer for CloudProviderAccessSetup

c953bd9

example single apply terraform

d213220

clean up

c7b0372

documentation update and description

5a7fe8a

terratest single apply

c7a4b22

datasource for setup

57ca2d3

adding test case for datasource :)

969880f

leofigy requested a review from themantissa March 24, 2021 00:58

update in authorization resource

6ee487a

leofigy added 4 commits March 24, 2021 08:30

testing for setup resource and import

32197c9

import test

26404b5

document for cloud provider access setup datasource

602462d

documentation update

a9df805

leofigy marked this pull request as ready for review March 24, 2021 15:37

leofigy changed the title ~~Preview INTMDB 186 - Added authorization resource to split the cloud access provider config~~ INTMDB 186 - Added authorization resource to split the cloud access provider config Mar 24, 2021

leofigy commented Mar 24, 2021

View reviewed changes

themantissa requested a review from shum March 24, 2021 16:16

themantissa requested changes Apr 6, 2021

View reviewed changes

coderGo93 reviewed Apr 8, 2021

View reviewed changes

coderGo93 approved these changes Apr 8, 2021

View reviewed changes

pr comments :)

cbf50ba

leofigy requested a review from themantissa April 9, 2021 14:19

themantissa requested changes Apr 9, 2021

View reviewed changes

website/docs/r/cloud_provider_access.markdown Outdated Show resolved Hide resolved

Update cloud_provider_access.markdown

33ef00b

leofigy requested a review from themantissa April 12, 2021 15:31

themantissa approved these changes Apr 12, 2021

View reviewed changes

themantissa mentioned this pull request Apr 12, 2021

encryption_at_rest failing with UNEXPECTED ERROR (and discussion of Cloud Provider Access possible improvement) #409

Closed

shum approved these changes Apr 13, 2021

View reviewed changes

leofigy added 2 commits April 13, 2021 19:31

pr comment :)

2004650

pr comment typo

dac7f6f

leofigy added the run-testacc To run acceptance tests label Apr 14, 2021

leofigy merged commit 733570c into master Apr 14, 2021

leofigy deleted the INTMDB-186 branch April 14, 2021 04:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTMDB 186 - Added authorization resource to split the cloud access provider config #420

INTMDB 186 - Added authorization resource to split the cloud access provider config #420

leofigy commented Mar 17, 2021

leofigy commented Mar 24, 2021

leofigy Mar 24, 2021

themantissa left a comment

themantissa Apr 2, 2021

leofigy Apr 8, 2021

themantissa Apr 9, 2021

leofigy Apr 12, 2021

themantissa Apr 12, 2021

coderGo93 Apr 8, 2021

coderGo93 Apr 8, 2021

coderGo93 left a comment

themantissa left a comment

themantissa left a comment

leofigy commented Apr 13, 2021

shum left a comment

shum Apr 13, 2021

shum Apr 13, 2021

shum Apr 13, 2021

	For especific aws related interactions
	For specific aws related interactions

	AWS_CUSTOMER_MASTER_KEY_ID (only cloud at rest)
	AWS_CUSTOMER_MASTER_KEY_ID (only required for encryption at rest with customer managed keys)

	fmt.Sprintf("unsopported provider type %s", providerName))
	fmt.Sprintf("unsupported provider type %s", providerName))

INTMDB 186 - Added authorization resource to split the cloud access provider config #420

INTMDB 186 - Added authorization resource to split the cloud access provider config #420

Conversation

leofigy commented Mar 17, 2021

Description

Type of change:

Required Checklist:

Further comments

leofigy commented Mar 24, 2021

Choose a reason for hiding this comment

themantissa left a 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

coderGo93 left a comment

Choose a reason for hiding this comment

themantissa left a comment

Choose a reason for hiding this comment

themantissa left a comment

Choose a reason for hiding this comment

leofigy commented Apr 13, 2021

shum left a 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