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

feat(conf) allow user-specified OpenResty install path #8412

Merged
merged 5 commits into from
Apr 7, 2022

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented Feb 14, 2022

Summary

I maintain several different installations of OpenResty (both vanilla and Kong-flavored) on my machine for dev/testing of different projects.

The first time I went to run the Kong test suite locally, I spent a half hour or so scratching my head and cursing the sky over this error:

nginx: [emerg] unknown directive "lua_kong_load_var_index" in /home/flrgh/git/kong/kong/servroot/nginx.conf:69
nginx: configuration file /home/flrgh/git/kong/kong/servroot/nginx.conf test failed

But I prepended the correct NGINX bin dir to my PATH already--why doesn't it work?!

As I would soon discover, Kong searches some common OpenResty system/default paths before consulting PATH, making it impossible to use a non-standard OpenResty installation for running the Kong cli, busted, etc. if system OpenResty is already installed.

The commit message has some more details, but in short, with this change, operators can use a custom OpenResty install with Kong by setting openresty_prefix openresty_path.

Why not just update things so that we search PATH first?

This would be a nice improvement, and in my opinion would nicely correct things as to not violate the principle of least surprise. However, that would be a breaking change with potential far-reaching, headache-inducing impact as operators, package maintainers, and CI pipeline owners now suffer through the same debug session that I went through.

Why openresty_prefix openresty_path and not nginx_bin?

Kong is tightly-coupled to OpenResty than vanilla NGINX, so it "feels" more cohesive this way, to me at least. Also this leaves the door open to expand our usage of this variable. For instance, I think it would be nice if it had a cascading effect on the default values of lua package.path/package.cpath.

Why use openresty_prefix openresty_path exclusively instead of inserting it at the head of the search list?

Because the operator told us explicitly which path to use, and if it doesn't exist, we should fail fast rather than potentially falling back to another version of OpenResty that happens to exist on the same filesystem.


For discussion:

Is it okay for this to slide in as an undocumented, not-exactly-public-but-not-completely-private/internal feature, or do we need to make it a fully-documented first-class citizen of the conf_loader? IMO the latter would be nice, but I stopped short of that because I think it would need to be more fleshed-out first--potentially incorporating some of the other behaviors I mentioned (re: lua package.path). I'm adding it to the Kong config.

@flrgh flrgh requested a review from a team February 14, 2022 21:18
Copy link
Contributor

@locao locao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was indeed missing.
IMO this should be fully-documented.

@locao locao added the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Feb 15, 2022
@locao locao added this to the 3.0 milestone Feb 15, 2022
@flrgh flrgh changed the title feat(cli) allow user-specified OpenResty prefix feat(conf) allow user-specified OpenResty prefix Feb 17, 2022
@kikito
Copy link
Member

kikito commented Feb 24, 2022

note: no tests on this pR

flrgh added 3 commits March 21, 2022 10:13
Kong detects and decides which nginx binary to use by searching a couple
hard-coded paths:

* /usr/local/openresty/nginx/sbin
* /opt/openresty/nginx/sbin

...before finally exec-ing out to check PATH for a found nginx
executable. Therefore, if OpenResty is installed at one of the
hard-coded paths, it is always used, leaving the operator no means of
specifying any alternative. In most workflows, this is not a problem at
all, but during local development and testing it can be limiting.

Changing the behavior to search PATH first is one possible way of
addressing this, but it is backwards-incompatible in ways that will
likely cause headaches for operators, developers, package maintainers,
and CI pipeline owners. Therefore, this development
quality-of-life-inspired change introduces a new environment variable
(KONG_OPENRESTY_PREFIX) that short-circuits the aforementioned behavior.
When this variable is set, Kong uses it exclusively for locating the
nginx binary.

Using KONG_OPENRESTY_PREFIX instead of something like KONG_NGINX_BIN
allows us to expand its usage later on (i.e. potentially having a
cascading effect on LUA_PATH/LUA_CPATH).
@flrgh flrgh force-pushed the feat/cli-custom-resty-prefix branch from c713bfa to ae7eb69 Compare March 21, 2022 18:04
@flrgh
Copy link
Contributor Author

flrgh commented Mar 21, 2022

Test coverage has been added.

@flrgh flrgh requested review from gszr and locao March 21, 2022 18:05
@dndx
Copy link
Member

dndx commented Mar 28, 2022

@flrgh This is a nice idea, but I would like to point out that openresty_prefix may not be the best name for it. prefix in OpenResty really means "working directory", that is, where OpenResty and Nginx logs, runtime data and etc should be stored at. It generally does not have much to do with where the nginx binary is located.

Maybe openresty_path makes more sense, as it is indeed a PATH that Kong should be searching for.

@flrgh
Copy link
Contributor Author

flrgh commented Mar 29, 2022

@flrgh This is a nice idea, but I would like to point out that openresty_prefix may not be the best name for it. prefix in OpenResty really means "working directory", that is, where OpenResty and Nginx logs, runtime data and etc should be stored at. It generally does not have much to do with where the nginx binary is located.

Maybe openresty_path makes more sense, as it is indeed a PATH that Kong should be searching for.

This is a fair point, and I'm not married to openresty_prefix by any stretch. I'll rename to openresty_path.

@flrgh flrgh requested a review from dndx March 29, 2022 17:22
@flrgh flrgh changed the title feat(conf) allow user-specified OpenResty prefix feat(conf) allow user-specified OpenResty install path Mar 29, 2022
@flrgh flrgh removed the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Apr 7, 2022
@flrgh flrgh merged commit 7e71548 into master Apr 7, 2022
@flrgh flrgh deleted the feat/cli-custom-resty-prefix branch April 7, 2022 18:02
@gszr
Copy link
Member

gszr commented Apr 7, 2022

yay! nice work there @flrgh!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants