Consider allowing ownership of container log files by config

[lachlanmunro]

On debian for example, it is common for log files to have the group adm. This allows us to run our logging agents as non-root.

I have tried using something like:

chgrp -R adm /var/log/pods
chmod -R g+s /var/log/pods

But since the logs are stored within the structure /var/log/pods/<idhere>/<log> this approach only works for containers running at that time. I could cron/something else but such an approach feels pretty hacky.

I did a little source-diving and hit https://github.com/containerd/cri-containerd/blob/master/pkg/server/container_start.go#L143 where container logs are created/reopened. Would you consider allowing some way to add groups or set users to influence this? I can have a fiddle if that suits?

[Random-Liu]

The directory /var/log/pods/<idhere>/ is created by kubelet.
The log file <log> is created by cri-containerd.

Which one do you need to set the permission?

[lachlanmunro]

I can use a sticky bit for the /var/log/pods/ folder to set the right groups on the <idhere> folder. That's fine, I can work around that easily (and given the folder permissions, I don't need to).

drwxr-sr-x 2 root adm 4096 Feb 13 19:28 /var/log/pods/ea50ac61-10f3-11e8-b3ac-0022156cfc6a

Its the log files themselves that require root to be read:

-rw-r-S--- 1 root root 86567 Feb 14 15:59 /var/log/pods/ea50ac61-10f3-11e8-b3ac-0022156cfc6a/speaker_0.log

So it's the <log> file that I would need the group/permissions set on.

[Random-Liu]

We set 0640 because docker is doing so today https://github.com/moby/moby/blob/master/daemon/logger/loggerutils/logfile.go#L40.

@lachlanmunro How do you solve the problem for Docker? Do you have a solution for Docker already?

I'm fine with making this configurable, but I'd like to see whether there is any other solution. :)

[lachlanmunro]

Within docker we "solve" this by running our logging agents (fluentd/logstash etc) as root. This is not really a solution, and would prefer not to do this.

The file permissions are fine. 0640 is perfectly reasonable for a logfile. The problem for our usecase is that the logs are entirely owned by root. Other logs (often from rsyslog: fileGroup/fileGroupNum) allow you to specify a group that log files will be created under.

A solution on Linux would probably be pretty Linux specific. Perhaps: Split out io.NewCRILogger into two build versions. One file that builds on Linux (// +build linux), one that builds on everything else (// +build !linux).

Example Linux only version:

// +build linux

// SetLogGroup tries to set the group ID that logs will be logged as to the provided group name. If it
// fails the group ID will not be changed.
func SetLogGroup(group string) error {
	grp, err := user.LookupGroup(group)
	if err != nil {
		return err
	}
	if grp == nil {
		return fmt.Errorf("group retrieved by name '%s' was nil, cannot retrieve group id", group)
	}
	gid, err := strconv.Atoi(grp.Gid)
	if err != nil {
		return err
	}
	if gid < 0 {
		return fmt.Errorf("group retrieved by name '%s' has a negative group ID, this is an indication of using groups on windows", group)
	}
	logGroupID = gid
	return nil
}

// initialise the group and user ID's to match the current running user
func init() {
	logGroupID = os.Getgid()
	logUserID = os.Geteuid()
}

// the group ID to log as, on posix systems this is an int
var logGroupID int

// the user ID to log as
var logUserID int

// NewCRILogger returns a write closer which redirect container log into
// log file, and decorate the log line into CRI defined format.
func NewCRILogger(path string, stream StreamType) (io.WriteCloser, error) {
	logrus.Debugf("Start writing log file %q", path)
	prc, pwc := io.Pipe()
	f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0640)
	if err != nil {
		return nil, fmt.Errorf("failed to open log file: %v", err)
	}
	err = f.Chown(logUserID, logGroupID)
	if err != nil {
		logrus.WithError(err).Errorf("Could not correctly change file ownership of %q", path)
	}
	go redirectLogs(path, prc, f, stream)
	return pwc, nil
}

[Random-Liu]

A solution on Linux would probably be pretty Linux specific.

It is fine to only have linux implementation for now. We are going to support other OS, especially Windows later.

However, we may want to make the flag a string, so that we can extend it for Windows later. Probably uid:gid which follows linux convention.

Both rsyslog and logrotate support this. I can see why people need this, and the change should be small and non-disruptive (we can always keep default behavior as not setting this). It makes sense to me to support this.

@containerd/containerd-maintainers WDYT?

[Deaddy]

How is the progress on this problem? Docker people have showed animosity to this option, as it is not the docker way (tm). I am not sufficiently familiar with Windows' permissions to comment on a feasibility, but I figure that there this is less of an issue than on linux and unix, where everything should be a file and hence access of the logs as files readable by non-root-users would be the preferable option.