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(stream) nginx directive injections for stream config #4148

Merged
merged 1 commit into from Jan 7, 2019

Conversation

Projects
None yet
3 participants
@bungle
Copy link
Member

bungle commented Jan 3, 2019

Summary

Makes it possible to inject Nginx directives also to the stream configurations with configuration or environment variables:

KONG_NGINX_STREAM_* (see KONG_NGINX_HTTP)
KONG_NGINX_SPROXY_* (see KONG_NGINX_PROXY)

E.g.

KONG_NGINX_SPROXY_PROXY_BIND = 127.0.0.1 transparent

Some Notes:

TLDR; using SPROXY prefix comes from the limitations of original design introduced prior stream routing.

I didn't enjoy using SPROXY but I needed some unique prefix that did not conflict with already used prefixes. I wanted to use prefix such as STREAM_PROXY or STREAM_SERVER or PROXY_STREAM but those would conflict with already taken STREAM_ and PROXY_ prefixes. It would make code a bit more complicated if we would like to have longest prefix match first semantics.

If I was to rename these prefixes I would probably name them according to Nginx blocks, e.g.:

  • KONG_NGINX_HTTP
  • KONG_NGINX_HTTP_SERVER (or perhaps KONG_NGINX_HTTP_PROXY)
  • KONG_NGINX_HTTP_ADMIN
  • KONG_NGINX_STREAM
  • KONG_NGINX_STREAM_SERVER (or perhaps KONG_NGINX_STREAM_PROXY)

But that would be a breaking change and also require longest prefix match first semantics.

@bungle bungle requested review from kikito , hishamhm and hbagdi Jan 3, 2019

@bungle bungle force-pushed the feat/stream-injections branch 3 times, most recently from b1bfe39 to 089d3c3 Jan 3, 2019

@hishamhm

This comment has been minimized.

Copy link
Member

hishamhm commented Jan 3, 2019

@bungle Agreed with the sentiments. SPROXY is kind of ugly but no better non-breaking option comes to mind right now...

@bungle bungle force-pushed the feat/stream-injections branch from 089d3c3 to f4fc47c Jan 4, 2019

@Kong Kong deleted a comment from sultttan Jan 4, 2019

@kikito

kikito approved these changes Jan 4, 2019

@kikito

This comment has been minimized.

Copy link
Member

kikito commented Jan 4, 2019

Tentatively approved, let's see if we can get Travis to pass...

feat(stream) nginx directive injections for stream config
Makes it possible to inject Nginx directives also to the
stream configurations with configuration or environment
variables:

- KONG_NGINX_STREAM_* (see KONG_NGINX_HTTP)
- KONG_NGINX_SPROXY_* (see KONG_NGINX_PROXY)

E.g.

- KONG_NGINX_SPROXY_PROXY_BIND = 127.0.0.1 transparent

@bungle bungle force-pushed the feat/stream-injections branch from c0550c3 to 4089de1 Jan 7, 2019

@bungle bungle merged commit c854961 into next Jan 7, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.

@bungle bungle deleted the feat/stream-injections branch Jan 7, 2019

@hishamhm hishamhm modified the milestone: 1.1.0 Jan 10, 2019

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