feat(htpasswd): add autoreload for htpasswd #2933
Conversation
|
Unsure, htpasswd change do not require full reload with server restarts. But putting the logic into existing watcher much easier. |
|
Looks like it works, but not as fast as file updates. |
vooon
force-pushed
the
htpasswd-reload
branch
6 times, most recently
from
1f35ccb to
e4605a5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2933 +/- ##
==========================================
- Coverage 91.06% 90.94% -0.12%
==========================================
Files 174 175 +1
Lines 32655 32787 +132
==========================================
+ Hits 29736 29819 +83
- Misses 2207 2246 +39
- Partials 712 722 +10 ☔ View full report in Codecov by Sentry. |
…eload Signed-off-by: Vladimir Ermakov <[email protected]>
vooon
force-pushed
the
htpasswd-reload
branch
from
e4605a5 to
259df17
Compare
|
Ideally, we need the watching logic to move outside the server/controller logic, so it is just another config file we watch (along with zot config) That could be second step. Let other folks take a look also. |
What do you mean by file updates? |
|
@andaaron i mean the delay observed after file write. |
If a separate watcher is an improvement, I think we should go for it. It doesn't make sense to restart the whole server for a new user. |
vooon
force-pushed
the
htpasswd-reload
branch
3 times, most recently
from
743d50c to
8db17b2
Compare
Signed-off-by: Vladimir Ermakov <[email protected]>
vooon
force-pushed
the
htpasswd-reload
branch
from
8db17b2 to
3790d39
Compare
|
@andaaron i've added separate watcher. Need to test it, and also write some tests, but I'm more used to testify... |
Signed-off-by: Vladimir Ermakov <[email protected]>
Signed-off-by: Vladimir Ermakov <[email protected]>
Signed-off-by: Vladimir Ermakov <[email protected]>
| } | ||
|
|
||
| case err := <-ret.watcher.Errors: | ||
| ret.log.Panic().Err(err).Str("htpasswd-file", ret.filePath).Msg("fsnotfy error while watching config") |
There was a problem hiding this comment.
Isn't panic a bit harsh? Do we want to stop the server in case of an error watching the credentials file?
There was a problem hiding this comment.
Well, i copied that behaviour from config watcher. Unsure if i ever seen that error, so probably better to die, than left unpredictable state.
There was a problem hiding this comment.
A change in the config file not being applied is something impacting the entire service, while a change in htpasswd not being applied impacts just specific users who were added/removed/had the passwords changed.
I don't think we should stop the service in this specific case.
There was a problem hiding this comment.
But that's more like entire system error, than this file error. Or i'm wrong?
There was a problem hiding this comment.
Well yes, but do we want to interrupt service for all users, or just the ones which don't have access anyway? Or would it help the admin with troubleshooting the issue if the entire service stops?
What type of PR is this?
feature
Which issue does this PR fix:
Fix #2925
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.