fix(core): ensure absolute path is used for event sock

[flrgh]

Summary

This fixes an issue with how Kong initializes resty.events. The code was
previously using ngx.config.prefix() to determine the listening socket
path to provide to the resty.events module. This causes breakage when
NGINX is started with a relative path prefix:

failed to instantiate 'kong.worker_events' module: failed to disable listening: unix:my-prefix/worker_events.sock

Instead of using ngx.config.prefix(), Kong will now prefer
kong.configuration.prefix when available, as it is already normalized
to an absolute path. If kong.configuration.prefix is not defined, the
result of ngx.config.prefix() will be used after resolving it to an
absolute path.

Also contains an unrelated fix in kong/cmd/start.lua.

Full changelog

• prefer kong.configuration.prefix to ngx.config.prefix()
• convert ngx.config.prefix() to an absolute path before using it
• fix pcall usage in kong.cmd.start

[Tieske]

@flrgh There is a related issue; Pongo is having permission issues with this unix socket. The problem is that in Pongo, the prefix is located on a mount (where the plugin code is located). Creating the unix socket on that mount causes permission issues.

Error: nginx configuration is invalid (exit code 1):
nginx: the configuration file /kong-plugin/servroot/nginx.conf syntax is ok
nginx: [emerg] bind() to unix:/kong-plugin/servroot/worker_events.sock failed (1: Operation not permitted)
nginx: configuration file /kong-plugin/servroot/nginx.conf test failed

We had the same issue with Vagrant a long time ago (because test nginx uses unix sockets as well), it was fixed like this: https://github.com/Kong/kong-vagrant/blob/master/provision.sh#L331-L332

So besides making it an absolute path, it should also become configurable by an env var.

EDIT: added the error message

[chronolaw]

I have added the env KONG_RESTY_EVENTS_SOCK_PATH and did a simple test, it works well, but I don't know how to write a unit test.

Also, we should consider PR #9254, which needs some change to fit this PR.

Update: I have added the code for PR #9254, now we can remove socket file properly.

[kikito]

Once this is approved, send cherry-pick against release/3.0.1, please.

[flrgh]

👋 hi folks, I want to back things up a bit here, because this PR has seen some scope creep while I was out on vacation.

The original bugfix commit (4158cee) that prompted this PR was intended to address a very simple bug--we were using a relative path in a place where we ought to be using an absolute path, causing a fatal error.

It has since been pushed beyond the fix scope and is now in feat territory.

The bugfix is not mutually-exclusive with the notion of a new config field (even if the latter eventually supplants the work of the former). Additionally, I see no reason that these two things (the fix and the new config field) need to happen in the same PR. Therefore, unless anyone has a compelling case to make otherwise, I'd like to return this branch/PR to it's prior state+scope, because I don't want the bugfix to be held back while the merits and implementation details of the feature are still being sorted out. The socket path change for Pongo can then be carried out in another PR.

[flrgh]

My thoughts re: Pongo and unix sockets

The worker events socket is not the only unix socket that Kong places in the KONG_PREFIX directory--it just so happens to be the only unix socket that is always on/enabled by default. The various other unix sockets (i.e. stream_rpc.sock, stream_config.sock, etc) would all cause the same problem, and they're not new. What I mean is, if we need to make socket paths configurable, we should do so in a way that works for all unix sockets that Kong manages--not just the worker events socket.

[flrgh]

I restored the branch to the original 2 bugfix items (with a couple style fixes) and rebased onto master. This is ready for another round of review.

PR was rebased to an earlier state

[dndx]

I am still not convinced why the socket path needs to be configurable in the first place. If we need to fix the relative path issue, we should do just that and nothing more. Please share your reasoning in the ticket before merging anything.

[chronolaw]

I am still not convinced why the socket path needs to be configurable in the first place. If we need to fix the relative path issue, we should do just that and nothing more. Please share your reasoning in the ticket before merging anything.

You are right.
Now, this PR is rebased on the earlier state, so it only contains the solution of the relative path.

Datong changed this PR in order to contain the relative path solution #9337 (comment)