Skip to content

Rework labels in Opensearch chart to match standard recommendations #37

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

Merged
merged 2 commits into from
Oct 19, 2021
Merged

Rework labels in Opensearch chart to match standard recommendations #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7546c4e
feat: rework labels to match standard recommendations
smlx Aug 6, 2021
6e067fe
chore: bump opensearch chart version
smlx Oct 7, 2021
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
2 changes: 1 addition & 1 deletion charts/opensearch/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ type: application
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 1.1.0
version: 1.2.0

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
Expand Down
45 changes: 40 additions & 5 deletions charts/opensearch/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,52 @@
Expand the name of the chart.
*/}}
{{- define "opensearch.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
{{- end -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "opensearch.fullname" -}}
{{- $name := default .Chart.Name .Values.nameOverride -}}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
{{- end -}}
{{- if contains .Chart.Name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name .Chart.Name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}

{{/*
Create chart name and version as used by the chart label.
*/}}
{{- define "opensearch.chart" -}}
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Common labels
*/}}
{{- define "opensearch.labels" -}}
helm.sh/chart: {{ include "opensearch.chart" . }}
{{ include "opensearch.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
Copy link
Collaborator

@DandyDeveloper DandyDeveloper Sep 15, 2021

Choose a reason for hiding this comment

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

This will break the statefulset if you try to deploy with an upgraded version, won't it?

Copy link
Contributor Author

@smlx smlx Sep 15, 2021

Choose a reason for hiding this comment

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

I did test updating the appVersion and it seems to work. How do you see it breaking?

Copy link
Member

@TheAlgo TheAlgo Oct 17, 2021

Choose a reason for hiding this comment

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

@smlx Can you resolve this conversation?

{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
app.kubernetes.io/component: {{ include "opensearch.uname" . }}
{{- with .Values.labels }}
{{ toYaml . }}
{{- end }}
{{- end }}

{{/*
Selector labels
*/}}
{{- define "opensearch.selectorLabels" -}}
app.kubernetes.io/name: {{ include "opensearch.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}

{{- define "opensearch.uname" -}}
{{- if empty .Values.fullnameOverride -}}
Expand Down
5 changes: 1 addition & 4 deletions charts/opensearch/templates/configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ kind: ConfigMap
metadata:
name: {{ template "opensearch.uname" . }}-config
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- include "opensearch.labels" . | nindent 4 }}
data:
{{- range $configName, $configYaml := .Values.config }}
{{ $configName }}: |
Expand Down
4 changes: 1 addition & 3 deletions charts/opensearch/templates/ingress.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ kind: Ingress
metadata:
name: {{ $fullName }}
labels:
app: {{ .Chart.Name }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
{{- include "opensearch.labels" . | nindent 4 }}
{{- with .Values.ingress.annotations }}
annotations:
{{ toYaml . | indent 4 }}
Expand Down
2 changes: 2 additions & 0 deletions charts/opensearch/templates/networkpolicy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: {{ template "opensearch.uname" . }}-opensearch-net
labels:
{{- include "opensearch.labels" . | nindent 4 }}
spec:
ingress:
- from:
Expand Down
4 changes: 3 additions & 1 deletion charts/opensearch/templates/poddisruptionbudget.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
name: "{{ template "opensearch.uname" . }}-pdb"
labels:
{{- include "opensearch.labels" . | nindent 4 }}
spec:
maxUnavailable: {{ .Values.maxUnavailable }}
selector:
matchLabels:
app: "{{ template "opensearch.uname" . }}"
{{- include "opensearch.selectorLabels" . | nindent 6 }}
{{- end }}
5 changes: 1 addition & 4 deletions charts/opensearch/templates/podsecuritypolicy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ kind: PodSecurityPolicy
metadata:
name: {{ default $fullName .Values.podSecurityPolicy.name | quote }}
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
app: {{ $fullName | quote }}
{{- include "opensearch.labels" . | nindent 4 }}
spec:
{{ toYaml .Values.podSecurityPolicy.spec | indent 2 }}
{{- if .Values.sysctl.enabled }}
Expand Down
6 changes: 1 addition & 5 deletions charts/opensearch/templates/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@ apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: {{ $fullName | quote }}
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
app: {{ $fullName | quote }}
{{- include "opensearch.labels" . | nindent 4 }}
rules:
- apiGroups:
- extensions
Expand Down
5 changes: 1 addition & 4 deletions charts/opensearch/templates/rolebinding.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ kind: RoleBinding
metadata:
name: {{ $fullName | quote }}
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
app: {{ $fullName | quote }}
{{- include "opensearch.labels" . | nindent 4 }}
subjects:
- kind: ServiceAccount
{{- if eq .Values.rbac.serviceAccountName "" }}
Expand Down
4 changes: 1 addition & 3 deletions charts/opensearch/templates/securityconfig.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ metadata:
name: {{ include "opensearch.uname" . }}-securityconfig
namespace: {{ .Release.Namespace }}
labels:
app: {{ .Chart.Name }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
{{- include "opensearch.labels" . | nindent 4 }}
type: Opaque
stringData:
{{- range $key, $val := .Values.securityConfig.config.data }}
Expand Down
16 changes: 4 additions & 12 deletions charts/opensearch/templates/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ metadata:
name: {{ template "opensearch.uname" . }}
{{- end }}
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- include "opensearch.labels" . | nindent 4 }}
{{- if .Values.service.labels }}
{{ toYaml .Values.service.labels | indent 4}}
{{- end }}
Expand All @@ -20,9 +17,7 @@ metadata:
spec:
type: {{ .Values.service.type }}
selector:
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- include "opensearch.selectorLabels" . | nindent 4 }}
ports:
- name: {{ .Values.service.httpPortName | default "http" }}
protocol: TCP
Expand Down Expand Up @@ -53,10 +48,7 @@ metadata:
name: {{ template "opensearch.uname" . }}-headless
{{- end }}
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- include "opensearch.labels" . | nindent 4 }}
{{- if .Values.service.labelsHeadless }}
{{ toYaml .Values.service.labelsHeadless | indent 4 }}
{{- end }}
Expand All @@ -67,7 +59,7 @@ spec:
# Create endpoints also if the related pod isn't ready
publishNotReadyAddresses: true
selector:
app: "{{ template "opensearch.uname" . }}"
{{- include "opensearch.selectorLabels" . | nindent 4 }}
ports:
- name: {{ .Values.service.httpPortName | default "http" }}
port: {{ .Values.httpPort }}
Expand Down
7 changes: 2 additions & 5 deletions charts/opensearch/templates/serviceaccount.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,10 @@ metadata:
{{- else }}
name: {{ .Values.rbac.serviceAccountName | quote }}
{{- end }}
labels:
{{- include "opensearch.labels" . | nindent 4 }}
annotations:
{{- with .Values.rbac.serviceAccountAnnotations }}
{{- toYaml . | nindent 4 }}
{{- end }}
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
app: {{ $fullName | quote }}
{{- end -}}
42 changes: 16 additions & 26 deletions charts/opensearch/templates/statefulset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,14 @@ kind: StatefulSet
metadata:
name: {{ template "opensearch.uname" . }}
labels:
heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- range $key, $value := .Values.labels }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- include "opensearch.labels" . | nindent 4 }}
Copy link
Collaborator

