Backup protected status can vary per location #5569
Conversation
mdegat01
added
missing-documentation
📝 WalkthroughWalkthroughThe changes introduce a more comprehensive approach to managing backup locations in the supervisor's backup system. Modifications enhance the data structure for backup locations by adding attributes for file path and protection status. This involves updates across multiple files including Changes
Sequence DiagramsequenceDiagram
participant BackupManager
participant Backup
participant API
BackupManager->>Backup: Create backup with location details
Backup-->>BackupManager: Backup object with path and protection status
BackupManager->>BackupManager: Validate location and password
BackupManager->>API: Return backup information
API->>API: Process backup details including protection status
The sequence diagram illustrates the enhanced flow of backup creation and information retrieval, highlighting the new approach of storing and passing detailed location information including path and protection status. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/api/test_backups.py (1)
626-630: Consider checking the return status of assertionsAfter making assertions about file existence, it's good practice to handle potential exceptions or check the return status to ensure the test behaves as expected even if the files are missing.
While this is a test, adding error handling can make debugging easier if the test fails unexpectedly.
tests/backups/test_backup.py (2)
30-47: Avoid code duplication by utilizing a fixture or helper functionThe test
test_consolidate_conflict_varied_encryptionduplicates code for setting up backups. Consider creating a fixture or a helper function to reduce duplication and enhance maintainability.Extract the backup setup code into a fixture:
@pytest.fixture def encrypted_and_unencrypted_backups(coresys, tmp_path): enc_tar = Path(copy(get_fixture_path("test_consolidate.tar"), tmp_path)) enc_backup = Backup(coresys, enc_tar, "test", None) await enc_backup.load() unc_tar = Path(copy(get_fixture_path("test_consolidate_unc.tar"), tmp_path)) unc_backup = Backup(coresys, unc_tar, "test", None) await unc_backup.load() return enc_backup, unc_backupThen use it in the test:
async def test_consolidate_conflict_varied_encryption( coresys: CoreSys, encrypted_and_unencrypted_backups, caplog: pytest.LogCaptureFixture ): enc_backup, unc_backup = encrypted_and_unencrypted_backups ...
44-47: Improve assertion clarityThe assertion message can be enhanced for better clarity in case of test failure.
Include a message in the assertion:
assert enc_backup.all_locations == {None: {"path": unc_tar, "protected": False}}, "Backup locations did not consolidate as expected"supervisor/api/const.py (1)
56-56: Maintain alphabetical order of constantsTo improve readability and maintainability, consider inserting
ATTR_PROTECTED_LOCATIONSin alphabetical order within the list of constants.Place
ATTR_PROTECTED_LOCATIONSafterATTR_PROTECTED(if it exists) or at the appropriate position to keep the list sorted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/fixtures/test_consolidate.taris excluded by!**/*.tartests/fixtures/test_consolidate_unc.taris excluded by!**/*.tar
📒 Files selected for processing (7)
supervisor/api/backups.py(5 hunks)supervisor/api/const.py(1 hunks)supervisor/backups/backup.py(10 hunks)supervisor/backups/manager.py(6 hunks)tests/api/test_backups.py(11 hunks)tests/backups/test_backup.py(2 hunks)tests/backups/test_manager.py(2 hunks)
🔇 Additional comments (18)
tests/api/test_backups.py (2)
952-986: Clarify the handling of protected backups intest_protected_backupThe test creates a protected backup and asserts that
protected_locationsis[None]. Ensure that the API behavior aligns with the expected outcome, especially when backups exist in multiple locations.Please confirm that the
protected_locationsfield accurately reflects all locations where the backup is protected. If backups can exist in multiple locations, some protected and some not, theprotected_locationsshould include all relevant locations.
925-950: Ensure testtest_backup_mixed_encryptionreflects accurate backup locationsThe test checks that
backup.all_locationsincludes bothNoneand.cloud_backuplocations, but later asserts thatbody["data"]["backups"][0]["locations"]is[None]. This seems inconsistent.Please verify that the API correctly reports all backup locations. Run the following script to check the backup locations returned by the API:
This script checks the locations reported for the backup with slug
d9c48f8b.✅ Verification successful
The test case correctly validates the API's security behavior
The test is correctly verifying that when a backup exists in multiple locations with mixed encryption states, the API only exposes the protected location (
None) while hiding the unprotected location (.cloud_backup). This appears to be an intentional security feature to prevent exposing information about unprotected backup copies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the backup locations are correctly reported by the API. # Expected: The backup should report both None and .cloud_backup in its locations. curl -s http://localhost:8123/backups | jq '.data.backups[] | select(.slug=="d9c48f8b") | .locations'Length of output: 103
Script:
#!/bin/bash # Find the API endpoint implementation for backups ast-grep --pattern 'async def get_backups($$$)' # Search for any documentation about backup locations rg -l "backup.*location" -g "*.md" # Find similar test cases rg -l "test_backup.*location" -g "test_*.py"Length of output: 189
Script:
#!/bin/bash # Check backup manager tests for location handling rg -A 5 "all_locations" tests/backups/test_manager.py # Find API route handler ast-grep --pattern 'router.get("/backups"$$$)' # Check the test file content cat tests/api/test_backups.py | grep -A 10 "async def test_backup.*location"Length of output: 3209
Script:
#!/bin/bash # Find the API implementation with a different pattern ast-grep --pattern 'async def api_backups($$$)' # Look for the route definition rg -A 5 '@router.get\("/backups"\)' # Check for any security-related comments rg -B 2 "protected.*locations"Length of output: 1081
supervisor/api/backups.py (4)
36-36: LGTM! New attributes added for backup locations.The addition of
ATTR_PATHandATTR_PROTECTED_LOCATIONSenhances the API's ability to handle backup locations and their protection status.Also applies to: 58-58
168-172: LGTM! Protected locations list added to backup info.The list comprehension efficiently filters locations based on their protection status, providing valuable information to API consumers.
244-248: LGTM! Protected locations list added to detailed backup info.The implementation is consistent with the list in the backup overview.
473-473: LGTM! Updated path access for backup files.The change correctly uses the new nested dictionary structure to access backup file paths.
supervisor/backups/backup.py (7)
42-42: LGTM! New attributes added for backup locations.The addition of
ATTR_PATHandATTR_PROTECTEDaligns with the changes in the backup location structure.
95-100: LGTM! Enhanced backup location storage.The new structure stores both path and protection status for each location, improving data organization.
130-130: LGTM! Updated protected property.The property now correctly accesses the protection status from the new location structure.
207-207: LGTM! Updated path access methods.The properties correctly handle the new nested dictionary structure for accessing backup paths and locations.
Also applies to: 245-245
261-281: LGTM! Enhanced backup equality comparison.The comparison now correctly ignores protection status when determining if backups are equal, as current encryption status does not affect equality.
293-306: LGTM! Improved conflict handling in backup consolidation.The code now properly handles and logs conflicts when consolidating backups from different locations.
335-335: LGTM! Updated protection status handling.The code correctly sets protection status in the new location structure during backup creation and loading.
Also applies to: 462-464
supervisor/backups/manager.py (4)
15-16: LGTM! New attributes added for backup locations.The addition of
ATTR_PATHandATTR_PROTECTEDaligns with the changes in the backup location structure.
291-291: LGTM! Updated path access in remove method.The code correctly accesses the backup file path from the new location structure.
345-357: LGTM! Enhanced location handling in copy operations.The code properly updates the backup locations with both path and protection status after copying.
681-704: LGTM! New validation method for location and password.The new method centralizes location and password validation logic, improving code organization and reusability.
tests/backups/test_manager.py (1)
1992-1995: LGTM! Updated test assertions for backup locations.The test assertions correctly reflect the new structure of backup locations, including both path and protection status.
Also applies to: 2012-2015, 2021-2021
|
The I've updated the test to actually copy the backup to that mount location as well, and fixed the test accordingly. |
agners
force-pushed
the
protected-per-location
branch
2 times, most recently
from
e8b5452 to
7eab589
Compare
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| @@ -263,7 +263,7 @@ def __eq__(self, other: Any) -> bool: | |||
|
|
|||
| # Compare all fields except ones about protection. Current encryption status does not affect equality | |||
| keys = self._data.keys() | other._data.keys() | |||
| for k in keys - {ATTR_PROTECTED, ATTR_CRYPTO}: | |||
| for k in keys - {ATTR_PROTECTED, ATTR_CRYPTO, ATTR_DOCKER}: | |||
There was a problem hiding this comment.
Do we have tests to ensure these attributes are ignored?
There was a problem hiding this comment.
There's a test to confirm an encrypted backup and unencrypted backup can be consolidated, which means those fields were ignored. I didn't have docker in there before so my backup fixtures didn't have that, I'll have to check on that.
Technically for docker we should only be ignoring it if one backup is encrypted and one isn't, otherwise it should be the same. I don't know if we want to deal with that for a field to be deprecated though
There was a problem hiding this comment.
I don't know if we want to deal with that for a field to be deprecated though
Yeah let's not bother about that.
But adding docker attributes and make sure they get ignored in the test make sense 👍 .
There was a problem hiding this comment.
Ok, I've updated the test fixture files to include a docker repository setting. The test fails without the change above, but with the change merged succeeds 👍 .
| @@ -1734,12 +1734,20 @@ async def test_backup_remove_error( | |||
| healthy_expected: bool, | |||
| ): | |||
| """Test removing a backup error.""" | |||
There was a problem hiding this comment.
Why did this test start failing?
There was a problem hiding this comment.
Essentially, because one of the backup didn't exist before. But previously the test just "created" the location, and now because all_locations it is a dictionary in a dictionary that lead to an error.
A bit more detail in the comment below at #5569 (comment).
tests/backups/test_manager.py
Outdated
| if location_name is None: | ||
| copy(backup_file, coresys.config.path_backup) | ||
| location = None | ||
| else: | ||
| (mount_dir := coresys.config.path_mounts / location_name).mkdir() | ||
| copy(backup_file, mount_dir) | ||
| location = coresys.mounts.get(location_name) |
There was a problem hiding this comment.
Instead of the if-statement, consider adding additional parametrization variable, for the subdirectory and maybe the key to get the location from coresys.mounts.get
There was a problem hiding this comment.
Unfortunately how to get the location is different for None and mount backups. I've reused existing code now, so that makes the if statement unnecessary.
Wait what was the failure? I didn't think the file not being in that location would cause a failure, I'm pretty sure other tests do that too? |
It is not that the file did not exist which caused the error per se, it was that the location was not in the backup.all_locations[location_name]["path"] = (tar_file_mock := MagicMock())I guess this would have worked too: backup.all_locations[location_name] = {}
backup.all_locations[location_name]["path"] = (tar_file_mock := MagicMock())But I feel it is cleaner to actually create the backup file, let the code make sure that assert location_name in backup.all_locations
backup.all_locations[location_name]["path"] = (tar_file_mock := MagicMock()) |
mdegat01
force-pushed
the
protected-per-location
branch
from
072b656 to
f0cc2d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/api/test_backups.py (1)
547-547:⚠️ Potential issueAvoid accessing protected member
_locationsdirectly.Direct modification of protected member
_locationsbreaks encapsulation. Consider adding a public method or using dependency injection for testing.
🧹 Nitpick comments (2)
tests/backups/test_backup.py (1)
30-47: LGTM with suggestions for improvement!The test function effectively validates the consolidation behavior for backups with varied encryption. Consider these improvements:
- Use more descriptive variable names like
encrypted_backup_tarandunencrypted_backup_tarfor better readability.- Consider parameterizing the test to cover more encryption combinations.
- enc_tar = Path(copy(get_fixture_path("test_consolidate.tar"), tmp_path)) - enc_backup = Backup(coresys, enc_tar, "test", None) + encrypted_backup_tar = Path(copy(get_fixture_path("test_consolidate.tar"), tmp_path)) + encrypted_backup = Backup(coresys, encrypted_backup_tar, "test", None) - unc_tar = Path(copy(get_fixture_path("test_consolidate_unc.tar"), tmp_path)) - unc_backup = Backup(coresys, unc_tar, "test", None) + unencrypted_backup_tar = Path(copy(get_fixture_path("test_consolidate_unc.tar"), tmp_path)) + unencrypted_backup = Backup(coresys, unencrypted_backup_tar, "test", None)tests/backups/test_manager.py (1)
1759-1769: Consider extracting test setup to a fixture.The test setup logic could be moved to a fixture to improve reusability and reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/fixtures/test_consolidate.taris excluded by!**/*.tartests/fixtures/test_consolidate_unc.taris excluded by!**/*.tar
📒 Files selected for processing (7)
supervisor/api/backups.py(5 hunks)supervisor/api/const.py(1 hunks)supervisor/backups/backup.py(10 hunks)supervisor/backups/manager.py(6 hunks)tests/api/test_backups.py(11 hunks)tests/backups/test_backup.py(2 hunks)tests/backups/test_manager.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- supervisor/api/const.py
- supervisor/api/backups.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run tests Python 3.13.1
- GitHub Check: Check pylint
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (7)
supervisor/backups/backup.py (3)
95-100: LGTM! Well-structured location data model.The new dictionary structure for
_locationsimproves data organization by keeping related attributes together.
261-281: LGTM! Robust equality comparison with detailed logging.The equality comparison now correctly handles protection status and provides detailed debug logging for mismatches.
296-306: LGTM! Clear conflict handling with informative logging.The consolidation logic now provides clear warning messages when conflicts are detected.
supervisor/backups/manager.py (1)
688-711: LGTM! Well-structured validation method.The new
_validate_location_passwordmethod effectively centralizes validation logic and provides clear error messages.tests/api/test_backups.py (2)
927-950: LGTM! Comprehensive test for mixed encryption scenarios.The test effectively validates backup behavior with different encryption states across locations.
952-986: LGTM! Well-structured test for protected backups.The test thoroughly validates the protected status of backups and their locations.
tests/backups/test_manager.py (1)
2019-2022: LGTM! Clear assertions for location structure.The assertions effectively validate the new location structure with both path and protection status.
Also applies to: 2039-2042
mdegat01
removed
the
need-client-library
mdegat01
removed
the
missing-documentation
Proposed change
Backups can now be easily encrypted and decrypted while streaming and core does this regularly. Therefore two backups that are otherwise identical except one is encrypted and one is not need to be considered the same. And Supervisor should tell clients the protected status of backups across locations.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
These changes provide a more comprehensive approach to managing backup locations, ensuring detailed information about backup protection and file paths.