feat(instance): attach to an externally-managed LiteLLM deployment - #32
feat(instance): attach to an externally-managed LiteLLM deployment#32davidgibbons wants to merge 2 commits into
Conversation
…alenaAI#29) Adds `spec.workload.managed: false`, which stops the operator provisioning the proxy workload so the entity CRDs (LiteLLMTeam, LiteLLMVirtualKey, LiteLLMBudget, LiteLLMModel, ...) can be used against a proxy owned by a Helm chart, a GitOps pipeline or an internal platform. Nothing is created and nothing existing is adopted or mutated, replacing the RBAC-denial workaround that worked but left the instance permanently Degraded. Adds `spec.workload.endpoint` alongside it, so an unmanaged instance no longer has to be named after a Service it does not own and can attach to a proxy in another namespace or outside the cluster. A CEL rule rejects it when the workload is managed; the pattern requires a host, since the value becomes an outbound request URL. Readiness for an unmanaged instance now comes from the admin API answering at that endpoint rather than a name-matched Deployment, which also makes a StatefulSet-backed or off-cluster proxy work, and stops a Deployment that merely shares the CR's name from faking readiness. The Ready condition reports ProxyReachable / ProxyNotReachable and no PodsHealthy condition is set, because the operator owns no pods. status.version is not taken from spec.image.tag, which describes nothing the operator deployed and would print a fabricated "latest". It is filled from the litellm_version the proxy reports on /health/readiness, which LiteLLM includes only when its own general_settings sets allow_public_health_readiness_details: true — that endpoint is unauthenticated, so the master key does not unlock it. Absent that the field stays empty, which is honest: the operator does not know. spec.database.migration is ignored entirely. An externally-managed proxy owns its own schema: LiteLLM migrates on startup and whatever deployed it ships its own migration hook, so the operator would be a second migrator racing the real one. BuildMigrationJob also takes its image from spec.image.tag (defaulting to "latest"), so the Job would run prisma migrate deploy at an arbitrary schema version against a database the operator does not own. DatabaseReady reports WorkloadUnmanaged, and says the migration was ignored rather than skipping it silently when one was configured anyway. Health probing, config sync, license detection and finalizer-based cleanup of upstream entities are unaffected. Only workload reconciliation, auto-rollback and database migration are gated. The master-key-with-autogenerate-fallback block was duplicated in resolveInstance and probeInstanceHealth; it is now one masterKeyRef helper, which the new readiness probe also uses. Signed-off-by: David Gibbons <david@dgibbons.net>
0b95a2c to
05645fe
Compare
|
Thanks for this — it's a genuinely excellent PR, and the write-up made reviewing it a pleasure. A few things I want to call out before the nitpicks, because they're the parts I'd have most likely gotten wrong myself:
I also test-merged it against current Two things from the checklist in #29 are still open, though, and they're the two I'd want to settle before merging. 1. Config inertness still needs a status conditionThis is the one I care most about. Right now The reason docs aren't enough: nobody attaching to an existing proxy starts from a blank CR. They copy a managed one and flip the flag. Then What I had in mind is a condition that names only the sections the user actually set — not the whole catalogue, which would be noise for someone who did start clean. Something like: Happy to be talked out of the exact shape, but I don't think "documented only" survives contact with a copied CR. 2.
|
- Add a WorkloadUnmanaged status condition naming the spec sections (sso, caching, rbac, ...) a CR still carries but the operator never applies while workload.managed is false, so a CR copied from a managed instance doesn't look healthy while half its config is inert. - Require masterKey.secretRef via CEL when workload.managed is false. autoGenerate's Secret is only ever created by the managed-workload reconcile path, so it previously dangled: every entity CRD failed on a missing Secret with no mention of autoGenerate. - LiteLLMGuardrail now reports Ready=False/InstanceUnmanaged instead of Validated when its instanceRef points at an unmanaged instance, since guardrail config only ever renders through the ConfigMap an unmanaged instance never builds. - Revert the hand-edit to docs/changelog.md; the release workflow generates it from CHANGELOG.md.
|
Thanks for the thorough review — pushed
Also updated |
- Add a WorkloadUnmanaged status condition naming the spec sections (sso, caching, rbac, ...) a CR still carries but the operator never applies while workload.managed is false, so a CR copied from a managed instance doesn't look healthy while half its config is inert. - Require masterKey.secretRef via CEL when workload.managed is false. autoGenerate's Secret is only ever created by the managed-workload reconcile path, so it previously dangled: every entity CRD failed on a missing Secret with no mention of autoGenerate. - LiteLLMGuardrail now reports Ready=False/InstanceUnmanaged instead of Validated when its instanceRef points at an unmanaged instance, since guardrail config only ever renders through the ConfigMap an unmanaged instance never builds. - Revert the hand-edit to docs/changelog.md; the release workflow generates it from CHANGELOG.md. Signed-off-by: David Gibbons <david@dgibbons.net>
4000b3e to
266e28f
Compare
Closes #29.
Adds
spec.workload.managed: falseso aLiteLLMInstancecan describe a LiteLLM proxy the operator did not deploy, making the entity CRDs usable against an existing installation without handing the operator the workload.This implements the issue's proposal, plus the
endpointhalf of "worth considering alongside".readinessRefis deliberately not implemented — see below.What changes
spec.workload.managed(*bool, defaultstrue). When false,reconcileResourcesandreconcileAutoRollbackare skipped entirely. Nothing is created, nothing existing is adopted or mutated, and noForbiddenis logged on every loop — the instance reports its actual state instead of sittingDegradedabout resources it doesn't manage.spec.workload.endpoint(optional, unmanaged only, CEL-enforced). Without it the endpoint stays today's formula, which requires the CR to be named after a Service it doesn't own. With it you can attach to a Service under another name, in another namespace, or to a proxy outside the cluster.Readiness for an unmanaged instance comes from the admin API answering at
status.endpoint(CheckLiveness, whichprobeInstanceHealthalready performs) rather than a name-matched Deployment. This is what makesendpointcoherent: with an explicit endpoint there may be no Deployment of that name to look at, and the proxy may be a StatefulSet or off-cluster. It also closes a hole in the RBAC workaround — a Deployment that merely shares the CR's name can no longer make an unreachable proxy reportready: true.updateInstanceStatuscomputes the endpoint before deriving readiness, since readiness now depends on it.Status differences when unmanaged
status.readystatus.endpointstatus.replicas/readyReplicas0status.versionspec.image.taglitellm_versionPodsHealthyconditionReadyreasonAllResourcesReady/DeploymentNotReadyProxyReachable/ProxyNotReachablestatus.versionis worth calling out.spec.image.tagdescribes nothing the operator deployed, so leaving it in place would print a fabricatedlatestin theVERSIONprint column.probeInstanceHealthalready fetches/health/readinessand discarded the payload, so it now readslitellm_versionoff it — but that field is only present when the proxy's owngeneral_settingssetsallow_public_health_readiness_details: true. The endpoint is unauthenticated, so the master key does not unlock it, and the default payload is{"status", "db"}. In the common casestatus.versionis therefore empty for an unmanaged instance. That is deliberate: empty means "the operator does not know", which is true, wherelatestwould be a guess. Both payload shapes are covered by tests. Say the word if you would rather it fell back to the image tag.Decisions worth a look
Managedis*bool, notbool. An envtest spec caught this: with a plainbooland noomitempty, a Go-typed client constructingWorkloadSpec{Endpoint: ...}marshals"managed": falseexplicitly, so the API-server default never fires and the caller silently gets an unmanaged instance. Nil now means managed, matchingenableServiceLinksand friends upstream. YAML users see no difference —workload: {}still defaults totrue.No
readinessRef. Probing the endpoint covers Deployment, StatefulSet and off-cluster uniformly, so kind-switching would add surface without adding capability. Happy to add it if you'd rather have the Kubernetes-object signal.spec.database.migrationis gated too, and ignored entirely when unmanaged. An externally-managed proxy owns its own schema — LiteLLM migrates on startup, and whatever deployed it ships its own migration hook, so the operator would be a second migrator racing the real one. Mechanically it is worse than that:BuildMigrationJobtakes its image fromspec.image.tag(falling back tolatest), which for an unmanaged instance describes nothing the operator deployed. Leaving it open meant runningprisma migrate deployat an arbitrary schema version against a database the operator does not own.DatabaseReadyreportsWorkloadUnmanaged, and the message says the migration was ignored rather than skipping it silently when one was configured anyway.Tests
internal/controller/litellminstance_unmanaged_test.go— no resources created; a name-colliding Helm-owned Deployment left byte-identical, un-mutated and un-adopted; readiness tracking the probe in both directions; a name-matched Deployment not faking readiness; version read from the proxy;workloadManagedandinstanceEndpointtables.internal/controller/litellminstance_controller_test.go— four envtest specs for the CEL rule and themanageddefault, since only envtest runs API-server validation.What was verified against upstream
/health/livelinessand/health/readinessare the paths the operator's shipped client already uses for every managed instance, so the readiness signal reuses a proven code path rather than a new one./health/livelinessreturns the bare stringI'm alive!;CheckLivenesspasses a nil result and so never unmarshals it, which is why it is safe as a reachability probe. Thelitellm_versiongating described above was checked againstlitellm/proxy/health_endpoints/_health_endpoints.pyonmain, not assumed — the field had been declared in this repo'sReadinessResponsesince the initial commit but never read by anything until now.spec.workload.endpointis trimmed of trailing slashes by the client (strings.TrimRight(endpoint, "/")), sohttp://host:4000/andhttp://host:4000behave identically. The CRD pattern is^https?://[^\s/?#]+, which requires a host — the value becomes an outbound request URL, so a scheme alone should not pass.Housekeeping
make manifests generate sync-helm-crdsandmake bundleare committed.make test,make lint,helm lintandhelm templatepass locally. Commit is signed off, README anddocs/reference/litellminstance.mddocument the field, and there's a sample atconfig/samples/litellm_v1alpha1_litellminstance_unmanaged.yaml.A small drive-by: the master-key-with-autogenerate-fallback block was copy-pasted in
resolveInstanceandprobeInstanceHealth; it's now onemasterKeyRefhelper that the new readiness probe also uses.