Ideas collection

[patman15]

Hi @ikifar2012!

I was looking pretty long for an addon that would do automatic backup to my NAS but use SSH based data transfer (rsync, scp, ...). So with your addon I found the perfect match. The last days I have played around with it and gathered a few ideas on improvements. Just let me know which of them sound interesting to you:

Code

1. key-based authentication for rsync, PR#51
2. use bashio logger instead of own functions, PR#52
3. introduce log levels to be set by configuration (depends on 2.), PR#52, debug enable as decided in this comment
4. shorten script by using for loopfor various individual rsync folders, PR#54

Configuration

1. make ssh_port type port, covered by PR#54
2. use arrays for excludes of addons/folders, covered by PR#54

Documentation

1. Clarify a bit which options are used for which type of operation (especially necessary due to code/1)
2. make clear that due to StrictHostKeyChecking no security is impacted -> think about solution for code
3. document date-based cleanup function in delete-local-backup (if I saw that correctly in the code) misinterpretation

For almost all I already have an idea, if you let me know which interest you, I would create more pull requests.

Thanks again, you solved one of my longest running issues with backup of HA!

[ikifar2012]

HI, thank you for your contributions, I have been looking at your changes and I haven't had time to test everything yet but I do like them, however I do have some questions

Code

1. I have been thinking of removing SSH pass entirely, enforcing SSH keys in order to use rsync, but it does appear that you can indeed use an SSH key without changing the rsync command as per your PR but I am still wondering if I should remove the use of passwords
2. I have been thinking of doing this but wanted colour coding, that being said it does look like bashio does now support color coding so I say go ahead I just have to test
3. So when I first saw the PR in the second item I was going to ask why you removed the if statements for debug but if we can have proper log levels that is even better

Other than that everything else you have listed here seems good, I will have to take a look to see how an array appears on the UI for the user

[patman15]

Hi!

ad 1) I had to patch my NAS from default setting on command line in order to get key-based authentication working. I would be ok with removing, but not sure this is a good idea for a number of people out there.

ad 3) do you want to be able to set a logging level (1-7) or a similar setting to what you had which was "debugging yes/no"?

[ikifar2012]

1. then it makes sense to keep, I say keep it
2. I was thinking debug yes/no but if you can think of a use for separate log levels I am good with that too

[patman15]

Thought about some more things:

• since the key for ssh and rsync is the same, shouldn't the whole configuration be simplified by making only one field for [ssh_user, rsync_user], [ssh_host, rsync_host]? Is there a use-case to copy files to different hosts?
• The script doesn't report any fails. So e.g. if the backup host is not available you don't get notified. I have no immediate idea, but I would suggest to introduce some sort of notification if something fails. E.g. use the service persistent_notification.create What do you think?

Debug switch done see PR#52 .

[ikifar2012]

Thanks again for all the help I have been super busy lately and these are all much need improvements I just saw the updates to PR #52 and will merge that now

• I am not sure if there are use cases where people need separate hosts for each, I would say no but the only reason I kept it like that is because that's how it was when I took over the project. I think it is probably safe to consolidate into one option, although I will also ask @SirGoodenough for his opinion as he uses both ssh and rsync but I believe its to the same host

