Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

galmasi
Copy link

@galmasi galmasi commented Mar 20, 2024

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.

…rity context constraints for agents

Signed-off-by: George Almasi <[email protected]>
@sarroutbi
Copy link
Collaborator

sarroutbi commented Mar 21, 2024

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 make helm-deploy) to work appropriately.

I am not a Helm expert, so I doubt if it makes sense ...

@galmasi
Copy link
Author

galmasi commented Mar 27, 2024

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 -}}
Copy link
Collaborator

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:

Suggested change
{{- if .Values.serviceAccount.create -}}
{{- if .Values.serviceAccount.create -}}

Copy link
Author

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.

@galmasi
Copy link
Author

galmasi commented Apr 10, 2024

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 values.yaml:

global:
  service:
    agent:
      developer: false
      privileged: true
  openshift: true

@galmasi galmasi changed the title [DO NOT MERGE] Role bindings, service accounts, [openshift] security context constraints for keylime agents Role bindings, service accounts, [openshift] security context constraints for keylime agents Apr 10, 2024
@@ -8,6 +8,9 @@ spec:
selector:
matchLabels:
{{- include "agent.selectorLabels" . | nindent 6 }}
{{- if .Values.serviceAccount.create }}
serviceAccountName: {{ include "agent.serviceAccountName" . }}
{{- end }}
Copy link
Contributor

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" . }}
Copy link
Contributor

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?

Copy link
Contributor

@mheese mheese left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants