Dbless secret support

[alice-sawatzky]

What this PR does / why we need it:

Add the option to use Secrets for dblessConfig, instead of ConfigMaps.

Special notes for your reviewer:

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
• New or modified sections of values.yaml are documented in the README.md
• Commits follow the Kong commit message guidelines

[CLAassistant]

All committers have signed the CLA.

[rainest]

To avoid needing to keep the two settings aligned, can you remove the kind field and make the mount use the ConfigMap if it's set, the Secret if it's set, or fail the config if both are set?

[alice-sawatzky]

i guess the current schema is confusing. the intent is to preserve backwards compatibility by having all fields behave the same as before. as written now, kind only takes effect if passing in the config data directly to dblessConfig.config; if setting dblessConfig.configMap or dblessConfig.secret, that value is ignored so it doesn't need to be kept in sync. i couldn't think of a better key name to make it explicit that the kind key specifically describes the kind of the object created by .config if set.

cleanest way forward i see is to break backwards compatibility by replacing dblessConfig.config and dblessConfig.kind with dblessConfig.config.value and dblessConfig.config.kind, i'm not sure if it's worth the breakage to make the relationship more clear or whether i should just make the docs more clear

[rainest]

Ah, I forgot about dblessConfig.config, I thought it was only available through a user-created ConfigMap.

I think some breakage is fine here since it'd only affect values.yaml configurations that shouldn't really exist. Right now you can define both configMap and config, and the chart will ultimately use the configMap version because that's how the if clause is written in the Deployment.

There's no reason to ever define both since you can only use one, so I think it's entirely reasonable to add validation that fails if more than one of config, configMap, or the new secret field are set. If anyone has an ambiguous configuration, they'll only need a simple change to remove whichever variant they're not actually using.

For config, I'd say only support the existing ConfigMap storage: if you want to use a Secret, you need to create it independently. We generally want to discourage storing sensitive information within values.yaml.

[alice-sawatzky]

the request to add validation to prevent both configMap|secret and config being set makes sense to me, I'll push that soon.

I understand discouraging storing secrets in chart values in general, but there are use cases where that's very handy and its risks are mitigated; for example we use yaml-crypt to manage secret values safely and efficiently; even without such a specialized tool, cascading values files can be used to section off secret data from the main values.

I'm not advocating this be a recommended workflow, but I think it's reasonable enough to support it. I can certainly add a disclaimer to the docs to make it clear this is not a recommended configuration, but I don't see any harm in surfacing the option. If this is a firm policy I'll stop pushing back on this; this feature is less crucial since I can always work around it with a wrapper chart on my end.

Note, the breakage I'm describing would also affect non-ambiguous configurations, since it would move the config key. Here's the options as I see them:

Current format

dblessConfig:
  configMap: ""
  secret: ""
  config: "yaml string right here"
  kind: ConfigMap

Option 1: No breakage

dblessConfig:
  configMap: ""
  secret: ""
  config: "yaml string right here"
  createConfigKind: ConfigMap # or some other key name that makes its purpose clearer

Option 2: Breakage, but clearer

dblessConfig:
  configMap: ""
  secret: ""
  createConfig:
    enabled: true
    kind: ConfigMap
    data: "yaml string right here"

Option 3: Even more breakage, but even clearer

dblessConfig:
  existingConfig:
    enabled: false
    kind: ConfigMap
    name: somename
  createConfig:
    enabled: true
    kind: ConfigMap
    data: "yaml string right here"

I think option 3 is the nicest, but option 1 makes the most sense if a major version chart release isn't planned anytime soon

[alice-sawatzky]

I've implemented Option 3 since it seems like the cleanest option, I added a disclaimer to the kind key, hopefully this addresses your concerns.

[rainest]

I'm pretty firm on not wanting to populate Secrets from values.yaml as standard functionality, so I'd still want the version where the options are mutually exclusive and both the configMap and secret keys require referencing a resource that's defined elsewhere.

You can, as an alternative to an umbrella chart, use the catch-all additional resource creator to create a Secret directly from this chart.

[alice-sawatzky]

alright, I've removed the createConfig.kind key and its implementation. Would you like me to revert the values scheme back to its original form as well?

[rainest]

Yeah, basically option 1 minus the create type switch, so

dblessConfig:
  configMap: ""
  secret: ""
  config: "yaml string right here"

and then stricter template logic to require that you only set one of the three.

[rainest]

Checking on this, do you know if you'd be able to make the changes in the next few days? We plan to release KIC 2.8 on the coming Monday, 2022-12-19, and will release the next chart version shortly after.

I'd love to get this in if possible, so if you have time to complete it and #696 next week, we can hold the chart release for a bit to complete this.

[alice-sawatzky]

sorry, I just saw this reply now, looks like i missed the release. I've rebased against main and cleared the merge conflicts, and have reverted to the schema you described. as requested, the mutual exclusion logic is in line 481 of _helpers.tpl and will fail if more than 1 of those fields is set at once.

[alice-sawatzky]

just checking in, I think all the outstanding issues are addressed now, anything else need another look?

[rainest]

We were just all out for the past week or so for the holidays. Looks good now aside from the stupid linter issue.

[rainest]

And then I got sick after the holidays ☹️

Not sure what's making CI upset and not running/not letting me approve the run, but I ran ct lint-and-install locally and it ran into an issue trying to mount the config volume even though it shouldn't be enabled:

Events:
  Type     Reason       Age                  From               Message
  ----     ------       ----                 ----               -------
  Normal   Scheduled    5m                   default-scheduler  Successfully assigned kong-muc3xwk61w/kong-muc3xwk61w-kong-84b6fb847b-ckq7c to kind-control-plane
  Warning  FailedMount  50s (x10 over 5m)    kubelet            MountVolume.SetUp failed for volume "kong-custom-dbless-config-volume" : configmap "kong-muc3xwk61w-kong-custom-dbless-config" not found
  Warning  FailedMount  40s (x2 over 2m57s)  kubelet            Unable to attach or mount volumes: unmounted volumes=[kong-custom-dbless-config-volume], unattached volumes=[kong-muc3xwk61w-kong-prefix-dir kong-muc3xwk61w-kong-tmp kong-custom-dbless-config-volume tmpdir]: timed out waiting for the condition

run.txt

@alice-sawatzky are you able to determine what's making it attempt to mount when it shouldn't? Opening a PR on your fork from the PR branch to your main should make GitHub Actions run CI if you prefer that over running it locally (or if you do want to run it locally, installing chart-testing and setting up a KIND instance should let you run ct lint-and-install from the repo dir to run the tests).

[alice-sawatzky]

looks like the behavior is correct; the conditions for dblessConfig are present, therefore it should attempt to mount a dblessConrfig volume. The problem is that the test values file has a invalid config, and the chart doesn't create a sensible error message for that invalid config.

The values file enables dblessConfig but does not set any of dblessConfig.[config|configMap|secret]. I've expanded the error checking so that if dblessConfig is enabled but none of the needed keys are set, this will fail with an error message explaining the situation, and I've also updated the test file to contain a valid configuration. See the body of commit 2e37896 for more details.

[rainest]

Looks like the new fail for DB-less but no config provided means CI 4 needs a test configuration, e.g.

diff --git a/charts/kong/ci/test4-values.yaml b/charts/kong/ci/test4-values.yaml
index d4a2dd3..8d912a0 100644
--- a/charts/kong/ci/test4-values.yaml
+++ b/charts/kong/ci/test4-values.yaml
@@ -24,3 +24,20 @@ proxy:
     - ssl
   ingress:
     enabled: true
+dblessConfig:
+  config: |
+    _format_version: "3.0"
+    _transform: true
+    services:
+    - name: my-service
+      url: https://example.com
+      plugins:
+      - name: key-auth
+      routes:
+      - name: my-route
+        paths:
+        - /
+    consumers:
+    - username: my-user
+      keyauth_credentials:
+      - key: my-key

With that in place the test

[rainest]

@alice-sawatzky checking in again, are you able to make the CI change?

[alice-sawatzky]

got it!