logfile created with root:root ownership when "log_file" config option specified

[Karol Mroz <kmroz@xxxxxxxx>]

Greetings Ceph Developers,

I recently discovered that when explicitly setting "log_file" for radosgw in
ceph.conf, the logfile was created with root:root ownership. Initially I thought
that this was a RGW issue, which prompted me to open: http://tracker.ceph.com/issues/14613

Tracing this a bit further, I believe it to be a more general problem with when the
LogObs observer is invoked.

The LogObs observer is first invoked from global_pre_init() by way of:

global_pre_init():
...
  conf->apply_changes(NULL)
...

Which then invokes:

md_config_t::_apply_changes()
{
...
  // Make any pending observer callbacks
  for (rev_obs_map_t::const_iterator r = robs.begin(); r != robs.end(); ++r) {
    md_config_obs_t *obs = r->first;
    obs->handle_conf_change(this, r->second);
  }
...
}

Which in turn fires the LogObs observer code:

class LogObs : public md_config_obs_t {
...
void handle_conf_change()
...
  if (changed.count("log_file")) {
    log->set_log_file(conf->log_file);
    log->reopen_log_file();
  }
...

When "log_file" is specified in ceph.conf, Log::reopen_log_file() is called and
the logfile is open()'d. However, at this point in time, the daemon is still running
as root, and thus the logfile is created with root:root ownership.

A quick workaround is to manually not set "log_file" in ceph.conf. The default
logfile is then created later on in global_init(), by way of:

global_init()
...
  g_conf->call_all_observers();
...

This is called _after_ the correct permissions have been set.

From a quick glance, removing changed.count("log_file"...) from
LogObs::handle_conf_changes() would skip over the early logfile creation in this observer.
A more invasive option would be to remove conf->apply_changes() from global_pre_init(),
thus delaying running the observers for the first time until after the permissions drop.
However, I suspect there may be valid reason why conf->apply_changes() is needed in
global_pre_init()? As I'm not familiar with the nuances here, a comment from someone more
familiar with this would be appreciated.

-- 
Regards,
Karol

Attachment: signature.asc
Description: Digital signature