Change config specification #213

davidmogar · 2020-02-24T13:31:36Z

As it is right now, the specification of the config is done through a string. When using storage backends like PostgreSQL, the password for the database has to be included in the config variable of the values file.

This change allows to specify the configuration through a map, making the chart GitOps friendly (eg. using it with Flux). Now, sensitive values can be stored in a different values file or passed on deployment time with --set.

To have a very generic specification:

I've assumed that the combination stanza (eg. storage) name (eg. file) is unique.
Quoted values for all stanza parameters. I tested a generated configuration in a vault docker image and it seems to work just fine.

As an example, your values file could contain something like this:

server:
  standalone:
    config:
      ui: true
      listener:
        tcp:
          address: "[::]:8200"
          cluster_address: "[::]:8201"
          tls_disable: 1
      storage:
        file:
          path: "/vault/data"
      telemetry:
        statsite_address: "127.0.0.1:8125"
        disable_hostname: true

Edit:

As stated by @jasonodonnell, Vault support json as a format for the configuration. The new change will generate the following ConfigMap when providing the values shown above:

---
# Source: vault/templates/server-config-configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: RELEASE-NAME-vault-config
  namespace: default
  labels:
    helm.sh/chart: vault-0.4.0
    app.kubernetes.io/name: vault
    app.kubernetes.io/instance: RELEASE-NAME
    app.kubernetes.io/managed-by: Helm
data:
  extraconfig-from-values.hcl: |-
    {
      "disable_mlock": true,
      "listener": {
        "tcp": {
          "address": "[::]:8200",
          "cluster_address": "[::]:8201",
          "tls_disable": 1
        }
      },
      "storage": {
        "file": {
          "path": "/vault/data"
        }
      },
      "telemetry": {
        "disable_hostname": true,
        "statsite_address": "127.0.0.1:8125"
      },
      "ui": true
    }

hashicorp-cla · 2020-02-24T13:31:40Z

All committers have signed the CLA.

jasonodonnell · 2020-02-26T09:30:15Z

Hi @davidmogar, thanks for the contribution!

This feature has been on our list to do for some time, so I'm happy to see some progress here. I'm wondering if this feature could simply be reduced to using the toJson since JSON is a valid HCL alternative.

For example, the config could be defined such:

server:
  standalone:
    config:
      ui: true
      listener:
        tcp:
          address: "[::]:8200"
          cluster_address: "[::]:8201"
          tls_disable: 1
      storage:
        file:
          path: "/vault/data"
      telemetry:
        statsite_address: "127.0.0.1:8125"
        disable_hostname: true

Then the configmap could simply be:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "{{ template "vault.fullname" . }}-config"
data:
  config.json: |
    {{ .Values.server.standalone.config | toJson }}

Thoughts on your implementation verse this approach?

davidmogar · 2020-02-26T10:04:39Z

Thanks for your comment @jasonodonnell.

I wasn't aware of the possibility of specifying the config through json. In this case the code is way more simple and we can use toPrettyJson to have a much friendly output, as shown in the new commit. Let's use this commit to start discussing any further change required to push this change forward.

Not sure about the need for disable_mlock as not the docs mention is not suit for prod. If needed, could be merged into the generated json.

jasonodonnell · 2020-02-26T10:49:15Z

@davidmogar disable_mlock is set for a few reasons (and will need to stay):

swap is disabled in Kubernetes, so mlock doesn't really do anything
the container isn't running as root, so it doesn't have privileges to mlock (which causes errors in the startup script).

Hope that helps!

templates/server-config-configmap.yaml

davidmogar · 2020-02-26T12:05:57Z

Initial documentation for the change: hashicorp/vault#8423

davidmogar · 2020-03-16T12:04:53Z

Any news on this?

jasonodonnell · 2020-03-20T13:45:41Z

Sorry for the delay @davidmogar, going to review this today. My only hesitation is this breaks existing functionality. On the other hand this is a way better UX. If you get a chance please resolve conflicts.

davidmogar · 2020-03-20T21:02:55Z