• That was actually on my todo list for a while (first mentioned by @SirGoodenough in #36) to make a sensor for backup status I think that and/or a persistent notification would be great

[SirGoodenough]

I am using the same host for both. That is generally not a problem, because if that is safe enough for one it's safe enough for both. One of my backups is over the internet, thru zerotier but still thru the wan. Doing encryption and no password would be a good option.

Do you have a beta branch I can load?

[ikifar2012]

Currently I do not have an easy way for you to load a beta but you can pull down the main branch remove the image from the config.yaml and run the addon locally by putting it in your addons folder which you can access via SAMBA (see the section here titled "Remote Development" https://developers.home-assistant.io/docs/add-ons/testing#remote-development)

[patman15]

Thanks again for all the help I have been super busy lately and these are all much need improvements

You're welcome, I just have some spare time at the moment. Just direct me to your preferences what to do any time.

@SirGoodenough I am using the same host for both.

Ok, I'll do a configuration option cleanup in a new branch to simplify configuration in general.

@SirGoodenough Doing encryption and no password would be a good option.

Not sure what you mean. The transfer is already encrypted and you cannot encrypt a backup without password (as far as I know). Can you explain / give an example?

@ikifar2012 to that extend, I see that you are using zip to encrypt the backups but there is the --password flag already for the ha backups new command. Any reason that is not used?

[ikifar2012]

@ikifar2012 to that extend, I see that you are using zip to encrypt the backups but there is the --password flag already for the ha backups new command. Any reason that is not used?

It was just something I was waiting to do as I didn't want to break anyone's config's but since we are already changing the rsync stuff I have addressed it in #57

[patman15]

Ok great, a lot of progress. :-) If you are fine with merging https://github.com/ikifar2012/remote-backup-addon/pull/54 and https://github.com/ikifar2012/remote-backup-addon/pull/57 then I would start with a new branch for cleaning up the configuration options as mentioned in my previous comment and create a new PR. Next I'll work on a solution for https://github.com/ikifar2012/remote-backup-addon/issues/36 but I guess it will be notifications, I have no idea, how to create a sensor from a script. I'm not sure this is possible, at least I couldn't find something from the bashio functions. Need to keep that sequence otherwise it gets messy.

[patman15]

Thanks for merging, I continued to develop.

• I created a new PR https://github.com/ikifar2012/remote-backup-addon/pull/58 that cleans up the configuration options. Note that this is a breaking change, thus the PR is against your development branch.
• I also started to work on better error handling (and I'm looking for a method to notify users). If you want to have a look, it's here: https://github.com/patman15/remote-backup-addon/compare/master...patman15:remote-backup-addon:improve-error-handling Notifications actually do work! :+1: Will still take a while ...
• Through a lot of debugging I found out, that the scp command actually uses sftp by default. There are these options -O and -s and depending on the implementation either the one or the other is default. For HA it is currently -s. Not sure what to do / document about this. @ikifar2012 any ideas? Same as https://github.com/ikifar2012/remote-backup-addon/issues/50

[patman15]

Created PR https://github.com/ikifar2012/remote-backup-addon/pull/59 that also solves https://github.com/ikifar2012/remote-backup-addon/issues/36 in my opinion.

[patman15]

@ikifar2012 with PR #60 being ready and the documentation update I'm done with all my changes I had thought of. If anyone has feedback on the changes, please let me know, I still have a bit of time to do fixes. Otherwise I will pause here, wait for the merges and hope that my changes are helpful for a number of people.

[SirGoodenough]

@patman15

@ikifar2012 with PR #60 being ready and the documentation update I'm done with all my changes I had thought of. If anyone has feedback on the changes, please let me know,

I did add #61 , thinking about the order things are done. Not critical, but just a thought that may improve function. No, I didn't look if this still applies to the updated version as I have not tried to load that...

[ikifar2012]

I will test this more tomorrow but upon running the dev backup addon I got the following error

[00:17:13] NOTICE: Excluded folder(s):
ssl
addons/local
homeassistant
sed: bad option in substitution expression
[00:17:13] NOTICE: Excluded addon(s):
local_tdm
[00:17:13] DEBUG: Including folder(s) ""
[00:17:13] DEBUG: Including addon(s) "core_samba", "a0d7b954_ssh", "local_rsyslog", "core_mosquitto", "core_ssh", "local_amcrest2mqtt-addon", "local_remote_backup"
[00:17:13] DEBUG: Requested API resource: http://supervisor/backups/new/partial
[00:17:13] DEBUG: Request method: POST
[00:17:13] DEBUG: Request data: {"name":"Automated backup 2022-08-30 00-17", "password": "null", "addons": ["core_samba", "a0d7b954_ssh", "local_rsyslog", "core_mosquitto", "core_ssh", "local_amcrest2mqtt-addon", "local_remote_backup"], "folders": [""]}
[00:17:13] DEBUG: API HTTP Response code: 400
[00:17:13] DEBUG: API Response: {"result": "error", "message": "value must be one of ['addons/local', 'homeassistant', 'media', 'share', 'ssl'] @ data['folders'][0]. Got ''"}
[00:17:13] ERROR: Got unexpected response from the API: value must be one of ['addons/local', 'homeassistant', 'media', 'share', 'ssl'] @ data['folders'][0]. Got ''
[00:17:13] FATAL: Error creating partial backup!
[00:17:13] DEBUG: Requested API resource: http://supervisor/core/api/services/persistent_notification/create
[00:17:13] DEBUG: Request method: POST
[00:17:13] DEBUG: Request data: {"message":"Local backup process failed! See log for details.", "title":"Addon: Remote Backup Failed!", "notification_id":"addon-remote-backup"}
[00:17:13] DEBUG: API HTTP Response code: 200
[00:17:13] DEBUG: API Response: []
jq: error (at <stdin>:1): Cannot index array with string "result"
jq: error (at <stdin>:1): Cannot index array with string "data"
[00:17:13] DEBUG: Requested API resource: http://supervisor/core/api/events/remote_backup_status
[00:17:13] DEBUG: Request method: POST
[00:17:13] DEBUG: Request data: {"result":"error","message:":"Local backup process failed! See log for details."}
[00:17:13] DEBUG: API HTTP Response code: 200
[00:17:13] DEBUG: API Response: {"message":"Event remote_backup_status fired."}
[00:17:13] FATAL: Local backup process failed! See log for details.

config

remote_host: 192.168.0.18
remote_port: 22
remote_user: ha-dev
remote_key: id_ed25519
backup_friendly_name: true
backup_custom_prefix: Automated backup
backup_keep_local: all
ssh_enabled: true
ssh_remote_directory: /home/ha-dev/ha-backup-testing
rsync_enabled: true
rsync_rootfolder: /home/ha-dev/ha-backup-testing/rsync
rsync_exclude:
  - /config/*.db-shm
  - /config/*.db-wal
  - /config/*.db
backup_exclude_folders:
  - ssl
  - addons/local
  - homeassistant
backup_exclude_addons:
  - local_tdm
debug: true

[patman15]

Sorry for that, should be fixed using PR https://github.com/ikifar2012/remote-backup-addon/pull/62

[ikifar2012]

I also noticed the single quotes that were added in

https://github.com/ikifar2012/remote-backup-addon/blob/393faf6c96185ea19cb1ea9ceef7b5644be480d2/remote-backup/run.sh#L159

cause the scp command to fail

[ikifar2012]

Strangely, removing the single quotes even with a path that includes spaces works

[patman15]

Strangely, removing the single quotes even with a path that includes spaces works

I can have a look tomorrow, do you want me to check/fix/generate a pull request or have you fixed/checked it yourself already?

[ikifar2012]

Strangely, removing the single quotes even with a path that includes spaces works

I can have a look tomorrow, do you want me to check/fix/generate a pull request or have you fixed/checked it yourself already?

If you can take a look at that, that would be great, as I am unsure if I should just remove the single quotes and leave it as is or do some additional input sanitization as it appears to work for me, even with spaces in the path

[extera-nl]

Not sure if I need to open a new feature request, if so, please let me know.

Is it possible to add SSH password authentication instead of only ssh public key auth? I am using a transip STACK account to connect to, which only supports webdav and sftp with username + password authentication.

edit: did not found this option in the documentation. But after some digging found the doc update: https://github.com/ikifar2012/addons.mathesonsteplock.ca/blob/3a97b43c3bb6cfea79c6ae02a1b23a2f84a364ee/docs/addons/remote-backup/basic-config.md

Do I need to use the dev branch for ssh password authentication? or is this just a documentation update?

edit2: doesn't work, I'll wait for the new release :-D

[patman15]

I also noticed the single quotes that were added in

https://github.com/ikifar2012/remote-backup-addon/blob/393faf6c96185ea19cb1ea9ceef7b5644be480d2/remote-backup/run.sh#L159

cause the scp command to fail

@ikifar2012 I tested it again in my setup with the path /volume1/NetMirror/home.lan/ssh/this folder has spaces!//Automated backup 2022-09-01 10-12.tar. With the code I had in the PR (including the single quotes, i.e. like in quoted above) it works as soon as I remove them it results in

scp: ambiguous target

Which is the expected behavior for me. Can you please double-check and probably provide configuration, error messages, etc.? I do not know how to reproduce and to my understanding the syntax as provided should be the correct one. Weird idea: Could it be a an issue with the character encoding of the code which causes the single quotes to be wrong? I mean a mix of those three: ` ' '

edit: I (hopefully) figured it out: I locally was using SCP while the default of the command scp is nowadays SFTP as we discussed in https://github.com/ikifar2012/remote-backup-addon/issues/50. The fun part is that SCP requires different quoting than using SFTP. That might explain why it worked for me but not for @ikifar2012. Anyhow, I now decided to implement both (try first SFTP (more secure) then fall back to SCP). This should solve several topics for several users for quite some time. Note, that (at least on my Synology) also the path is different for SFTP and SCP! In addition @extera-nl I also fixed the command to support password authentication. @ikifar2012 All changes are in PR https://github.com/ikifar2012/remote-backup-addon/pull/64 or can be tested from my fork https://github.com/patman15/remote-backup-addon, I also provided a documentation update with my learnings.

[ikifar2012]

Again, I'd just like to say thank you @patman15 for all your work this release, you did an incredible job and really helped me check off basically everything on my to-do list. Let me know if you have any social media or anything you would like me to shout out, otherwise I think we are ready to publish the release

[patman15]

@ikifar2012 you are welcome, I hope it is useful for a number of people. I'm currently away for my computer so I can't check the final release version in my setup, but from the changes I saw all should be good for releasing. I have no relevant social media appearance your feedback gives me a lot of motivation. Let me know if I can help you in the future as well, unfortunately I won't have that much time soon. It took much longer than I thought although it looks like only a bit of functionality. Still there are a lot of things and options that can go wrong. Thanks for the enjoyable work with you!

P.S.: there are some minor things regarding coding style that still could be improved.

• script should be at a different location, looking at https://github.com/hassio-addons/repository
• authors of HA are not happy with the usage of the API, see https://github.com/hassio-addons/bashio/issues/113 I have no idea how to fix that though.

Nevertheless, this is for some future...

[ikifar2012]

@ikifar2012 you are welcome, I hope it is useful for a number of people. I'm currently away for my computer so I can't check the final release version in my setup, but from the changes I saw all should be good for releasing. I have no relevant social media appearance your feedback gives me a lot of motivation. Let me know if I can help you in the future as well, unfortunately I won't have that much time soon. It took much longer than I thought although it looks like only a bit of functionality. Still there are a lot of things and options that can go wrong. Thanks for the enjoyable work with you!

P.S.: there are some minor things regarding coding style that still could be improved.

• script should be at a different location, looking at https://github.com/hassio-addons/repository
• authors of HA are not happy with the usage of the API, see [FR] HA Proxy API addition hassio-addons/bashio#113 I have no idea how to fix that though.

Nevertheless, this is for some future...

I definitely do agree, there is still a lot to improve, I am learning more and more everyday and I learned a ton whilst reviewing your changes.

As per your recommendations, I have been thinking of submitting this to the community addons but I am unsure if it will be accepted,

Before you offered to help I was thinking of writing a component to do the sensor based things , I have also thought of re-writing the whole thing in python but that is way down the road, I am very much a beginner when it comes to python and your changes pretty much covered everything I wanted to do.

Thank you for everything, you are always welcome to help out when/if you have time

[patman15]

Yeah, I fear for submitting to the community add-ons there are some conceptual things that need to be fixed. Unfortunately, I have no experience with that either. Anyways, probably letting people know in the "share your projects" section of the community could help. Otherwise, if you want to reach out to me, my username is patman in the HA community.