fix subchart dns macros #359
No reviewers
Labels
No Label
has
backport
in progress
invalid
kind
breaking
kind
bug
kind
build
kind
dependency
kind
deployment
kind
docs
kind
enhancement
kind
feature
kind
lint
kind
proposal
kind
question
kind
refactor
kind
security
kind
testing
kind
translation
kind
ui
need
backport
priority
critical
priority
low
priority
maybe
priority
medium
reviewed
duplicate
reviewed
invalid
reviewed
wontfix
skip-changelog
status
blocked
status
needs-feedback
status
needs-reviews
status
wip
upstream
gitea
upstream
other
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: gitea/helm-chart#359
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "jouve/helm-chart:dns"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description of the change
inspired from https://github.com/bitnami/charts/blob/master/bitnami/common/templates/_names.tpl#L35, add a macro to calculate subchart fullname and use it to calculate dns
trim all metadata.name to not go above 63 chars
Benefits
fix for #345
Possible drawbacks
Applicable issues
Additional information
⚠ BREAKING
Checklist
values.yaml
and added to theREADME.md
using readme-generator-for-helmREADME.md
3c365f2545
to86f0b1abcd
86f0b1abcd
to923385674a
923385674a
todc3f7aa714
I don't mind this change given the issues of too long namespace names etc.
Yet I wonder why three different lenghts were chosen here (49, 58, 59). Should we agree on one and maybe use a round number (50?)?
@ -109,3 +122,3 @@
{{- define "gitea.default_domain" -}}
{{- printf "%s-gitea.%s.svc.%s" (include "gitea.fullname" .) .Release.Namespace .Values.clusterDomain | trunc 63 | trimSuffix "-" -}}
{{- printf "%s-http.%s.svc.%s" (include "gitea.fullname" . | trunc 58) .Release.Namespace .Values.clusterDomain -}}
Is there a strong reason for the naming change here? Otherwise I'd probably keep it as is?
Ah sorry, I'm dumb 🤦 The different values should add up to 63 with the additional characters in the respective sections.
@jouve Can you resolve the current conflicts? I guess we could merge this then, I would also ask @justusbunsi for a review then.
Reviewing these changes once more I noticed that this should be considered a breaking change due to potential name changes in RBAC related matters and cluster cross connectivity (changing service name), requiring cluster adjustments outside of the Helm Chart. First coming in mind: backup cronjob accessing the secrets or another application using the services.
Please adjust the PR description accordingly.
As there are different trimming limits for a reason, I also vote for explicit unit tests here to detect changes of those values and ensure correct templating behavior.
dc3f7aa714
to471222b1d1
471222b1d1
to788aa805b2
I removed change to the secret as it does not have the 63 length limit.
As for the services, I would say the change is backward compatible:
788aa805b2
to0b3d7dc836
0b3d7dc836
tod14a17a605
@ -101,3 +114,3 @@
{{- define "gitea.default_domain" -}}
{{- printf "%s-gitea.%s.svc.%s" (include "gitea.fullname" .) .Release.Namespace .Values.clusterDomain | trunc 63 | trimSuffix "-" -}}
{{- printf "%s-http.%s.svc.%s" (include "gitea.fullname" . | trunc 58) .Release.Namespace .Values.clusterDomain -}}
@jouve Can you explain why you changed the name from
gitea
tohttp
here?It probably won't cause any issues but I would still like to understand it 🙂
there is no
{{ include "gitea.fullname" }}-gitea
service defined in the chart, while -http exists:helm install -n gitea gitea gitea/gitea
:Ah good catch! This looks like an uncaught bug then. Thanks for the reply!
Awesome catch.
LGTM, thanks @jouve!
I agree it won't cause a breaking change in it's current form.
Would be good to get a second review from @justusbunsi on it!
Reject to prevent merge. Still testing.
@ -94,2 +106,3 @@
{{- define "postgresql.dns" -}}
{{- printf "%s-postgresql.%s.svc.%s:%g" .Release.Name .Release.Namespace .Values.clusterDomain .Values.postgresql.global.postgresql.service.ports.postgresql -}}
{{- printf "%s.%s.svc.%s:%g" (include "gitea.dependency.fullname" (dict "chartName" "postgresql" "chartValues" .Values.postgresql "context" .)) .Release.Namespace .Values.clusterDomain .Values.postgresql.global.postgresql.servicePort -}}
The proposed changes do not match the values specification for postgresql. Correct would be:
@ -97,3 +110,3 @@
{{- define "memcached.dns" -}}
{{- printf "%s-memcached.%s.svc.%s:%g" .Release.Name .Release.Namespace .Values.clusterDomain .Values.memcached.service.ports.memcached | trunc 63 | trimSuffix "-" -}}
{{- printf "%s.%s.svc.%s:%g" (include "gitea.dependency.fullname" (dict "chartName" "memcached" "chartValues" .Values.memcached "context" .)) .Release.Namespace .Values.clusterDomain .Values.memcached.service.port -}}
The proposed changes do not match the values specification for memcached. Correct would be:
Follow-up to my review. I've played a bit more with it.
From what I see in my tests, we don't need to truncate the
app.ini
settings in the first place. I was able to create a working release with FQDNs inapp.ini
that exceed the current limits by a lot:helm upgrade -i -n some-really-long-namespace-with-lots-of-dashed-parts --create-namespace long-gitea-release-name ./
creates a memcached host with 104 characters (port excluded):long-gitea-release-name-memcached.some-really-long-namespace-with-lots-of-dashed-parts.svc.cluster.local:11211
.According to the Domain Name RFC, the size limit for FQDN is 255. With Helm limiting its release name to 53 characters and the Kubernetes internal label/name restriction of 63 characters, there is a much higher chance that such release will fail at a much earlier stage than seeing it fail due to a too long FQDN inside
app.ini
(if even possible with the mentioned restrictions). It seems much more important to ensure that the resource names and labels do not exceed their limits.Especially when Kubernetes automatically adds suffixes to names.
When I stretch the limit of release and namespace names, I the release does not work, as I am getting errors from and Postgres StatefulSet.
helm upgrade -i --create-namespace --namespace some-really-long-namespace-with-lots-of-dashed-parts-in-it release-name-covering-so-much-more-components ./
This generates a Gitea StatefulSet that creates a pod with label
controller-revision-hash=release-name-covering-so-much-more-components-gitea-f858b9499
which is 62 characters long. This is something we have under control by not allowing a very long release name. So far so good.What we don't have under control is what the Postgres dependency does. In my case it tries to render a pod with the similar label
controller-revision-hash=release-name-covering-so-much-more-components-postgresql-7c8cc97779
, which is longer than 63 characters. This leads to a release that will never be ready-to-use.I tested if tools like
kubeval
(or its successor kubeconform) would detect such cases. Unfortunately, this is Kubernetes internal naming and not detectable. We would need to reject name lengths that would potentially create names/labels longer than 63 characters.Or a chart dependency trims on its own - incorrectly.
helm upgrade -i --create-namespace --namespace some-really-long-namespace-with-lots-of-dashed-parts-in-it release-name-covering-so-much-more-components-n-stuff ./
does not render the templates correctly. The Postgres chart trucates the two service names in such way that it leaves two identical service names.Leading to
So my suggestion is to drop the
trunc
withingitea.default_domain
(namespace limit restriction by Kubernetes catches that first) andmemcached.dns
helper functions while keeping the fix withingitea.default_domain
. That is a valid fix. Good catch.Those changes would be three lines and definitely no breaking change. Apologies to @jouve for not mentioning this earlier. I actually did not knew it behaves like it does before testing your Pull Request and trying to break Kubernetes. 😃
d14a17a605
tob8d2dbdd0a
b8d2dbdd0a
toebe07ef640
@ -97,3 +97,3 @@
{{- define "memcached.dns" -}}
{{- printf "%s-memcached.%s.svc.%s:%g" .Release.Name .Release.Namespace .Values.clusterDomain .Values.memcached.service.ports.memcached | trunc 63 | trimSuffix "-" -}}
{{- printf "%s.%s.svc.%s:%g" (include "common.names.dependency.fullname" (dict "chartName" "memcached" "chartValues" .Values.memcached "context" .)) .Release.Namespace .Values.clusterDomain .Values.memcached.service.ports.memcached -}}
What's the intention of using the common package from bitnami? Consistency with names?
This has the potential to break when updating memcached only and keep Postgres version, or vice versa. The "common" package is not a direct dependency of this chart.
it's to match how bitnami charts calculate the name :
own name with common.names.fullname and common.names.dependency.fullname when in the context of a subchart.
example in bitnami/wordpress
you don't need direct dependency to common, templates are shared between all charts / there is no scope: https://helm.sh/docs/chart_template_guide/subcharts_and_globals/#sharing-templates-with-subcharts
Same a you, I'd expect that a backward imcompatible of memcached or pg would break many things.
I'll revert that tomorrow anyway
ebe07ef640
toffbc3ae44e
Dang it. I totally missed that you already committed changes.
Sorry for not reviewing earlier.
@ -96,4 +96,3 @@
{{- end -}}
{{- define "memcached.dns" -}}
{{- printf "%s-memcached.%s.svc.%s:%g" .Release.Name .Release.Namespace .Values.clusterDomain .Values.memcached.service.ports.memcached | trunc 63 | trimSuffix "-" -}}
Same question as
abovebelow.EDIT: I expected this to be second comment 😆.
@ -92,15 +92,15 @@ app.kubernetes.io/instance: {{ .Release.Name }}
{{- end -}}
{{- define "postgresql.dns" -}}
{{- printf "%s-postgresql.%s.svc.%s:%g" .Release.Name .Release.Namespace .Values.clusterDomain .Values.postgresql.global.postgresql.service.ports.postgresql -}}
Any reason you removed the
-postgresql
suffix? Runninghelm template --create-namespace --namespace gitea gitea-release ./
and looking at the generated Postgres service, its name isgitea-release-postgresql
, leading to FQDN (+port)gitea-release-postgresql.gitea.svc.cluster.local:5432
.With these changes, the app.ini gets the value
HOST=gitea-release.gitea.svc.cluster.local:5432
, which is incorrect from what I see.@jouve Do you want to continue with this PR?
This looks stale meanwhile. @justusbunsi Do you want to continue here and finish it? Since you've done so much reviewing already. Or should we close? You decide!
@pat-s Yes, it contains valid fixes. I am going to create a superseding PR.
Superseded by #560.
Pull request closed