No worries @jasonodonnell. Was merely a heads-up. I've fixed the conflicts. Let me know if you need anything else.

nesl247 · 2020-04-09T17:55:24Z

I wouldn't worry about breaking BC. This still isn't even a 1.0 version of the chart.

davidmogar · 2020-04-16T15:32:18Z

@jasonodonnell Any news on this?

jasonodonnell · 2020-04-23T16:11:56Z

Hi @davidmogar, sorry for the long delay. We talked about this internally across other teams that use Helm and we're definitely hesitant on changing the UX across our products. That being said, we've experimented with a way of supporting the old style of config (string) vs this new style. An example of this can be found here: #272

I'm wondering if we can modify this PR to include similar logic? Users who have been using Vault Helm for some time won't need to upgrade their configuration to the new UX and we can gave them fair warning about deprecating the string style in the future.

Thoughts?

As it is right now, the specification of the config is done through an string. When using storage backends like PostgreSQL, the password for the database has to be included in the config variable of the values file. This change allows to specify the configuration through a map, making the chart GitOps friendly. Now, sensitive values can be stored in a different values file or passed on deployment time with --set. To have a very generic specification: - I've assumed that the combination stanza (eg. storage) name (eg. file) is unique. - Quoted values for all stanza parameters. I tested a generated configuration in a vault docker image and it seems to work just fine.

davidmogar · 2020-04-24T08:58:43Z

Hi @jasonodonnell,

Makes sense to have both definitions working at the same time. I strongly suggest you to keep the yaml one. It's way more friendly and standard. In the meantime I left the original values file unchanged though. Your call on what to do with it although, again, I would use the yaml definition there too.

Take a look and let me know if the last change goes more on the line of what you want. If so, would be nice if you could put this on the fast track as this PR is already 2 months old and forcing us to use a fork.

jasonodonnell · 2020-04-24T14:14:32Z

templates/server-config-configmap.yaml

@@ -14,6 +14,9 @@ metadata:
    app.kubernetes.io/managed-by: {{ .Release.Service }}
 data:
  extraconfig-from-values.hcl: |-
+  {{- if or (eq .mode "ha") (eq .mode "standalone")}}
+  {{- $type := typeOf (index .Values.server .mode).config }}


There's actually a new ha submode, raft, which has it's own configuration server.ha.raft.config. I think we'll need additional logic for that config file.

In the future we hope to consolidate these modes but we'll need to change this for now.

Sorry, I didn't realize about the new submode. My last commit should address this problem.

In a side note, I think that you are over complicating the values file addressing problems where shouldn't be addressed.

It would be extremely more easy to define a single config variable with the specifics. If you want to provide examples, do so by creating multiple values files that the users can choose from or simply add comments. That would simplify the code a lot.

The only difference between modes, apart from the config, is the disruptionBudget in the ha, which could go outside the mode and be used if defined.

Hi @davidmogar, thanks for adding this.

As I mentioned in my last comment:

In the future we hope to consolidate these modes but we'll need to change this for now.

Testing this now and looking to get it merged today.

jasonodonnell

LGTM

davidmogar · 2020-04-28T19:25:09Z

Thank you for merging this, @jasonodonnell.

* Change config specification As it is right now, the specification of the config is done through an string. When using storage backends like PostgreSQL, the password for the database has to be included in the config variable of the values file. This change allows to specify the configuration through a map, making the chart GitOps friendly. Now, sensitive values can be stored in a different values file or passed on deployment time with --set. To have a very generic specification: - I've assumed that the combination stanza (eg. storage) name (eg. file) is unique. - Quoted values for all stanza parameters. I tested a generated configuration in a vault docker image and it seems to work just fine. * Change config format to json * Add conditional formatting * Add config for raft mode

gw0 · 2020-10-07T16:01:26Z

disable_mlock is set for a few reasons (and will need to stay):
* swap is disabled in Kubernetes, so mlock doesn't really do anything
* the container isn't running as root, so it doesn't have privileges to mlock (which causes errors in the startup script).

