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) add support for 'transparent' in listeners #3884

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

james-callahan
Copy link
Contributor

@Tieske
Copy link
Member

Tieske commented Oct 22, 2018

Shouldn't this include the output in the Nginx template (and related tests)?

@james-callahan
Copy link
Contributor Author

Shouldn't this include the output in the Nginx template (and related tests)?

Tests would fail if the patch is not applied; @thibaultcha mentioned he didn't want them as a requirement if possible.

@Tieske
Copy link
Member

Tieske commented Oct 23, 2018

hmmm, they would fail if you'd try to use the listeners with that option? but just implementing it in the temnplates and testing whether the template gets the proper flags passed shouldn't be a problem right?

@james-callahan james-callahan force-pushed the feat/listen_transparent branch from bea005f to 4222636 Compare October 24, 2018 00:28
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

When the patch is not applied to nginx, this option should be disabled, and an error printed to the user that a patch is missing. As of today, we will attempt to start nginx and the configuration loader will fail to parse our nginx.conf, resulting in an obscure error for the user.

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Oct 24, 2018
@bungle
Copy link
Member

bungle commented Oct 24, 2018

One thing about this patch is that it needs usually root privileges to start a Kong. And that is a pretty huge change from the previous. There is also an option to make it work even without root when this is applied (of course by root user):

setcap cap_net_admin=eip /usr/local/openresty/nginx/sbin/nginx

This works at least on Linux and we may need to consider adding it to our distributions when this transparent feature gets applied. Or at least documented. So I leave this as a note here too.

@james-callahan
Copy link
Contributor Author

When the patch is not applied to nginx, this option should be disabled, and an error printed to the user that a patch is missing.

How can we feature-detect this missing patch?

@james-callahan james-callahan force-pushed the feat/listen_transparent branch from 4222636 to 01234b5 Compare October 31, 2018 00:57
@thibaultcha
Copy link
Member

I was envisioning the patch introducing a new dummy function on the C side, and a protected call to load it via the FFI in our Lua. If the loading fails, we now the patch wasn't applied. Since our CLI runs with the resty interpreter, this would work for our configuration loading phase.
It is a bit hacky, but a price I'd be willing to pay to support vanilla OpenResty with good error messages for the neophytes, in case the patch is not applied (instead of the cryptic nginx configuration parsing error message saying transparent is an unrecognized option.

Anyway, due to our timing constraints, we can merge this as-is and consider the parsing error good enough, so I will be dismissing my review.

@thibaultcha thibaultcha removed the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Oct 31, 2018
@thibaultcha thibaultcha merged commit 2f8264a into Kong:next Oct 31, 2018
@james-callahan james-callahan deleted the feat/listen_transparent branch October 31, 2018 01:16
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.

4 participants