Skip to content
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

backupccl: fix bug in resolving encrypted backup manifests #91911

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

adityamaru
Copy link
Contributor

In #87311 we refactored ResolveBackupManifests to concurrently load manifests from base and incremental layers by calling FetchPreviousBackups. The mistake in that diff was to pass in only the incremental layers of the backup to FetchPreviousBackup instead of passing in the base backup + incremental layers. This was silently passing all tests because in ResolveBackupManifests we explicitly setup the base backup layer in all the relevant slices before processing each incremental layer. The one case this was not okay in was encrypted backups. FetchPreviousBackups apart from doing the obvious would also read the encryption options from the base backups before reading the manifests from each layer. Now, because we stopped sending the base backup in the input slice, this step would fail since the method would go to the first incremental backup (instead of the base backup) and attempt to read the ENCRYPTION_INFO file. This file is only ever written to the base backup and so a SHOW BACKUP or a RESTORE of an encrypted backup would fail with a file not found error.

In this diff we:

  1. Fix ResolveBackupManifests by passing in the base backup + incremental backups to FetchPreviousBackups.

  2. Make it the callers responsibility to pass in the fully hydrated encryption options before calling FetchPreviousBackups so that the method is only fetching backup manifests.

Fixes: #91886

Release note (bug fix): fixes a bug that would cause SHOW BACKUP and RESTORE of encrypted incremental backups to fail

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru
Copy link
Contributor Author

I'm going to add some tests that should've caught this (mis)behaviour when the original PR was merged.

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. We really need some tests here.

pkg/ccl/backupccl/backup_job.go Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backupinfo/manifest_handling.go Outdated Show resolved Hide resolved
@adityamaru adityamaru force-pushed the fix-enc-backup branch 2 times, most recently from ef0b578 to ef83ee6 Compare November 15, 2022 17:33
@adityamaru adityamaru marked this pull request as ready for review November 15, 2022 18:15
@adityamaru adityamaru requested review from a team as code owners November 15, 2022 18:15
In cockroachdb#87311 we refactored `ResolveBackupManifests` to concurrently
load manifests from base and incremental layers by calling
`FetchPreviousBackups`. The mistake in that diff was to
pass in only the incremental layers of the backup to `FetchPreviousBackup`
instead of passing in the base backup + incremental layers.
This was silently passing all tests because in `ResolveBackupManifests`
we explicitly setup the base backup layer in all the relevant slices before
processing each incremental layer. The one case this was not okay in
was encrypted backups. `FetchPreviousBackups` apart from doing the obvious would
also read the encryption options from the base backups before reading the
manifests from each layer. Now, because we stopped sending the base backup
in the input slice, this step would fail since the method would go to the
first incremental backup (instead of the base backup) and attempt to
read the ENCRYPTION_INFO file. This file is only ever written to the base
backup and so a `SHOW BACKUP` or a `RESTORE` of an encrypted backup would
fail with a file not found error.

In this diff we:

1) Fix `ResolveBackupManifests` by passing in the base backup + incremental
backups to `FetchPreviousBackups`.

2) Make it the callers responsibility to pass in the fully hydrated encryption
options before calling `FetchPreviousBackups` so that the method is *only* fetching
backup manifests.

Fixes: cockroachdb#91886

Release note (bug fix): fixes a bug that would cause `SHOW BACKUP` and
`RESTORE` of encrypted incremental backups to fail
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test!

@adityamaru
Copy link
Contributor Author

blathers backport release-22.2

@adityamaru
Copy link
Contributor Author

blathers backport release-22.2.0

@adityamaru
Copy link
Contributor Author

TFTR!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Nov 15, 2022

Build succeeded:

@craig craig bot merged commit 56b139b into cockroachdb:master Nov 15, 2022
@blathers-crl
Copy link

blathers-crl bot commented Nov 15, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating backport branch refs/heads/blathers/backport-release-22.2-91911: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists []

Backport to branch 22.2.x failed. See errors above.


error creating backport branch refs/heads/blathers/backport-release-22.2.0-91911: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists []

Backport to branch 22.2.0 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@adityamaru adityamaru deleted the fix-enc-backup branch November 15, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backupccl: encrypted restore of an incremental backup fails
3 participants