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

Reject boolean log envvars #733

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Reject boolean log envvars #733

merged 2 commits into from
Feb 2, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Feb 2, 2023

What this PR does / why we need it:

Checks the type of any environment variable containing _log. If it is a boolean, emit a failure instructing users to add quotes.

Reverts 2edf22d, which previously addressed this by enforcing string types for all variables.

Which issue this PR fixes

Special notes for your reviewer:

In general, we don't care if YAML's stupid type coercion nonsense changes off to false: Pod envvars are ultimately all treated as strings by Kubernetes, and Kong accepts either value for boolean config values.

The various _log variables are a special case: these are not booleans, and the literal string off is a special value that disables logging. YAML nonsense converting these to booleans (and then Kubernetes converting them back to strings) is a problem, since false is valid value for the non-special case of this of setting, where it's instead interpreted as a file path. I'm not entirely sure why using <prefix dir>/false breaks the container (it's a valid path and Kong should be able to write to it fine), but in any case, it's something we can check special for these.

Manual testing

Dunno why --set doesn't work to set the boolean of this for me, so these are in values.yaml files instead. With this change:

17:24:29-0800 esenin $ cat /tmp/values_string_off.yaml
env:
  proxy_access_log: "off"

17:24:34-0800 esenin $ cat /tmp/values_raw_off.yaml   
env:
  proxy_access_log: off

17:24:36-0800 esenin $ helm template --debug bread /tmp/symkong -f /tmp/values_string_off.yaml | grep -A1 PROXY_ACCESS
        - name: KONG_PROXY_ACCESS_LOG
          value: "off"
--
        - name: KONG_PROXY_ACCESS_LOG
          value: "off"

17:25:01-0800 esenin $ helm template --debug bread /tmp/symkong -f /tmp/values_raw_off.yaml                        
Error: execution error at (kong/templates/deployment.yaml:99:12): env.proxy_access_log must use string 'off' to disable. Without quotes, YAML will coerce the value to a boolean and Kong will reject it
helm.go:84: [debug] execution error at (kong/templates/deployment.yaml:99:12): env.proxy_access_log must use string 'off' to disable. Without quotes, YAML will coerce the value to a boolean and Kong will reject it

Checklist

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

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • Commits follow the Kong commit message guidelines

Emit a template failure if a log environment variable is set to a raw
'off' value without quotes, which YAML coerces to a boolean 'false'.
This avoids an edge case where attempting to disable a log without a
string '"off"' instructs Kong to instead attempt to log to a file named
'false', which causes a failure.
@rainest rainest marked this pull request as ready for review February 2, 2023 01:35
@rainest rainest requested a review from a team as a code owner February 2, 2023 01:35
@randmonkey randmonkey merged commit c528d42 into main Feb 2, 2023
@randmonkey randmonkey deleted the feat/log-fail branch February 2, 2023 02:12
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.

Validate that env values are strings (and not bools/numbers)
2 participants