WIP: Config file

As discussed in https://gitlab.com/gitlab-org/gitlab-workhorse/issues/85#note_21435732

First point on #95

added 2 commits

• a1451ed1 - vendor BurntSushi/toml
• dddfe203 - This is gonna break spectacularly

Compare with previous version

added 1 commit

• 8f3aab61 - This is gonna break spectacularly

Compare with previous version

added 1 commit

• df21ab4c - Don't check config-flags when we have a file.

Compare with previous version

'Disabled' -> 'Disables'

I don't think this is the behaviour we want. The first usage will be to load redis details from config file while loading all other details from flags.

That part is no longer valid since jacob pointed out flag.Valid :)

Looks nice @bkc.

@nick.thomas looking at the config struct, what stands out for you as something you'd want to change?

Not sure if this is the best word. Workhorse will have two backends (gitlab-rails, gitaly) and in the future maybe even more. Maybe GitlabRails?

You can do URLs like this:

package main

import (
	"fmt"
	"net/url"

	"github.com/BurntSushi/toml"
)

type parsedURL struct {
	*url.URL

}

func (u *parsedURL) UnmarshalText(data []byte) (err error) {
	u.URL, err = url.Parse(string(data))
	return
}

type X struct {
	URL *parsedURL
}

var data = `URL = "http://example.com"`

func main() {
	var x X

	mapped, err := toml.Decode(data, &x)
	fmt.Println(mapped, err, x)
}

You can do something similar for duration, and it is in fact an example here: https://github.com/BurntSushi/toml

(Look for encoding.TextUnmarshaler)

@nick.thomas I looked at that (for time.Duration etc) but is gets convoluted in the end having to use the new struct everywhere instead of time.Duration (or url.URL in this case...) That said, I'd love to use TextUnmarshaler if it worked

@nick.thomas what do you think the config option that is now authBackend should be called?

You'd just pass/use config.URL.URL (or config.ProxyHeadersTimeout.Duration) rather than the enclosing struct.

Another option is to have a struct that represents the config file and a transfer step to a separate config struct. I don't like this thing where we have separate members and explicit parse steps for each member that needs unmarshalling that's not in the stdlib.

As for authBackend, i'd probably do something a bit like:

[upstream]
backend = "..."

Maybe not upstream, but something.

@nick.thomas that works, just looks ugly, but it works

@nick.thomas I'd rather do that in reverse

[backend]
auth = "..."
gitaly = "..."

I don't know if anybody uses this. Maybe we should just drop it and use umask 0.

I'd expect it to obey the umask the process was started with?

I'd expect it to run at 0600 by default

I believe (we should check) that rwx is needed to access a Unix socket. Bear in mind that this Unix socket is opened by NGINX which runs as a different user.

Having a unix socket with mode 0777, in a world-readable directory, is the equivalent of opening a listening socket on localhost. For this particular listening socket that seems appropriate.

Heh, it seems the semantics vary by OS. Manpage for linux says:

In the Linux implementation, sockets which are visible in the filesystem honor the permissions of the directory they are in. Their owner, group and their permissions can be changed. Creation of a new socket will fail if the process does not have write and search (execute) permission on the directory the socket is created in. Connecting to the socket object requires read/write permission. This behavior differs from many BSD-derived systems which ignore permissions for UNIX domain sockets. Portable programs should not rely on this feature for security.

Some examples from elsewhere:

• /var/run/docker.sock: root:docker 660
• /var/run/postgresql/.s.PGSQL.5432: postgres:postgres 777

So I guess it doesn't matter much.

I think it does. Default umask is 022 on most systems, so you would create a listening socket with mode 0644. This is not sufficient for NGINX to connect to workhorse: NGINX would need write permission on the socket 'file'.

The reason /var/run/docker.sock has a trailing 0 is for security reasons: connecting to this socket gives you the equivalent of root access. That last 0 stops Docker from being a way for any process on the system that can see /var/run/docker.sock to root the system. :)

The way I remember it, the Linux-specific thing is that it allows you to put the socket in an insecure directory and then restrict access with 'file' permissions alone, while on other Unixes you should not be putting the socket in an insecure directory to begin with.

We have to be mindful of the opposite problem however. The workhorse socket is is supposed to be 'insecure' (exposed to the entire system, or at least the gitlab-www user) and we must be careful that the gitlab-www user is not blocked from using it. Hence, change the process umask to 0 before opening the socket.

Sorry, what I meant was that it doesn't matter much if the socket is world-writable for workhorse.

so the umask for docker is 027 then?, and for workhorse it should be 117 (leading to 0660 permission)?

@bkc I think you mentioned this the other day too. We might be able to collapse Socket and ListenAddress.

nope, Socket is Backend, but those can be collapsed in a nicer way

flag.String("authSocket", "", "Optional: Unix domain socket to dial authBackend at")
[...]
(func NewRoundTripper)
if backend != nil && socket == "" {
	address := mustParseAddress(backend.Host, backend.Scheme)
	tr.Dial = func(_, _ string) (net.Conn, error) {
		return DefaultDialer.Dial("tcp", address)
	}
} else if socket != "" {
	tr.Dial = func(_, _ string) (net.Conn, error) {
		return DefaultDialer.Dial("unix", socket)
	}
}

Just a heads up on that though...

Note that the Path field is stored in decoded form: /%47%6f%2f becomes /Go/. A consequence is that it is impossible to tell which slashes in the Path were slashes in the raw URL and which were %2f. This distinction is rarely important, but when it is, code must not use Path directly.

ref: https://golang.org/pkg/net/url/#URL

For listenAddress & friends we could do something like this https://play.golang.org/p/_O1M8g62ks

This can all go if we use encoding.TextUnmarshaler

We can't (easily), we'd have to replace all uses of time.Duration => config.Duration which looks nasty
Ref: https://play.golang.org/p/TM9Vt1JWKT

Can we start with sections straightaway?

IMO yes

Should we start using *Config instead of Config? Right now, it's not obvious whether any given modification of the struct will have side-effects

mentioned in issue #85 (closed)

It really feels like we're going too far, too early here, and it's slowing us down. Do we need any more than this:

type RedisConfig struct {
    URL string
    Username string
    Password string
}

type ConfigFile struct {
    Redis *RedisConfig `toml:"redis"`
}

?

This can go alongside the existing Config struct, we can unmarshal into it and punt all these decisions we're trying to make into next week.

@nick.thomas I'm fine with that

@nick.thomas I'd prefer if we kept it in the current Config-struct though, and just do this:

type Config struct {
  Redis *RedisCondig `toml:"redis"`
  Backend *url.Url `toml:"-"`
}

Then we can just gradually do the others later w/o changing anything

added 2 commits

• cb7b5fb4 - overload flags on config-file
• cc76cd30 - Pretty things are pretty

Compare with previous version

Depricated in favour of not-doing-everything-at-once !112 (merged)

closed