diff --git a/internal/controller/decofile_controller.go b/internal/controller/decofile_controller.go index 80456ae..7578c72 100644 --- a/internal/controller/decofile_controller.go +++ b/internal/controller/decofile_controller.go @@ -19,6 +19,7 @@ package controller import ( "context" "encoding/base64" + stderrors "errors" "fmt" "net/http" "time" @@ -307,6 +308,7 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // Notify pods if ConfigMap data changed var podsNotified bool var notificationError string + notificationReason := "NotificationFailed" if dataChanged { notifyStart := time.Now() @@ -316,9 +318,14 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c err = notifier.NotifyPodsForDecofile(ctx, decofile.Namespace, deploymentId, timestamp, jsonContent) notifyDuration := time.Since(notifyStart) if err != nil { - log.Error(err, "Failed to notify pods", "deploymentId", deploymentId, "duration", notifyDuration) notificationError = err.Error() podsNotified = false + if stderrors.Is(err, ErrMissingReloadToken) { + notificationReason = "MissingReloadToken" + log.Error(err, "Pods are missing DECO_RELEASE_RELOAD_TOKEN; the running revision cannot be hot-reloaded and must be redeployed so the mutating webhook re-injects the token", "deploymentId", deploymentId, "duration", notifyDuration) + } else { + log.Error(err, "Failed to notify pods", "deploymentId", deploymentId, "duration", notifyDuration) + } // Don't return error - update status with failure condition } else { log.Info("Successfully notified all pods", "timestamp", timestamp, "deploymentId", deploymentId, "duration", notifyDuration) @@ -376,11 +383,15 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c LastTransitionTime: metav1.Now(), } } else { + failMessage := fmt.Sprintf("Failed to notify pods for %s: %s", updateIdentifier, notificationError) + if notificationReason == "MissingReloadToken" { + failMessage = fmt.Sprintf("Pods for %s are missing DECO_RELEASE_RELOAD_TOKEN, so the fail-closed /.decofile/reload endpoint returns 401. Redeploy the site to create a revision with the token injected: %s", updateIdentifier, notificationError) + } podsNotifiedCondition = metav1.Condition{ Type: condTypePodsNotified, Status: metav1.ConditionFalse, - Reason: "NotificationFailed", - Message: fmt.Sprintf("Failed to notify pods for %s: %s", updateIdentifier, notificationError), + Reason: notificationReason, + Message: failMessage, LastTransitionTime: metav1.Now(), } } @@ -427,15 +438,15 @@ func updateCondition(decofile *decositesv1alpha1.Decofile, newCondition metav1.C // syncRevisionOwnerRefs ensures the Decofile carries an ownerReference for // every Knative Revision in the same namespace that targets it (matched via -// the app.deco/deploymentId label). When the Revision is later deleted — +// the app.deco/deploymentId label). When the Revision is later deleted -- // either by Knative GC (maxNonActiveRevisions) or by the knative-clean-* -// CronJobs — Kubernetes garbage collection cascades through Revision -> +// CronJobs -- Kubernetes garbage collection cascades through Revision -> // Decofile -> ConfigMap, so we never accumulate orphaned configuration. // // Multiple matching Revisions (e.g. during a rollback that reuses a // deploymentId) all become owners; the Decofile is only GC'd once every // owner Revision is gone. Stale UIDs are cleaned by Kubernetes GC on its -// own — we never remove ownerReferences here. +// own -- we never remove ownerReferences here. func (r *DecofileReconciler) syncRevisionOwnerRefs(ctx context.Context, decofile *decositesv1alpha1.Decofile) error { log := logf.FromContext(ctx) @@ -548,7 +559,7 @@ func (r *DecofileReconciler) mapRevisionToDecofile(ctx context.Context, obj clie // SetupWithManager sets up the controller with the Manager. func (r *DecofileReconciler) SetupWithManager(mgr ctrl.Manager) error { - // Only react to Revision Create — Updates and Deletes don't add new + // Only react to Revision Create -- Updates and Deletes don't add new // ownerRef linkage information (Kubernetes GC already handles deletes // via existing ownerReferences). Skipping them keeps the controller // from spinning on Status churn of running Revisions. diff --git a/internal/controller/notifier.go b/internal/controller/notifier.go index 4fab2a8..ccb0afd 100644 --- a/internal/controller/notifier.go +++ b/internal/controller/notifier.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "net/http" "strings" @@ -47,6 +48,19 @@ const ( idleConnTimeout = 90 * time.Second ) +// ErrMissingReloadToken indicates that the operator had no reload token to +// send for a target pod (no DECO_RELEASE_RELOAD_TOKEN in its env) AND the pod +// actually answered 401 -- i.e. a fail-closed /.decofile/reload endpoint +// (deco-cx/deco 51af3349) rejected the unauthenticated request. Such a +// revision cannot be hot-reloaded and must be redeployed so the mutating +// webhook injects the token into a fresh revision. +// +// Deliberately narrow: a 401 with a token present (token rejected) or a +// non-401 failure (5xx, connection reset / broken pipe) is NOT classified as +// this -- a 401 has other possible causes. Wrapped into the batch aggregate +// error so callers can branch on it via errors.Is. +var ErrMissingReloadToken = errors.New("target pod is missing DECO_RELEASE_RELOAD_TOKEN") + // NewHTTPClient creates a shared HTTP client with proper connection pooling configuration. // This client should be reused across all reconciliations to prevent memory leaks. func NewHTTPClient() *http.Client { @@ -186,6 +200,7 @@ func (n *Notifier) NotifyPodsForDecofile(ctx context.Context, namespace, deploym successCount := 0 failCount := 0 skippedCount := 0 + missingToken := false for i := 0; i < len(podNames); i++ { select { @@ -196,6 +211,9 @@ func (n *Notifier) NotifyPodsForDecofile(ctx context.Context, namespace, deploym log.V(1).Info("Pod no longer exists", "pod", result.podName) } else { failCount++ + if errors.Is(result.err, ErrMissingReloadToken) { + missingToken = true + } allErrors = append(allErrors, fmt.Sprintf("%s: %v", result.podName, result.err)) log.Error(result.err, "Failed to notify pod", "pod", result.podName) } @@ -211,6 +229,9 @@ func (n *Notifier) NotifyPodsForDecofile(ctx context.Context, namespace, deploym log.Info("Notification summary", "success", successCount, "failed", failCount, "skipped", skippedCount, "total", len(podNames)) if len(allErrors) > 0 { + if missingToken { + return fmt.Errorf("%w: failed to notify %d pod(s): %s", ErrMissingReloadToken, failCount, strings.Join(allErrors, "; ")) + } return fmt.Errorf("failed to notify %d pod(s): %s", failCount, strings.Join(allErrors, "; ")) } @@ -237,6 +258,7 @@ func (n *Notifier) notifyPodWithRetry(ctx context.Context, pod *corev1.Pod, time } backoff := initialBackoff + lastStatusCode := 0 for attempt := 1; attempt <= maxRetries; attempt++ { log.V(1).Info("Attempting to notify pod", "pod", pod.Name, "attempt", attempt, "timestamp", timestamp) @@ -256,6 +278,7 @@ func (n *Notifier) notifyPodWithRetry(ctx context.Context, pod *corev1.Pod, time if err == nil { // Read status code before closing body statusCode := resp.StatusCode + lastStatusCode = statusCode // Close body immediately - do NOT use defer inside loop to avoid connection leak if closeErr := resp.Body.Close(); closeErr != nil { log.Error(closeErr, "Failed to close response body", "pod", pod.Name) @@ -270,6 +293,17 @@ func (n *Notifier) notifyPodWithRetry(ctx context.Context, pod *corev1.Pod, time // If this was the last attempt, return the error if attempt == maxRetries { + // Only attribute the failure to a missing token when we have hard + // evidence: the operator sent no Authorization header (no token in the + // pod env) AND the pod actually answered 401 -- i.e. a fail-closed + // /.decofile/reload rejected the unauthenticated request. A 401 has + // other possible causes (e.g. a token that IS present but rejected, or + // an upstream block), and non-401 failures (5xx, connection reset / + // broken pipe) are not necessarily token-related -- leave those as + // generic failures. + if token == "" && lastStatusCode == http.StatusUnauthorized { + return fmt.Errorf("%w: max retries reached: %v", ErrMissingReloadToken, err) + } return fmt.Errorf("max retries reached: %w", err) } diff --git a/internal/webhook/v1/service_webhook.go b/internal/webhook/v1/service_webhook.go index 3351c0c..7b91bd8 100644 --- a/internal/webhook/v1/service_webhook.go +++ b/internal/webhook/v1/service_webhook.go @@ -265,12 +265,21 @@ func (d *ServiceCustomDefaulter) Default(ctx context.Context, obj runtime.Object return err } - // Find matching Decofile (non-blocking - allow Service creation even if not found) + // Find matching Decofile. Intentionally non-blocking: the Decofile CRD may + // not have propagated yet (e.g. cross-cluster fanout), and rejecting the + // Service here would break otherwise-valid deploys. + // + // Trade-off: a Service admitted on this path boots WITHOUT + // DECO_RELEASE_RELOAD_TOKEN. On a fail-closed runtime its POST + // /.decofile/reload returns 401 for every operator notification until a + // later revision injects the token -- an un-reloadable revision whose + // failure only surfaces on the next block-only publish. Log it loudly so it + // is diagnosable instead of silently created. decofile, err := d.findDecofileByDeploymentId(ctx, service.Namespace, deploymentId) if err != nil { - servicelog.Info("Decofile not found, skipping injection (Service will be created without Decofile)", - "service", service.Name, "deploymentId", deploymentId, "reason", err.Error()) - return nil // Allow Service creation + servicelog.Info("WARNING: decofile-inject requested but no matching Decofile found; Service will be created WITHOUT a reload token and cannot be hot-reloaded (POST /.decofile/reload will 401) until redeployed with the token injected", + "service", service.Name, "namespace", service.Namespace, "deploymentId", deploymentId, "reason", err.Error()) + return nil // Allow Service creation (non-blocking) } // Get mount path from annotation or use default directory