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

@@ -625,6 +633,8 @@ jobs:

  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

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?

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

internal/testutil/acc/pre_check.go

 	if os.Getenv("AWS_ACCESS_KEY_ID") == "" ||
 		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?

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

internal/service/encryptionatrest/resource_test.go

 		ExternalProviders:        acc.ExternalProvidersOnlyAWS(),
 		ProtoV6ProviderFactories: acc.TestAccProviderV6Factories,
 		CheckDestroy:             acc.EARDestroy,
 		Steps: []resource.TestStep{
 			{
 				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

@@ -625,6 +633,8 @@ jobs:

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


why the ${{ github.repository}}?

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

internal/service/encryptionatrest/resource_migration_test.go

@@ -27,14 +25,14 @@ func TestMigEncryptionAtRest_basicAWS(t *testing.T) {
 			Enabled:             conversion.Pointer(true),


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

EspenAlbert · 2025-01-24T17:23:19Z

internal/service/encryptionatrest/resource_test.go

+					acc.CheckEARExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "project_id", projectID),
+					resource.TestCheckResourceAttr(resourceName, "aws_kms_config.0.customer_master_key_id", awsKms.GetCustomerMasterKeyID()),
+					resource.TestCheckResourceAttr(resourceName, "aws_kms_config.0.egion", awsKms.GetRegion()),


typo: egion

Are we sure the tests are running in CI?

EspenAlbert · 2025-01-24T17:23:55Z

internal/testutil/acc/pre_check.go

 	}
 }

 func PreCheckEncryptionAtRestEnvAWS(tb testing.TB) {
 	tb.Helper()
+	PreCheckBasic(tb) // temp


// temp ?

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

Any reason why TestMigEncryptionAtRest_withRole_basicAWS is still skipped?

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

maastha commented Jan 23, 2025

maastha Jan 24, 2025

lantoli Jan 24, 2025 •

edited

Loading

marcosuma Jan 24, 2025

marcosuma Jan 24, 2025

lantoli Jan 24, 2025

EspenAlbert Jan 24, 2025

EspenAlbert Jan 24, 2025

EspenAlbert Jan 24, 2025

EspenAlbert Jan 24, 2025

EspenAlbert Jan 24, 2025

EspenAlbert commented Jan 24, 2025

		@@ -27,14 +25,14 @@ func TestMigEncryptionAtRest_basicAWS(t *testing.T) {
		Enabled: conversion.Pointer(true),

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

Are you sure you want to change the base?

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

Conversation

maastha commented Jan 23, 2025

Description

Type of change:

Required Checklist:

Further comments

Choose a reason for hiding this comment

lantoli Jan 24, 2025 • edited Loading

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

Choose a reason for hiding this comment

EspenAlbert commented Jan 24, 2025

lantoli Jan 24, 2025 •

edited

Loading