Refactor deploymentForMCPServer for platform detection. (#1063) (#1285) #1500
Conversation
… (stacklok#1285) Signed-off-by: Roddie Kieley <rkieley@redhat.com> Co-authored-by: Cursor claude-4-sonnet <cursor@redhat.com>
RoddieKieley
force-pushed
the
issue1063-refactor-deploymentForMCPServer
branch
from
e94903e to
2961795
Compare
| // detectPlatform detects the Kubernetes platform type (Kubernetes vs OpenShift) | ||
| // It uses sync.Once to ensure the detection is only performed once and cached | ||
| func (r *MCPServerReconciler) detectPlatform(ctx context.Context) (kubernetes.Platform, error) { | ||
| var err error |
There was a problem hiding this comment.
this might not even be a problem, but wouldn't it be better to also cache the detection error from DetectPlatform and return the cached error on subsequent calls?
There was a problem hiding this comment.
actually, this might be correct as if the call fails, we just keep assuming kubernetes...
| WithRunAsUser(int64(1000)). | ||
| WithRunAsGroup(int64(1000)), | ||
| ) | ||
| securityBuilder := NewSecurityContextBuilder(platform) |
There was a problem hiding this comment.
nit for readability: cine the builder and applying the config is needed for both if and else could we put the declaration earlier?
| container.SecurityContext.Capabilities = &corev1apply.CapabilitiesApplyConfiguration{ | ||
| Drop: []corev1.Capability{"ALL"}, | ||
| // For OpenShift, override certain fields even if they exist | ||
| if platform == PlatformOpenShift { |
There was a problem hiding this comment.
I wonder if we could have an if-else or switch for the platform that would skip setting the fields that OCP nils anyway. But that might be just personal preference and doesn't affect functionality
|
It will be so nice to do all of this code in one place when we implement #1497 😄 |
The PR#1301 for #1285 set the security context for ProxyRunner deployments without taking into account kubernetes platform differences and detection as was introduced for #1063 via PR#1253.
This addresses that concern by refactoring the code dealing with security context, encapsulating that specific functionality better than was previously done for either #1063 or #1285.
Also while this should not directly impact #1483, assuming the operator was running it would have required manual changes to the MCPServer instances Deployment to remove the incorrectly set runAsUser, runAsGroup, and fsGroup values once it was so mentioning here.
I am marking this as Draft; while it works well here there is a good bit of code addition and change so having @jhrozek and/or @ChrisJBurns take a look as a follow up to #1063 is probably in order.
Feel free to modify/update as require and feedback welcome.