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): Support nginx user conf #2180

Merged
merged 1 commit into from
Mar 29, 2017
Merged

Conversation

depay
Copy link
Contributor

@depay depay commented Mar 9, 2017

Summary

Support user config for nginx, especially when using Kong with docker where the user may be someone weird.

Full changelog

  • Add1 parameters nginx_user(string) to configure user for nginx.

Signed-off-by: Wei.ZHAO [email protected]

@depay depay force-pushed the support_nginx_user branch from d9a66eb to ee2a839 Compare March 9, 2017 08:01
@p0pr0ck5
Copy link
Contributor

Rather than the extra boolean, why not just set the default value as nobody (or some other appropriate entry)?

@depay
Copy link
Contributor Author

depay commented Mar 22, 2017

@p0pr0ck5 , user may specify some other user during compiling, and he wanna treat it as default user other tan nobody in that case.

@Tieske
Copy link
Member

Tieske commented Mar 22, 2017

I'd say that if a user compiles using a different user, then it's up to them to set it properly in the config file as well. This really seems like an edge case to me.

I'd go for the single property with default nobody as proposed by @p0pr0ck5

Maybe @thibaultcha has some thought s on this?

@depay
Copy link
Contributor Author

depay commented Mar 22, 2017

it's duplicate both comping and configuring the same user. I really wanna set only one parameter nginx_user with relationship: nginx_user=nil -> no such line while nginx_user=xxx -> user xxx. But finally i found it hard for me a lua newbie.

@Tieske
Copy link
Member

Tieske commented Mar 24, 2017

We'd be bothering everyone with 2 config properties, for a very, very, few people compiling with a different user. Have 1 property for everyone, and 2 actions for only a very few makes more sense to me.

Besides that it clutters the config file.

@depay depay force-pushed the support_nginx_user branch from ee2a839 to 5c958a4 Compare March 27, 2017 01:31
@depay
Copy link
Contributor Author

depay commented Mar 27, 2017

@Tieske @p0pr0ck5 ok, i've made an update to only 1 param.

@p0pr0ck5
Copy link
Contributor

Should we also update spec/fixtures/custom_nginx.template, to keep these files in sync as much as possible?

@@ -68,6 +68,8 @@
# HTTPS requests to the admin API, if
# `admin_ssl` is enabled.

#nginx_user = nobody # Specify nginx user.
Copy link
Member

Choose a reason for hiding this comment

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

We should elaborate on this property's description. Taking inspiration from the Nginx documentation about this directive, we could go with:

Defines user and group credentials used by worker processes. 
If group is omitted, a group whose name equals that of user is used.
Ex: [user] [group].

@depay depay force-pushed the support_nginx_user branch from 5c958a4 to fbf8f0e Compare March 29, 2017 01:46
@depay
Copy link
Contributor Author

depay commented Mar 29, 2017

@p0pr0ck5 @thibaultcha Spec codes added and description updated.

@p0pr0ck5
Copy link
Contributor

lgtm, but deferring to a more senior project member.

@@ -23,6 +23,7 @@ anonymous_reports = off

lua_package_path = ?/init.lua;./kong/?.lua

nginx_user = root
Copy link
Member

Choose a reason for hiding this comment

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

This will make the test instances run as root, which is a no-no imo. We can avoid that change and not care about the "override" test, its goal is not to test that each and every property being overridden correctly.

Support user config for nginx, especially when using Kong with docker
where the user may be someone weird.

Signed-off-by: Wei.ZHAO <[email protected]>
@depay depay force-pushed the support_nginx_user branch from fbf8f0e to cfc15e3 Compare March 29, 2017 05:15
@depay
Copy link
Contributor Author

depay commented Mar 29, 2017

fixed: using default nobody instead of root in test

@thibaultcha thibaultcha merged commit f3572ac into Kong:next Mar 29, 2017
@thibaultcha
Copy link
Member

@depay Thank you!

@depay depay deleted the support_nginx_user branch March 30, 2017 01:16
@@ -126,6 +126,7 @@ describe("NGINX conf compiler", function()
describe("compile_nginx_conf()", function()
it("compiles a main NGINX conf", function()
local nginx_conf = prefix_handler.compile_nginx_conf(helpers.test_conf)
assert.matches("user nobody;", nginx_conf, nil, true))
Copy link
Member

Choose a reason for hiding this comment

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

Too many ) at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops..my fat finger..let me fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in #2298

@thibaultcha
Copy link
Member

My bad, I didn't have my glasses on.

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

Successfully merging this pull request may close these issues.

5 participants