@jasonodonnell Just noticed these arguments and would like to leave a comment... I find it confusing that the config block is not directly used as provided by the user. Overriding values in templates is bad UX and complicates the code. As does having 2 HA modes each with its own config block (that are pretty similar).

davidmogar mentioned this pull request Feb 26, 2020

Use native kubernetes ways of configuration #55

Open

jasonodonnell reviewed Feb 26, 2020

View reviewed changes

templates/server-config-configmap.yaml Outdated Show resolved Hide resolved

davidmogar mentioned this pull request Feb 26, 2020

Update docs for Helm's new config declaration hashicorp/vault#8423

Closed

davidmogar requested a review from jasonodonnell March 3, 2020 09:44

tvoran added enhancement New feature or request chart Area: helm chart labels Mar 4, 2020

David Moreno García and others added 3 commits April 24, 2020 10:26

Change config format to json

dee3b22

Add conditional formatting

6969135

jasonodonnell reviewed Apr 24, 2020

View reviewed changes

Add config for raft mode

efec3f3

jasonodonnell approved these changes Apr 27, 2020

View reviewed changes

jasonodonnell merged commit 0f36ee3 into hashicorp:master Apr 27, 2020

FWest98 mentioned this pull request Aug 31, 2020

Allow also yaml configuration for nodeSelector, tolerations and affinities #380

Closed

arpithank mentioned this pull request Sep 8, 2020

hashicorp/vault-helm/values: Using YAML syntax for config instead of a string #383

Closed

gw0 mentioned this pull request Oct 21, 2020

Fix Raft config provided in YAML #406

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change config specification #213

Change config specification #213

davidmogar commented Feb 24, 2020 •

edited

Loading

hashicorp-cla commented Feb 24, 2020 •

edited

Loading

jasonodonnell commented Feb 26, 2020

davidmogar commented Feb 26, 2020 •

edited

Loading

jasonodonnell commented Feb 26, 2020 •

edited

Loading

davidmogar commented Feb 26, 2020 •

edited

Loading

davidmogar commented Mar 16, 2020

jasonodonnell commented Mar 20, 2020

davidmogar commented Mar 20, 2020

nesl247 commented Apr 9, 2020

davidmogar commented Apr 16, 2020

jasonodonnell commented Apr 23, 2020

davidmogar commented Apr 24, 2020

jasonodonnell Apr 24, 2020 •

edited

Loading

davidmogar Apr 24, 2020

jasonodonnell Apr 27, 2020

jasonodonnell left a comment

davidmogar commented Apr 28, 2020

gw0 commented Oct 7, 2020

Change config specification #213

Change config specification #213

Conversation

davidmogar commented Feb 24, 2020 • edited Loading

hashicorp-cla commented Feb 24, 2020 • edited Loading

jasonodonnell commented Feb 26, 2020

davidmogar commented Feb 26, 2020 • edited Loading

jasonodonnell commented Feb 26, 2020 • edited Loading

davidmogar commented Feb 26, 2020 • edited Loading

davidmogar commented Mar 16, 2020

jasonodonnell commented Mar 20, 2020

davidmogar commented Mar 20, 2020

nesl247 commented Apr 9, 2020

davidmogar commented Apr 16, 2020

jasonodonnell commented Apr 23, 2020

davidmogar commented Apr 24, 2020

jasonodonnell Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

davidmogar Apr 24, 2020

Choose a reason for hiding this comment

jasonodonnell Apr 27, 2020

Choose a reason for hiding this comment

jasonodonnell left a comment

Choose a reason for hiding this comment

davidmogar commented Apr 28, 2020

gw0 commented Oct 7, 2020

davidmogar commented Feb 24, 2020 •

edited

Loading

hashicorp-cla commented Feb 24, 2020 •

edited

Loading

davidmogar commented Feb 26, 2020 •

edited

Loading

jasonodonnell commented Feb 26, 2020 •

edited

Loading

davidmogar commented Feb 26, 2020 •

edited

Loading

jasonodonnell Apr 24, 2020 •

edited

Loading