@DandyDeveloper DandyDeveloper Sep 15, 2021

Choose a reason for hiding this comment

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

See previous comment, if I remember correctly. Statefulsets will not allow updates to the spec other than the template, replicas and updateStrat.

So, if someone upgraded this would break.

Copy link
Contributor Author

@smlx smlx Sep 15, 2021

Choose a reason for hiding this comment

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

The spec starts a few lines below - I think metadata is not immutable.

Copy link
Collaborator

@DandyDeveloper DandyDeveloper Sep 20, 2021

Choose a reason for hiding this comment

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

@smlx Let me triple check this right now.

annotations:
majorVersion: "{{ include "opensearch.majorVersion" . }}"
spec:
serviceName: {{ template "opensearch.uname" . }}-headless
selector:
matchLabels:
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- include "opensearch.selectorLabels" . | nindent 6 }}
Copy link

@acjohnson acjohnson Dec 14, 2021

Choose a reason for hiding this comment

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

This change breaks upgrades for statefulsets. I just ran into this upgrading from 1.0.8 to 1.5.4

Copy link

@acjohnson acjohnson Dec 14, 2021

Choose a reason for hiding this comment

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

Error: UPGRADE FAILED: cannot patch "opensearch-1-master" with kind StatefulSet: StatefulSet.apps "opensearch-1-master" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden

