Implement Security Contexts for Kubernetes MCP Servers #304
Conversation
There was a problem hiding this comment.
Apologies for not reviewing this sooner @teodor-yanev! Was trying to get the operator announcement out ready for today!
| groupID := int64(1000) | ||
| container.WithSecurityContext( | ||
| corev1apply.SecurityContext(). | ||
| WithAllowPrivilegeEscalation(false). |
There was a problem hiding this comment.
We able to add defaults for privileged as well?
If privileged is set to false, by default allowPrivilegeEscalation will also be false, if you wanted to add allowPrivilegeEscalation explicitly though, i don't mind.
| @@ -1019,6 +1029,19 @@ func configureContainer( | |||
| WithStdin(attachStdio). | |||
| WithTTY(false). | |||
| WithEnv(envVars...) | |||
|
|
|||
| // Add container security context if not already present | |||
| if container.SecurityContext == nil { | |||
There was a problem hiding this comment.
We may want to do other checks too. Currently if users have specified a securityContext but say only added allowPrivilegeEscalation: false, this check technically wouldn't be true and we'd skip over it. We might want to add more checks so that if securityContext != nil, we check for specific fields and add the sensible defaults if they aren't provided.
For example:
User specifies:
securityContext:
allowPrivilegeEscalation: falseWe should read that but apply
securityContext:
allowPrivilegeEscalation: false
runAsUser: 1000
runAsGroup: 1000
...
....If that makes sense
| @@ -19,10 +19,18 @@ spec: | |||
| app.kubernetes.io/name: toolhive | |||
| spec: | |||
| serviceAccountName: toolhive | |||
| securityContext: | |||
There was a problem hiding this comment.
Fun fact, now we've released our super duper funky wunky operator, we should probably deprecate these old YAMLs. Or at least make a decision on whether we want to support both operator and non operator installs!
| @@ -968,6 +968,16 @@ func ensurePodTemplateConfig( | |||
| podTemplateSpec.Spec = podTemplateSpec.Spec.WithRestartPolicy(corev1.RestartPolicyAlways) | |||
| } | |||
|
|
|||
| // Add pod-level security context if not already present | |||
| if podTemplateSpec.Spec.SecurityContext == nil { | |||
There was a problem hiding this comment.
Same comment below as doing more checks
…ernetes-MCP-Servers
Closes #92