diff --git a/api/v1alpha1/coderaiprovider_types.go b/api/v1alpha1/coderaiprovider_types.go index 453299b5..a717749e 100644 --- a/api/v1alpha1/coderaiprovider_types.go +++ b/api/v1alpha1/coderaiprovider_types.go @@ -95,6 +95,11 @@ type CoderAIProviderStatus struct { // ProviderID is the Coder provider ID. // +optional ProviderID string `json:"providerID,omitempty"` + // Enabled is the enabled flag of the Coder provider at the controller's + // last read or update of it. It is empty while Coder holds no provider of + // this object, or before the first read. + // +optional + Enabled *bool `json:"enabled,omitempty"` // CreateAttemptTime is set before every provider create request. While it // is set and ProviderID is empty, a create request can be in flight or // already committed in Coder. @@ -115,7 +120,7 @@ type CoderAIProviderStatus struct { // +kubebuilder:resource:path=coderaiproviders,scope=Namespaced // +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Type",type=string,JSONPath=`.spec.type` -// +kubebuilder:printcolumn:name="Enabled",type=boolean,JSONPath=`.spec.enabled` +// +kubebuilder:printcolumn:name="Enabled",type=boolean,JSONPath=`.status.enabled` // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status` // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" // +kubebuilder:validation:XValidation:rule="size(self.metadata.name) <= 63 && self.metadata.name.matches('^[a-z0-9]+(-[a-z0-9]+)*$')",message="metadata.name is the Coder provider name: at most 63 lowercase letters, digits, and single hyphens between them" diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 4c37b03b..e70d03b1 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -385,6 +385,11 @@ func (in *CoderAIProviderSpec) DeepCopy() *CoderAIProviderSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CoderAIProviderStatus) DeepCopyInto(out *CoderAIProviderStatus) { *out = *in + if in.Enabled != nil { + in, out := &in.Enabled, &out.Enabled + *out = new(bool) + **out = **in + } if in.CreateAttemptTime != nil { in, out := &in.CreateAttemptTime, &out.CreateAttemptTime *out = (*in).DeepCopy() diff --git a/config/crd/bases/coder.com_coderaiproviders.yaml b/config/crd/bases/coder.com_coderaiproviders.yaml index 6b8a6187..291b3c7b 100644 --- a/config/crd/bases/coder.com_coderaiproviders.yaml +++ b/config/crd/bases/coder.com_coderaiproviders.yaml @@ -18,7 +18,7 @@ spec: - jsonPath: .spec.type name: Type type: string - - jsonPath: .spec.enabled + - jsonPath: .status.enabled name: Enabled type: boolean - jsonPath: .status.conditions[?(@.type=="Ready")].status @@ -245,6 +245,12 @@ spec: description: SecretUID is the UID of the Secret that was read. type: string type: object + enabled: + description: |- + Enabled is the enabled flag of the Coder provider at the controller's + last read or update of it. It is empty while Coder holds no provider of + this object, or before the first read. + type: boolean observedGeneration: description: ObservedGeneration is the generation the controller last processed. diff --git a/config/gitops/argocd-health-coderaiprovider.lua b/config/gitops/argocd-health-coderaiprovider.lua new file mode 100644 index 00000000..fe43f283 --- /dev/null +++ b/config/gitops/argocd-health-coderaiprovider.lua @@ -0,0 +1,35 @@ +-- Argo CD health check for CoderAIProvider (coder.com/v1alpha1). +-- Install it as the argocd-cm key +-- resource.customizations.health.coder.com_CoderAIProvider. +-- docs/how-to/gitops.md copies this file. A test keeps the copy equal. +local hs = { status = "Progressing", message = "Waiting for the controller" } +if obj.metadata.deletionTimestamp ~= nil then + -- Argo CD shows deletionMessage, not message, for a resource being deleted. + hs.deletionMessage = "Deleting the Coder AI provider" + hs.message = hs.deletionMessage + return hs +end +local st = obj.status +if st == nil or st.observedGeneration == nil or st.observedGeneration ~= obj.metadata.generation then + return hs +end +-- A condition from an older generation does not describe the current spec. +local conditions = {} +for _, c in ipairs(st.conditions or {}) do + if c.observedGeneration == obj.metadata.generation then + table.insert(conditions, c) + end +end +for _, c in ipairs(conditions) do + if c.type == "Ready" and c.reason ~= nil and c.message ~= nil then + hs.message = c.reason .. ": " .. c.message + end +end +for _, c in ipairs(conditions) do + if c.type == "Ready" and c.status == "True" then + hs.status = "Healthy" + elseif c.type == "Stalled" and c.status == "True" then + hs.status = "Degraded" + end +end +return hs diff --git a/config/gitops/flux-healthcheck-coderaiprovider.yaml b/config/gitops/flux-healthcheck-coderaiprovider.yaml new file mode 100644 index 00000000..baf2aee1 --- /dev/null +++ b/config/gitops/flux-healthcheck-coderaiprovider.yaml @@ -0,0 +1,9 @@ +# Flux health check expressions for CoderAIProvider (coder.com/v1alpha1). +# Add this entry to spec.healthCheckExprs of a Flux v2.5+ Kustomization +# with spec.wait: true. +# docs/how-to/gitops.md copies this file. A test keeps the copy equal. +- apiVersion: coder.com/v1alpha1 + kind: CoderAIProvider + inProgress: "!has(status.observedGeneration) || status.observedGeneration != metadata.generation || !has(status.conditions) || !status.conditions.exists(c, c.type == 'Ready' && has(c.observedGeneration) && c.observedGeneration == metadata.generation) || status.conditions.exists(c, c.type == 'Reconciling' && c.status == 'True')" + failed: "status.conditions.exists(c, c.type == 'Stalled' && c.status == 'True')" + current: "status.conditions.exists(c, c.type == 'Ready' && c.status == 'True')" diff --git a/config/gitops/health_test.go b/config/gitops/health_test.go index ca7cc4ba..b91405d4 100644 --- a/config/gitops/health_test.go +++ b/config/gitops/health_test.go @@ -1,6 +1,6 @@ -// Package gitops_test checks the GitOps health rules for CoderTemplateTest -// and CoderMCPServer. The rules are data files, so this package has no -// production code. +// Package gitops_test checks the GitOps health rules for CoderTemplateTest, +// CoderMCPServer and CoderAIProvider. The rules are data files, so this +// package has no production code. package gitops_test import ( @@ -29,6 +29,7 @@ type ruleSet struct { var ruleSets = []ruleSet{ {"CoderTemplateTest", "argocd-health-codertemplatetest.lua", "flux-healthcheck-codertemplatetest.yaml", "../../docs/how-to/gitops.md", healthCases}, {"CoderMCPServer", "argocd-health-codermcpserver.lua", "flux-healthcheck-codermcpserver.yaml", "../../docs/how-to/manage-mcp-servers.md", mcpServerCases}, + {"CoderAIProvider", "argocd-health-coderaiprovider.lua", "flux-healthcheck-coderaiprovider.yaml", "../../docs/how-to/gitops.md", aiProviderCases}, } // fluxHealthCheck is one entry of a Flux Kustomization spec.healthCheckExprs. @@ -128,10 +129,10 @@ func mcpServerObject(status map[string]any) map[string]any { return obj } -// mcpServerStatus returns the three conditions that the controller always -// writes together, with one reason and message: ready, reconciling (neither -// ready nor stalled), or stalled. -func mcpServerStatus(state, reason string) map[string]any { +// readyReconcilingStalled returns the three conditions that the CoderMCPServer +// and CoderAIProvider controllers always write together, with one reason and +// message: ready, reconciling (neither ready nor stalled), or stalled. +func readyReconcilingStalled(state, reason string) map[string]any { ready, reconciling, stalled := "False", "True", "False" switch state { case "ready": @@ -140,7 +141,7 @@ func mcpServerStatus(state, reason string) map[string]any { reconciling, stalled = "False", "True" case "reconciling": default: - panic("assertion failed: unknown MCP server fixture state " + state) + panic("assertion failed: unknown condition fixture state " + state) } condition := func(conditionType, status string) any { return map[string]any{"type": conditionType, "status": status, "reason": reason, "message": "details", "observedGeneration": int64(2)} @@ -152,31 +153,81 @@ func mcpServerStatus(state, reason string) map[string]any { } func mcpServerCases() []healthCase { - stale := mcpServerStatus("ready", "Applied") + stale := readyReconcilingStalled("ready", "Applied") stale["observedGeneration"] = int64(1) noConditions := map[string]any{"observedGeneration": int64(2)} - deleting := mcpServerObject(mcpServerStatus("ready", "Applied")) + deleting := mcpServerObject(readyReconcilingStalled("ready", "Applied")) deleting["metadata"].(map[string]any)["deletionTimestamp"] = "2026-10-14T10:00:00Z" - deletingWait := mcpServerObject(mcpServerStatus("reconciling", "CoderUnavailable")) + deletingWait := mcpServerObject(readyReconcilingStalled("reconciling", "CoderUnavailable")) deletingWait["metadata"].(map[string]any)["deletionTimestamp"] = "2026-10-14T10:00:00Z" return []healthCase{ {"no status", mcpServerObject(nil), "Progressing", "Waiting for the controller", "Unknown", ""}, {"stale observedGeneration", mcpServerObject(stale), "Progressing", "Waiting for the controller", "InProgress", ""}, {"no conditions", mcpServerObject(noConditions), "Progressing", "Waiting for the controller", "InProgress", ""}, - {"applied", mcpServerObject(mcpServerStatus("ready", "Applied")), "Healthy", "Applied: details", "Current", ""}, - {"control plane not ready", mcpServerObject(mcpServerStatus("reconciling", "ControlPlaneNotReady")), "Progressing", "ControlPlaneNotReady: details", "InProgress", ""}, - {"Coder unavailable", mcpServerObject(mcpServerStatus("reconciling", "CoderUnavailable")), "Progressing", "CoderUnavailable: details", "InProgress", ""}, - {"group not found", mcpServerObject(mcpServerStatus("stalled", "GroupNotFound")), "Degraded", "GroupNotFound: details", "Failed", ""}, - {"not allowed", mcpServerObject(mcpServerStatus("stalled", "MCPServersNotAllowed")), "Degraded", "MCPServersNotAllowed: details", "Failed", ""}, - {"disabled externally", mcpServerObject(mcpServerStatus("stalled", "DisabledExternally")), "Degraded", "DisabledExternally: details", "Failed", ""}, - {"credential not opted in", mcpServerObject(mcpServerStatus("stalled", "CredentialNotOptedIn")), "Degraded", "CredentialNotOptedIn: details", "Failed", ""}, - {"slug conflict", mcpServerObject(mcpServerStatus("stalled", "SlugConflict")), "Degraded", "SlugConflict: details", "Failed", ""}, + {"applied", mcpServerObject(readyReconcilingStalled("ready", "Applied")), "Healthy", "Applied: details", "Current", ""}, + {"control plane not ready", mcpServerObject(readyReconcilingStalled("reconciling", "ControlPlaneNotReady")), "Progressing", "ControlPlaneNotReady: details", "InProgress", ""}, + {"Coder unavailable", mcpServerObject(readyReconcilingStalled("reconciling", "CoderUnavailable")), "Progressing", "CoderUnavailable: details", "InProgress", ""}, + {"group not found", mcpServerObject(readyReconcilingStalled("stalled", "GroupNotFound")), "Degraded", "GroupNotFound: details", "Failed", ""}, + {"not allowed", mcpServerObject(readyReconcilingStalled("stalled", "MCPServersNotAllowed")), "Degraded", "MCPServersNotAllowed: details", "Failed", ""}, + {"disabled externally", mcpServerObject(readyReconcilingStalled("stalled", "DisabledExternally")), "Degraded", "DisabledExternally: details", "Failed", ""}, + {"credential not opted in", mcpServerObject(readyReconcilingStalled("stalled", "CredentialNotOptedIn")), "Degraded", "CredentialNotOptedIn: details", "Failed", ""}, + {"slug conflict", mcpServerObject(readyReconcilingStalled("stalled", "SlugConflict")), "Degraded", "SlugConflict: details", "Failed", ""}, // Flux has no deletion rule: it waits for pruned objects separately. {"being deleted", deleting, "Progressing", "Deleting the Coder MCP server config", "Current", "Deleting the Coder MCP server config"}, {"being deleted while Coder is down", deletingWait, "Progressing", "Deleting the Coder MCP server config", "InProgress", "Deleting the Coder MCP server config"}, } } +// aiProviderObject returns a CoderAIProvider at generation 2 with the given +// status. +func aiProviderObject(status map[string]any) map[string]any { + obj := map[string]any{ + "apiVersion": "coder.com/v1alpha1", + "kind": "CoderAIProvider", + "metadata": map[string]any{"name": "openai", "namespace": "coder", "generation": int64(2)}, + "spec": map[string]any{"type": "openai", "baseURL": "https://api.openai.com/v1"}, + } + if status != nil { + obj["status"] = status + } + return obj +} + +func aiProviderCases() []healthCase { + stale := readyReconcilingStalled("ready", "Applied") + stale["observedGeneration"] = int64(1) + noConditions := map[string]any{"observedGeneration": int64(2)} + // An update interrupted before Coder committed it, as stored by + // controllers before the #321 fix: only the top-level generation moved. + interrupted := readyReconcilingStalled("ready", "Applied") + interrupted["appliedGeneration"] = int64(1) + for _, c := range interrupted["conditions"].([]any) { + c.(map[string]any)["observedGeneration"] = int64(1) + } + deleting := aiProviderObject(readyReconcilingStalled("ready", "Applied")) + deleting["metadata"].(map[string]any)["deletionTimestamp"] = "2026-10-30T10:00:00Z" + deletingWait := aiProviderObject(readyReconcilingStalled("reconciling", "CoderUnavailable")) + deletingWait["metadata"].(map[string]any)["deletionTimestamp"] = "2026-10-30T10:00:00Z" + return []healthCase{ + {"no status", aiProviderObject(nil), "Progressing", "Waiting for the controller", "Unknown", ""}, + {"stale observedGeneration", aiProviderObject(stale), "Progressing", "Waiting for the controller", "InProgress", ""}, + {"no conditions", aiProviderObject(noConditions), "Progressing", "Waiting for the controller", "InProgress", ""}, + {"applied", aiProviderObject(readyReconcilingStalled("ready", "Applied")), "Healthy", "Applied: details", "Current", ""}, + {"stale conditions", aiProviderObject(interrupted), "Progressing", "Waiting for the controller", "InProgress", ""}, + {"control plane not ready", aiProviderObject(readyReconcilingStalled("reconciling", "ControlPlaneNotReady")), "Progressing", "ControlPlaneNotReady: details", "InProgress", ""}, + {"confirming a create", aiProviderObject(readyReconcilingStalled("reconciling", "ConfirmingCreate")), "Progressing", "ConfirmingCreate: details", "InProgress", ""}, + // A disable that failed retries with the credential reason. + {"disable retrying", aiProviderObject(readyReconcilingStalled("reconciling", "CredentialNotOptedIn")), "Progressing", "CredentialNotOptedIn: details", "InProgress", ""}, + {"credential kill switch", aiProviderObject(readyReconcilingStalled("stalled", "CredentialNotOptedIn")), "Degraded", "CredentialNotOptedIn: details", "Failed", ""}, + {"name conflict", aiProviderObject(readyReconcilingStalled("stalled", "NameConflict")), "Degraded", "NameConflict: details", "Failed", ""}, + {"name reserved by env", aiProviderObject(readyReconcilingStalled("stalled", "NameReservedByEnv")), "Degraded", "NameReservedByEnv: details", "Failed", ""}, + {"disabled externally", aiProviderObject(readyReconcilingStalled("stalled", "DisabledExternally")), "Degraded", "DisabledExternally: details", "Failed", ""}, + // Flux has no deletion rule: it waits for pruned objects separately. + {"being deleted", deleting, "Progressing", "Deleting the Coder AI provider", "Current", "Deleting the Coder AI provider"}, + {"being deleted while Coder is down", deletingWait, "Progressing", "Deleting the Coder AI provider", "InProgress", "Deleting the Coder AI provider"}, + } +} + func TestArgoCDHealth(t *testing.T) { t.Parallel() for _, rules := range ruleSets { diff --git a/dist/install.yaml b/dist/install.yaml index 7172ee67..7348b38a 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -409,7 +409,7 @@ spec: - jsonPath: .spec.type name: Type type: string - - jsonPath: .spec.enabled + - jsonPath: .status.enabled name: Enabled type: boolean - jsonPath: .status.conditions[?(@.type=="Ready")].status @@ -636,6 +636,12 @@ spec: description: SecretUID is the UID of the Secret that was read. type: string type: object + enabled: + description: |- + Enabled is the enabled flag of the Coder provider at the controller's + last read or update of it. It is empty while Coder holds no provider of + this object, or before the first read. + type: boolean observedGeneration: description: ObservedGeneration is the generation the controller last processed. diff --git a/docs/how-to/gitops.md b/docs/how-to/gitops.md index 51be92f6..5ec375a5 100644 --- a/docs/how-to/gitops.md +++ b/docs/how-to/gitops.md @@ -1,6 +1,6 @@ # Gate template promotion with GitOps -This guide shows how to make Argo CD or Flux wait for a [`CoderTemplateTest`](test-templates.md) result, and how to promote a template version only after its test passed. The health rules live in `config/gitops/` in this repository. This page copies them, and a unit test keeps the copies equal to the files. +This guide shows how to make Argo CD or Flux wait for a [`CoderTemplateTest`](test-templates.md) result, and how to promote a template version only after its test passed. The health rules live in `config/gitops/` in this repository. This page copies them, and a unit test keeps the copies equal to the files. The page also holds the [health rules for `CoderAIProvider`](#health-rules-for-coderaiprovider). ## Why custom health rules are needed @@ -162,6 +162,73 @@ A `CoderTemplate` update of `spec.files` activates the new version at once, befo If the import takes longer than the 34-second write budget of the aggregated API ([#117](https://github.com/coder/coder-k8s/issues/117)), the update fails and the previous version stays active, so the test resolves the previous version. `status.templateVersionName` shows which version ran. +## Health rules for CoderAIProvider + +Argo CD and Flux also need health rules to wait for a [`CoderAIProvider`](manage-ai-providers.md). Without them, Argo CD has no health check for the kind, and Flux reports a new provider as current before the controller writes its first status. The controller writes the conditions `Ready`, `Reconciling` and `Stalled` together, with one reason and message. It sets `status.observedGeneration` to `metadata.generation` only when it writes the conditions for that generation. Both rules wait until `status.observedGeneration` equals `metadata.generation`. They also ignore a condition whose own `observedGeneration` is older, so a status that an older controller wrote in the middle of an update does not count as healthy. Then they read the conditions: + +| Conditions | Argo CD | Flux | +| --- | --- | --- | +| no status, or a stale `observedGeneration` on the status or the `Ready` condition | `Progressing` | `InProgress` (`Unknown` before the first status) | +| `Reconciling=True` | `Progressing` | `InProgress` | +| `Ready=True` | `Healthy` | `Current` | +| `Stalled=True` | `Degraded` | `Failed` | + +A [kill switch](manage-ai-providers.md#kill-switches) stalls the provider, so it shows `Degraded` or `Failed` until the spec or the key Secret changes. The Argo CD message is `: ` of the `Ready` condition. While the object is being deleted, Argo CD shows `Deleting the Coder AI provider`. + +Add the script as the `argocd-cm` key `resource.customizations.health.coder.com_CoderAIProvider` (`config/gitops/argocd-health-coderaiprovider.lua`): + +```lua +-- Argo CD health check for CoderAIProvider (coder.com/v1alpha1). +-- Install it as the argocd-cm key +-- resource.customizations.health.coder.com_CoderAIProvider. +-- docs/how-to/gitops.md copies this file. A test keeps the copy equal. +local hs = { status = "Progressing", message = "Waiting for the controller" } +if obj.metadata.deletionTimestamp ~= nil then + -- Argo CD shows deletionMessage, not message, for a resource being deleted. + hs.deletionMessage = "Deleting the Coder AI provider" + hs.message = hs.deletionMessage + return hs +end +local st = obj.status +if st == nil or st.observedGeneration == nil or st.observedGeneration ~= obj.metadata.generation then + return hs +end +-- A condition from an older generation does not describe the current spec. +local conditions = {} +for _, c in ipairs(st.conditions or {}) do + if c.observedGeneration == obj.metadata.generation then + table.insert(conditions, c) + end +end +for _, c in ipairs(conditions) do + if c.type == "Ready" and c.reason ~= nil and c.message ~= nil then + hs.message = c.reason .. ": " .. c.message + end +end +for _, c in ipairs(conditions) do + if c.type == "Ready" and c.status == "True" then + hs.status = "Healthy" + elseif c.type == "Stalled" and c.status == "True" then + hs.status = "Degraded" + end +end +return hs +``` + +Add this entry (`config/gitops/flux-healthcheck-coderaiprovider.yaml`) to `spec.healthCheckExprs` of a Flux v2.5+ Kustomization with `spec.wait: true`: + +```yaml +# Flux health check expressions for CoderAIProvider (coder.com/v1alpha1). +# Add this entry to spec.healthCheckExprs of a Flux v2.5+ Kustomization +# with spec.wait: true. +# docs/how-to/gitops.md copies this file. A test keeps the copy equal. +- apiVersion: coder.com/v1alpha1 + kind: CoderAIProvider + inProgress: "!has(status.observedGeneration) || status.observedGeneration != metadata.generation || !has(status.conditions) || !status.conditions.exists(c, c.type == 'Ready' && has(c.observedGeneration) && c.observedGeneration == metadata.generation) || status.conditions.exists(c, c.type == 'Reconciling' && c.status == 'True')" + failed: "status.conditions.exists(c, c.type == 'Stalled' && c.status == 'True')" + current: "status.conditions.exists(c, c.type == 'Ready' && c.status == 'True')" +``` + ## What CI covers The unit test in `config/gitops/` runs the Lua script with gopher-lua, the Lua engine of Argo CD, and the Flux expressions with cel-go, both over fixed test objects. The unit test in `examples/gitops-promotion/` renders the example with Kustomize and runs `promote.sh` against a fake `kubectl`. Its cases include a test that someone created first with other parameters, which the script must refuse, and a test that runs for the longest `timeoutSeconds`. CI does not run Argo CD or Flux. diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 4dfeef5e..cca07574 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -66,7 +66,9 @@ kubectl -n coder get coderaiproviders kubectl -n coder get coderaiprovider openai -o jsonpath='{range .status.conditions[*]}{.type}={.status} {.reason}: {.message}{"\n"}{end}' ``` -The object has the conditions `Ready`, `Reconciling` and `Stalled`. A stalled object waits for a change of its spec or of an input. The most common reasons: +The `ENABLED` column shows `status.enabled`: the enabled flag of the provider in Coder at the operator's last read or update. It is empty while Coder holds no provider of this object, for example when the key Secret is refused before the create. + +The object has the conditions `Ready`, `Reconciling` and `Stalled`. A stalled object waits for a change of its spec or of an input. For Argo CD and Flux health rules, see [Health rules for CoderAIProvider](gitops.md#health-rules-for-coderaiprovider). The most common reasons: | Reason | Meaning | | --- | --- | @@ -77,7 +79,7 @@ The object has the conditions `Ready`, `Reconciling` and `Stalled`. A stalled ob | `NameConflict` | Coder has a provider with this name that the operator did not create. | | `NameReservedByEnv` | The control plane can seed a provider with this name. See [Seeding limits](#seeding-limits). | | `CoderRejected` | Coder rejected the create or update request. Fix the spec. | -| `DisabledExternally`, `KeyChangedExternally`, `DeletedExternally` | Someone disabled the provider, changed its key or deleted it in Coder. See [Kill switches](#kill-switches). | +| `DisabledExternally`, `KeyChangedExternally`, `DeletedExternally` | Someone disabled the provider, changed its key or deleted it in Coder. `DisabledExternally` also follows the credential kill switch, after the Secret was repaired. See [Kill switches](#kill-switches). | ## Kill switches @@ -85,7 +87,7 @@ The object has the conditions `Ready`, `Reconciling` and `Stalled`. A stalled ob When the key Secret fails the credential contract for a provider that the operator created, the operator disables the provider in Coder at once. This covers every credential error: the Secret is deleted, loses its label or the destinations annotation, no longer lists the origin that Coder uses, is refused, has no such key, or holds a malformed value. The update holds only `enabled: false`. Coder keeps the old key, but no longer uses it. When a Coder call for the disable fails, including a 404 that Coder did not send (for example from a proxy), the operator retries every 30 seconds, and the condition message starts with `Coder can still send the key`. -The object stalls with the credential reason. Repairing the Secret does not enable the provider again. To enable it, fix the Secret, then edit the spec (any change of `spec`, for example `displayName`). A short read error of the Secret, such as an API server timeout, is not a credential error: the operator retries and changes nothing. +The object stalls with the credential reason. Repairing the Secret does not enable the provider again: the object then reports `DisabledExternally`, and the message names both causes, a Coder admin or the credential kill switch. To enable it, fix the Secret, then edit the spec (any change of `spec`, for example `displayName`). A short read error of the Secret, such as an API server timeout, is not a credential error: the operator retries and changes nothing. ### Dashboard changes diff --git a/docs/reference/api/coderaiprovider.md b/docs/reference/api/coderaiprovider.md index 19cbf542..acd8ce4f 100644 --- a/docs/reference/api/coderaiprovider.md +++ b/docs/reference/api/coderaiprovider.md @@ -28,6 +28,7 @@ | `observedGeneration` | integer | ObservedGeneration is the generation the controller last processed. | | `appliedGeneration` | integer | AppliedGeneration is the last generation whose spec Coder fully holds. | | `providerID` | string | ProviderID is the Coder provider ID. | +| `enabled` | boolean | Enabled is the enabled flag of the Coder provider at the controller's last read or update of it. It is empty while Coder holds no provider of this object, or before the first read. | | `createAttemptTime` | [Time](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#time-v1-meta) | CreateAttemptTime is set before every provider create request. While it is set and ProviderID is empty, a create request can be in flight or already committed in Coder. | | `credential` | [CoderAIProviderCredentialStatus](#coderaiprovidercredentialstatus) | Credential records the Secret value last sent to Coder. | | `conditions` | [Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#condition-v1-meta) array | Conditions are Ready, Reconciling, and Stalled. | diff --git a/internal/controller/coderaiprovider_column_test.go b/internal/controller/coderaiprovider_column_test.go new file mode 100644 index 00000000..2ecf9e09 --- /dev/null +++ b/internal/controller/coderaiprovider_column_test.go @@ -0,0 +1,77 @@ +package controller_test + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/rest" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// enabledColumn returns the Enabled cell that kubectl get prints for +// provider, read as a server-side table from the API server. +func enabledColumn(ctx context.Context, t *testing.T, provider *coderv1alpha1.CoderAIProvider) any { + t.Helper() + server, _, err := rest.DefaultServerUrlFor(cfg) + require.NoError(t, err) + httpClient, err := rest.HTTPClientFor(cfg) + require.NoError(t, err) + server.Path = "/apis/coder.com/v1alpha1/namespaces/" + provider.Namespace + "/coderaiproviders/" + provider.Name + req, err := http.NewRequestWithContext(ctx, http.MethodGet, server.String(), nil) + require.NoError(t, err) + req.Header.Set("Accept", "application/json;as=Table;v=v1;g=meta.k8s.io") + resp, err := httpClient.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + require.Equal(t, http.StatusOK, resp.StatusCode) + var table metav1.Table + require.NoError(t, json.NewDecoder(resp.Body).Decode(&table)) + require.Len(t, table.Rows, 1) + for i, column := range table.ColumnDefinitions { + if column.Name == "Enabled" { + return table.Rows[0].Cells[i] + } + } + t.Fatal("assertion failed: the table has no Enabled column") + return nil +} + +// #321: the Enabled column shows what Coder holds, not spec.enabled. A +// refused object that Coder does not have shows no value. +func TestCoderAIProviderEnabledColumn(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("refused before a create", func(t *testing.T) { + t.Parallel() + env := newAIProviderEnv(ctx, t, true) + got, _ := env.reconcile(ctx, t, env.createProvider(ctx, t, nil)) + requireAIProviderStep(t, got, true, credentialsecret.ReasonSecretNotFound) + require.True(t, *got.Spec.Enabled) + require.Nil(t, enabledColumn(ctx, t, got)) + }) + t.Run("kill switch, then deleted in Coder", func(t *testing.T) { + t.Parallel() + env, secret, provider := newReadyAIProvider(ctx, t) + require.Equal(t, true, enabledColumn(ctx, t, provider)) + + consented := secret.DeepCopy() + editSecret(ctx, t, secret, func(s *corev1.Secret) { delete(s.Labels, credentialsecret.LabelKey) }) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, true, credentialsecret.ReasonNotOptedIn) + require.Equal(t, false, enabledColumn(ctx, t, got)) + + editSecret(ctx, t, secret, func(s *corev1.Secret) { s.Labels = consented.Labels }) + require.NoError(t, env.fake.client(t, 2*time.Second).DeleteAIProvider(ctx, got.Status.ProviderID)) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, true, "DeletedExternally") + require.Nil(t, enabledColumn(ctx, t, got)) + }) +} diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index c8f96fa1..2fede36b 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -347,8 +347,11 @@ func (r *CoderAIProviderReconciler) patchFinalizer(ctx context.Context, provider } // applyAIProviderStep sets the kstatus conditions for step. Messages are cut -// to the CoderTemplateTest limit. +// to the CoderTemplateTest limit. status.observedGeneration moves only here, +// so a status write between two steps never claims a generation that the +// conditions do not describe yet. func applyAIProviderStep(provider *coderv1alpha1.CoderAIProvider, step *aiProviderStep) { + provider.Status.ObservedGeneration = provider.Generation message := truncateTemplateTestMessage(step.message) ready, reconciling, stalled := metav1.ConditionFalse, metav1.ConditionTrue, metav1.ConditionFalse switch { @@ -373,7 +376,6 @@ func applyAIProviderStep(provider *coderv1alpha1.CoderAIProvider, step *aiProvid // writeStatus writes the status only when it changed. func (r *CoderAIProviderReconciler) writeStatus(ctx context.Context, provider *coderv1alpha1.CoderAIProvider, before *coderv1alpha1.CoderAIProviderStatus) error { - provider.Status.ObservedGeneration = provider.Generation if equality.Semantic.DeepEqual(before, &provider.Status) { return nil } diff --git a/internal/controller/coderaiprovider_disable.go b/internal/controller/coderaiprovider_disable.go index acc08229..cf969d32 100644 --- a/internal/controller/coderaiprovider_disable.go +++ b/internal/controller/coderaiprovider_disable.go @@ -55,6 +55,7 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( switch { case isCoderResourceNotFound(err): // Only Coder's own 404 proves that nothing can use the key (#327). + provider.Status.Enabled = nil return stalled("Provider %s does not exist in Coder.", provider.Name), nil case err != nil: return unchecked("get provider: %s", coderErrorSummary(err)), nil @@ -62,6 +63,7 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( if step := checkAIProviderAnswer(ctx, provider, found); step != nil { return step, nil } + provider.Status.Enabled = &found.Enabled if !found.Enabled { return stalled("Provider %s is disabled in Coder.", provider.Name), nil } @@ -89,6 +91,7 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( if step := checkAIProviderAnswer(ctx, provider, updated); step != nil { return step, nil } + provider.Status.Enabled = &updated.Enabled if updated.Enabled { return aiProviderWait(aiProviderRetry, "CoderAnswerMismatch", "Coder answered the disable of provider %s with an enabled provider. The controller disables it again.", provider.Name), nil diff --git a/internal/controller/coderaiprovider_disable_test.go b/internal/controller/coderaiprovider_disable_test.go index 12ba9517..c1f5dd0b 100644 --- a/internal/controller/coderaiprovider_disable_test.go +++ b/internal/controller/coderaiprovider_disable_test.go @@ -99,7 +99,9 @@ func TestCoderAIProviderDisablesWithoutConsent(t *testing.T) { editSecret(ctx, t, secret, func(s *corev1.Secret) { s.Labels, s.Annotations = consented.Labels, consented.Annotations }) writes = env.writes() got, _ = env.reconcile(ctx, t, got) - requireAIProviderStep(t, got, true, "DisabledExternally") + // #321: the controller disabled the provider, so the message + // names the kill switch, not only a Coder admin. + require.Contains(t, requireAIProviderStep(t, got, true, "DisabledExternally"), "by a Coder admin or by the controller's credential kill switch") require.Equal(t, writes, env.writes()) stored, _ = env.fake.aiProvider("openai") require.Equal(t, "sk-dashboard-0123456789abcdef", stored.keys[0].plaintext) diff --git a/internal/controller/coderaiprovider_env.go b/internal/controller/coderaiprovider_env.go index 24c8832b..99a385a0 100644 --- a/internal/controller/coderaiprovider_env.go +++ b/internal/controller/coderaiprovider_env.go @@ -9,6 +9,8 @@ import ( // coderd seeds AI providers from env and flags at startup (Coder v2.37.3 // codersdk/deployment.go:1929-2021, :4440-4561 and cli/server.go:3174-3176). +// TestCoderAIProviderSeedingFactsMatchVendoredCoder fails on a Coder bump +// until these facts are checked again. // A seeded provider row with another config stops coderd from starting // (coderd/ai_providers_migrate.go:76-146). // diff --git a/internal/controller/coderaiprovider_env_test.go b/internal/controller/coderaiprovider_env_test.go index fb4542e1..505dac1d 100644 --- a/internal/controller/coderaiprovider_env_test.go +++ b/internal/controller/coderaiprovider_env_test.go @@ -3,6 +3,8 @@ package controller_test import ( "context" "net/http" + "os" + "strings" "testing" "time" @@ -111,3 +113,32 @@ func TestCoderAIProviderPendingCreateBeforeEnvCheck(t *testing.T) { }) } } + +// aiProviderSeedingFactsChecked lists the Coder versions whose source backs +// the seeding facts in coderaiprovider_env.go. v2.37.8 has the same +// codersdk/deployment.go, cli/server.go, codersdk/aiproviders_bedrock.go and +// coderd/ai_providers_migrate.go as v2.37.3. +var aiProviderSeedingFactsChecked = []string{"v2.37.3", "v2.37.8"} + +// #321: a Coder bump fails here until someone re-checks the seeding facts +// against the new Coder source. +func TestCoderAIProviderSeedingFactsMatchVendoredCoder(t *testing.T) { + t.Parallel() + data, err := os.ReadFile("../../vendor/modules.txt") + require.NoError(t, err) + var version string + for line := range strings.Lines(string(data)) { + fields := strings.Fields(line) + if len(fields) >= 3 && fields[0] == "#" && fields[1] == "github.com/coder/coder/v2" { + require.Len(t, fields, 3, "assertion failed: a replaced Coder module needs a manual check of the seeding facts: %s", line) + version = fields[2] + } + } + require.NotEmpty(t, version, "assertion failed: vendor/modules.txt does not list github.com/coder/coder/v2") + require.Contains(t, aiProviderSeedingFactsChecked, version, + "The vendored Coder is %s, but the AI provider seeding facts in coderaiprovider_env.go were checked only at %v. "+ + "Re-check in the Coder %s source which env variables, flags and config file (YAML) keys seed providers: "+ + "codersdk/deployment.go, cli/server.go, codersdk/aiproviders_bedrock.go and coderd/ai_providers_migrate.go. "+ + "Update the facts, their tests and docs/how-to/manage-ai-providers.md#seeding-limits, then add %s to aiProviderSeedingFactsChecked.", + version, aiProviderSeedingFactsChecked, version, version) +} diff --git a/internal/controller/coderaiprovider_sync.go b/internal/controller/coderaiprovider_sync.go index 019ab901..07d0c797 100644 --- a/internal/controller/coderaiprovider_sync.go +++ b/internal/controller/coderaiprovider_sync.go @@ -50,6 +50,7 @@ func (r *CoderAIProviderReconciler) syncAIProvider( if step := checkAIProviderAnswer(ctx, provider, found); step != nil { return step, false, nil } + provider.Status.Enabled = &found.Enabled enabled := provider.Spec.Enabled == nil || *provider.Spec.Enabled keys := found.APIKeys @@ -61,8 +62,10 @@ func (r *CoderAIProviderReconciler) syncAIProvider( // resend that committed without an answer. Such a key stalls too. switch { case applied && enabled && !found.Enabled: + // The credential kill switch keeps appliedGeneration too, so the + // controller itself can have disabled the provider. return aiProviderStalled(aiProviderResync, "DisabledExternally", - "Provider %s was disabled in Coder. The controller does not enable it again until the spec changes.", provider.Name), false, nil + "Provider %s is disabled in Coder, by a Coder admin or by the controller's credential kill switch. The controller does not enable it again until the spec changes.", provider.Name), false, nil case applied && !ownKey && resending: return aiProviderStalled(aiProviderResync, "KeyChangedExternally", "The key of provider %s in Coder is not the recorded key: it was changed in Coder, or a key resend committed without an answer. The controller does not send its key again until the spec changes.", provider.Name), false, nil @@ -141,6 +144,7 @@ func (r *CoderAIProviderReconciler) syncAIProvider( if step := checkAIProviderAnswer(ctx, provider, updated); step != nil { return step, false, nil } + provider.Status.Enabled = &updated.Enabled if updated.DisplayName != cmp.Or(provider.Spec.DisplayName, provider.Name) || updated.Icon != provider.Spec.Icon || updated.Enabled != enabled || updated.BaseURL != provider.Spec.BaseURL { return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", @@ -168,6 +172,7 @@ func (r *CoderAIProviderReconciler) syncAIProvider( // providerGone handles a provider that is missing in Coder. After a full // apply it is a kill switch. Before one, the object creates it again. func (r *CoderAIProviderReconciler) providerGone(provider *coderv1alpha1.CoderAIProvider, applied bool) (*aiProviderStep, bool, error) { + provider.Status.Enabled = nil if applied { return aiProviderStalled(aiProviderResync, "DeletedExternally", "Provider %s (ID %s) was deleted in Coder. The controller does not create it again until the spec changes.", provider.Name, provider.Status.ProviderID), false, nil diff --git a/internal/controller/coderaiprovider_sync_test.go b/internal/controller/coderaiprovider_sync_test.go index eb385959..e02203dc 100644 --- a/internal/controller/coderaiprovider_sync_test.go +++ b/internal/controller/coderaiprovider_sync_test.go @@ -11,6 +11,7 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -657,3 +658,34 @@ func TestCoderAIProviderSyncUpdate404NotCoderRead(t *testing.T) { }) } } + +// #321: a key update interrupted before Coder commits leaves the resend +// marker, but status.observedGeneration must not claim the edit before the +// conditions describe it, or GitOps health checks report the old Ready. +func TestCoderAIProviderInterruptedUpdateKeepsObservedGeneration(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + readyGeneration := provider.Generation + provider = editSpec(ctx, t, provider, func(s *coderv1alpha1.CoderAIProviderSpec) { s.DisplayName = "OpenAI v2" }) + setSecretValue(ctx, t, secret, testAIProviderKeyRotated) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Hang: true}) + short, cancel := context.WithTimeout(ctx, hangTimeout) + defer cancel() + _, err := env.r.Reconcile(short, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.Error(t, err) + + stored := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), stored)) + require.NotNil(t, stored.Status.Credential.ResendAttemptTime, "the marker was written before the PATCH") + require.Equal(t, readyGeneration, stored.Status.ObservedGeneration, "the marker write does not observe the edit") + require.Equal(t, readyGeneration, meta.FindStatusCondition(stored.Status.Conditions, "Ready").ObservedGeneration) + coder, _ := env.fake.aiProvider("openai") + require.NotEqual(t, "OpenAI v2", coder.displayName) + + got, _ := env.reconcile(ctx, t, stored) + requireAIProviderReady(t, got) + coder, _ = env.fake.aiProvider("openai") + require.Equal(t, "OpenAI v2", coder.displayName) + require.Equal(t, testAIProviderKeyRotated, coder.keys[0].plaintext) +}