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.

Sign up for GitHub

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

values.yaml updated to include explicit cluster_name #889

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

values.yaml updated to include explicit cluster_name #889

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@

# Available parameters and their default values for the Vault chart.

# Set cluster name instead of having it auto-generated.
# examples may include: "vault1-gke-uswest", "vault2-eks-useast", etc.
cluster_name: ""
Comment on lines +6 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a Vault specific parameter and should not be at the root level but instead should be in server.ha (it's only used for HA).

Copy link
Author

Choose a reason for hiding this comment

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

I can move this to server.ha section on subsequent commit.


global:
# enabled is the master enabled switch. Setting this to true or false
# will enable or disable all the components within this chart by default.
Expand Down Expand Up @@ -834,6 +838,7 @@ server:
# https://www.vaultproject.io/docs/platform/k8s/helm/run#protecting-sensitive-vault-configurations
config: |
ui = true
cluster_name = {{ .Values.cluster_name | default (printf "%s-k8s" (include "vault.name" .)) | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually work? I've never seen the templates used outside of the K8s yaml helm is generating.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it works - I've tested it (with both explicit value set & nothing set) before making PR

Copy link
Author

Choose a reason for hiding this comment

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

Example as per current helm documentation.
https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#using-the-default-function

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't set a default here because if someone has already deployed Vault and it has a generated cluster name, this is going to introduce a new set of metrics.

Suggested change
cluster_name = {{ .Values.cluster_name | default (printf "%s-k8s" (include "vault.name" .)) | quote }}
cluster_name = {{ .Values.cluster_name | default (printf "%s-k8s" (include "vault.name" .)) | quote }}

Copy link
Author

Choose a reason for hiding this comment

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

vault.name is just vault right? - so if nothing is set it will always result in the same value no?
If users change it then it may result in what you're describing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm referencing Vault's code that generates the cluster name if it's not set in the config. It generates a cluster name that looks something like this: vault-cluster-b0435276. If you now start setting it in the config with a default anyone who was collecting metrics using the Vault generated name will now have a new set metrics.

Copy link
Author

@aphorise aphorise May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you're referring to existing customers (with an existing deployment that has an auto-generated name) that may update their helm chart - in that case they would need to explicit set the cluster_name in values.yaml (may need another call out on the main helm page). I am not aware of any other way of importing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't, so we should not have a default here, because they're going to need to explicitly set it.

Copy link
Author

Choose a reason for hiding this comment

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

I added defaults in case for new deployment no value is set. I'm open to any proposal that you may be kind enough to commit.


listener "tcp" {
tls_disable = 1
Expand Down