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

[stable/kong] refactor env block generation #4

Merged
merged 1 commit into from Jan 23, 2020
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jan 18, 2020

What this PR does / why we need it:

tl;dr it makes generating EnvVar blocks cleaner and allows us to guarantee that we only provide one version of an environment variable, because providing multiple versions causes bad things to happen.

This change significantly refactors the generation of Kong container env blocks. It also consolidates various secretKeyRef templates into a single generic template, and makes several minor changes elsewhere in the chart to accomodate the new templates.

The current template handles user-provided environment variables and automatically-generated variables differently. User variables are rendered in a loop over the env section of values.yaml, and generated variable values are inserted into a static template. If a user provides a variable that is also generated automatically, the resulting EnvVar block will contain both versions of the variable.

The new template adds both user and generated variables to dictionaries, which are then combined and run through a render loop to generate the EnvVar block. Dictionaries include built-in support for overriding a key's (variable name) value with another value consistently. User-provided variable values take precedence over generated values, and only a single version of a variable will be present in the resulting EnvVar block.

Special notes for your reviewer:

Previously, based on observed behavior in our test environments, we believed that the last instance of a variable in an EnvVar block took precedence in the running container, so user-provided variables were rendered after the generated variables. As seen in helm/charts#18715, Kubernetes' behavior in this scenario is actually undefined: some providers give precedence to the first variable instance in the block. As discussed in kubernetes/kubernetes#59593, providing duplicate variable definitions should not actually be permitted at all. However, per kubernetes/kubernetes#64841, fixing this (i.e. rejecting configurations that include multiple definitions of a variable) is fairly difficult, and Kubernetes will continue to accept multiple instances of a variable going forward.

At present, I believe we should actually reverse our current precedence order: there should not be many cases where it makes sense for users to override generated values (if there are, we should consider those bugs in the value generators and fix them), and overriding values can break the deployment in unexpected ways (e.g. providing your own KONG_ADMIN_LISTEN directly will typically result in the generated admin API Service not matching the listen in the container/blocking admin API access). We could, if necessary, add an additional dictionary that looks for special override values in env (e.g. instead of admin_listen, override_admin_listen) and give it final precedence. However, in the spirit of the original intent of the old template, the new template gives all user variables precedence.

The new template relies on a magic string: if a value contains "valueFrom", it is passed to a different case in the render loop to generate a secretKeyRef block (effectively, dump the string as-is). This is necessary because values in a dictionary are always strings, and cannot be detected by the existing if eq $valueType "map[string]interface {}" case. We furthermore cannot handle these like plain variable values, as those are passed to quote, which mangles the valueFrom block. While this is imperfect, I think it's reasonable, as at present (K8S API version 1.17), valueFrom is the only multi-line block suppported by the EnvVar specification.

Checklist

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

  • Chart Version bumped
  • Variables are documented in the README.md

@rainest rainest force-pushed the refactor/env-block branch 3 times, most recently from 5c63933 to 48e84c7 Compare January 22, 2020 02:09
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 amazing work. I tested locally a bit and it is pretty neat.

charts/kong/templates/_helpers.tpl Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Show resolved Hide resolved
charts/kong/templates/_helpers.tpl Show resolved Hide resolved
@hbagdi
Copy link
Member

hbagdi commented Jan 23, 2020

Can you fix the conflicts and we will be good to go?

@rainest rainest requested a review from hbagdi January 23, 2020 21:09
Significantly refactor the generation of Kong container env blocks. Add
both user-provided variables and variables generated from other sections
of values.yaml to dictionaries, merge the dictionaries to ensure only a
single instance of a variable is present, and render the resulting
dictionary into an EnvVar block. In addition:

* Condense kong.env and kong.final_env back into a single kong.env template.
* Remove special-case env generation from kong.wait-for-db and use
  kong.env to ensure consistent behavior accross all Kong containers.
* Add a new kong.no_daemon_env template to disable NGINX daemonization for
  containers other than wait-for-db.
* DRY license, session configuration, and auth configuration templates into
  a generic "secretkeyref" template.
* Add a new case to the renderer for handling templated secretKeyRef
  blocks. Templates can only generate strings, not string maps, and the
  quote function condenses multi-line strings onto a single line, so
  neither of the existing render cases work.

The additional renderer relies on the presence of "valueFrom" as a magic
string to indicate that it should handle that variable. It cannot handle
other multi-line blocks as such. At present (API version 1.17),
valueFrom is the only such block suppported by the EnvVar specification.

Signed-off-by: Travis Raines <traines@konghq.com>
@hbagdi
Copy link
Member

hbagdi commented Jan 23, 2020

@rainest Thank you for this fix. Your persistence and hard-work is truly inspiring!

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

Successfully merging this pull request may close these issues.

None yet

2 participants