-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[bitnami/elasticsearch] Service name #33775
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
base: main
Are you sure you want to change the base?
[bitnami/elasticsearch] Service name #33775
Conversation
Signed-off-by: tommyluk <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hket-tommyluk, thank you for your contribution! Could you please take a look at my comment?
{{- else -}} | ||
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} | ||
{{- $name := .Values.global.elasticsearch.service.name -}} | ||
{{- $releaseName := regexReplaceAll "(-?[^a-z\\d\\-])+-?" (lower .Release.Name) "-" -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the regexp, I understand that the goal is to replace non-alphanumeric characters with '-'.
While I understand its utility, we do not usually add this kind of 'fixes' for users invalid inputs.
For example, we have other values such as nameOverride
or fullnameOverride
which do not include this kind of regexp replacement.
I would rather have helm/kubernetes complain about invalid service name, rather than the chart automatically modifying my inputs to make them valid, for example replacing 'my-n@me' with 'my-n-me'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your feedback.
I understand the point that we usually avoid adding fixes for invalid user inputs. However, in this case, .Release.Name
is not user input:
.Release.Name
does not come fromvalues.yaml
.- A valid Helm release name can include lowercase alphanumeric characters, hyphens, and dots. However, dots are not allowed in DNS labels, so the release name may not be a valid DNS label.
- The release name is designed to identify the Kubernetes release/project, not necessarily to serve as a DNS label.
According to the RFC 1123 DNS Label standard, resource names must:
- Contain at most 63 characters
- Contain only lowercase alphanumeric characters or
-
- Start with an alphanumeric character
- End with an alphanumeric character
For convenience, Bitnami charts use the release name as the default service name if the user does not override it. However, to comply with DNS label requirements, the release name must be sanitized and converted into a valid DNS label. This is why the existing template includes | trunc 63 | trimSuffix "-"
, which ensures the name is at most 63 characters and ends with an alphanumeric character (in most cases).
The addition of regexReplaceAll "(-?[^a-z\\d\\-])+-?" (lower .Release.Name) "-"
in this PR further ensures the second requirement—that the name contains only lowercase alphanumeric characters or hyphens—is met by replacing invalid characters with -
.
In summary, this change is necessary to guarantee that the default service name derived from .Release.Name
is always a valid DNS label, avoiding potential issues in Kubernetes resource naming.
Note: This change is to align with #33773.
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
@migruiz4 Please take a look at my reply in your comment. Thanks you. |
Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
Description of the change
This PR updates the Elasticsearch chart to improve the configuration of the service name used by the Kibana subchart.
Specifically, it introduces a new
global.elasticsearch.service.fullname
value to allow referencing the full Elasticsearch service name, enhancing flexibility whenkibanaEnabled
is true.Additionally, the documentation and default values have been updated to reflect this new option and clarify the behavior of
global.elasticsearch.service.name
.Benefits
Possible drawbacks
service.fullname
parameter.Applicable issues
Additional information
Tested the chart with Kibana enabled and verified that the new
service.fullname
parameter correctly overrides the service name reference in the Kibana subchart.Checklist
Chart.yaml
according to semver. This is not necessary when the changes only affect README.md files.README.md
using readme-generator-for-helm