Draft
Conversation
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| rm -rf "${tmpdir}/lost+found" | ||
|
|
||
| if [[ -n "${ENCRYPTION_KEY_FILE}" ]]; then |
Contributor
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
Suggested change
| if [[ -n "${ENCRYPTION_KEY_FILE}" ]]; then | |
| if [[ -n ${ENCRYPTION_KEY_FILE} ]]; then |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for encrypted backups/restores by introducing an encryption spec in backup storage configuration, wiring it through the backup sidecar, and updating restore flow to decrypt before prepare/move-back.
Changes:
- Add
encryptionconfiguration to Backup/Storage specs and propagate it into the sidecar backup request/config. - Implement encryption key handling in the sidecar (read key from Secret, write temp key file, pass
--encrypt-*flags to xtrabackup). - Introduce MySQL pod RBAC/ServiceAccount resources to allow in-pod Secret reads, and update CRDs/bundles/examples accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/xtrabackup/xtrabackup.go | Pass encryption options into backup jobs; mount secret + set ENCRYPTION_KEY_FILE for restore jobs; extend BackupConfig. |
| pkg/mysql/mysql.go | Add RBAC object constructors; conditionally set MySQL StatefulSet serviceAccountName for newer CR versions. |
| pkg/controller/ps/controller.go | Create Role/RoleBinding/ServiceAccount during DB reconcile for newer CR versions. |
| cmd/sidecar/main.go | Switch to constructing a backup handler via backup.NewHandler(). |
| cmd/sidecar/handler/handler.go | Remove backup handler factory wrapper. |
| cmd/sidecar/handler/backup/handler.go | Add handler constructor/init with a controller-runtime k8s client. |
| cmd/sidecar/handler/backup/create.go | Create temp encryption key file and pass encryption flags to xtrabackup. |
| build/run-backup.sh | Include encryption in the JSON payload sent to the sidecar. |
| build/run-restore.sh | Decrypt backup contents when ENCRYPTION_KEY_FILE is set. |
| api/v1/perconaservermysql_types.go | Add EncryptionSpec to BackupStorageSpec and implement Secret key reading helper. |
| api/v1/perconaservermysqlbackup_types.go | Add per-backup encryption override + storage fallback getter. |
| api/v1/zz_generated.deepcopy.go | Update deep-copy generation for new encryption types/fields. |
| config/crd/bases/*.yaml | Extend CRD OpenAPI schemas to include encryption blocks. |
| deploy/.yaml, deploy/backup/.yaml | Update shipped CRDs/bundles and example manifests with encryption fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+159
to
+167
| func (b *PerconaServerMySQLBackup) GetEncryption(storage *BackupStorageSpec) *EncryptionSpec { | ||
| if b.Spec.Encryption != nil { | ||
| return b.Spec.Encryption | ||
| } | ||
| if storage != nil && storage.Encryption != nil { | ||
| return storage.Encryption | ||
| } | ||
| return nil | ||
| } |
Comment on lines
+152
to
+155
| if cr.CompareVersion("1.1.0") >= 0 { | ||
| _, _, sa := RBAC(cr) | ||
| spec.ServiceAccountName = sa.GetName() | ||
| } |
| { | ||
| APIGroups: []string{corev1.SchemeGroupVersion.Group}, | ||
| Resources: []string{"secrets"}, | ||
| Verbs: []string{"get", "list"}, |
Comment on lines
+51
to
+52
| k8sClient, err := client.New(config.GetConfigOrDie(), client.Options{}) | ||
| if err != nil { |
| http.Error(w, "backup failed", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| defer os.Remove(encryptionKeyFile) //nolint:errcheck |
Comment on lines
40
to
47
| h.getNamespaceFunc = func() (string, error) { | ||
| ns, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "read namespace file") | ||
| } | ||
|
|
||
| return string(ns), nil | ||
| } |
Comment on lines
+548
to
+559
| if cr.CompareVersion("1.1.0") >= 0 { | ||
| role, binding, sa := mysql.RBAC(cr) | ||
| if err := k8s.EnsureObjectWithHash(ctx, r.Client, cr, role, r.Scheme); err != nil { | ||
| return errors.Wrap(err, "create role") | ||
| } | ||
| if err := k8s.EnsureObjectWithHash(ctx, r.Client, cr, sa, r.Scheme); err != nil { | ||
| return errors.Wrap(err, "create service account") | ||
| } | ||
| if err := k8s.EnsureObjectWithHash(ctx, r.Client, cr, binding, r.Scheme); err != nil { | ||
| return errors.Wrap(err, "create role binding") | ||
| } | ||
| } |
Comment on lines
+252
to
+263
| tempFile, err := os.CreateTemp(os.TempDir(), "encryption-key-*.key") | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "create temporary file") | ||
| } | ||
|
|
||
| if _, err := tempFile.WriteString(key); err != nil { | ||
| return "", errors.Wrap(err, "write key to file") | ||
| } | ||
| if err := tempFile.Close(); err != nil { | ||
| return "", errors.Wrap(err, "close file") | ||
| } | ||
| return tempFile.Name(), nil |
Comment on lines
+366
to
+385
| type EncryptionSpec struct { | ||
| Enabled bool `json:"enabled,omitempty"` | ||
| SecretName string `json:"secretName,omitempty"` | ||
| Key string `json:"key,omitempty"` | ||
| } | ||
|
|
||
| func (e *EncryptionSpec) ReadEncryptionKey(ctx context.Context, cl client.Reader, namespace string) (string, error) { | ||
| if !e.Enabled { | ||
| return "", fmt.Errorf("encryption is not enabled") | ||
| } | ||
|
|
||
| secret := &corev1.Secret{} | ||
| if err := cl.Get(ctx, types.NamespacedName{Namespace: namespace, Name: e.SecretName}, secret); err != nil { | ||
| return "", errors.Wrap(err, "get encryption secret") | ||
| } | ||
|
|
||
| value := secret.Data[e.Key] | ||
| if len(value) == 0 { | ||
| return "", fmt.Errorf("encryption key not found in secret") | ||
| } |
Collaborator
commit: 27b1d97 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability