Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CLOUDP-228588: Migrate Encryption at Rest #1919
base: main
Are you sure you want to change the base?
CLOUDP-228588: Migrate Encryption at Rest #1919
Changes from all commits
5cb64c3
3ceb8c3
a7bf48c
3a1eda3
2e19218
ead68f8
dd27b32
a6a3227
661c238
0658268
224c937
c45a719
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this bit? not sure why we need to dig into the Kubernetes status of the project for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure - this is just taken from the current implementation, just this happens in the controller. I think we take the IAM Role from Cloud Provider Integrations if one is not explicitly defined in the encryption at rest CRD. I agree it's not ideal, but I'm trying to be cautious about rewriting or changing stuff like this at risk of changing the behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this came from initially? did you git blame it and check for context with the original author? (if that is possible)
If we cannot get context on why is this done in this way, I would at the very least add a comment, as it is another non obvious part of the conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a corner case. If customers had Encryption At Rest enabled for their projects created before 2022 (or 2023), they can still use it with AWS configurable. For other projects, this won't work, because Atlas API now requires CPA to be configured before enabling encryption at rest for AWS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we leave a comment explaining why this is needed? I fear we will forget over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is true, why are we even doing the comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, that's a good question, let me debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sample for making this < go native Fuzz test while still using
gofuzz
:https://github.com/mongodb/mongodb-atlas-kubernetes/pull/1884/files#diff-845d71c11ed89ac707e9ff64ff4273b11ab5f62125de863d4c0af3317e8f5086R23