sftp-backup: Add hostname prefix and allow usage of filedrop sftp server #4602
Conversation
…ledrop only sftp server
AdSchellevis
left a comment
AdSchellevis
left a comment
There was a problem hiding this comment.
@beposec I left some small remarks, but generally it looks good, thanks
| $config = $cnf->object(); | ||
| if ($this->model->prefixhostname->isEmpty()) { | ||
| $fileprefix = "config-"; | ||
| } else { |
There was a problem hiding this comment.
better move $config = Config::getInstance()->object(); inside the block
There was a problem hiding this comment.
Moved $config to the else block.
| } | ||
| return $remote_backups; | ||
| } else { | ||
| return array($target_filename); |
There was a problem hiding this comment.
| return array($target_filename); | |
| return $this->ls(sprintf('%s*.xml', $fileprefix)); |
There was a problem hiding this comment.
Listing the file here is not possible because it is moved to another directory immediately after creation.
There was a problem hiding this comment.
That's pretty inconsistent with the block above, which starts with querying the the other end to know if it should actually do a backup. If we always want an unconditional push, it might be better to flag this as a separate option with appropriate code path.
I do expect that returning an empty list doesn't cause any real issues by the way, which is technically also the correct answer, after the backup, we can't know the actual result as the file has been moved. In which case just returning the remote list should be good enough.
There was a problem hiding this comment.
You are absolutely right. I have adapted it so that it lists the existing files consistently like the other blocks, but if there are no files, it simply tells you so.
There was a problem hiding this comment.
ok, but why not just let it return [] ? just checking in case I'm missing something here
There was a problem hiding this comment.
I tried it. When it returns [] the backup is executed, but throws an error. I think this is the reason:
https://github.com/opnsense/core/blob/master/src/www/diag_backup.php#L401C1-L401C50
There was a problem hiding this comment.
the actual backup doesn't look at the result, but the admin page is, I'll take a look, thanks
…urned by the remote host, for opnsense/plugins#4602
…urned by the remote host, for opnsense/plugins#4602
…ver (#4602) * Add possibility for hostname prefix for backups and allow usage of filedrop only sftp server * Move config variable into else block * Set value in case no backups where found on the server and housekeeping is disabled. --------- Co-authored-by: Ad Schellevis <AdSchellevis@users.noreply.github.com>
|
@beposec this 037cb53 combined with opnsense/core@c48d393 should do the trick. |
|
I will verify that soon, but it should work. Thanks for the quick and productive implementation process. Can't wait to implement it in production |
…urned by the remote host, for opnsense/plugins#4602 (cherry picked from commit c48d393)
|
Do you have any idea when this will go into production? |
|
probably next release. |
2 participants
The goal of this feature is to store backups on an SFTP server that automatically relocates the backup files to a non-accessible folder after they are created. Additionally, it should support storing backups from multiple firewalls.
Identified Issues:
Enhancements Introduced in This Pull Request: