Skip to content

feat: Adds encryption_at_rest_provider to mongodbatlas_search_deployment resource and data source #3152

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 11 commits into from
Mar 18, 2025

Conversation

oarbusi
Copy link
Collaborator

@oarbusi oarbusi commented Mar 11, 2025

Description

Adds computed attribute encryption_at_rest_provider to mongodbatlas_search_deployment resource and data source.
It takes 10-15 minutes to apply the encryption, so the API does not return the attribute until encryption is applied

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 changed the base branch from master to CLOUDP-296693-dev-byok March 14, 2025 08:19
@oarbusi
Copy link
Collaborator Author

oarbusi commented Mar 14, 2025

test failures are not related to these changes and are because of the usage of the preview sdk. Will be fixed in CLOUDP-306803

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

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

Copy link
Contributor

@lmkerbey-mdb lmkerbey-mdb left a comment

Choose a reason for hiding this comment

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

LGTM.

)
mig.SkipIfVersionBelow(t, "1.13.0")
mig.SkipIfVersionBelow(t, "1.30.0") // encryption_at_rest_provider was added in 1.30.0
Copy link
Collaborator

@maastha maastha Mar 14, 2025

Choose a reason for hiding this comment

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

Are we sure this feat will be released in 1.30?
It helps to capture all things to do before merge to master in the ticket. Would ensure this version is correct before merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, added a comment in CLOUDP-296693 specifying what needs to be done before we merge the dev branch to master and included TODO in b83f556 so that it's harder to miss

Copy link
Member

@AgustinBettati AgustinBettati left a comment

Choose a reason for hiding this comment

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

LGTM, nice refactors on the file name convention

"github.com/hashicorp/terraform-plugin-framework/types"
)

type TFSearchDeploymentDSModel struct {
Copy link
Member

Choose a reason for hiding this comment

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

nit: You can consider refactoring to use DataSourceSchemaFromResource

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But DataSourceSchemaFromResource is for the resource schema, not the TF models? I tried to use TFSearchDeploymentRSModel everywhere, but the resource model has the timeout attribute, and it should not be in the data source

}
}

func configBasic(orgID, projectName, clusterName, instanceSize string, searchNodeCount int) string {
clusterConfig := advancedClusterConfig(orgID, projectName, clusterName)
func configBasic(projectID, clusterName, instanceSize string, searchNodeCount int) string {
Copy link
Member

Choose a reason for hiding this comment

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

question: Does encryption_at_rest resource need to be defined? At which point is this computes attribute returned?

Copy link
Collaborator Author

@oarbusi oarbusi Mar 17, 2025

Choose a reason for hiding this comment

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

No need to define it, encryption_at_rest_provider attribute already exists in adv_cluster and is Optional/Computed, we want to add it in search_deployment only as Computed, this value can't be changed in search deployments, only in adv_cluster, and search nodes will get the same encryption as the cluster. Attribute is always returned by search deployments API.

@oarbusi oarbusi merged commit 4806d29 into CLOUDP-296693-dev-byok Mar 18, 2025
38 of 39 checks passed
@oarbusi oarbusi deleted the CLOUDP-296693-ear-provider branch March 18, 2025 11:32
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