-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Role bindings, service accounts, [openshift] security context constraints for keylime agents #76
base: main
Are you sure you want to change the base?
Conversation
…rity context constraints for agents Signed-off-by: George Almasi <[email protected]>
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. |
@@ -8,6 +8,9 @@ spec: | |||
selector: | |||
matchLabels: | |||
{{- include "agent.selectorLabels" . | nindent 6 }} | |||
{{- if .Values.serviceAccount.create -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might also be an indentation issue here:
{{- if .Values.serviceAccount.create -}} | |
{{- if .Values.serviceAccount.create -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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
|
@@ -8,6 +8,9 @@ spec: | |||
selector: | |||
matchLabels: | |||
{{- include "agent.selectorLabels" . | nindent 6 }} | |||
{{- if .Values.serviceAccount.create }} | |||
serviceAccountName: {{ include "agent.serviceAccountName" . }} | |||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -167,6 +172,7 @@ spec: | |||
selector: | |||
matchLabels: | |||
{{- include "agentplugin.selectorLabels" . | nindent 6 }} | |||
serviceAccountName: {{ include "agent.serviceAccountName" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.