search

nusenu / ansible-relayor

An Ansible Role for Tor Relay Operators
GNU General Public License v3.0
245 stars 43 forks source link

Reload tor instances if tor-exit-notice.html is changed #214

Closed anadahz closed 3 years ago

anadahz commented 3 years ago

When tor-exit-notice.html is changed the tor instances should reload for the changes to take effect.

Fixes: https://github.com/nusenu/ansible-relayor/issues/213

nusenu commented 3 years ago

Hi anadahz,

thanks for this change. It has been a known issue for a while.

If you only change the HTML file, reloading is safe, but when multiple things are changed at the same time reloading is not supported in all cases by tor, that is why we switched from reloading to restarting tor - regardless of the change. See #189.

Ideally run the kitchen test with the changes applied.

anadahz commented 3 years ago

Hi @nusenu,

If you only change the HTML file, reloading is safe, but when multiple things are changed at the same time reloading is not supported in all cases by tor,

Is there an issue to have the reload handler?

In my case I did only change the HTML file and the changes did not propagate (see https://github.com/nusenu/ansible-relayor/issues/213). I can change the handler to restart instead of reload but this will trigger an unnecessary restart when only the HTML file changes.

nusenu commented 3 years ago

Is there an issue to have the reload handler?

Yes, because reloads fail when settings got changed that do not support reloads. Since it is to complex to dynamically check what did actually change lets use restart.

On mixed relays (running exits and non-exits) all instances will get restarted even if we only care about exits and not non-exits, but I believe this is an acceptable trade-off.

In my case I did only change the HTML file and the changes did not propagate (see https://github.com/nusenu/ansible-relayor/issues/213). I can change the handler to restart instead of reload but this will trigger an unnecessary restart when only the HTML file changes.

There is a known dilemma:

a) use reload and fail sometimes if setting changed that do not support a reload b) use restart and cause unnecessary restarts but at least don't fail

Let's use restart for the handler.

The exit-notice-handler branch contains an untested simple way to implement it. e1e7d9ab2569fe8d79323bb39404f0bfa30c62f8

The branch might has an issue where the torrc is not changed but the notice file has changed because in that case tor_instances_tmp might not be defined when the handler is run. This needs some testing.

anadahz commented 3 years ago

@nusenu I still do not understand what is the issue to keep both the reload and restart handlers?

We know for sure that the HTML file get updated when a reload is issued. Is this not a valid reason to keep the reload handler or do I miss anything?

nusenu commented 3 years ago

after testing it turned out that the reload handler will never be invoked before the restart handler since they are invoked in the order they are defined in handler/main.yml

thanks for your patch, it is merged now.