itzg / minecraft-server-charts

MIT License
279 stars 144 forks source link

[minecraft] Service Label and Annotations #235

Open Anghille opened 1 month ago

Anghille commented 1 month ago

It is usually usefull to be able to add labels and annotations to services. One usecase might be to use cilium L2LoadbalancerIpPool, which usually needs labels to point to the pool that will hand ips to the service of type Loadbalancer and annotations such as io.cilium/lb-ipam-ips which fix the IP of the loadbalancer.

I tried to use the serviceLabels: {} and serviceAnnotations: {} fields as it is declared here in the templates:

apiVersion: v1
kind: Service
metadata:
  name: {{ template "minecraft.fullname" . }}
  namespace: {{ .Release.Namespace }}
  labels:
    app: {{ template "minecraft.fullname" . }}
    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
    release: "{{ .Release.Name }}"
    heritage: "{{ .Release.Service }}"
    app.kubernetes.io/name: "{{ .Chart.Name }}"
    app.kubernetes.io/instance: {{ template "minecraft.fullname" . }}
    app.kubernetes.io/version: "{{ .Chart.Version }}"
  {{- if .Values.serviceLabels }}
    {{- range $key, $value := .Values.serviceLabels}} #<--------- HERE
    {{ $key }}: {{ $value | quote }}
    {{- end }}
  {{- end }}
  annotations:
{{ toYaml .Values.serviceAnnotations | indent 4 }} #<--------- HERE

But despites my efforts, I cannot add labels and annotations to this service. I managed to fix the bug by patching the service through kustomize for know.

I didnt found anyone reporting this. Maybe I am doing something wrong ?

itzg commented 1 month ago

Please clarify what your values looked like on command line or values file. Also clarify what happened when you tried.

Anghille commented 1 month ago

Sure thing:

My values looks like this (using latest 4.23.2 helmRelease):

serviceLabels:
  kubernetes.io/type: external

serviceAnnotations:
  io.cilium/lb-ipam-ips: x.x.x.x

minecraftServer:
  # This must be overridden, since we can't accept this for the user.
  eula: "TRUE"
  [...]
  # DEPRECATED: use top-level rconServiceAnnotations instead
  serviceAnnotations: {}
  serviceType: ClusterIP
  servicePort: 25565
 [...]

I also tried (since the comment says "DEPRECATED: use top-level rconServiceAnnotations instead"):

rconServiceLabels:
  kubernetes.io/type: external

rconServiceAnnotations:
  io.cilium/lb-ipam-ips: x.x.x.x

minecraftServer:
  # This must be overridden, since we can't accept this for the user.
  eula: "TRUE"
  [...]
  # DEPRECATED: use top-level rconServiceAnnotations instead
  serviceAnnotations: {}
  serviceType: ClusterIP
  servicePort: 25565
 [...]

But since no labels nor annotation were applied (a kubectl describe svc minecraftminecraft -n minecraft yield none of the custom labels and annotations I added).

itzg commented 1 month ago

@eaglesemanation I was wondering if you had any advice about this since the feature was added with #146

eaglesemanation commented 1 month ago

Honestly not sure what's the issue here. Just pulled most recent commit from master branch, modified serviceLabels and serviceAnnotations in values.yaml without touching anything else, and ran helm template .. Here is the result:

# Source: minecraft/templates/minecraft-svc.yaml
apiVersion: v1
kind: Service
metadata:
  name: release-name-minecraft
  labels:
    app: release-name-minecraft
    chart: "minecraft-4.23.2"
    release: "release-name"
    heritage: "Helm"
    app.kubernetes.io/name: "minecraft"
    app.kubernetes.io/instance: release-name-minecraft
    app.kubernetes.io/version: "4.23.2"
    kubernetes.io/type: "external"
  annotations:
    io.cilium/lb-ipam-ips: x.x.x.x
spec:
  type: ClusterIP
  ports:
  - name: minecraft
    port: 25565
    targetPort: minecraft
    protocol: TCP
  selector:
    app: release-name-minecraft
itzg commented 1 month ago

Thanks for checking @eaglesemanation . Yeah, from reviewing the code it seemed like it would have worked fine.

@Anghille rather than inspecting the deployed services, can you confirm using helm template or dry run of upgrade?

cpfarhood commented 3 weeks ago

I'm also seeing this as of the current version when attempting to apply similar service annotations for MetalLB BGP.

cpfarhood commented 3 weeks ago

Nevermind! service annotations just needs to be indented to the same level as minecraftserver and it works.

Anghille commented 3 weeks ago

Sorry if I am not responding to this, I am currently quite busy but I will soon confirm it works and that is was, as expected, a problem between the chair and the keyboard indeed