chore: Enables AWS encryption_at_rest acceptance tests to run in CI #3000

maastha · 2025-01-23T23:36:46Z

Description

Enables AWS encryption_at_rest acceptance tests to run in CI

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

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

…ws-ci-enable

maastha · 2025-01-24T16:29:41Z

.github/workflows/acceptance-tests-runner.yml


  encryption:
    needs: [ change-detection, get-provider-version ]
+    concurrency:


Each test run for encryption takes about ~8min to complete. Since the tests use shared resources, I want to prevent multiple simultaneous runs.
I'm thinking as a follow-up I can separate out AWS and Azure tests in separate jobs using ACCTEST_REGEX_RUN to reduce the test run time more.
Will wait to see if any other suggestions.

I see tests are using resource.Test instead of resource.ParallelTest, so I understand it's in case this is running in multiple PRs / test suites.

The main issue I see, as cancel-in-progress=false, is that there can be some pile-up of jobs, and for example some PR checks are blocked until older ones finish, if some gets stucks, it can be a long time until they time out. This could also block Test Suite.

but we can go ahead and see if this happens

The main issue I see, as cancel-in-progress=false, is that there can be some pile-up of jobs, and for example some PR checks are blocked until older ones finish, if some gets stucks, it can be a long time until they time out.

Right. I don't think it should block the test suite given the cadence but yes PR checks could take longer. we can't use cancel-inprogress=true because that may leave resources in an inconsistent state.

but we can go ahead and see if this happens

yes sounds good.

marcosuma · 2025-01-24T16:52:26Z

.github/workflows/acceptance-tests-runner.yml

+          AWS_SECRET_ACCESS_KEY: ${{ secrets.aws_secret_access_key }}
+          AWS_ACCESS_KEY_ID: ${{ secrets.aws_access_key_id }}
+          AWS_CUSTOMER_MASTER_KEY_ID: ${{ secrets.aws_customer_master_key_id }}
+          MONGODB_ATLAS_PROJECT_EAR_PE_AWS_ID: ${{ inputs.mongodb_atlas_project_ear_pe_aws_id }}


n00b question: what does PE stand for?

Private Endpoint. Happy to rename if this isn't readable, lmk in case of any suggestions

marcosuma · 2025-01-24T16:53:27Z

internal/testutil/acc/pre_check.go

 		os.Getenv("AWS_SECRET_ACCESS_KEY") == "" ||
 		os.Getenv("AWS_CUSTOMER_MASTER_KEY_ID") == "" ||
 		os.Getenv("MONGODB_ATLAS_PROJECT_EAR_PE_AWS_ID") == "" ||
-		os.Getenv("AWS_PRIVATE_ENDPOINT_REGION") == "" {


just checking is this change correct? what is the diff between the two EV?

yes. AWS_PRIVATE_ENDPOINT_REGION was added in a previous PR and currently is only in the CLOUDP-262752-ear-aws-kms-dev dev branch, not in master. I just decided to reuse another existing env AWS_REGION instead of adding this (AWS_PRIVATE_ENDPOINT_REGION ) new one

lantoli · 2025-01-24T17:12:23Z

internal/service/encryptionatrest/resource_test.go

 				Config: testAccMongoDBAtlasEncryptionAtRestConfigAwsKmsWithRole(projectID, awsIAMRoleName, awsIAMRolePolicyName, awsKeyName, &awsKms),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					acc.CheckEARExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "project_id", projectID),


being a TPF resource, consider checking only computed values

EspenAlbert · 2025-01-24T17:20:52Z

.github/workflows/acceptance-tests-runner.yml

  encryption:
    needs: [ change-detection, get-provider-version ]
+    concurrency:
+      group: ${{ github.repository }}-global-ear-concurrency


why the ${{ github.repository}}?

since these tests use same AWS and Azure resources (Atlas project, etc), we want all workflows across the repository to not run concurrently which could result in race conditions.

I have refactored the other TestMigEncryptionAtRest_withRole_basicAWS test though as per your comment. Hope that helps!

I am under the impression that group simply is a key, as long as it is the same, it will not run concurrently, this means:

group: ${{ github.repository }}-global-ear-concurrency

group: global-ear-concurrency

would be equivalent

EspenAlbert · 2025-01-24T17:22:44Z

