Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add syslog's ident and facility parameters to config #2701

Closed

Conversation

9 participants
@pprindeville
Copy link
Contributor

commented Aug 22, 2017

You might want to be able to set the ident (syslog.ident) and facility (syslog.facility) before a single line of scripting gets executed... especially if you're debugging code and it might crash before it has a chance to call openlog(), etc.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2017

@bukka Should the existing support for ident and facility in FPM be refactored to share code?

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch 3 times, most recently from 93d2765 to 393e350 Aug 22, 2017

@bukka

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

@pprindeville This is nice but this won't give us an ability for setting an external server without going through the system logger. As I said I really like what nginx does as it re-implements the syslog client completely and allows direct remote logging. It has got many advantages and it's more flexible solution that could be especially useful for future improvements (especially in FPM where we could make things work in more non-blocking way...)

@bukka

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

That being said I think it would be good to at least have this if we don't have an alternative solution before 7.3 feature deadline (this is a new feature so it's just for master which means 7.3).

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch from 393e350 to a56e2d9 Aug 23, 2017

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2017

This is nice but this won't give us an ability for setting an external server without going through the system logger.

@bukka Going directly to an external logger is, in some circles, considered a liability. One of the requirements of the Red Book section "2.2.2.2 Audit" is for centralized logging and collection, as it makes it easier to enforce access control, log rotation, etc. if it goes through a centralized mechanism.

Allowing individual apps or subsystems to defeat that would be counter to the the requirements of Red Book.

I would hope that this mechanism is configurable at build-time so that one could build PHP without logging bypass.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2017

That being said I think it would be good to at least have this if we don't have an alternative solution before 7.3 feature deadline (this is a new feature so it's just for master which means 7.3).

@bukka These are really orthogonal issues. My fix is to make the usage of the syslog API more complete. Yours is an entire alternative to it.

It's not an "either/or" but rather "and".

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2017

Amended: if we call openlog() explicitly from the script, we don't want the first call to syslog() calling it again.

@bukka

This comment has been minimized.

Copy link
Member

commented Aug 24, 2017

@pprindeville There are many use cases when direct remote logging is useful especially when it comes to containers. For example you might want to run Docker containers for rsyslog and another for PHP and log into the rsyslog container. Of course I'm sure that there are many more examples and I also believe that it wouldn't be in nginx if it was pointless :). The fact is that Red Book was released in 1987 and lots of things have changed since then.

Personally I see the future of PHP (namely FPM) running as a single app inside the containers and that's why I'm looking to the solutions how to improve logging in there because currently it there are lots of issues and limitations.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2017

