-
Notifications
You must be signed in to change notification settings - Fork 20
Discussion: CRD refactoring #36
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,248 @@ | ||
| apiVersion: mcp.x-k8s.io/v1alpha1 | ||
| kind: MCPServer | ||
| metadata: | ||
| name: example | ||
| spec: | ||
| source: | ||
| type: ContainerImage | ||
| containerImage: | ||
| ref: ghcr.io/example/server:latest | ||
| config: | ||
| port: ... | ||
| arguments: | ||
| - ... | ||
| env: | ||
| - ... | ||
| envFrom: | ||
| - ... | ||
| storage: | ||
| - mountPath: ... | ||
| type: ConfigMap | ||
| configMap: | ||
| ... | ||
| runtime: | ||
| security: | ||
| serviceAccountName: ... | ||
| podSecurityContext: | ||
| ... | ||
| securityContext: | ||
| ... | ||
| resources: | ||
| requests: | ||
| ... | ||
| replicas: 1 | ||
| scheduling: | ||
| ... | ||
| health: | ||
| ... | ||
| networking: | ||
| ... | ||
| --- | ||
| apiVersion: mcp.x-k8s.io/v1alpha1 | ||
| kind: MCPServer | ||
| metadata: | ||
| name: example | ||
| spec: | ||
|
|
||
| # The source section defines where the MCP server's container image (or other source types in the future) is located. | ||
| source: | ||
| type: ContainerImage # Start with this, might grow to Git, OCI, MCP catalog entry, etc. | ||
| # having the image details nested under a field named "containerImage" allows us to add more fields related to | ||
| # the image in the future without breaking compatibility. | ||
| # ... instead of having the image details at the top level of the source. | ||
| containerImage: | ||
| ref: ghcr.io/example/server:latest | ||
| # future fields could include: | ||
| # - imagePullSecrets | ||
| # - pullPolicy | ||
| # - ... | ||
|
|
||
| # The server section defines how the MCP server should be configured when it runs. | ||
| config: | ||
| port: 8080 | ||
|
|
||
| arguments: | ||
| - --config=/etc/config/config.toml | ||
|
|
||
| env: | ||
| # item types https://pkg.go.dev/k8s.io/api@v0.35.2/core/v1#EnvVar | ||
| - name: ... | ||
| # ... | ||
|
|
||
| # direct value | ||
| - name: EXAMPLE_ENV_VAR | ||
| value: example-value | ||
|
|
||
| # value from Secret or ConfigMap | ||
| - name: ANOTHER_ENV_VAR | ||
| valueFrom: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.2/core/v1#EnvVarSource | ||
| configMapKeyRef: # or secretKeyRef for Secrets | ||
| name: example-configmap | ||
| key: example-key | ||
|
|
||
| # value from downward API (e.g. pod metadata) | ||
| - name: POD_NAME | ||
| valueFrom: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.2/core/v1#EnvVarSource | ||
| fieldRef: | ||
| fieldPath: metadata.name | ||
|
|
||
| # value from resource field (e.g. limits.cpu) | ||
| - name: CPU_LIMIT | ||
| valueFrom: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35 | ||
| resourceFieldRef: | ||
| resource: limits.cpu | ||
|
|
||
|
|
||
| # envFrom to allow importing all keys from a Secret or ConfigMap as environment variables with a common prefix | ||
| envFrom: | ||
| - secretRef: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.2/core/v1#EnvFromSource | ||
| name: all-env-vars | ||
| prefix: GITHUB_ # optional prefix to add to all imported environment variable names (e.g. GITHUB_TOKEN, GITHUB_REPO, etc.) | ||
| - configMapRef: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.2/core/v1#EnvFromSource | ||
| name: more-env-vars | ||
|
|
||
| # Storage mounts for ConfigMaps, Secrets, and volumes | ||
| # Each item uses native Kubernetes volume source types for consistency and feature parity | ||
| storage: | ||
| # Mount a single file from a ConfigMap | ||
| - mountPath: /etc/config/config.toml | ||
| type: ConfigMap | ||
| configMap: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#ConfigMapVolumeSource | ||
| name: example-configmap | ||
| items: | ||
| - key: config.toml | ||
| path: config.toml # relative path within the mount | ||
| optional: false # Pod fails if ConfigMap/key missing | ||
| defaultMode: 0644 # file permissions (octal) | ||
| # ... other fields from ConfigMapVolumeSource | ||
|
|
||
| # Mount multiple files from a ConfigMap into a directory | ||
| - mountPath: /var/data | ||
| type: ConfigMap | ||
| configMap: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#ConfigMapVolumeSource | ||
| name: example-data | ||
| items: # optional - if omitted, all keys mounted as files | ||
| - key: myData1.txt | ||
| path: data1.txt # rename the file in the mount | ||
| mode: 0600 # optional per-file permissions override | ||
| # ... other fields from ConfigMapVolumeSource | ||
|
|
||
| # Mount a single file using subPath (avoids overwriting the parent directory) | ||
| - mountPath: /app/settings.json | ||
| type: ConfigMap | ||
| configMap: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#ConfigMapVolumeSource | ||
| name: app-configs | ||
| items: | ||
| - key: prod-settings | ||
| path: settings.json | ||
| # ... other fields from ConfigMapVolumeSource | ||
|
|
||
| # TODO: which one do you think we need from these VolumeMount fields? | ||
| # IMO, we should only have `readOnly` as the other fields are more advanced features. | ||
| # Alternatively, we can a copy of the https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#VolumeMount and | ||
| # make it a sub-field. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with your comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would it make sense for an end-user to set read only to false? Could you reasonably default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that would be a reasonable starting point I think. |
||
| readOnly: ... | ||
| # recursiveReadonly: ... | ||
| # subPath: ... | ||
| # mountPropagation: ... | ||
| # subPathExpr: ... | ||
| # ... more fields from https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#VolumeMount | ||
| # Since we are not separating the volume source (ConfigMap, Secret, PVC, etc.) from the mount configuration | ||
| # (mountPath, subPath, readOnly, etc.) in this CRD design, we can only include these fields here in the mount | ||
| # configuration directly | ||
|
Comment on lines
+148
to
+160
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In native Kubernetes, storage mounting uses two separate concepts:
Native Kubernetes Example: spec:
volumes:
- name: config-vol
configMap:
name: app-config
containers:
- volumeMounts:
- name: config-vol # References volume by name
mountPath: /etc/config
readOnly: trueThis separation makes things flexible but adds verbosity. Here, I think we should avoid that separation and use a single struct to specify both the volume source and the mount configuration. Reason is that we're providing an opinionated API and not the full Kubernetes API. This would mean that we shouldn't have all fields from https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#VolumeMount, but some. spec:
server:
storage:
- mountPath: /etc/config # VolumeMount field
readOnly: true # VolumeMount field
type: ConfigMap # Type discriminator
configMap: # VolumeSource (all native K8s fields)
.. |
||
|
|
||
| # Mount a single file from a Secret | ||
| - mountPath: /etc/config/tls.crt | ||
| type: Secret | ||
| secret: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#SecretVolumeSource | ||
| secretName: example-tls-secret | ||
| items: | ||
| - key: tls.crt | ||
| path: tls.crt | ||
| # ... other fields from SecretVolumeSource | ||
|
|
||
| # Mount all keys from a Secret as files in a directory | ||
| - mountPath: /etc/secrets | ||
| type: Secret | ||
| secret: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#SecretVolumeSource | ||
| secretName: example-secret | ||
| # no items specified = mount all keys as files | ||
| # ... other fields from SecretVolumeSource | ||
|
|
||
| # NOTE: following YAML shows mounting volumes. | ||
| # although we don't need to mount volumes for the MCP server right now, we want to design the CRD in a way | ||
| # that allows us to easily add support for mounting volumes in the future without breaking compatibility. | ||
| # So, the YAML is written to get a feeling of the structure's extensibility, even though we won't implement these features right away. | ||
| # Temporary scratch space (lifetime of pod) | ||
| - mountPath: /tmp/scratch | ||
| type: EmptyDir | ||
| emptyDir: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#EmptyDirVolumeSource | ||
| sizeLimit: 1Gi # optional size limit | ||
| # medium: Memory # optional: use tmpfs (RAM-backed storage) | ||
|
|
||
| # NOTE: following YAML shows mounting volumes. | ||
| # although we don't need to mount volumes for the MCP server right now, we want to design the CRD in a way | ||
| # that allows us to easily add support for mounting volumes in the future without breaking compatibility. | ||
| # So, the YAML is written to get a feeling of the structure's extensibility, even though we won't implement these features right away. | ||
| # Persistent storage (reference to existing PVC) | ||
| - mountPath: /data | ||
| type: PVC | ||
| persistentVolumeClaim: | ||
|
Comment on lines
+200
to
+201
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think similar to above the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, whatever we decide above, let's also apply that approach here too! |
||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#PersistentVolumeClaimVolumeSource | ||
| claimName: mcp-server-data # PVC must exist in same namespace | ||
| readOnly: false # PVC-specific readOnly flag | ||
|
|
||
| # NOTE: following YAML shows mounting volumes. | ||
| # although we don't need to mount volumes for the MCP server right now, we want to design the CRD in a way | ||
| # that allows us to easily add support for mounting volumes in the future without breaking compatibility. | ||
| # So, the YAML is written to get a feeling of the structure's extensibility, even though we won't implement these features right away. | ||
| # Mount from host path (use with caution - security implications) | ||
| - mountPath: /host-data | ||
| type: HostPath | ||
| hostPath: | ||
| # type: https://pkg.go.dev/k8s.io/api@v0.35.0/core/v1#HostPathVolumeSource | ||
| path: /data/mcp # path on host | ||
| type: DirectoryOrCreate # optional: DirectoryOrCreate, Directory, FileOrCreate, File, Socket, CharDevice, BlockDevice | ||
|
|
||
| # TODO: Support for projected volumes (combine multiple volume sources into a single mount)? | ||
| # https://kubernetes.io/docs/concepts/storage/projected-volumes/ | ||
|
|
||
|
|
||
| runtime: | ||
|
|
||
| security: | ||
| serviceAccountName: example-service-account | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need to allow setting a service account name here? Why would a user want to be able to configure this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For something like the kubernetes mcp server the serviceaccount it runs under will influence the access it has to the kube api. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if you deterministically defaulted this so that end users don't have to create the service account first? What is the expected behavior if this field isn't provided? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the field is not provided then it won't be set on the deployment and the pods will run under the default serviceaccount for that namespace. |
||
|
|
||
| # pod-level security context that applies to the entire pod (as opposed to container-level security context) | ||
| podSecurityContext: | ||
| runAsNonRoot: true | ||
| # ... other security context fields from https://pkg.go.dev/k8s.io/api@v0.35.2/core/v1#SecurityContext | ||
|
Comment on lines
+227
to
+230
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the pod going to have more than a single container for the MCP server? When might a user want to be able to configure the overall pod security context over you defaulting it to the most secure security context you can?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rationale for adding that was that we may want a way to specify |
||
|
|
||
| # container-level security context that applies to the MCP server container (as opposed to the entire pod) | ||
| securityContext: | ||
| # ... fields from https://pkg.go.dev/k8s.io/api@v0.35.2/core/v1#PodSecurityContext | ||
|
|
||
| # ------- | ||
| # Rest of the following fields are not needed yet, as the existing CRD doesn't have any relevant fields. | ||
| # Just writing this out to show how we can expand the spec in the future to include more management options like scheduling and networking. | ||
| resources: | ||
| requests: {memory: 256Mi, cpu: 100m} | ||
| replicas: 1 | ||
| scheduling: | ||
| nodeSelector: { workload: mcp } | ||
| health: | ||
| liveness: { httpGet: { path: /healthz, port: 8080 } } | ||
| networking: | ||
| serviceType: ClusterIP | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe also a labeling/annotation section?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could probably also be where resource references go for the BYO use case in the future
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was just writing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. runtime:
metadata:
labels:
cost-center: ml-team
annotations:
prometheus.io/scrape: "true"I think when needed is there to above works well with the "runtime" section from this proposal
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I think @jaideepr97 is right that once we really need it - instead of bloating up ours e.g. with |
||
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 like the
sourcegrouping and the possibilty for doingtypein the future.I think for now we anyways just want to do image? So I'd just do that.
For future, I think we can revisit. However, what I do not like is that the
containerImageis not nested in there. Not sure that type is really a good choice. Perhaps something like actualtype/strcutmight be betterthat way we do not end up confusing string-based APIs (
type: my text) and actual fieds?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.
IMO this reads more natural/fluent
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.
to
So, get rid of
typethen? And, rename some fields?Renaming fields is ok overall.
Do we want to remove the
type?I want to hear other people's opinion as well!
Uh oh!
There was an error while loading. Please reload this page.
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.
yep - because the
typeis just text, while something like an actual field (e.g.container) gives concrete and type-safe hints:(no need to also say
containerImage, since that is now obvious)and than if we want, we can later have other things... like
npm? (just making things up)That way
imagecan really only be applied when inside the "container api"So I'd instead of
type: containerintroduce an actualtype/strcutfor better groupingThere 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.
Just noting from a API reviewer perspective, the
typefield is useful for version skew situations between the API version and the controller version.For example, if you are going to support newer versions of the API being used with some skew of the controller - or use cases have more than just your controller reading the API and taking some action - adding a new optional field that can be set instead of say
containerwill mean that the older client does not know how to reconcile that (that field didn't previously exist). Using the discriminated union withtype: Containermeans that the older client can explicitly encode some default behavior if it seestypeis set to a value it doesn't know how to handle.In practice, this may not really be an issue for you folks so you just need to weigh the use cases and what kind of skew you do or do not support here.
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.
Oh, I forgot to mention that up until more recently it made enforcing "at most one of" semantics a bit easier.
Instead of checking that every other field isn't set when another field is you can just write a check that is equivalent to
type == 'Container' ? has(container) : !has(container)to prevent container from being set iftypeisn't set toContainer.There was recently the addition of the
kubebuilder:validation:AtMostOneOfmarker to controller-gen that makes this semantic a bit easier to enforce now though.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.
AtMostOneOfis a good idea for enfocring this.Let's do it! 👍