Feature/enrich grafana controller manager labels

[mikaayil]

I've enhanced the controller manager with additional labels.
I also executed "make all" before creating the PR and tested that the controller-manager
pod has the new labels in my kind cluster.

While working on this PR, I noticed that the bundle wasn't updated after the commit from
@HubertStefanski (Commit: 171038b Title: Update go dependencies).

Because of this, I first executed "make bundle" before implementing my changes. So,
my initial commit "3eb81dc" was simply to build the new bundle. Am I missing something
when updating the bundle? My assumption is that after changing any CRDs or the CSV in the
config/ folder, the bundle should also be updated.

Regarding the new labels:

I think it's a good idea to talk about whether the labels I added are a good fit. So, I
just want to start the discussion with this PR. I didn't remove the label "control-plane:
controller-manager". Should we consider removing this label in the long run?

Out of curiosity: What was the original purpose of setting this label in the first place?

[CLAassistant]

All committers have signed the CLA.

[NissesSenap]

First of all, thanks for your PR.

When creating the latest version of the operator, I don't think we gave the operator label any thought, so from my point of view you can remove control-plane: controller-manager.

The reason why the makefil generates new config is that we no longer keep the OLM yaml in sync in this repo, this to be able to support disconnected mode in an easier way. See #1234 for more info.

Don't have time to clone down your branch right now, will this affect the kustomize yaml in any way?

Is there any best practice around operator labels in OLM?
To me, it feels a bit strange that we have to add app.kubernetes.io/managed-by: olm and it wouldn't surprise me if OLM did this automatically.

But I don't have access to OCP clusters anymore, so I can't test.

[NissesSenap]

Would prefer not to have version as a part of this. Then we will have to add logic to update the version.
I also don't think we gain anything by adding it.

[mikaayil]

Hey @NissesSenap,

ah, okay, thanks. Good to know.

Regarding best practices for operator labels:

I just found some information on labels for multiple architectures [1]. Besides that, I have
been just using the Kubernetes documentation for recommended labels [2].

In the next days, when I find some time, I'll check if other operators have interesting
labels that seem like best practices.

Regarding your question about whether OLM automatically creates the label app.kubernetes.io/managed-by: olm:

So, I tried the operator in my Kind cluster, which includes the OLM initialized with
bin/operator-sdk olm install.

The OLM didn't create any default labels for the deployment. So, I guess not. But I will
investigate more when I have more time.

(Why do you need an OCP cluster for this? Isn't the OLM the same, or am I missing something?)

Regarding your question about whether the changes will affect the kustomize files:

I don't think it affects the kustomize files in any way.

The question is whether we should also add the new labels as defaults to the kustomize
deployment. The labels for the controller manager currently look like this:

# deploy/kustomize/base/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: grafana-operator-controller-manager
  namespace: default
  labels:
    app: grafana-operator-controller-manager <--
spec:
  replicas: 1
  selector:
    matchLabels:
      control-plane: grafana-operator-controller-manager

Another Point: Should we adjust the Helm Template:

Should we also add the additional labels to the Helm template, or should we leave it to
the user to define additional labels with .Values.additionalLabels? It currently only
contains the following configuration for the labels:

# deploy/helm/grafana-operator/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ include "grafana-operator.fullname" . }}
  namespace: {{ .Release.Namespace | quote }}
  labels:
    {{- include "grafana-operator.labels" . | nindent 4 }}
    {{- with .Values.additionalLabels }}
    {{- toYaml . | nindent 4 }}
    {{- end }}

# deploy/helm/grafana-operator/templates/_helpers.tpl
{{/*
Common labels
*/}}
{{- define "grafana-operator.labels" -}}
helm.sh/chart: {{ include "grafana-operator.chart" . }}
{{ include "grafana-operator.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}

[1] https://olm.operatorframework.io/docs/advanced-tasks/ship-operator-supporting-multiarch/
[2] https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/

[NissesSenap]

Hi @mikaayil , sorry for the slow answer. Thanks for a well described answer to all my questions.

As you point out, it's of course possible to install OLM in k8s, I just have never tried it.
I think it would be really nice to add the Kubernetes labels to Kustomize, helm and OLM just as you point out.

And I think the labels as described in k8s docs would be good to follow.
I would suggest that we skip app.kubernetes.io/version in kustomize and OLM since this will create painful stuff in our CI.

As you say add app.kubernetes.io/managed-by: olm for the OLM part also sounds like a good idea.

Could you update the PR to reflect this and I'm happy to merge it.

[mikaayil]

Hey @NissesSenap,

thank you as well + you're welcome.

I've added the labels to both the Kustomize deployment and the bundle configuration.

Adjusting the Helm deployment wasn't necessary since it already comes with appropriate
labels by default.

$ helm install --dry-run grafana-operator deploy/helm/grafana-operator/

NAME: grafana-operator
LAST DEPLOYED: Sun Jan 21 00:03:20 2024
NAMESPACE: default
STATUS: pending-install
REVISION: 1
TEST SUITE: None
HOOKS:
MANIFEST:
---
...
  labels:
    helm.sh/chart: grafana-operator-0.1.2
    app.kubernetes.io/name: grafana-operator
    app.kubernetes.io/instance: grafana-operator
    app.kubernetes.io/version: "v5.6.0" # The version is by default defined in the `Chart.yaml`
    app.kubernetes.io/managed-by: Helm

I decided to only add the label app.kubernetes.io/name: grafana-operator and, depending
on the deployment tool used, app.kubernetes.io/managed-by: olm|helm|kustomize. What are
your thoughts on this?

I believe the other labels might be too much after reading the Helm documentation 1,
which mentions app.kubernetes.io/version, app.kubernetes.io/component, and
app.kubernetes.io/part-of as optional.

[NissesSenap]

Nice work @mikaayil ,
I have run make bundle/redhat to update the bundle files in the new way. It's easy to know and it's something we only do when cutting new releases. But to minimize the changes, it felt like a good thing.

I decided to remove

commonLabels:
  app.kubernetes.io/managed-by: kustomize

from kustomize, since kustomize isn't a deployment tool, it's a templating tool. In my mind, this field should be something like argocd or flux.