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

values.yaml updated to include explicit cluster_name #889

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

Conversation

aphorise
Copy link

@aphorise aphorise commented May 15, 2023

Setting an explicit (HCL) cluster_name as part of the cluster roll-out / configuration is required without which an undesirable behavior arises where the reported Prometheus label does not correctly contain the cluster name - eg (at present):

curl -Lks ${VAULT_ADDR}v1/sys/metrics?format=prometheus | grep dr_primary{
  # vault_core_replication_dr_primary{cluster="",host="vault-2"} 0

Setting an explicit cluster_name as part of the cluster roll-out / configuration is required without which a undesirable behavior arises where the reported Prometheus label does not correctly contain the cluster name - eg (at present):

vault_core_replication_dr_primary{cluster="",host="vault-2"} 0
@aphorise aphorise requested a review from a team May 15, 2023 12:54
Comment on lines +6 to +8
# Set cluster name instead of having it auto-generated.
# examples may include: "vault1-gke-uswest", "vault2-eks-useast", etc.
cluster_name: ""
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.

@@ -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.

@@ -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.

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

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.

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.

2 participants