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.

Sign up for GitHub

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

Jump to bottom

[kong] add support for stream listens #103

Merged
merged 5 commits into from
Apr 8, 2020
Merged

[kong] add support for stream listens #103

merged 5 commits into from
Apr 8, 2020

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Apr 3, 2020

What this PR does / why we need it:

Adds support for stream listens as an additional listen type on the proxy.

Special notes for your reviewer:

I'm still running into issues with the balancer when attempting to configure these:

2020/04/03 20:45:38 [error] 37#0: *37954 lua entry thread aborted: runtime error: /usr/local/share/lua/5.1/kong/runloop/balancer.lua:569: bad argument #1 to 'ipairs' (table expected, got nil)
stack traceback:
coroutine 0:
	[C]: in function 'ipairs'
	/usr/local/share/lua/5.1/kong/runloop/balancer.lua:569: in function 'get_all_upstreams'
	/usr/local/share/lua/5.1/kong/runloop/balancer.lua:612: in function 'get_upstream_by_name'
	/usr/local/share/lua/5.1/kong/runloop/balancer.lua:638: in function 'get_balancer'
	/usr/local/share/lua/5.1/kong/runloop/balancer.lua:919: in function 'execute'
	/usr/local/share/lua/5.1/kong/runloop/handler.lua:930: in function 'balancer_execute'
	/usr/local/share/lua/5.1/kong/runloop/handler.lua:1097: in function 'after'
	/usr/local/share/lua/5.1/kong/init.lua:622: in function 'preread'
	preread_by_lua(nginx-kong-stream.conf:66):2: in main chunk while prereading client data, client: 10.138.15.225, server: 0.0.0.0:9001

I'd run into them previously with TCPIngress testing and got them again when configuring manual test config for this. That definitely warrants investigation, but I don't think it's anything to do with the PR: the PR code is only setting up the listens and making them accessible, and that does work.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the next branch and targets next, not master
  • New or modified sections of values.yaml are documented in the README.md
  • Title of the PR and commit headers start with chart name (e.g. [kong])

@rainest
[kong] add support for stream listens
49590c5 
* Add values.yaml configuration for stream listens under "proxy".
* Add new "kong.singleListen" helper for generating individual listen
  configurations. Refactor "kong.listen" to use "kong.singleListen".
* Add new "kong.streamListen" helper to generate stream listen
  directives.
* Update deployment and proxy service templates to include stream listen
  ports.
hbagdi
hbagdi suggested changes Apr 6, 2020
View reviewed changes
charts/kong/values.yaml Outdated Show resolved Hide resolved
charts/kong/values.yaml Show resolved Hide resolved
charts/kong/values.yaml Outdated Show resolved Hide resolved
charts/kong/README.md Outdated Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Show resolved Hide resolved
@rainest
[kong] improve stream listens
5111362 
* Correct and reorganize documentation for stream listens.
* Remove duplicate parameters from (all) listens.
* Generate stream listen names automatically.
@rainest rainest requested a review from hbagdi April 6, 2020 23:57
@hbagdi
Copy link
Member

hbagdi commented Apr 7, 2020

Please update an existing test to include a stream listen in DB and DB-less mode. Otherwise, looks good.

@rainest
[kong] add stream listens to CI
caf2847 
Add stream listen configuration to the existing CI tests for DB-backed
and DB-less deployments without an ingress controller.
hbagdi
hbagdi approved these changes Apr 7, 2020
View reviewed changes
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

One comment, please correct that and then merge.
Please squash and merge.

charts/kong/ci/test3-values.yaml Outdated
@@ -1,4 +1,4 @@
# CI test for testing dbless deployment without ingress controllers
# CI test for testing dbless deployment without ingress controllers and stream listens
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this diff before merging in.

@rainest rainest changed the base branch from master to next April 8, 2020 18:33
@rainest rainest merged commit acf1c27 into next Apr 8, 2020
@hbagdi hbagdi deleted the feat/stream-listen branch April 10, 2020 20:52
rainest added a commit that referenced this pull request Apr 10, 2020
@rainest
[kong] add support for stream listens (#103)
cb91651 
Add support for stream listens as additional proxy ports:
* Add values.yaml configuration for stream listens under "proxy".
* Add new "kong.singleListen" helper for generating individual listen
  configurations. Refactor "kong.listen" to use "kong.singleListen".
* Add new "kong.streamListen" helper to generate stream listen
  directives.
* De-duplicate listen parameters.
* Update deployment and proxy service templates to include stream listen
  ports.
* Add stream listens to CI values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbagdi hbagdi hbagdi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rainest @hbagdi