Skip to content

feature(chart): Allow configuring labels and annotations#2443

Open
brookelew wants to merge 4 commits into
flannel-io:masterfrom
brookelew:allow-configuring-labels-annotations
Open

feature(chart): Allow configuring labels and annotations#2443
brookelew wants to merge 4 commits into
flannel-io:masterfrom
brookelew:allow-configuring-labels-annotations

Conversation

@brookelew

@brookelew brookelew commented May 5, 2026

Copy link
Copy Markdown

lets users configure the labels and annotations in the Helm chart

Resolves #2442

Description

Feature for the Helm chart that lets users modify the labels and annotations of deployed resources.

Todos

  • Tests
  • Documentation
  • Release note

Release Note

None required

@mgfritch
mgfritch requested a review from Copilot May 6, 2026 15:14
@brookelew
brookelew force-pushed the allow-configuring-labels-annotations branch from 73fe8c7 to 75056b8 Compare May 14, 2026 03:01
@mgfritch
mgfritch requested review from Copilot and removed request for Copilot May 14, 2026 22:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

chart/kube-flannel/templates/rbac.yaml:55

  • Same issue as above for the ClusterRoleBinding: labels: is always emitted, but the merged label map may be empty (defaults are {}), resulting in labels: with a null value. Make the labels: key conditional or render an explicit empty map.
  labels:
    {{- with (merge .Values.flannel.rbac.clusterRoleBinding.labels .Values.flannel.rbac.labels .Values.global.commonLabels) }}
    {{- toYaml . | nindent 4 }}
    {{- end }}

Comment thread chart/kube-flannel/templates/rbac.yaml Outdated
Comment thread chart/kube-flannel/templates/daemonset.yaml

@mgfritch mgfritch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution!

Would you mind adding a named template (helpers file) for generating the default flannel labels?

Comment thread chart/kube-flannel/templates/daemonset.yaml Outdated
Comment thread chart/kube-flannel/templates/rbac.yaml Outdated
Comment thread chart/kube-flannel/templates/rbac.yaml
Comment thread chart/kube-flannel/templates/serviceaccount.yaml Outdated
Comment thread chart/kube-flannel/templates/daemonset.yaml Outdated
Comment thread chart/kube-flannel/templates/daemonset.yaml
Comment thread chart/kube-flannel/templates/_helpers.tpl
Comment thread chart/kube-flannel/templates/daemonset.yaml Outdated
@mgfritch
mgfritch force-pushed the allow-configuring-labels-annotations branch from 6cdccab to 922aad2 Compare May 19, 2026 23:11
lets users configure the labels and annotations in the Helm chart

Resolves flannel-io#2442
@brookelew
brookelew force-pushed the allow-configuring-labels-annotations branch from 922aad2 to 114926c Compare July 9, 2026 22:13
@brookelew

Copy link
Copy Markdown
Author

@mgfritch Sorry for the delay! I used some fromYamls to handle the merging in a more intuitive (to me at least) manner. Also wrote the tests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Comment thread chart/kube-flannel/templates/daemonset.yaml
Comment thread chart/kube-flannel/templates/daemonset.yaml
set:
flannel.rbac.labels:
rbac-non-overridden-label: "rbac label should not be overridden"
rbac-overrridden-label: "rbac label should be overridden"
rbac-overridden-label: "rbac label has been overridden"
flannel.rbac.annotations:
rbac-non-overridden-annotation: "rbac annotation should not be overridden"
rbac-overrridden-annotation: "rbac annotation should be overridden"
app: "flannel"
tier: "node"
app.kubernetes.io/name: {{ include "flannel.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
Comment on lines +4 to +10

{{- define "flannel.selectorLabels" -}}
app: "flannel"
tier: "node"
app.kubernetes.io/name: {{ include "flannel.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end -}}

@mgfritch mgfritch Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- define "flannel.selectorLabels" -}}
app: "flannel"
tier: "node"
app.kubernetes.io/name: {{ include "flannel.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end -}}
+{{/* Immutable selector labels */}}
{{- define "flannel.selectorLabels" -}}
app: "flannel"
{{- end -}}

sorry, this one is my fault, but adding labels here will cause the DaemonSet to fail during helm upgrade.

Can we move them into the common labels below?

Comment on lines +18 to +21
helm.sh/chart: {{ include "flannel.chart" . }}
app.kubernetes.io/version: {{ .Chart.AppVersion }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{ include "flannel.selectorLabels" . }}

@mgfritch mgfritch Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
helm.sh/chart: {{ include "flannel.chart" . }}
app.kubernetes.io/version: {{ .Chart.AppVersion }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{ include "flannel.selectorLabels" . }}
tier: "node"
helm.sh/chart: {{ include "flannel.chart" . }}
app.kubernetes.io/name: {{ include "flannel.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{ include "flannel.selectorLabels" . }}

Add common labels here instead of in the (immutable) selector labels

{{- include "flannel.selectorLabels" . | nindent 6 }}
template:
metadata:
{{- with (merge .Values.flannel.podLabels (include "flannel.labels" . | fromYaml)) }}

@mgfritch mgfritch Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- with (merge .Values.flannel.podLabels (include "flannel.labels" . | fromYaml)) }}
{{- with (merge (include "flannel.labels" . | fromYaml) .Values.flannel.podLabels) }}

reversing the order should ensure the the match labels cannot be overwritten by the pod labels

@@ -0,0 +1,95 @@
suite: test labels annotations overrides

@mgfritch mgfritch Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these!

Can we also add a test to ensure the selector labels are immutable? Feel free to reuse (or modify) this if it helps: mgfritch@61396f7

@mgfritch

mgfritch commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@mgfritch Sorry for the delay! I used some fromYamls to handle the merging in a more intuitive (to me at least) manner. Also wrote the tests.

@brookelew No worries! The fromYaml approach is much cleaner and thanks for writing the tests. I know we've gone through a few rounds on this, but I really appreciate your patience.

I think this should be ready to finally merge after resolving the last few small revisions above. This PR will be a really nice addition to modernize the charts! 😀

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.

Allow configuring labels and annotations

3 participants