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.

Sign up for GitHub

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

Jump to bottom

backupccl: encrypted restore of an incremental backup fails #91886

Closed
adityamaru opened this issue Nov 15, 2022 · 2 comments · Fixed by #91911
Closed

backupccl: encrypted restore of an incremental backup fails #91886

adityamaru opened this issue Nov 15, 2022 · 2 comments · Fixed by #91911
Assignees
@adityamaru
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-disaster-recovery

Comments

@adityamaru
Copy link
Contributor

adityamaru commented Nov 15, 2022 •
edited by exalate-issue-sync bot
Loading 

root@localhost:26257/defaultdb> backup into 'nodelocal://1/full' WITH incremental_location='nodelocal://1/incremental', encryption_passphrase='123';
        job_id       |  status   | fraction_completed | rows | index_entries | bytes
---------------------+-----------+--------------------+------+---------------+--------
  813950484531609601 | succeeded |                  1 |   25 |             6 |  1740
(1 row)


Time: 352ms total (execution 351ms / network 0ms)

root@localhost:26257/defaultdb> backup into latest in 'nodelocal://1/full' WITH incremental_location='nodelocal://1/incremental', encryption_passphrase='123';
        job_id       |  status   | fraction_completed | rows | index_entries | bytes
---------------------+-----------+--------------------+------+---------------+--------
  813950540301697025 | succeeded |                  1 |    0 |             0 |     0
(1 row)


Time: 165ms total (execution 164ms / network 0ms)

root@localhost:26257/defaultdb> restore from latest in 'nodelocal://1/full' WITH incremental_location='nodelocal://1/incremental', encryption_passphrase='123';
ERROR: could not find or read encryption information: no ENCRYPTION-INFO files found

The regression was introduced in #87311 and backported to 22.2 as well.

Jira issue: CRDB-21468

Epic CRDB-21944
@adityamaru adityamaru added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery labels Nov 15, 2022
@blathers-crl
Copy link

blathers-crl bot commented Nov 15, 2022

cc @cockroachdb/disaster-recovery

adityamaru added a commit to adityamaru/cockroach that referenced this issue Nov 15, 2022
@adityamaru
backupccl: fix bug in resolving encrypted backup manifests
d0dcad1 
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
@adityamaru adityamaru mentioned this issue Nov 15, 2022
Merged
@adityamaru adityamaru self-assigned this Nov 15, 2022
@adityamaru
Copy link
Contributor Author

cc: @celiala this is a new GA-blocker for release-22.2.0 since it breaks restores and show backup for encrypted backups and is a regression from 22.1 behaviour. I have the fix in #91911 and this will be merged by the end of the day, so it shouldn't slow anything down as far as picking the next RC?

@blathers-crl blathers-crl bot mentioned this issue Nov 15, 2022
Merged
blathers-crl bot pushed a commit that referenced this issue Nov 15, 2022
@adityamaru
backupccl: fix bug in resolving encrypted backup manifests
1d196f5 
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
@blathers-crl blathers-crl bot mentioned this issue Nov 15, 2022
Merged
blathers-crl bot pushed a commit that referenced this issue Nov 15, 2022
@adityamaru
backupccl: fix bug in resolving encrypted backup manifests
4783270 
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
craig bot pushed a commit that referenced this issue Nov 15, 2022
@adityamaru
Merge #91911
56b139b 
91911: backupccl: fix bug in resolving encrypted backup manifests r=stevendanna a=adityamaru

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

Co-authored-by: adityamaru <[email protected]>
@craig craig bot closed this as completed in 9a30575 Nov 15, 2022
@exalate-issue-sync exalate-issue-sync bot reopened this Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adityamaru adityamaru

Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-disaster-recovery
Projects
No open projects
Disaster Recovery Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

backupccl: fix bug in resolving encrypted backup manifests adityamaru/cockroach
1 participant
@adityamaru