@bukka Can you please explain to me the order of .ini and .conf files read by php-cli vs. php-fpm? Or point me at where the differences are explained? Because with this change, it seems like php-fpm is picking up the syslog.ident and syslog.facility and the values from php7-fpm.ini or php7-fpm.d/*.conf are being ignored (or overwritten).

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch from a56e2d9 to d1a74f4 Aug 27, 2017

@bukka

This comment has been minimized.

Copy link
Member

commented Aug 28, 2017

@pprindeville The main ini file (e.g. php.ini) is loaded during the startup in fpm which happens when you start master. You can find it in fpm_main.c and the call flow is like this:

main -> cgi_sapi_module.startup -> php_module_startup -> php_init_config

FPM allows to redefine the ini values which is described in here

; Additional php.ini defines, specific to this pool of workers. These settings
; overwrite the values previously defined in the php.ini. The directives are the
; same as the PHP SAPI:
; php_value/php_flag - you can set classic ini defines which can
; be overwritten from PHP call 'ini_set'.
; php_admin_value/php_admin_flag - these directives won't be overwritten by
; PHP call 'ini_set'
; For php_*flag, valid values are on, off, 1, 0, true, false, yes or no.
; Defining 'extension' will load the corresponding shared extension from
; extension_dir. Defining 'disable_functions' or 'disable_classes' will not
; overwrite previously defined php.ini values, but will append the new value
; instead.
; Note: path INI options can be relative and will be expanded with the prefix
; (pool, global or @prefix@)
; Default Value: nothing is defined by default except the values in php.ini and
; specified at startup with the -d argument
;php_admin_value[sendmail_path] = /usr/sbin/sendmail -t -i -f www@my.domain.com
;php_flag[display_errors] = off
;php_admin_value[error_log] = /var/log/fpm-php.www.log
;php_admin_flag[log_errors] = on
;php_admin_value[memory_limit] = 32M

and it happens during the child init (search for fpm_php_init_child in fpm_php.c where you can see the logic - fpmi_php_apply_defines is the main function for that) which happens when the new child process is created.

GDB should help you with the rest. The redefinition is after fork so it's useful to set follow-fork-mode to child and detach-on-fork to off ;)

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2017

@bukka Thanks, that helped. I had forgotten to set the have_called_openlog flag from inside fpm_stdio_open_error_log() but that's fixed now.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2017

@nikic Can you please assign a reviewer?

@@ -37,6 +38,8 @@
#define syslog std_syslog
#endif

zend_bool have_called_openlog = 0;

This comment has been minimized.

Copy link
@weltling

weltling Aug 31, 2017

Contributor

This is not thread safe, thus it should go into globals.

This comment has been minimized.

Copy link
@pprindeville

pprindeville Aug 31, 2017

Author Contributor

This is not thread safe, thus it should go into globals.

Not sure I understand. What's an example of a global variable that I can use as an example? Are you talking about SG() and sapi_globals or something else?

This comment has been minimized.

Copy link
@weltling

weltling Aug 31, 2017

Contributor

Yeah, something in this direction. Perhaps core globals were more suitable, as it's not something SAPI specific. Another option in PHP 7 is using ZEND_TLS, but should check whether its usage were suitable in this case.

This comment has been minimized.

Copy link
@pprindeville

pprindeville Aug 31, 2017

Author Contributor

Isn't core globals for configuration state?

This comment has been minimized.

Copy link
@pprindeville

pprindeville Aug 31, 2017

Author Contributor

This is not thread safe, thus it should go into globals.

@weltling Done

@weltling

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2017

@pprindeville new features for feature frozen branches are handled on case by case base. As @bukka works on a patch for this for quite some time already, it probably would make sense you to coordinate with him. IMO the remote logging solution he suggests is promising for both functionality and maintenance.

Thanks.

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch 3 times, most recently from 0cf970b to a34f3c1 Aug 31, 2017

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

Not sure what to do about the Travis failure. Looks like the build is taking too long and that master has some unrelated test case failures going on.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

Is anything else needed at this point?

@@ -127,6 +127,8 @@ struct _zend_compiler_globals {
zval **static_members_table;
int last_static_member;
#endif

zend_bool have_called_openlog;

This comment has been minimized.

Copy link
@laruence

laruence Sep 2, 2017

Member

I don't understand why this need to in CG? PG maybe better? thanks

This comment has been minimized.

Copy link
@pprindeville

pprindeville Sep 2, 2017

Author Contributor

Well, I asked and no one answered...

Isn't core_globals for configurable state?

This comment has been minimized.

Copy link
@laruence

laruence Sep 2, 2017

Member

CG is for compiler globals, PG is more suitable in this case IMO.

This comment has been minimized.

Copy link
@bukka

bukka Dec 18, 2017

Member

@pprindeville This really should be in PG...

This comment has been minimized.

Copy link
@pprindeville

pprindeville Dec 18, 2017

Author Contributor

@pprindeville This really should be in PG...

Where/how do you initialize a core_globals variable that's not user settable? What's the appropriate modifiable value?

I think I've got the correct STD_PHP_INI_BOOLEAN(...) for have_called_openlog but would appreciate someone else giving it a quick look. Thanks

@KalleZ

This comment has been minimized.

Copy link
Member

commented Sep 2, 2017

Hi

CG is compiler globals
EG is engine and executor globals
PG is PHP globals like configuration and inis

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2017

@KalleZ: okay, so where should I put have_called_openlog given that it's not a configuration option? Are we good with CG()?

@KalleZ

This comment has been minimized.

Copy link
Member

commented Sep 3, 2017

It's a PHP specific thing so leave it in PG, Note that only engine and core language is Zend, whereas PHP is sapis, standard libraries and extensions

@krakjoe krakjoe added the Enhancement label Sep 6, 2017

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2017

Okay. No further rework required then? When might it merge?

Thanks

main/main.c Outdated
@@ -636,6 +773,9 @@ PHP_INI_BEGIN()
#ifdef PHP_WIN32
STD_PHP_INI_BOOLEAN("windows.show_crt_warning", "0", PHP_INI_ALL, OnUpdateBool, windows_show_crt_warning, php_core_globals, core_globals)
#endif
STD_PHP_INI_ENTRY("syslog.facility", "LOG_USER", PHP_INI_SYSTEM, OnSetFacility, syslog_facility, php_core_globals, core_globals)
STD_PHP_INI_ENTRY("syslog.ident", "php", PHP_INI_SYSTEM, OnUpdateString, syslog_ident, php_core_globals, core_globals)
STD_PHP_INI_BOOLEAN("have_called_openlog", "0", PHP_INI_SYSTEM, NULL, have_called_openlog, php_core_globals, core_globals)

This comment has been minimized.

Copy link
@bukka

bukka Jun 20, 2018

Member

I would probably put it to the module startup - maybe somewhere around this line:

But not sure if there is a better place - I don't think there is GINIT for PG but there might be a better place though :)

@@ -97,7 +97,7 @@ void vsyslog(int priority, const char *message, va_list args)

/* default event source */
if (INVALID_HANDLE_VALUE == PW32G(log_source))
openlog("php", LOG_PID, LOG_SYSLOG);