Copy link
Contributor Author

@smlx smlx Dec 15, 2021

Choose a reason for hiding this comment

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

Yes sorry this PR should have been a major version bump.

Copy link
Contributor

@mprimeaux mprimeaux Dec 15, 2021

Choose a reason for hiding this comment

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

Agreed.

Copy link
Collaborator

@DandyDeveloper DandyDeveloper Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@smlx I was right! #37 (comment)

Copy link
Contributor Author

@smlx smlx Dec 15, 2021

Choose a reason for hiding this comment

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

yes I didn't consider upgrading from previous versions - only going forward from this change.

Copy link

@acjohnson acjohnson Dec 15, 2021

Choose a reason for hiding this comment

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

I was able to delete the statefulset and rerun helm upgrade as the reclaim policy of my PVCs were set to Retain so thankfully there was no data loss... be sure to check this first to avoid accidental deletion... ☠️

Copy link
Contributor Author

@smlx smlx Dec 15, 2021

Choose a reason for hiding this comment

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

thanks @acjohnson for sharing your workaround and sorry for the breakage! 🙏

replicas: {{ .Values.replicas }}
podManagementPolicy: {{ .Values.podManagementPolicy }}
updateStrategy:
Expand All @@ -30,12 +22,7 @@ spec:
name: {{ template "opensearch.uname" . }}
{{- if .Values.persistence.labels.enabled }}
labels:
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- range $key, $value := .Values.labels }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- include "opensearch.labels" . | nindent 8 }}
{{- end }}
{{- with .Values.persistence.annotations }}
annotations:
Expand All @@ -61,12 +48,7 @@ spec:
metadata:
name: "{{ template "opensearch.uname" . }}"
labels:
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}"
app: "{{ template "opensearch.uname" . }}"
{{- range $key, $value := .Values.labels }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- include "opensearch.labels" . | nindent 8 }}
annotations:
{{- range $key, $value := .Values.podAnnotations }}
{{ $key }}: {{ $value | quote }}
Expand Down Expand Up @@ -113,10 +95,14 @@ spec:
requiredDuringSchedulingIgnoredDuringExecution:
- labelSelector:
matchExpressions:
- key: app
- key: app.kubernetes.io/instance
operator: In
values:
- "{{ template "opensearch.uname" .}}"
- {{ .Release.Name }}
- key: app.kubernetes.io/name
operator: In
values:
- {{ include "opensearch.name" . }}
topologyKey: {{ .Values.antiAffinityTopologyKey }}
{{- else if eq .Values.antiAffinity "soft" }}
podAntiAffinity:
Expand All @@ -126,10 +112,14 @@ spec:
topologyKey: {{ .Values.antiAffinityTopologyKey }}
labelSelector:
matchExpressions:
- key: app
- key: app.kubernetes.io/instance
operator: In
values:
- {{ .Release.Name }}
- key: app.kubernetes.io/name
operator: In
values:
- "{{ template "opensearch.uname" . }}"
- {{ include "opensearch.name" . }}
{{- end }}
{{- with .Values.nodeAffinity }}
nodeAffinity:
Expand Down