Split controller and webhook in two separate deployments

[bobertrublik]

Once the changes to split the controller and webhook logic in the db-operator codebase are merged we need to adjust the Helm chart.

Changes

• the dbUser CRD contained the field annotation twice. I removed one because it doesn't seem necessary to me.
• The controller and webhook are now two independent Deployments. I had to make some changes to the templates and values file
• The separation required changes to the template naming in the helpers.tpl file. The changes should give us flexibility in the future in case we want to set common labels or independent labels to the controller or webhook. I followed cert-manager's pattern here

Testing

I created a Postgres and MySQL dbInstance and they seem to be running correctly.

❯ k get dbinstances.kinda.rocks
NAME               PHASE     STATUS
generic-postgres   Running   true
generic-mysql      Running   true

I'm not sure if there are other ways to test it. 🙂

[allanger]

You also need to upgrade the version in Chart.yaml, it causes the linter error.
And I wonder, maybe it would make sense to add better labels everywhere, not only to deployments

[bobertrublik]

I also noticed that the webhook deployment had an empty spec:template:annotations field if .Values.annotations is not set. I fixed it in 6d59ca8.

Once we get rid of all bugs I will squash the commits. :)

[allanger]

Once we get rid of all bugs I will squash the commits. :)

In this repo we're using squash and merge anyway, so you don't have to do it manually

[bobertrublik]

I had to remove the two lines in c521e65 because they currently have no effect after my changes.

Also commit 2f801d8 is possibly out of scope regarding the removel of -sasuffix for this issue and I can drop it if you prefer.

The PR got a little bigger than anticipated with the whole restructuring business. 😅 Looking at charts like ArgoCD or cert-manager they mostly use name: {{ template "component.fullname" . }} in all manifests. Maybe we could do the same? Then we would be able to streamline all component naming including the mutating and validating webhook resources.

[allanger]

Also commit 2f801d8 is possibly out of scope regarding the removel of -sasuffix for this issue and I can drop it if you prefer.

To me, it looks fine.

[allanger]

Cool, thanks for fixing the unit test!

The only thing that is missing atm are those lables that were added to resources by this helper:

{{- define "db-operator.selectorLabels" -}}
app.kubernetes.io/name: {{ include "db-operator.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end -}}

It was added to all (or almost all) resources before, but now, as I see you're adding it to each deployment. Maybe we can define two vars in the helper, like

• controller.selectorLables
• webhook.selectorLables

And append all this labels to each resource by adding

labels: 
  {{- include "controller.selectorLabels" . | nindent 4 }}
# or
  {{- include "webhook.selectorLabels" . | nindent 4 }}

And the same applies to selectors:

selector:
  matchLabels:
      {{- include "controller.selectorLabels" . | nindent 6 }}
      # or
      {{- include "webhook.selectorLabels" . | nindent 6 }}

[bobertrublik]

Cool, thanks for fixing the unit test!

The only thing that is missing atm are those lables that were added to resources by this helper:

{{- define "db-operator.selectorLabels" -}}
app.kubernetes.io/name: {{ include "db-operator.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end -}}

It was added to all (or almost all) resources before, but now, as I see you're adding it to each deployment. Maybe we can define two vars in the helper, like

* controller.selectorLables

* webhook.selectorLables

And append all this labels to each resource by adding

labels: 
  {{- include "controller.selectorLabels" . | nindent 4 }}
# or
  {{- include "webhook.selectorLabels" . | nindent 4 }}

And the same applies to selectors:

selector:
  matchLabels:
      {{- include "controller.selectorLabels" . | nindent 6 }}
      # or
      {{- include "webhook.selectorLabels" . | nindent 6 }}

Look at the latest commits :D

[bobertrublik]

I guess there are two ways to approach this. Either create separate controller.selectorLabels and webhook.selectorLabels as you suggested or setting app.kubernetes.io/component: "controller" and app.kubernetes.io/component: "webhook" to each manifest.

What do you prefer?

[allanger]

I guess there are two ways to approach this. Either create separate controller.selectorLabels and webhook.selectorLabels as you suggested or setting app.kubernetes.io/component: "controller" and app.kubernetes.io/component: "webhook" to each manifest.

What do you prefer?

I think we can add to each now

[bobertrublik]

Sorry now the CI should work.

[bobertrublik]

I think I fixed the last broken file now. I'm not sure what the issue is regarding Error from server (NotFound): namespaces "operator" not found.

[allanger]

I think I fixed the last broken file now. I'm not sure what the issue is regarding Error from server (NotFound): namespaces "operator" not found.

I think it was also caused by the broken file. The namespace wasn't created because chart couldn't be installed, and it's creating the namespace

[allanger]

To me, it looks good. If you don't want to change anything else, I'll merge

[bobertrublik]

Fine for me too, thanks for your help debugging my PR! 🙂 I‘ll update the original post with the current changes later.

[allanger]

Fine for me too, thanks for your help debugging my PR! 🙂

Thanks for your contribution