This comment has been minimized.

Copy link
@bukka

bukka Jun 20, 2018

Member

should this be really changed with use of PG. I think this is a win re-implementation of syslog so it should be probably internal (it's kind of what glibc implements and we don't change glibc... :))

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 20, 2018

Author Contributor

Sorry, not following. This is really the essence of the fix: allowing configuring syslog.facility and syslog.ident and having these values be used with openlog().

This comment has been minimized.

Copy link
@bukka

bukka Jun 22, 2018

Member

Yeah but this is basically called by syslog on Windows. It seems quite pointless as you already call openlog explicitly so this won't be even used (as PW32G(log_source) will be set).

In addition I don't really want to touch any of the win32/ files as I don't have way how to test it and not even sure if I have karma to commit in there which complicates a merge of the whole PR.

So please remove this change from this PR. If you really feel it needs to be done which I don't think, please create a PR after this is merged and ping some of the win guys... :)

This comment has been minimized.

Copy link
@KalleZ

KalleZ Jun 22, 2018

Member

@bukka Anyone with basic php-src karma can do changes to win32/, its only Zend/ and TSRM/ that has an extra karma level. Both me and @weltling should be able to help out if there is an issue with any Windows stuff

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 22, 2018

Author Contributor

Yeah but this is basically called by syslog on Windows. It seems quite pointless as you already call openlog explicitly so this won't be even used (as PW32G(log_source) will be set).

Sorry, where have we already called openlog() explicitly in the WIN32 case?

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 22, 2018

Author Contributor

But the WIN32 instance of php_syslog() doesn't check to see if openlog() has been called nor does it call it...

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 22, 2018

Author Contributor

Or are you saying we should call php_openlog() here as well?

This comment has been minimized.

Copy link
@bukka

bukka Jun 22, 2018

Member

Ah ok sorry I completely missed that it has got it's own instance. Not really sure why it has to be different but I guess it's probably ok.

I would still like to check with @weltling or @KalleZ if it's appropriate to use PG in win32 as it seems to me like an independent part. Alternatively it could be moved to the WIN32 instance in php_syslog.c

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 22, 2018

Author Contributor

If we add the same check in the WIN32 version of php_syslog() then we know that log_source will be valid (it gets set in WIN32 openlog()) and we can remove:

	/* default event source */
 	if (INVALID_HANDLE_VALUE == PW32G(log_source))
 		openlog("php", LOG_PID, LOG_SYSLOG);

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 22, 2018

Author Contributor

Okay, pushing new changes.

* not already been done; call it ourselves and pass the
* correct parameters!
*/
if (!PG(have_called_openlog)) {

This comment has been minimized.

Copy link
@bukka

bukka Jun 20, 2018

Member

Ok I see. It makes sense I guess

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch 2 times, most recently from 9d3931e to 59d7164 Jun 20, 2018

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

Any other issues?

@@ -152,6 +152,7 @@ PHP_FUNCTION(openlog)
RETURN_FALSE;
}
openlog(BG(syslog_device), option, facility);
PG(have_called_openlog) = 1;

This comment has been minimized.

Copy link
@bukka

bukka Jun 22, 2018

Member

I would suggest one small improvement. Maybe we could introduce php_openlog with the same args as openlog that would just call openlog and set PG(have_called_openlog) so it's done internally in that function. Then it would be used in here and fpm.

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 22, 2018

Author Contributor

Then... syslog() would need to call php_openlog()... What would that change?

