-
Notifications
You must be signed in to change notification settings - Fork 103
Enable encryption in backup operator test #2355
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
base: main
Are you sure you want to change the base?
Conversation
8e290a8
to
421ed56
Compare
The backup test is failing because of #2332 In order to test encryption changes, I reverted that PR locally and tested encryption changes. |
Result of fdb-kubernetes-operator-pr on Linux RHEL 9
|
Result of fdb-kubernetes-operator-pr on Linux RHEL 9
|
@@ -73,6 +73,7 @@ func (factory *Factory) CreateBackupForCluster( | |||
AllowTagOverride: ptr.To(true), | |||
ClusterName: fdbCluster.Name(), | |||
Version: fdbVersion.String(), | |||
EncryptionKeyPath: "/tmp/encryption-key/key.bin", |
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.
This means all tests will be running with encryption enabled. I would prefer having an option in the FdbBackupConfiguration
to enabled encryption if wanted.
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.
Thank you for the review! I'm new to this repo so didn't know exactly where to add the encryption key logic.
I will check the FdbBackupConfiguration
and update the logic there.
@@ -68,6 +68,8 @@ var _ = BeforeSuite(func() { | |||
|
|||
// Create a blobstore for testing backups and restore | |||
factory.CreateBlobstoreIfAbsent(fdbCluster.Namespace()) | |||
|
|||
// Note: Encryption key secret is automatically created during namespace setup |
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.
Is there anything we should change in the e2e test case to ensure we test backups with and without encryption (and also validate that the backup is actually encrypted)?
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.
That's a great point.
I'm trying to add a file - metadata that contains if backup is encrypted or not.
After that we can check that metadata file has encryption enabled in the test and
if encryption is enabled and restore works then that can give us a some kind of gurantee.
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.
Also I'm trying to change this test to include both encryption and without encryption test.
I would be surprised, the newly added test case was not running 🤔 |
Description
Enable encryption in backup operator test.
Type of change
Discussion
Testing
CLEANUP=false NAMESPACE=ak make -C e2e test_operator_backups.run
Documentation
Follow-up