Role bindings, service accounts, [openshift] security context constraints for keylime agents#76
Role bindings, service accounts, [openshift] security context constraints for keylime agents#76galmasi wants to merge 2 commits intokeylime:mainfrom
Conversation
…rity context constraints for agents Signed-off-by: George Almasi <gheorghe@us.ibm.com>
|
Hello George. Just a minor comment. I had to perform next change: $ git diff
diff --git a/build/helm/keylime/charts/keylime-agent/templates/daemonset.yaml b/build/helm/keylime/charts/keylime-agent/templates/daemonset.yaml
index fdcb328..19f62e8 100644
--- a/build/helm/keylime/charts/keylime-agent/templates/daemonset.yaml
+++ b/build/helm/keylime/charts/keylime-agent/templates/daemonset.yaml
@@ -8,7 +8,7 @@ spec:
selector:
matchLabels:
{{- include "agent.selectorLabels" . | nindent 6 }}
- {{- if .Values.serviceAccount.create -}}
+ {{- if .Values.serviceAccount.create }}
serviceAccountName: {{ include "agent.serviceAccountName" . }}
{{- end }}
template:
@@ -24,7 +24,7 @@ spec:
imagePullSecrets:
{{- toYaml . | nindent 8 }}
{{- end }}
- {{- if .Values.serviceAccount.create -}}
+ {{- if .Values.serviceAccount.create }}... for the deployment (through I am not a Helm expert, so I doubt if it makes sense ... |
|
you are most likely correct. I have not had a chance to deploy my own code after the change i made, so this could well be a stupid typo. I'm fixing it now. |
| selector: | ||
| matchLabels: | ||
| {{- include "agent.selectorLabels" . | nindent 6 }} | ||
| {{- if .Values.serviceAccount.create -}} |
There was a problem hiding this comment.
There might also be an indentation issue here:
| {{- if .Values.serviceAccount.create -}} | |
| {{- if .Values.serviceAccount.create -}} |
There was a problem hiding this comment.
I believe the spacing of jinja conditionals does not matter, however the spacing of stanzas covered under jinja conditionals does. That being said, I believe that the service account specification in the case you are pointing out should be associated with the spec not the selector. I should probably move the declaration to make it less ambiguous or easier to read, but experimentation seems to confirm that it's at least correct. Let me know if you think otherwise.
Signed-off-by: George Almasi <gheorghe@us.ibm.com>
|
Thank you for the comments. I added some typo fixes and confirmed that on openshift 4.12 and 4.13 this helm chart deploys correctly when agents are in privileged mode. Relevant excerpt from my |
| {{- include "agent.selectorLabels" . | nindent 6 }} | ||
| {{- if .Values.serviceAccount.create }} | ||
| serviceAccountName: {{ include "agent.serviceAccountName" . }} | ||
| {{- end }} |
There was a problem hiding this comment.
serviceAccountName should be part of the spec, and I can see it already below here. Or is there something new going on which I'm missing?
| selector: | ||
| matchLabels: | ||
| {{- include "agentplugin.selectorLabels" . | nindent 6 }} | ||
| serviceAccountName: {{ include "agent.serviceAccountName" . }} |
There was a problem hiding this comment.
serviceAccountName should be part of the spec, and I can see it already below here. Or is there something new going on which I'm missing?
mheese
left a comment
There was a problem hiding this comment.
Looks good/correct except for the new introduction of the serviceAccountName at places that I don't understand 👍 Please clarify/educate me or remove those please.
This is the initial commit. It's not ready to merge.
Only works for OCP when agents are in privileged mode (have not found the correct SCC for unprivileged agents).
Still unsolved is the exact group matching in kubernetes for unprivileged containers.