internal/service/encryptionatrest/resource_migration_test.go

 		projectID    = os.Getenv("MONGODB_ATLAS_PROJECT_EAR_PE_AWS_ID") // to use RequirePrivateNetworking, Atlas Project is required to have FF enabled

 		awsKms = admin.AWSKMSConfiguration{
 			Enabled:             conversion.Pointer(true),


Same comment as earlier PR 😅
Why we cannot refactor out the test case?

so the acc test case checks update of RequirePrivateNetworking which is a required test case. Since this attr doesn't exist in previous versions, I'd like to keep these test cases separate

internal/service/encryptionatrest/resource_test.go

internal/testutil/acc/pre_check.go

EspenAlbert · 2025-01-24T17:27:45Z

Any reason why TestMigEncryptionAtRest_withRole_basicAWS is still skipped?

maastha · 2025-01-24T18:16:16Z

Any reason why TestMigEncryptionAtRest_withRole_basicAWS is still skipped?

Responded in above comment. Let's try to leave comments on specific lines as these are hard to respond to/have a conversation :)

…terraform-provider-mongodbatlas into CLOUDP-293831-ear-aws-ci-enable

maastha · 2025-01-27T14:07:16Z

Required env vars and secrets have been added to repo for dev and QA.
QA still doesn't have all required features deployed from MMS yet so some tests fail with FEATURE_UNSUPPORTED error at this point which is expected:
https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/12990429495/job/36225611851

Added an item to ensure QA tests succeed once required changes are in prod and before merging this to master in this ticket: https://jira.mongodb.org/browse/CLOUDP-296239

Merging this PR to dev branch.

maastha added 9 commits January 22, 2025 16:26

add ear pe tests AWS

c73ad0c

unit test

60b2d4e

cleanup

507dccc

minor

fad13f4

minor

a4aa1bf

minor

f23fbcb

min

515a67c

add to CI

ecc5dbe

test

3997eb7

maastha mentioned this pull request Jan 24, 2025

chore: Updates mongodbatlas_encryption_at_rest_private_endpoint tests to support private networking for AWS encryption at rest #2995

Merged

13 tasks

Base automatically changed from CLOUDP-295878-ear-pe to CLOUDP-262752-ear-aws-kms-dev January 24, 2025 11:32

maastha added 6 commits January 24, 2025 11:48

Merge branch 'CLOUDP-262752-ear-aws-kms-dev' into CLOUDP-293831-ear-a…

f0ccb89

…ws-ci-enable

previous PR comments

aadd089

minor

7898ac1

tmp

d2a157d

tmp tevert

270f01f

min

2031525

maastha changed the title ~~ci: Enables AWS encryption_at_rest acceptance tests to run in CI~~ chore: Enables AWS encryption_at_rest acceptance tests to run in CI Jan 24, 2025

maastha added 5 commits January 24, 2025 15:04

min

ac10655

min

703d789

min

56a45d2

tmp

59aaf74

min

c26b45f

maastha marked this pull request as ready for review January 24, 2025 16:24

maastha requested a review from a team as a code owner January 24, 2025 16:24

maastha commented Jan 24, 2025

View reviewed changes

marcosuma reviewed Jan 24, 2025

View reviewed changes

marcosuma approved these changes Jan 24, 2025

View reviewed changes

lantoli reviewed Jan 24, 2025

View reviewed changes

lantoli approved these changes Jan 24, 2025

View reviewed changes

EspenAlbert reviewed Jan 24, 2025

View reviewed changes

internal/service/encryptionatrest/resource_test.go Outdated Show resolved Hide resolved

EspenAlbert reviewed Jan 24, 2025

View reviewed changes

internal/testutil/acc/pre_check.go Outdated Show resolved Hide resolved

Update resource_test.go

aa378ab

maastha added 4 commits January 24, 2025 18:17

min

9041df7

Merge branch 'CLOUDP-293831-ear-aws-ci-enable' of github.com:mongodb/…

1fd8cee

…terraform-provider-mongodbatlas into CLOUDP-293831-ear-aws-ci-enable

address PR comments

09e1d26

PR comments

965fa2f

maastha merged commit fd0f885 into CLOUDP-262752-ear-aws-kms-dev Jan 27, 2025
93 of 94 checks passed

maastha deleted the CLOUDP-293831-ear-aws-ci-enable branch January 27, 2025 14:07

chore: Enables AWS encryption_at_rest acceptance tests to run in CI #3000

chore: Enables AWS encryption_at_rest acceptance tests to run in CI #3000

Uh oh!

Conversation

maastha commented Jan 23, 2025

Description

Type of change:

Required Checklist:

Further comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lantoli Jan 24, 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!

maastha Jan 27, 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!

Choose a reason for hiding this comment

Uh oh!

maastha Jan 27, 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!

Choose a reason for hiding this comment

Uh oh!

maastha Jan 27, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EspenAlbert commented Jan 24, 2025

Uh oh!

maastha commented Jan 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lantoli Jan 24, 2025 •

edited

Loading

maastha Jan 27, 2025 •

edited

Loading

maastha Jan 27, 2025 •

edited

Loading

maastha Jan 27, 2025 •

edited

Loading

maastha commented Jan 27, 2025 •

edited

Loading