Question regarding Git config management
Hi, we would like to use the Git config management as outlined here: <http://wiki.freeradius.org/guide/Git-config-management> For our new Freeradius environment we will have a separate development server, one staging server and four production servers, but right now it's not in production yet. It took me a while, but I got things working in general. However, I'm pretty sure I found a bug in the post-receive hook: <https://github.com/alandekok/freeradius-server/blob/master/scripts/git/post-receive> At least on a RHEL 6 system with Bash as /bin/sh, the syntax in line 109 causes an error: "if ! conf_check then" I has to be either "if ! conf_check; then" or "if ! conf_check then" But now I have found an issue that's potentially harder to work around. When I create an intentional syntax error, the post-receive hook works great: # git push radius4 Counting objects: 11, done. Delta compression using up to 2 threads. Compressing objects: 100% (7/7), done. Writing objects: 100% (7/7), 687 bytes, done. Total 7 (delta 5), reused 0 (delta 0) remote: HEAD is now at 90caabb Syntaxfehler remote: Checking new configuration... Copyright (C) 1999-2015 The FreeRADIUS server project and contributors remote: There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A remote: PARTICULAR PURPOSE remote: You may redistribute copies of FreeRADIUS under the terms of the remote: GNU General Public License remote: For more information about these matters, see the file named COPYRIGHT remote: Starting - reading configuration files ... remote: /etc/raddb/sites-enabled/rrzk-webprojekte[107]: Syntax error: Expected comma after '1818': { remote: Errors reading or parsing /etc/raddb/radiusd.conf remote: failed remote: WARNING: FreeRADIUS found errors in the configuration, remote: please fix the errors and push the corrected configuration. remote: Attempting to roll config back to tag: "stable"... HEAD is now at 29e1232 Absichtlicher Fehler remote: ok To ssh://radiusd@radius4/etc/raddb 29e1232..90caabb master -> master When I create an error that's only detected at runtime, however, it does not seem to work. I changed the configuration so that two virtual servers were listening on the same port. The "radmin -e hup" command did not throw an error. Consequently the commit was tagged as "stable". When I tried to manually restart Freeradius on the remote server, it didn't start anymore (as expected). At that point I wasn't even able to push a fix, because radmin doesn't work when the server isn't running, and so the post-receive hook reverted back to the "stable" version! I had to fix the error manually on the remote server to get things working again. I'm not sure what can be done about that, if anything, but at the very least I would like to have a better understanding of what class of error gets past a "radmin -e hup". Normally we would always test configuration changes locally on the staging server, so that errors like the one above shouldn't ever get pushed to a production system. But as you know, eventually everything that can go wrong will go wrong ... Thanks Sebastian -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
On May 27, 2015, at 11:15 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
For our new Freeradius environment we will have a separate development server, one staging server and four production servers, but right now it's not in production yet. It took me a while, but I got things working in general. However, I'm pretty sure I found a bug in the post-receive hook:
https://github.com/alandekok/freeradius-server/blob/master/scripts/git/post-...
That's an old copy of the code. I've fixed the link to point to the correct place.
At least on a RHEL 6 system with Bash as /bin/sh, the syntax in line 109 causes an error:
The script is fixed in the main FreeRADIUS git repository.
When I create an error that's only detected at runtime, however, it does not seem to work.
Yes, because the "check config" option checks if the configuration is correct. It doesn't check a number of run-time problems.
I changed the configuration so that two virtual servers were listening on the same port.
I suppose that could be checked, too. Feel free to send a patch. :)
The "radmin -e hup" command did not throw an error. Consequently the commit was tagged as "stable". When I tried to manually restart Freeradius on the remote server, it didn't start anymore (as expected). At that point I wasn't even able to push a fix, because radmin doesn't work when the server isn't running, and so the post-receive hook reverted back to the "stable" version! I had to fix the error manually on the remote server to get things working again.
That's a bit of a different issue. If the server isn't running, then you should be able to push updates. Please send a patch.
I'm not sure what can be done about that, if anything, but at the very least I would like to have a better understanding of what class of error gets past a "radmin -e hup". Normally we would always test configuration changes locally on the staging server, so that errors like the one above shouldn't ever get pushed to a production system. But as you know, eventually everything that can go wrong will go wrong ...
The check should really be without radmin. Just stop the server and re-start it. Alan DeKok.
--On 27. Mai 2015 11:25:52 -0400 Alan DeKok <aland@deployingradius.com> wrote:
The "radmin -e hup" command did not throw an error. Consequently the commit was tagged as "stable". When I tried to manually restart Freeradius on the remote server, it didn't start anymore (as expected). At that point I wasn't even able to push a fix, because radmin doesn't work when the server isn't running, and so the post-receive hook reverted back to the "stable" version! I had to fix the error manually on the remote server to get things working again.
That's a bit of a different issue. If the server isn't running, then you should be able to push updates. Please send a patch.
For that particular problem I have come up with this, which seems to work: --- post-receive.orig 2015-05-28 17:09:53.191609956 +0200 +++ post-receive 2015-05-28 17:07:36.175731010 +0200 @@ -15,11 +15,14 @@ # Descriptive name of daemon : ${DAEMON_DESC='FreeRADIUS'} +# Init script for radiusd +: ${DAEMON_STATUS='/etc/init.d/radiusd status'} + # Command used to restart the RADIUS daemon -: ${DAEMON_REST='radmin -e hup'} +: ${DAEMON_REST='/usr/sbin/radmin -e hup'} # Command used to verify the new configuration -: ${DAEMON_CONF='radiusd -Cxl stdout'} +: ${DAEMON_CONF='/usr/sbin/radiusd -Cxl stdout'} # Command used to execute git functions : ${GIT_EXEC='env -i git'} @@ -65,6 +68,13 @@ return $ret } +daemon_status () { + echo -n "Checking if radiusd is running ... " + $DAEMON_STATUS; ret=$? + + return $ret +} + daemon_restart () { echo -n "Restarting server... " @@ -113,18 +123,22 @@ conf_rollback exit 64 else - if ! daemon_restart; then - if ! conf_rollback; then - echo "WARNING: Manually verify $DAEMON_DESC status immediately!" - exit 64 - fi - + if daemon_status; then if ! daemon_restart; then - echo "WARNING: Manually verify $DAEMON_DESC status immediately!" + if ! conf_rollback; then + echo "WARNING: Manually verify $DAEMON_DESC status immediately!" + exit 64 + fi + + if ! daemon_restart; then + echo "WARNING: Manually verify $DAEMON_DESC status immediately!" + exit 64 + fi + exit 64 fi - - exit 64 + else + echo "radiusd isn't running, please verify manually" fi $GIT_EXEC tag -f $STABLE_TAG $newrev Sebastian Hagedorn -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
--On 28. Mai 2015 11:40:30 -0400 Alan DeKok <aland@deployingradius.com> wrote:
On May 28, 2015, at 11:13 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
For that particular problem I have come up with this, which seems to work:
The mailing list mangles whitespace in patches. Please use github, it makes things much easier.
I've created a pull request. Please let me know if I should do anything differently. Sebastian Hagedorn -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
On May 28, 2015, at 11:13 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
--On 27. Mai 2015 11:25:52 -0400 Alan DeKok <aland@deployingradius.com> wrote:
The "radmin -e hup" command did not throw an error. Consequently the commit was tagged as "stable". When I tried to manually restart Freeradius on the remote server, it didn't start anymore (as expected). At that point I wasn't even able to push a fix, because radmin doesn't work when the server isn't running, and so the post-receive hook reverted back to the "stable" version! I had to fix the error manually on the remote server to get things working again.
That's a bit of a different issue. If the server isn't running, then you should be able to push updates. Please send a patch.
For that particular problem I have come up with this, which seems to work:
I thought about that when I was writing the script. The issue is you don't know when the server has come up fully, so you may run into race conditions. At the time, I think there was still the issue where the child process wasn't signalling the parent that an error had occurred, so even if the child exited with an error, the parent would exit with 0. In later versions of v2 and v3 the parent waits until the child process signals that initialisation has completed successfully, so the parent should exit with a code that signals whether the server as a whole came up correctly. The proper way to achieve what you're trying to do, is to ensure that the init script passes back the exit code of the parent process, and use that to determine if the server has come up. When you check the status of the process using the init script, you're just checking whether the process still exists immediately after its created. There's no guarantee it'll still exist five seconds after its been created. Module initialization delays opening sockets, so whilst your patched script may work for you, it wouldn't work well for an instance of the server where several hundred connections are opened to a database, even in the same scenario, with the conflicting listen sections. -Arran
On May 28, 2015, at 11:52 AM, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
On May 28, 2015, at 11:13 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
--On 27. Mai 2015 11:25:52 -0400 Alan DeKok <aland@deployingradius.com> wrote:
The "radmin -e hup" command did not throw an error. Consequently the commit was tagged as "stable". When I tried to manually restart Freeradius on the remote server, it didn't start anymore (as expected). At that point I wasn't even able to push a fix, because radmin doesn't work when the server isn't running, and so the post-receive hook reverted back to the "stable" version! I had to fix the error manually on the remote server to get things working again.
That's a bit of a different issue. If the server isn't running, then you should be able to push updates. Please send a patch.
For that particular problem I have come up with this, which seems to work:
I thought about that when I was writing the script. The issue is you don't know when the server has come up fully, so you may run into race conditions.
At the time, I think there was still the issue where the child process wasn't signalling the parent that an error had occurred, so even if the child exited with an error, the parent would exit with 0.
In later versions of v2 and v3 the parent waits until the child process signals that initialisation has completed successfully, so the parent should exit with a code that signals whether the server as a whole came up correctly.
The proper way to achieve what you're trying to do, is to ensure that the init script passes back the exit code of the parent process, and use that to determine if the server has come up.
When you check the status of the process using the init script, you're just checking whether the process still exists immediately after its created. There's no guarantee it'll still exist five seconds after its been created. Module initialization delays opening sockets, so whilst your patched script may work for you, it wouldn't work well for an instance of the server where several hundred connections are opened to a database, even in the same scenario, with the conflicting listen sections.
Also adding full paths to the binaries is annoying. The server isn't always installed in the same location. Set PATH at the top of the script. -Arran
--On 28. Mai 2015 11:52:05 -0400 Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
That's a bit of a different issue. If the server isn't running, then you should be able to push updates. Please send a patch.
For that particular problem I have come up with this, which seems to work:
I thought about that when I was writing the script. The issue is you don't know when the server has come up fully, so you may run into race conditions.
At the time, I think there was still the issue where the child process wasn't signalling the parent that an error had occurred, so even if the child exited with an error, the parent would exit with 0.
In later versions of v2 and v3 the parent waits until the child process signals that initialisation has completed successfully, so the parent should exit with a code that signals whether the server as a whole came up correctly.
The proper way to achieve what you're trying to do, is to ensure that the init script passes back the exit code of the parent process, and use that to determine if the server has come up.
When you check the status of the process using the init script, you're just checking whether the process still exists immediately after its created. There's no guarantee it'll still exist five seconds after its been created. Module initialization delays opening sockets, so whilst your patched script may work for you, it wouldn't work well for an instance of the server where several hundred connections are opened to a database, even in the same scenario, with the conflicting listen sections.
Actually I think you misunderstood what I did. I'm not trying to start the server. In fact the user that's running the git hook isn't privileged to do so. What my patch does is just to check if the server is running. If it is, the hook tries the "radmin -e hup", just as it always did before my patch. If the server isn't running, the push is tagged as "stable" based on the config check. -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
--On 28. Mai 2015 11:52:05 -0400 Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
On May 28, 2015, at 11:13 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
--On 27. Mai 2015 11:25:52 -0400 Alan DeKok <aland@deployingradius.com> wrote:
The "radmin -e hup" command did not throw an error. Consequently the commit was tagged as "stable". When I tried to manually restart Freeradius on the remote server, it didn't start anymore (as expected). At that point I wasn't even able to push a fix, because radmin doesn't work when the server isn't running, and so the post-receive hook reverted back to the "stable" version! I had to fix the error manually on the remote server to get things working again.
That's a bit of a different issue. If the server isn't running, then you should be able to push updates. Please send a patch.
For that particular problem I have come up with this, which seems to work:
I thought about that when I was writing the script. The issue is you don't know when the server has come up fully, so you may run into race conditions.
At the time, I think there was still the issue where the child process wasn't signalling the parent that an error had occurred, so even if the child exited with an error, the parent would exit with 0.
In later versions of v2 and v3 the parent waits until the child process signals that initialisation has completed successfully, so the parent should exit with a code that signals whether the server as a whole came up correctly.
The proper way to achieve what you're trying to do, is to ensure that the init script passes back the exit code of the parent process, and use that to determine if the server has come up.
When you check the status of the process using the init script, you're just checking whether the process still exists immediately after its created. There's no guarantee it'll still exist five seconds after its been created. Module initialization delays opening sockets, so whilst your patched script may work for you, it wouldn't work well for an instance of the server where several hundred connections are opened to a database, even in the same scenario, with the conflicting listen sections.
Now that the patch that allows changes to be pushed when the server isn't running has been pulled, I'm returning to the other issue. Alan wrote:
The check should really be without radmin. Just stop the server and re-start it.
Arran's post seems to contradict that. Please correct me if my understanding is wrong: Arran is saying that you cannot rely on the return value of the init script, because it returns before the server has fully initialized. Is that really true? If that's really the case, I guess the only way to make absolutely sure would be to sleep for, say, 30 seconds and to check that the server is still up ... Sebastian Hagedorn -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
On 1 Jun 2015, at 06:14, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
--On 28. Mai 2015 11:52:05 -0400 Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
On May 28, 2015, at 11:13 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
--On 27. Mai 2015 11:25:52 -0400 Alan DeKok <aland@deployingradius.com> wrote:
The "radmin -e hup" command did not throw an error. Consequently the commit was tagged as "stable". When I tried to manually restart Freeradius on the remote server, it didn't start anymore (as expected). At that point I wasn't even able to push a fix, because radmin doesn't work when the server isn't running, and so the post-receive hook reverted back to the "stable" version! I had to fix the error manually on the remote server to get things working again.
That's a bit of a different issue. If the server isn't running, then you should be able to push updates. Please send a patch.
For that particular problem I have come up with this, which seems to work:
I thought about that when I was writing the script. The issue is you don't know when the server has come up fully, so you may run into race conditions.
At the time, I think there was still the issue where the child process wasn't signalling the parent that an error had occurred, so even if the child exited with an error, the parent would exit with 0.
In later versions of v2 and v3 the parent waits until the child process signals that initialisation has completed successfully, so the parent should exit with a code that signals whether the server as a whole came up correctly.
The proper way to achieve what you're trying to do, is to ensure that the init script passes back the exit code of the parent process, and use that to determine if the server has come up.
When you check the status of the process using the init script, you're just checking whether the process still exists immediately after its created. There's no guarantee it'll still exist five seconds after its been created. Module initialization delays opening sockets, so whilst your patched script may work for you, it wouldn't work well for an instance of the server where several hundred connections are opened to a database, even in the same scenario, with the conflicting listen sections.
Now that the patch that allows changes to be pushed when the server isn't running has been pulled, I'm returning to the other issue. Alan wrote:
The check should really be without radmin. Just stop the server and re-start it.
Arran's post seems to contradict that. Please correct me if my understanding is wrong: Arran is saying that you cannot rely on the return value of the init script, because it returns before the server has fully initialized. Is that really true?
I said it used to, I then said it was fixed, and that we need to verify that the exit code is passed through in the init scripts.
If that's really the case, I guess the only way to make absolutely sure would be to sleep for, say, 30 seconds and to check that the server is still up ...
No. -Arran Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS development team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
--On 27. Mai 2015 11:25:52 -0400 Alan DeKok <aland@deployingradius.com> wrote:
I'm not sure what can be done about that, if anything, but at the very least I would like to have a better understanding of what class of error gets past a "radmin -e hup". Normally we would always test configuration changes locally on the staging server, so that errors like the one above shouldn't ever get pushed to a production system. But as you know, eventually everything that can go wrong will go wrong ...
The check should really be without radmin. Just stop the server and re-start it.
I would, but I have found another problem. We use user and group "radiusd". When I try to start radiusd with that user, the following happens: $ /usr/sbin/radiusd -d /etc/raddb radiusd: Cannot change ownership of log file /var/log/radius/radius.log: Operation not permitted But the log file actually belongs to radiusd: $ ll /var/log/radius/radius.log -rw-r----- 1 radiusd radiusd 5322 29. Mai 15:03 /var/log/radius/radius.log With strace I was able to see that the chown tries to set the user to UID 0, i.e. root: ... open("/var/log/radius/radius.log", O_WRONLY|O_CREAT|O_APPEND, 0640) = 3 chown("/var/log/radius/radius.log", 0, 95) = -1 EPERM (Operation not permitted) futex(0x3fc403c3a8, FUTEX_WAKE_PRIVATE, 2147483647) = 0 write(2, "radiusd: Cannot change ownership"..., 97radiusd: Cannot change ownership of log file /var/log/radius/radius.log: Operation not permitted ) = 97 ... I'm not clear why that happens. The code in mainconfig.c->switch_users() looks OK at first glance ... ideas? Sebastian Hagedorn -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
On May 29, 2015, at 9:17 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
I would, but I have found another problem. We use user and group "radiusd". When I try to start radiusd with that user, the following happens:
$ /usr/sbin/radiusd -d /etc/raddb radiusd: Cannot change ownership of log file /var/log/radius/radius.log: Operation not permitted
But the log file actually belongs to radiusd:
$ ll /var/log/radius/radius.log -rw-r----- 1 radiusd radiusd 5322 29. Mai 15:03 /var/log/radius/radius.log
With strace I was able to see that the chown tries to set the user to UID 0, i.e. root:
Because you don't have "user = radiusd" in radiusd.conf. This is documented in radiusd.conf. The server assumes it starts as "root". If you don't have "user = radiusd", it tries to chown() the log files to "root". Alan DeKok.
--On 29. Mai 2015 09:26:19 -0400 Alan DeKok <aland@deployingradius.com> wrote:
With strace I was able to see that the chown tries to set the user to UID 0, i.e. root:
Because you don't have "user = radiusd" in radiusd.conf. This is documented in radiusd.conf.
The server assumes it starts as "root". If you don't have "user = radiusd", it tries to chown() the log files to "root".
But I do: # The server will also try to use "initgroups" to read # /etc/groups. It will join all groups where "user" is a # member. This can allow for some finer-grained access # controls. # user = radiusd group = radiusd Also: # service radiusd start radiusd starten: [ OK ] [root@radius1-neu ~]# ps auxww|grep radiusd radiusd 22734 0.5 0.4 301320 8460 ? Ssl 15:32 0:00 /usr/sbin/radiusd -d /etc/raddb So the switching works when the process is started by root ... Sebastian Hagedorn -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
--On 29. Mai 2015 10:56:31 -0400 Alan DeKok <aland@deployingradius.com> wrote:
On May 29, 2015, at 9:33 AM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
So the switching works when the process is started by root ...
I've pushed a fix. Please try the v3.0.x branch from github.
Now I get a different error message: $ radiusd -d /etc/raddb radiusd: Cannot initialize supplementary group list for user radiusd: Operation not permitted Sebastian Hagedorn -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
On May 29, 2015, at 12:02 PM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
Now I get a different error message:
$ radiusd -d /etc/raddb radiusd: Cannot initialize supplementary group list for user radiusd: Operation not permitted
I've pushed a fix for that. It should now be fine. Alan DeKok.
--On 29. Mai 2015 12:11:21 -0400 Alan DeKok <aland@deployingradius.com> wrote:
On May 29, 2015, at 12:02 PM, Sebastian Hagedorn <Hagedorn@uni-koeln.de> wrote:
Now I get a different error message:
$ radiusd -d /etc/raddb radiusd: Cannot initialize supplementary group list for user radiusd: Operation not permitted
I've pushed a fix for that. It should now be fine.
Confirmed. Thanks Sebastian Hagedorn -- .:.Sebastian Hagedorn - Weyertal 121 (Gebäude 133), Zimmer 2.02.:. .:.Regionales Rechenzentrum (RRZK).:. .:.Universität zu Köln / Cologne University - ✆ +49-221-470-89578.:.
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
Sebastian Hagedorn