Skip to content

fix: restore --rbac should not error when backup has no RBAC access dir#1439

Merged
Slach merged 2 commits into
Altinity:masterfrom
mvanhorn:fix/1432-restore-rbac-missing-access
Jun 25, 2026
Merged

fix: restore --rbac should not error when backup has no RBAC access dir#1439
Slach merged 2 commits into
Altinity:masterfrom
mvanhorn:fix/1432-restore-rbac-missing-access

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Makes restore --rbac a graceful no-op when a backup contains no RBAC objects, matching the existing behavior of download --rbac. Previously, restoring such a backup failed with restoreRBAC: restoreRBACResolveAllConflicts: walk backup access path: ... lstat .../access: no such file or directory.

Why this matters

fix #1432, create_remote --rbac on a cluster with no RBAC objects succeeds silently (done createBackupRBAC, size=0B), and download --rbac of that backup tolerates the missing access dir. But restore --rbac of the same backup errors out. This is an inconsistent, surprising failure for a perfectly valid backup.

The root cause is error-identity stripping: restoreRBACResolveAllConflicts runs filepath.Walk on <DefaultDataPath>/backup/<name>/access. When that dir is absent, the walk callback wraps the *os.PathError via errors.Wrap(...), so the function's tail guard os.IsNotExist(walkErr) no longer matches and the error propagates. The same pattern affects restoreBackupRelatedDir (and restoreRBACReplicated), whose os.Stat ENOENT was also wrapped before reaching the caller's os.IsNotExist(err) guards in restoreRBAC.

Changes

  • pkg/backup/restore.go
    • restoreRBACResolveAllConflicts: add a narrow early return when os.Stat(backupAccessPath) reports the dir does not exist (log a debug line, return nil). Only the not-exist case is swallowed; the existing os.IsNotExist(walkErr) tail guard is kept as defense-in-depth.
    • restoreBackupRelatedDir and restoreRBACReplicated: when the source dir does not exist, return the unwrapped os.Stat error so the callers' existing os.IsNotExist(err) guards in restoreRBAC can detect it and skip RBAC restore. Non-not-exist stat errors are still wrapped as before.

Testing

Added unit tests in pkg/backup/restore_test.go (no running ClickHouse required):

  • TestRestoreRBACResolveAllConflictsMissingAccessDir: with &Backuper{DefaultDataPath: t.TempDir()} and no access dir, restoreRBACResolveAllConflicts returns nil.
  • TestRestoreBackupRelatedDirMissingDirIsNotExist: a missing source dir yields an error for which os.IsNotExist is true, ensuring restoreRBAC's graceful-skip guards keep working.
gofmt -l pkg/backup/restore.go pkg/backup/restore_test.go   # clean
go vet ./pkg/backup/...                                      # ok
go build ./...                                               # ok
go test ./pkg/backup/ -run 'RBAC|RestoreBackupRelatedDir' -count=1   # pass

Fixes #1432

@Slach

Slach commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

thanks for contribution

@Slach Slach merged commit a270390 into Altinity:master Jun 25, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants