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

[kong] Refactor listener templates #72

Merged
merged 12 commits into from Mar 20, 2020
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Mar 12, 2020

What this PR does / why we need it:

Consolidates listeners templates into a single generic template. This DRYs helper template code, makes the listener/service configuration consistent, and paves the way towards consolidating Service/Ingress templates in the future.

  • Use the same listener/service configuration format in values.yaml for all Kong services. This mainly affects the admin API, which previously had a unique format. That format is still supported, but is deprecated. Users are warned to update it in NOTES.txt output if the old format is detected.
  • Use the same template for generating all KONG_<SERVICE>_LISTEN variables.
  • Add support for additional listener parameters (see previous discussion in kong: support for grpc proxying #47).
  • Combine documentation for various service values.yaml parameters.
  • Add CI values.yaml for legacy admin API configuration.
  • Move generation of the ingress controller --kong-url argument into a new kong.adminLocalURL helper.

Special notes for your reviewer:

PR contains un-squashed commits to show dev history, but should be squashed before merger, as most of the details are irrelevant after review. If merging, please use the commit message from 6451e7f with one additional bullet point:

 * Add a "kong.adminLocalURL" helper to generate the local URL for the
   admin API, for use with the ingress controller.

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])

Unify the listener templates while retaining support for the legacy
admin API listen configuration format.
Unify listener templates and make them more expressive:
* Listener strings are now generated by passing the listener/service
  configuration block (e.g. "proxy") to the "kong.listen" helper, rather
  than templating each listen string individually.
* The admin listen now uses the same configuration format as other
  listeners, with default values expressing the standard configuration
  (no service creation, TLS-only listen). Legacy configuration is still
  supported, but displays a deprecation warning in NOTES.txt.
* Listener configuration now allows specifying listen parameters, rather
  than hard-coding them into the template (though "ssl" is still
  hard-coded for TLS listens). To avoid too complicated a configuration
  change, users are still limited to two listens, HTTP and TLS, rather
  than being able to freely define listens.
* CI tests and README.md now account for the above.
* NOTES.txt spacing now tries to more closely adhere to 80-char columns.
Add a "kong.adminLocalURL" helper to generate the local URL for the
admin API, for use with the ingress controller.
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.

This is looking pretty good. Most of these are smaller comments.

charts/kong/values.yaml Show resolved Hide resolved
charts/kong/values.yaml Show resolved Hide resolved
charts/kong/values.yaml Show resolved Hide resolved
charts/kong/README.md Show resolved Hide resolved

For example, updating to `env.proxy_listen: 0.0.0.0:4444, 0.0.0.0:4443 ssl`
will need `proxy.http.containerPort: 4444` and `proxy.tls.containerPort: 4443`
to be set in order for the service definition to work properly.
Copy link
Member

Choose a reason for hiding this comment

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

Is override not possible at all? From the current read, it seems it should be possible.
There will be use-cases that will demand override, and for those, we should mention this.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What cases are you thinking of in particular? The simplest I can think of (restricting the listen IP) doesn't seem like it'd be of use in containerized deployments.

While it's still possible, I couldn't think of obvious cases for overriding and figured it would be best left unmentioned.

Copy link
Member

Choose a reason for hiding this comment

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

OSS users can get very creative sometimes.
We don't need a dedicated section but let's mention this in the env section itself maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be, but it's easier to support them if they aren't. I don't want to document this even though it does work, since IMO it shouldn't appear that it's a standard way of configuring things.

If there are cases where the standard listen config doesn't work, I'd want those to come in via issues for us to try and see if there's a way we can support it without overrides.

charts/kong/templates/_helpers.tpl Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Outdated Show resolved Hide resolved
@rainest rainest merged commit cc63bf2 into next Mar 20, 2020
@hbagdi hbagdi deleted the refactor/listener-templates branch March 20, 2020 20:35
rainest added a commit that referenced this pull request Mar 1, 2021
The admin API configuration could only set up a single port, either HTTP
or HTTPS. #72 added support for
dual-stack admin API configuraton using the same templates as other Kong
services.

Fix #287
rainest added a commit that referenced this pull request Mar 1, 2021
The admin API configuration could only set up a single port, either HTTP
or HTTPS. #72 added support for
dual-stack admin API configuraton using the same templates as other Kong
services.

Fix #287
rainest added a commit that referenced this pull request Mar 3, 2021
The admin API configuration could only set up a single port, either HTTP
or HTTPS. #72 added support for
dual-stack admin API configuraton using the same templates as other Kong
services.

Fix #287
rainest added a commit that referenced this pull request Mar 4, 2021
The admin API configuration could only set up a single port, either HTTP
or HTTPS. #72 added support for
dual-stack admin API configuraton using the same templates as other Kong
services.

Fix #287
ubergesundheit pushed a commit to giantswarm/kong-app that referenced this pull request Jun 14, 2021
The admin API configuration could only set up a single port, either HTTP
or HTTPS. Kong/charts#72 added support for
dual-stack admin API configuraton using the same templates as other Kong
services.

Fix #287
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.

None yet

2 participants