Skip to content

feat: Supports configuring BYOK encryption on search nodes #3142

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

Merged
merged 14 commits into from
Mar 12, 2025

Conversation

lantoli
Copy link
Member

@lantoli lantoli commented Mar 10, 2025

Description

Supports configuring BYOK encryption on search nodes. Examples will be done in a follow-up PR. TODOs will be there until changes are available in the latest SDK release (now only available in the preview)

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

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

@oarbusi oarbusi marked this pull request as ready for review March 11, 2025 11:34
@oarbusi oarbusi requested review from a team as code owners March 11, 2025 11:34
Copy link
Contributor

APIx bot: a message has been sent to Docs Slack channel

@@ -151,3 +153,19 @@ func NewAtlasAzureKeyVault(tfAzKeyVaultConfigSlice []TFAzureKeyVaultConfigModel)
RequirePrivateNetworking: v.RequirePrivateNetworking.ValueBoolPointer(),
}
}

func NewAtlasEncryptionAtRest(encryptionAtRestPlan, encryptionAtRestState *TfEncryptionAtRestRSModel, atlasEncryptionAtRest *admin.EncryptionAtRest) *admin.EncryptionAtRest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing a unit test here. Not sure if encryptionAtRestPlan.EnabledForSearchNodes.IsUnknown() will be handled as expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This made me think and I changed the approach a bit here: 7f97e3c
API has a default value (false), so we can have the default in TF aswell, and we will never have that as Unknown/Null making this simpler. Let me know what you think

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it, this way, removing the attribute will "go back" to the default value and trigger a plan change 👍

@@ -233,12 +234,27 @@ func TestCheckErrorMessageAndStatus(t *testing.T) {

for testName, tc := range testCases {
t.Run(testName, func(t *testing.T) {
diags := encryptionatrestprivateendpoint.CheckErrorMessageAndStatus(tc.SDKResp)
// TODO: update before merging to master: diags := encryptionatrestprivateendpoint.CheckErrorMessageAndStatus(tc.SDKResp)
diags := checkErrorMessageAndStatusPreview(tc.SDKResp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

temporary change to be able to use the preview. encryptionatrestprivateendpoint is using the latest release and not the preview

Copy link
Collaborator

@EspenAlbert EspenAlbert left a comment

Choose a reason for hiding this comment

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

LGTM. A few minor comments

@@ -135,40 +137,3 @@ func TestMigEncryptionAtRest_basicGCP(t *testing.T) {
},
})
}

func TestMigEncryptionAtRest_basicAWS_from_v1_11_0(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just double checking why this was removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Full context of this test is here. I think it's a good time to remove the test because 1.11.0 was released August 2023, and we already have migration tests that run with the previous released version

@oarbusi oarbusi merged commit 2fc6af3 into CLOUDP-296693-dev-byok Mar 12, 2025
39 checks passed
@oarbusi oarbusi deleted the CLOUDP-296693-byok-preview branch March 12, 2025 13:42
oarbusi added a commit that referenced this pull request Apr 4, 2025
* feat: Supports configuring BYOK encryption on search nodes (#3142)

* use SDK preview in encryption_at_rest

* changelog

* Revert "use SDK preview in encryption_at_rest"

This reverts commit 609c9dc.

* trigger change in EAR

* Revert "trigger change in EAR"

This reverts commit 15794dd.

* Reapply "use SDK preview in encryption_at_rest"

This reverts commit 1c2db30.

* TEMPORARY: send enabled_for_search_nodes = true

* finish resource implementation and tests

* data source implementation and test

* doc update

* default and refactor test

* remove old migration test

* default value in resource

* unit test

---------

Co-authored-by: Oriol Arbusi <[email protected]>

* feat: Adds `encryption_at_rest_provider` to `mongodbatlas_search_deployment` resource and data source (#3152)

* use preview

* add encryption_at_rest_provider computed attribute

* remove check

* dosc

* rename files

* move adv_cluster config out of resources

* fix config

* project id

* add TODO to version

* doc: Updates examples with newly added attributes to `mongodbatlas_search_deployment` and `mongodbatlas_encryption_at_rest` (#3174)

* add new attribute to the example

* examples updates

* nit: end with new line

* fix tf validate

* todos

* PR suggestions + test failure

---------

Co-authored-by: Leo Antoli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants