Add support for the spire-controller-manager #71
Conversation
marcofranssen
force-pushed
the
main
branch
2 times, most recently
from
3d59407
to
0f84633
Compare
developer-guy
force-pushed
the
feature/68
branch
2 times, most recently
from
4118a92
to
e7331de
Compare
|
everything seems fine to me, PTAL @marcofranssen |
charts/spire/crds/spire.spiffe.io_clusterfederatedtrustdomains.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Gave some inline feedback, but also have a general request which I also didn't clearly put in the issue.
Can we do the following
k8sregistrar:
enabled: false
spire-controller-manager:
enabled: true
So we can first have a couple of releases where people can still fall back on the old stuff.
I also think we might want to have a dedicated file, service-account for the spire-controller-manager to have it as a separate deployment with its own rbac.
In general first rebase on main, so you have a few new Values like the socketPath to be reused
| kind: CustomResourceDefinition | ||
| metadata: | ||
| annotations: | ||
| controller-gen.kubebuilder.io/version: v0.8.0 |
There was a problem hiding this comment.
should this be configurable? What and where is this version coming from? Could people use another version?
| @@ -11,12 +11,32 @@ rules: | |||
| resources: ["tokenreviews"] | |||
| verbs: ["get", "create"] | |||
| - apiGroups: [""] | |||
| resources: ["pods", "nodes"] | |||
| resources: ["pods", "nodes", "namespaces"] | |||
There was a problem hiding this comment.
Why does the server now need namespace access?
There was a problem hiding this comment.
There was a problem hiding this comment.
Isn't that only for the controller manager?
developer-guy
force-pushed
the
feature/68
branch
from
e7331de
to
64c0b98
Compare
totally agree with you! I've updated the helm templates according to your feedback! |
developer-guy
force-pushed
the
feature/68
branch
from
64c0b98
to
68d74ac
Compare
| @@ -75,7 +75,28 @@ spec: | |||
| periodSeconds: 5 | |||
| resources: | |||
| {{- toYaml .Values.server.resources | nindent 12 }} | |||
| - name: {{ .Chart.Name }}-workload-registrar | |||
| {{- if .Values.controllerManager.enabled }} | |||
There was a problem hiding this comment.
Can we isolate this into a separate Deployment/file? I assume it doesn't need persistence from code below. In case it needs persistence, please make it a StatefulSet.
Probably also include the wait-for-it initContainer to ensure spire server is up and running when launching the controller manager.
There was a problem hiding this comment.
I've created another deployment file for spire-controller-manager
developer-guy
force-pushed
the
feature/68
branch
2 times, most recently
from
6a22094
to
8ae020a
Compare
developer-guy
requested review from
marcofranssen
and removed request for
marcofranssen
| - name: {{ printf "%s-%s" $fullname .name }} | ||
| {{- end }} | ||
| {{- end }} | ||
| serviceAccountName: {{ include "spire.serviceAccountName" . }}-agent |
There was a problem hiding this comment.
Should we create a dedicated service account and RBAC?
There was a problem hiding this comment.
I'm not sure about that. It seems that it can cause duplicate the same CR/CRBs for the server for the controller manager too 🤔 Because I really don't know what are the exact permissions we need to have for controller manager
charts/spire/templates/spire-controller-manager-deployment.yaml
Outdated
Show resolved
Hide resolved
marcofranssen
changed the title
developer-guy
force-pushed
the
feature/68
branch
2 times, most recently
from
a9f362a
to
19a960a
Compare
developer-guy
force-pushed
the
feature/68
branch
2 times, most recently
from
69ffd43
to
cf68694
Compare
Signed-off-by: Batuhan Apaydın <[email protected]>
developer-guy
force-pushed
the
feature/68
branch
from
cf68694
to
84d3233
Compare
Signed-off-by: Batuhan Apaydın [email protected]
Fixes #68
/cc @marcofranssen