This comment has been minimized.

Copy link
@bukka

bukka Jun 22, 2018

Member

What I'm saying is to just replace:

openlog(BG(syslog_device), option, facility);
PG(have_called_openlog) = 1;

with

php_openlog(BG(syslog_device), option, facility);

and implement php_openlog as

void php_openlog(const char *ident, int option, int facility)
{
    openlog(BG(syslog_device), option, facility);
    PG(have_called_openlog) = 1;
}

Then you also replace it in other place. It's just a wrapper as you use the same pattern in 2 other places - no change in your logic...

@bukka

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

@pprindeville Just commented about two things and then it should be good if all tests are fine (I mean the manual testing that I plan to do). Sorry for the late requests, it's just because I finally found time to look into this and think about it... :)

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch 3 times, most recently from e07c1d8 to fe1163e Jun 22, 2018

@@ -95,10 +95,6 @@ void vsyslog(int priority, const char *message, va_list args)
DWORD evid;
wchar_t *strsw[2];

/* default event source */

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 22, 2018

Author Contributor

There are no calls to vsyslog() (directly), it all flows through php_syslog() so there's no way to reach here without openlog() having been called first (and thereby setting log_source).

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2018

BTW, what's the difference between PW32G() and PG()?

@KalleZ

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

@pprindeville PW32G is "PHP Win32 Globals", so only globals that do stuff specifically for Windows, where as PG is simply "PHP Globals", as in globals that interact within the PHP layer of the PHP stack (like ini settings)

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2018

PW32G is "PHP Win32 Globals", so only globals that do stuff specifically for Windows, where as PG is simply "PHP Globals", as in globals that interact within the PHP layer of the PHP stack (like ini settings)

Got it. Well, hopefully everyone is okay with the latest version?

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2018

Uh...

========DIFF========
3753 + Wrong result for Africa/Addis_Ababa!
3754 - ===DONE===
3755 + 
3756 + Wrong result for Africa/Algiers!
3757 + 
3758 + Wrong result for Africa/Asmara!
3759 + 
3760 + Wrong result for Africa/Bangui!
3761 + 
3762 + Wrong result for Africa/Blantyre!
3763 + 
3764 + Wrong result for Africa/Brazzaville!
3765 + 
3766 + Wrong result for Africa/Bujumbura!
...

Pretty sure this has nothing to do with these changes. Also reasonably sure this was working yesterday.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

If anyone is waiting on something from me to move this forward, please call it out.

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch from fe1163e to 69b46df Jun 29, 2018

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

So... has this fix missed 7.3.0 or am I mistaken?

@nikic

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

@pprindeville Nope, hasn't missed 7.3 (yet...) Projected release schedule can be found here: https://wiki.php.net/todo/php73#timetable

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

Nope, hasn't missed 7.3 (yet...)

Well, it's getting close. If we don't make Alpha 3 (in 3 weeks) it looks like we'll have missed the feature freeze.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

The CI builds are failing due to unrelated errors, btw.

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

@bukka Are there any blocking review comments still? You're showing (currently) as requesting changes.

@@ -577,6 +577,8 @@ html_errors = On
;error_log = php_errors.log
; Log errors to syslog (Event Log on Windows).
;error_log = syslog
;syslog.ident = php

This comment has been minimized.

Copy link
@bukka

bukka Jun 30, 2018

Member

Could you add a comment line (just short description and that it applies only if error_log is syslog) for each new ini and also update php.ini-production with the same setting?

This comment has been minimized.

Copy link
@pprindeville

pprindeville Jun 30, 2018

Author Contributor

Good idea.

Done!

@bukka

This comment has been minimized.

Copy link
Member

commented Jun 30, 2018

I did a little bit of testing and all seems good. Could you just update ini files and I will try to merge it tomorrow or sometimes next week.

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch from 69b46df to a490552 Jun 30, 2018

Add syslog's ident and facility parameters to config
Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>

@pprindeville pprindeville force-pushed the pprindeville:syslog-add-facility-and-ident branch from a490552 to a667e38 Jun 30, 2018

@php-pulls

This comment has been minimized.

Copy link

commented Jul 1, 2018

Comment on behalf of bukka at php.net:

Marge in 2475337

@php-pulls php-pulls closed this Jul 1, 2018

@pprindeville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2018

Awesome, thanks!

@pprindeville pprindeville deleted the pprindeville:syslog-add-facility-and-ident branch Jul 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.