From 09b4be088dda8ccdccdc4cfd57d551b0b287e812 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 2 Jul 2026 12:25:31 +0300 Subject: [PATCH 1/5] Add lastTrackedRevision field to the Canary status API The field records the server-side change fence of the target workload as /[/], where the counter is metadata.generation for Deployments and DaemonSets, resourceVersion for Services and the latest created revision name for Knative Services. It identifies the exact object state that lastAppliedSpec was computed from, allowing hash drift caused by Flagger or cluster upgrades to be told apart from real spec changes. No codegen changes required: the generated deepcopy covers the string field via the struct value copy. Signed-off-by: Stefan Prodan --- pkg/apis/flagger/v1beta1/status.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/apis/flagger/v1beta1/status.go b/pkg/apis/flagger/v1beta1/status.go index 9b0327160..cea774be8 100644 --- a/pkg/apis/flagger/v1beta1/status.go +++ b/pkg/apis/flagger/v1beta1/status.go @@ -85,6 +85,14 @@ type CanaryStatus struct { LastAppliedSpec string `json:"lastAppliedSpec,omitempty"` // +optional LastPromotedSpec string `json:"lastPromotedSpec,omitempty"` + // LastTrackedRevision is the server-side change fence for the target workload, + // recorded as /[/]. The counter is + // metadata.generation for Deployments/DaemonSets and metadata.resourceVersion + // for Services. It identifies the exact object state that LastAppliedSpec + // was computed from, allowing hash drift caused by Flagger or cluster + // upgrades to be absorbed without triggering a canary analysis. + // +optional + LastTrackedRevision string `json:"lastTrackedRevision,omitempty"` // +optional LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"` // +optional From 57759c11d3268ba84bffedf16cd0952cdaae57d6 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 2 Jul 2026 12:25:32 +0300 Subject: [PATCH 2/5] Add lastTrackedRevision to the Canary CRD status schema Additive change applied to all three CRD copies (artifacts, Helm chart and kustomize base). Signed-off-by: Stefan Prodan --- artifacts/flagger/crd.yaml | 3 +++ charts/flagger/crds/crd.yaml | 3 +++ kustomize/base/flagger/crd.yaml | 3 +++ 3 files changed, 9 insertions(+) diff --git a/artifacts/flagger/crd.yaml b/artifacts/flagger/crd.yaml index e02f5aa0e..aab521409 100644 --- a/artifacts/flagger/crd.yaml +++ b/artifacts/flagger/crd.yaml @@ -1276,6 +1276,9 @@ spec: lastPromotedSpec: description: LastPromotedSpec of this canary type: string + lastTrackedRevision: + description: LastTrackedRevision is the change fence of the canary target + type: string lastTransitionTime: description: LastTransitionTime of this canary format: date-time diff --git a/charts/flagger/crds/crd.yaml b/charts/flagger/crds/crd.yaml index e02f5aa0e..aab521409 100644 --- a/charts/flagger/crds/crd.yaml +++ b/charts/flagger/crds/crd.yaml @@ -1276,6 +1276,9 @@ spec: lastPromotedSpec: description: LastPromotedSpec of this canary type: string + lastTrackedRevision: + description: LastTrackedRevision is the change fence of the canary target + type: string lastTransitionTime: description: LastTransitionTime of this canary format: date-time diff --git a/kustomize/base/flagger/crd.yaml b/kustomize/base/flagger/crd.yaml index e02f5aa0e..aab521409 100644 --- a/kustomize/base/flagger/crd.yaml +++ b/kustomize/base/flagger/crd.yaml @@ -1276,6 +1276,9 @@ spec: lastPromotedSpec: description: LastPromotedSpec of this canary type: string + lastTrackedRevision: + description: LastTrackedRevision is the change fence of the canary target + type: string lastTransitionTime: description: LastTransitionTime of this canary format: date-time From 69bfaffcdac3e9ea6e645ae345d12718ce822e69 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 2 Jul 2026 12:25:32 +0300 Subject: [PATCH 3/5] Replace spew spec hashing with content hash and change fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flagger detected target changes by comparing spew-based hashes of decoded Go structs. Spew prints every struct field, so the hash encoded the k8s.io/api schema and changed on every dependency upgrade, triggering spurious canary analyses for unchanged workloads (#1673). The spec hash is now computed from the canonical JSON representation (unset fields omitted, sorted keys, sha256, 'v2:' prefix) and stored together with the change fence in status.lastTrackedRevision. When the fence proves the spec did not change, a differing hash is an upgrade artifact and is absorbed by re-baselining the status; when the fence moved, hashes decide between replicas-only changes, manual rollbacks and real changes. For Deployments, a revision annotation component (recorded only while rollouts are not paused and the deployment controller has observed the current generation) lets scale-only generation bumps coincide with hash drift without false triggers. Volatile counters (Service resourceVersion) are never chased with status writes, and untrusted fences (generation 0, empty uid) fall back to hash comparison and never absorb. Flagger's own scale-down writes refresh the fence from the write response, guarded so a concurrent change is never absorbed. Canaries with legacy status migrate once: settled phases adopt the current state as baseline (guarded by a canary-vs-primary image comparison for Deployments/DaemonSets and a latest-vs-promoted revision comparison for Knative), while in-flight and failed canaries restart their analysis once — loud but safe. All new status writes go through a DeepCopy and abort when a concurrent writer moved the tracked spec, so the in-memory canary object is never mutated and the (lastAppliedSpec, lastTrackedRevision) pair always describes the same observed object state. Signed-off-by: Stefan Prodan --- pkg/canary/daemonset_controller.go | 71 ++++++-- pkg/canary/daemonset_status.go | 17 +- pkg/canary/deployment_controller.go | 74 ++++++++- pkg/canary/deployment_status.go | 7 +- pkg/canary/factory.go | 1 + pkg/canary/knative_controller.go | 22 ++- pkg/canary/service_controller.go | 36 ++++- pkg/canary/spec.go | 243 ++++++++++++++++++++++++++-- pkg/canary/status.go | 78 ++++++++- 9 files changed, 507 insertions(+), 42 deletions(-) diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index 5b8f92e87..ab50ca402 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -37,6 +37,26 @@ var ( daemonSetScaleDownNodeSelector = map[string]string{"flagger.app/scale-to-zero": "true"} ) +// daemonSetSnapshot returns the content hash of the daemonset pod template — +// normalized so that Flagger's own scale-to-zero node selector does not +// register as a spec change — together with its change fence. +func daemonSetSnapshot(dae *appsv1.DaemonSet) (targetSnapshot, error) { + template := dae.Spec.Template.DeepCopy() + for key := range daemonSetScaleDownNodeSelector { + delete(template.Spec.NodeSelector, key) + } + + hash, err := ComputeSpecHash(template) + if err != nil { + return targetSnapshot{}, fmt.Errorf("daemonset %s.%s: %w", dae.Name, dae.Namespace, err) + } + + return targetSnapshot{ + hash: hash, + fence: encodeFence(dae.UID, fmt.Sprintf("%d", dae.Generation), ""), + }, nil +} + // DaemonSetController is managing the operations for Kubernetes DaemonSet kind type DaemonSetController struct { kubeClient kubernetes.Interface @@ -65,13 +85,33 @@ func (c *DaemonSetController) ScaleToZero(cd *flaggerv1.Canary) error { daeCopy.Spec.Template.Spec.NodeSelector[k] = v } - _, err = c.kubeClient.AppsV1().DaemonSets(dae.Namespace).Update(context.TODO(), daeCopy, metav1.UpdateOptions{}) + scaled, err := c.kubeClient.AppsV1().DaemonSets(dae.Namespace).Update(context.TODO(), daeCopy, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("updating daemonset %s.%s failed: %w", daeCopy.GetName(), daeCopy.Namespace, err) } + c.refreshTracking(cd, scaled) return nil } +// refreshTracking keeps the canary status fence current after Flagger's own +// scaling writes, which advance the daemonset generation while leaving the +// normalized pod template unchanged. The snapshot is taken from the object +// returned by the write (not a re-read, which could observe a concurrent user +// change); refreshTrackedRevision only moves the fence when the recorded +// lastAppliedSpec still matches the template hash, so a concurrent template +// change is never marked as seen. Failures are non-fatal: a stale fence +// degrades to hash comparison, never to absorption. +func (c *DaemonSetController) refreshTracking(cd *flaggerv1.Canary, dae *appsv1.DaemonSet) { + snap, err := daemonSetSnapshot(dae) + if err == nil { + err = refreshTrackedRevision(c.flaggerClient, cd, snap) + } + if err != nil { + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Warnf("failed to refresh tracked revision: %v", err) + } +} + func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error { targetName := cd.Spec.TargetRef.Name dep, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(context.TODO(), targetName, metav1.GetOptions{}) @@ -84,6 +124,9 @@ func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error { delete(depCopy.Spec.Template.Spec.NodeSelector, k) } + // no fence refresh here: this path is followed by a SyncStatus write in + // the scheduler, and a transiently stale fence self-heals through the + // fence-refresh rule on the next change detection _, err = c.kubeClient.AppsV1().DaemonSets(dep.Namespace).Update(context.TODO(), depCopy, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("scaling up daemonset %s.%s failed: %w", depCopy.GetName(), depCopy.Namespace, err) @@ -192,17 +235,27 @@ func (c *DaemonSetController) HasTargetChanged(cd *flaggerv1.Canary) (bool, erro return false, fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } - // ignore `daemonSetScaleDownNodeSelector` node selector - for key := range daemonSetScaleDownNodeSelector { - delete(canary.Spec.Template.Spec.NodeSelector, key) + snap, err := daemonSetSnapshot(canary) + if err != nil { + return false, err } - // since nil and capacity zero map would have different hash, we have to initialize here - if canary.Spec.Template.Spec.NodeSelector == nil { - canary.Spec.Template.Spec.NodeSelector = map[string]string{} - } + return hasSpecChanged(c.logger, c.flaggerClient, cd, snap, func() (bool, error) { + return c.canaryImagesDiffer(cd, canary) + }) +} - return hasSpecChanged(cd, canary.Spec.Template) +// canaryImagesDiffer compares the canary container images against the primary +// ones; used as a safety heuristic when migrating the spec tracking from a +// previous Flagger version, to catch rollouts applied while Flagger was not +// running. +func (c *DaemonSetController) canaryImagesDiffer(cd *flaggerv1.Canary, canary *appsv1.DaemonSet) (bool, error) { + primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) + primary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(context.TODO(), primaryName, metav1.GetOptions{}) + if err != nil { + return false, fmt.Errorf("daemonset %s.%s get query error: %w", primaryName, cd.Namespace, err) + } + return podImagesDiffer(canary.Spec.Template.Spec, primary.Spec.Template.Spec), nil } // GetMetadata returns the pod label selector and svc ports diff --git a/pkg/canary/daemonset_status.go b/pkg/canary/daemonset_status.go index 746d612b7..77575b9ea 100644 --- a/pkg/canary/daemonset_status.go +++ b/pkg/canary/daemonset_status.go @@ -32,22 +32,17 @@ func (c *DaemonSetController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1. return fmt.Errorf("daemonset %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } - // ignore `daemonSetScaleDownNodeSelector` node selector - for key := range daemonSetScaleDownNodeSelector { - delete(dae.Spec.Template.Spec.NodeSelector, key) - } - - // since nil and capacity zero map would have different hash, we have to initialize here - if dae.Spec.Template.Spec.NodeSelector == nil { - dae.Spec.Template.Spec.NodeSelector = map[string]string{} - } - configs, err := c.configTracker.GetConfigRefs(cd) if err != nil { return fmt.Errorf("GetConfigRefs failed: %w", err) } - return syncCanaryStatus(c.flaggerClient, cd, status, dae.Spec.Template, func(cdCopy *flaggerv1.Canary) { + snap, err := daemonSetSnapshot(dae) + if err != nil { + return err + } + + return syncCanaryStatus(c.flaggerClient, cd, status, snap, func(cdCopy *flaggerv1.Canary) { cdCopy.Status.TrackedConfigs = configs }) } diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index d8f8a946d..7202caa6c 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -44,6 +44,33 @@ type DeploymentController struct { includeLabelPrefix []string } +// deploymentRevisionAnnotation is set by the Kubernetes deployment controller +// and incremented only when the pod template changes (a rollout). +const deploymentRevisionAnnotation = "deployment.kubernetes.io/revision" + +// deploymentSnapshot returns the content hash of the deployment pod template +// together with its change fence. The revision component is recorded only +// while it provably corresponds to the current template: the revision +// annotation is written asynchronously by the deployment controller, so it is +// trusted only when rollouts are not paused and the controller has observed +// the current generation. +func deploymentSnapshot(dep *appsv1.Deployment) (targetSnapshot, error) { + hash, err := ComputeSpecHash(&dep.Spec.Template) + if err != nil { + return targetSnapshot{}, fmt.Errorf("deployment %s.%s: %w", dep.Name, dep.Namespace, err) + } + + revision := "" + if !dep.Spec.Paused && dep.Status.ObservedGeneration == dep.Generation { + revision = dep.Annotations[deploymentRevisionAnnotation] + } + + return targetSnapshot{ + hash: hash, + fence: encodeFence(dep.UID, fmt.Sprintf("%d", dep.Generation), revision), + }, nil +} + // Initialize creates the primary deployment if it does not exist. func (c *DeploymentController) Initialize(cd *flaggerv1.Canary) (bool, error) { if err := c.createPrimaryDeployment(cd, c.includeLabelPrefix); err != nil { @@ -144,7 +171,27 @@ func (c *DeploymentController) HasTargetChanged(cd *flaggerv1.Canary) (bool, err return false, fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } - return hasSpecChanged(cd, canary.Spec.Template) + snap, err := deploymentSnapshot(canary) + if err != nil { + return false, err + } + + return hasSpecChanged(c.logger, c.flaggerClient, cd, snap, func() (bool, error) { + return c.canaryImagesDiffer(cd, canary) + }) +} + +// canaryImagesDiffer compares the canary container images against the primary +// ones; used as a safety heuristic when migrating the spec tracking from a +// previous Flagger version, to catch rollouts applied while Flagger was not +// running. +func (c *DeploymentController) canaryImagesDiffer(cd *flaggerv1.Canary, canary *appsv1.Deployment) (bool, error) { + primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) + primary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(context.TODO(), primaryName, metav1.GetOptions{}) + if err != nil { + return false, fmt.Errorf("deployment %s.%s get query error: %w", primaryName, cd.Namespace, err) + } + return podImagesDiffer(canary.Spec.Template.Spec, primary.Spec.Template.Spec), nil } // ScaleToZero Scale sets the canary deployment replicas @@ -156,13 +203,33 @@ func (c *DeploymentController) ScaleToZero(cd *flaggerv1.Canary) error { } patch := []byte(fmt.Sprintf(`{"spec":{"replicas": %d}}`, 0)) - _, err = c.kubeClient.AppsV1().Deployments(dep.Namespace).Patch(context.TODO(), dep.GetName(), types.MergePatchType, patch, metav1.PatchOptions{}) + scaled, err := c.kubeClient.AppsV1().Deployments(dep.Namespace).Patch(context.TODO(), dep.GetName(), types.MergePatchType, patch, metav1.PatchOptions{}) if err != nil { return fmt.Errorf("deployment %s.%s patch query error: %w", targetName, cd.Namespace, err) } + c.refreshTracking(cd, scaled) return nil } +// refreshTracking keeps the canary status fence current after Flagger's own +// scaling writes, which advance the deployment generation without changing +// the pod template. The snapshot is taken from the object returned by the +// write (not a re-read, which could observe a concurrent user change); +// refreshTrackedRevision only moves the fence when the recorded +// lastAppliedSpec still matches the template hash, so a concurrent template +// change is never marked as seen. Failures are non-fatal: a stale fence +// degrades to hash comparison, never to absorption. +func (c *DeploymentController) refreshTracking(cd *flaggerv1.Canary, dep *appsv1.Deployment) { + snap, err := deploymentSnapshot(dep) + if err == nil { + err = refreshTrackedRevision(c.flaggerClient, cd, snap) + } + if err != nil { + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Warnf("failed to refresh tracked revision: %v", err) + } +} + func (c *DeploymentController) ScaleFromZero(cd *flaggerv1.Canary) error { targetName := cd.Spec.TargetRef.Name dep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(context.TODO(), targetName, metav1.GetOptions{}) @@ -203,6 +270,9 @@ func (c *DeploymentController) ScaleFromZero(cd *flaggerv1.Canary) error { } patch := []byte(fmt.Sprintf(`{"spec":{"replicas": %d}}`, *replicas)) + // no fence refresh here: this path is followed by a SyncStatus write in + // the scheduler, and a transiently stale fence self-heals through the + // fence-refresh rule on the next change detection _, err = c.kubeClient.AppsV1().Deployments(dep.Namespace).Patch(context.TODO(), dep.GetName(), types.MergePatchType, patch, metav1.PatchOptions{}) if err != nil { return fmt.Errorf("scaling up %s.%s to %d failed: %v", dep.GetName(), dep.Namespace, *replicas, err) diff --git a/pkg/canary/deployment_status.go b/pkg/canary/deployment_status.go index 46027499d..49ada8a09 100644 --- a/pkg/canary/deployment_status.go +++ b/pkg/canary/deployment_status.go @@ -37,7 +37,12 @@ func (c *DeploymentController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1 return fmt.Errorf("GetConfigRefs failed: %w", err) } - return syncCanaryStatus(c.flaggerClient, cd, status, dep.Spec.Template, func(cdCopy *flaggerv1.Canary) { + snap, err := deploymentSnapshot(dep) + if err != nil { + return err + } + + return syncCanaryStatus(c.flaggerClient, cd, status, snap, func(cdCopy *flaggerv1.Canary) { cdCopy.Status.TrackedConfigs = configs }) } diff --git a/pkg/canary/factory.go b/pkg/canary/factory.go index 27344771a..37b20ceb1 100644 --- a/pkg/canary/factory.go +++ b/pkg/canary/factory.go @@ -77,6 +77,7 @@ func (factory *Factory) Controller(obj v1beta1.LocalObjectReference) Controller includeLabelPrefix: factory.includeLabelPrefix, } knativeCtrl := &KnativeController{ + logger: factory.logger, flaggerClient: factory.flaggerClient, knativeClient: factory.knativeClient, } diff --git a/pkg/canary/knative_controller.go b/pkg/canary/knative_controller.go index a69a33fb0..1c2da4f52 100644 --- a/pkg/canary/knative_controller.go +++ b/pkg/canary/knative_controller.go @@ -58,13 +58,26 @@ func (kc *KnativeController) GetMetadata(canary *flaggerv1.Canary) (string, stri return "", "", make(map[string]int32), nil } +// knativeSnapshot returns the content hash of the latest created revision +// name together with its change fence. The hash input lives in the service +// status, which changes without bumping metadata.generation, so the fence +// counter is the revision name itself: the fence then matches exactly when +// the hash input is unchanged, making absorption safe for algorithm changes +// only. +func knativeSnapshot(service *serving.Service) targetSnapshot { + return targetSnapshot{ + hash: ComputeStringHash(service.Status.LatestCreatedRevisionName), + fence: encodeFence(service.UID, service.Status.LatestCreatedRevisionName, ""), + } +} + // SyncStatus encodes list of revisions and updates the canary status func (kc *KnativeController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1.CanaryStatus) error { service, err := kc.knativeClient.ServingV1().Services(cd.Namespace).Get(context.TODO(), cd.Spec.TargetRef.Name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("Knative Service %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } - return syncCanaryStatus(kc.flaggerClient, cd, status, service.Status.LatestCreatedRevisionName, func(copy *flaggerv1.Canary) {}) + return syncCanaryStatus(kc.flaggerClient, cd, status, knativeSnapshot(service), func(copy *flaggerv1.Canary) {}) } // SetStatusFailedChecks updates the canary failed checks counter @@ -141,7 +154,12 @@ func (kc *KnativeController) HasTargetChanged(cd *flaggerv1.Canary) (bool, error if err != nil { return true, fmt.Errorf("Knative Service %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } - return hasSpecChanged(cd, service.Status.LatestCreatedRevisionName) + // the migration heuristic compares the latest revision with the promoted + // one recorded on the service, catching revisions created while Flagger + // was not running + return hasSpecChanged(kc.logger, kc.flaggerClient, cd, knativeSnapshot(service), func() (bool, error) { + return service.Status.LatestCreatedRevisionName != service.Annotations["flagger.app/primary-revision"], nil + }) } func (kc *KnativeController) HaveDependenciesChanged(canary *flaggerv1.Canary) (bool, error) { diff --git a/pkg/canary/service_controller.go b/pkg/canary/service_controller.go index 68b705937..3abc625d8 100644 --- a/pkg/canary/service_controller.go +++ b/pkg/canary/service_controller.go @@ -222,14 +222,39 @@ func (c *ServiceController) Promote(cd *flaggerv1.Canary) error { return nil } -// HasServiceChanged returns true if the canary service spec has changed +// serviceSnapshot returns the content hash of the service spec together with +// its change fence. Services do not increment metadata.generation, so the +// fence uses resourceVersion: it moves on every write (including status +// updates by other controllers), which limits how often drift can be +// absorbed, but "resourceVersion unchanged" always proves the object is +// byte-identical, so absorption can never swallow a real change. +func serviceSnapshot(svc *corev1.Service) (targetSnapshot, error) { + hash, err := ComputeSpecHash(&svc.Spec) + if err != nil { + return targetSnapshot{}, fmt.Errorf("service %s.%s: %w", svc.Name, svc.Namespace, err) + } + + return targetSnapshot{ + hash: hash, + fence: encodeFence(svc.UID, svc.ResourceVersion, ""), + volatileCounter: true, + }, nil +} + +// HasTargetChanged returns true if the canary service spec has changed func (c *ServiceController) HasTargetChanged(cd *flaggerv1.Canary) (bool, error) { targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(context.TODO(), targetName, metav1.GetOptions{}) if err != nil { return false, fmt.Errorf("service %s.%s get query error: %w", targetName, cd.Namespace, err) } - return hasSpecChanged(cd, canary.Spec) + + snap, err := serviceSnapshot(canary) + if err != nil { + return false, err + } + + return hasSpecChanged(c.logger, c.flaggerClient, cd, snap, nil) } // Scale sets the canary deployment replicas @@ -247,7 +272,12 @@ func (c *ServiceController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1.Ca return fmt.Errorf("service %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } - return syncCanaryStatus(c.flaggerClient, cd, status, dep.Spec, func(cdCopy *flaggerv1.Canary) {}) + snap, err := serviceSnapshot(dep) + if err != nil { + return err + } + + return syncCanaryStatus(c.flaggerClient, cd, status, snap, func(cdCopy *flaggerv1.Canary) {}) } func (c *ServiceController) HaveDependenciesChanged(_ *flaggerv1.Canary) (bool, error) { diff --git a/pkg/canary/spec.go b/pkg/canary/spec.go index ff86c5b54..acdab2669 100644 --- a/pkg/canary/spec.go +++ b/pkg/canary/spec.go @@ -17,40 +17,265 @@ limitations under the License. package canary import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "encoding/json" "fmt" "hash/fnv" + "strings" "github.com/davecgh/go-spew/spew" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" + clientset "github.com/fluxcd/flagger/pkg/client/clientset/versioned" ) -// hasSpecChanged computes the hash of the spec and compares it with the -// last applied spec, if the last applied hash is different but not equal -// to last promoted one the it returns true -func hasSpecChanged(cd *flaggerv1.Canary, spec interface{}) (bool, error) { - if cd.Status.LastAppliedSpec == "" { +// specHashPrefix marks hashes computed from the canonical JSON representation +// of the target spec. Hashes without the prefix were computed by previous +// Flagger versions with spew over the decoded Go structs; those encode the +// k8s.io/api struct schema and are not comparable across binaries, so they +// are migrated by migrateSpecTracking instead of being compared. +const specHashPrefix = "v2:" + +// targetSnapshot captures the state of a canary target used for change +// detection. Both fields are taken from the same object read, so that +// (status.lastAppliedSpec, status.lastTrackedRevision) always forms a +// consistent pair: the hash describes the object at exactly that revision. +type targetSnapshot struct { + // hash is the content hash of the normalized target spec. + hash string + // fence is /[/], where counter is a + // server-maintained change indicator: metadata.generation for + // Deployments and DaemonSets, metadata.resourceVersion for Services and + // the latest created revision name for Knative Services. The optional + // third component is the deployment.kubernetes.io/revision annotation, + // recorded only while it provably corresponds to the hashed template + // (deployment not paused and observedGeneration caught up). + fence string + // volatileCounter marks counters that move on writes unrelated to the + // spec (resourceVersion changes on status updates and metadata churn); + // chasing such a counter would cost one status write per reconcile, so + // the fence is only refreshed by the regular status syncs. + volatileCounter bool +} + +// hasSpecChanged compares the target snapshot against the tracking fields in +// the canary status and decides whether a new revision must be analyzed. +// The fence tells apart real spec writes from hash drift: when the fence +// proves the spec did not change, a differing hash is an artifact of a +// Flagger or cluster upgrade and is absorbed by re-baselining the status. +// Absorb, fence-refresh and migration decisions are persisted through the +// status subresource without mutating cd in place; a failed write only delays +// persistence until the next reconcile, it never changes the verdict. +func hasSpecChanged(logger *zap.SugaredLogger, flaggerClient clientset.Interface, + cd *flaggerv1.Canary, snap targetSnapshot, imagesDiffer func() (bool, error)) (bool, error) { + logger = logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)) + + applied := cd.Status.LastAppliedSpec + if applied == "" { return true, nil } - newHash := ComputeHash(spec) + // status written by a previous Flagger version + if !strings.HasPrefix(applied, specHashPrefix) || cd.Status.LastTrackedRevision == "" { + return migrateSpecTracking(logger, flaggerClient, cd, snap, imagesDiffer) + } + + storedUID, storedCounter, storedRevision := parseFence(cd.Status.LastTrackedRevision) + currentUID, currentCounter, currentRevision := parseFence(snap.fence) + + // a deleted and recreated target is always a new revision + if storedUID != currentUID { + return true, nil + } + + // the fence is only trustworthy when the server actually maintains the + // counter (generation is 0 when unset, e.g. on fake clients); an + // untrusted fence must behave as "changed counter" so decisions fall + // back to hash comparison and never absorb + fenceCapable := currentUID != "" && currentCounter != "" && currentCounter != "0" + + sameCounter := fenceCapable && storedCounter == currentCounter + + if snap.hash == applied { + // content already tracked; keep the whole fence current (a + // replicas-only change moves the counter, and the deployment + // revision component may become available once the deployment + // controller catches up) so future hash drift can be absorbed + if !snap.volatileCounter && cd.Status.LastTrackedRevision != snap.fence { + persistOrLog(logger, flaggerClient, cd, snap, "fence refreshed") + } + return false, nil + } + + // the hash differs from the last applied spec: absorb the difference iff + // the fence proves that no spec change happened, which makes the + // difference an artifact of a hash algorithm or schema change + specUnchanged := sameCounter || + (fenceCapable && storedRevision != "" && currentRevision != "" && storedRevision == currentRevision) + if specUnchanged { + persistOrLog(logger, flaggerClient, cd, snap, "hash drift absorbed") + return false, nil + } // do not trigger a canary deployment on manual rollback - if cd.Status.LastPromotedSpec == newHash { + if snap.hash == cd.Status.LastPromotedSpec { + persistOrLog(logger, flaggerClient, cd, snap, "manual rollback detected") return false, nil } - if cd.Status.LastAppliedSpec != newHash { + return true, nil +} + +// migrateSpecTracking handles canaries whose status was written by a previous +// Flagger version (spew hash, no fence). Legacy hashes cannot be compared +// with the current ones, so the decision is made by phase: settled canaries +// adopt the current target state as their baseline without triggering, while +// canaries with an analysis in flight or in a failed state are reported as +// changed, restarting the analysis once after the upgrade — loud but safe, +// since adopting mid-run could silently swallow a change made while Flagger +// was down. Waiting, Promoting and Finalising canaries never reach this code: +// the scheduler advances them without checking for target changes, and their +// next status sync records the new tracking format. +func migrateSpecTracking(logger *zap.SugaredLogger, flaggerClient clientset.Interface, + cd *flaggerv1.Canary, snap targetSnapshot, imagesDiffer func() (bool, error)) (bool, error) { + switch cd.Status.Phase { + case flaggerv1.CanaryPhaseInitialized, flaggerv1.CanaryPhaseSucceeded: + if imagesDiffer != nil { + differ, err := imagesDiffer() + if err != nil { + // neither adopt (could swallow a change applied while + // Flagger was down) nor trigger (could start a spurious + // rollout); the reconciler retries on the next sync + return false, fmt.Errorf("spec tracking migration blocked, image comparison failed: %w", err) + } + if differ { + // the canary images differ from the primary ones, a rollout + // was most likely applied while Flagger was not running + return true, nil + } + } + persistOrLog(logger, flaggerClient, cd, snap, "spec tracking migrated") + return false, nil + default: return true, nil } +} + +// persistOrLog persists the (hash, fence) pair with persistSpecTracking and +// logs the outcome; persistence failures are not fatal, the decision is +// simply re-made and re-persisted on the next reconcile. +func persistOrLog(logger *zap.SugaredLogger, flaggerClient clientset.Interface, + cd *flaggerv1.Canary, snap targetSnapshot, reason string) { + if err := persistSpecTracking(flaggerClient, cd, snap); err != nil { + logger.Warnf("failed to persist spec tracking (%s), retrying next reconcile: %v", reason, err) + return + } + logger.Infof("spec tracking updated: %s", reason) +} - return false, nil +// encodeFence builds the lastTrackedRevision value from the target's uid, a +// server-maintained change counter and an optional deployment revision. +func encodeFence(uid types.UID, counter, revision string) string { + if revision != "" { + return fmt.Sprintf("%s/%s/%s", uid, counter, revision) + } + return fmt.Sprintf("%s/%s", uid, counter) +} + +// parseFence splits a lastTrackedRevision value into its components; missing +// components are returned as empty strings, which never compare as matching. +func parseFence(fence string) (uid, counter, revision string) { + parts := strings.SplitN(fence, "/", 3) + if len(parts) > 0 { + uid = parts[0] + } + if len(parts) > 1 { + counter = parts[1] + } + if len(parts) > 2 { + revision = parts[2] + } + return +} + +// ComputeSpecHash returns the content hash of a Kubernetes object spec, +// computed from its canonical JSON representation: unset fields are omitted +// and map keys are sorted, so the hash does not change when the k8s.io/api +// structs gain new fields on a Flagger dependency upgrade — only when the +// object content changes. +func ComputeSpecHash(spec interface{}) (string, error) { + data, err := json.Marshal(spec) + if err != nil { + return "", fmt.Errorf("spec hash: marshal failed: %w", err) + } + + // round-trip through an untyped value so that keys are re-serialized in + // sorted order, independent of the Go struct field order; UseNumber + // preserves the exact numeric representation + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + var untyped interface{} + if err := decoder.Decode(&untyped); err != nil { + return "", fmt.Errorf("spec hash: decode failed: %w", err) + } + canonical, err := json.Marshal(untyped) + if err != nil { + return "", fmt.Errorf("spec hash: canonical marshal failed: %w", err) + } + + return hashBytes(canonical), nil +} + +// ComputeStringHash returns the content hash of a plain string identifier +// (e.g. a Knative revision name); strings must not go through the JSON +// canonicalization used for object specs. +func ComputeStringHash(s string) string { + return hashBytes([]byte(s)) +} + +func hashBytes(b []byte) string { + digest := sha256.Sum256(b) + return specHashPrefix + hex.EncodeToString(digest[:8]) +} + +// podImagesDiffer compares the init and regular container images of two pod +// specs by container name; used as a safety heuristic during spec tracking +// migration to catch rollouts applied while Flagger was not running. +func podImagesDiffer(a, b corev1.PodSpec) bool { + am, bm := podImages(a), podImages(b) + if len(am) != len(bm) { + return true + } + for name, image := range am { + if bm[name] != image { + return true + } + } + return false +} + +func podImages(spec corev1.PodSpec) map[string]string { + images := make(map[string]string, len(spec.Containers)+len(spec.InitContainers)) + for _, c := range spec.InitContainers { + images["init:"+c.Name] = c.Image + } + for _, c := range spec.Containers { + images[c.Name] = c.Image + } + return images } // ComputeHash returns a hash value calculated from a spec using the spew library // which follows pointers and prints actual values of the nested objects // ensuring the hash does not change when a pointer changes. +// Deprecated for spec change detection (the output depends on the k8s.io/api +// struct schema, see ComputeSpecHash); still used for event payload UUIDs. func ComputeHash(spec interface{}) string { hasher := fnv.New32a() printer := spew.ConfigState{ diff --git a/pkg/canary/status.go b/pkg/canary/status.go index ec3b60cac..ae65fee38 100644 --- a/pkg/canary/status.go +++ b/pkg/canary/status.go @@ -29,9 +29,7 @@ import ( clientset "github.com/fluxcd/flagger/pkg/client/clientset/versioned" ) -func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, status flaggerv1.CanaryStatus, canaryResource interface{}, setAll func(cdCopy *flaggerv1.Canary)) error { - hash := ComputeHash(canaryResource) - +func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, status flaggerv1.CanaryStatus, snap targetSnapshot, setAll func(cdCopy *flaggerv1.Canary)) error { firstTry := true name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { @@ -47,9 +45,10 @@ func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, s cdCopy.Status.CanaryWeight = status.CanaryWeight cdCopy.Status.FailedChecks = status.FailedChecks cdCopy.Status.Iterations = status.Iterations - cdCopy.Status.LastAppliedSpec = hash + cdCopy.Status.LastAppliedSpec = snap.hash + cdCopy.Status.LastTrackedRevision = snap.fence if status.Phase == flaggerv1.CanaryPhaseInitialized { - cdCopy.Status.LastPromotedSpec = hash + cdCopy.Status.LastPromotedSpec = snap.hash } cdCopy.Status.LastTransitionTime = metav1.Now() setAll(cdCopy) @@ -69,6 +68,75 @@ func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, s return nil } +// persistSpecTracking records the (lastAppliedSpec, lastTrackedRevision) pair +// after a hash drift absorption, fence refresh, manual rollback or spec +// tracking migration decision, and rewrites lastPromotedSpec in lockstep when +// it pointed at the same spec as lastAppliedSpec (idle canary). The write is +// based on the status values the decision was computed from and is aborted if +// another writer changed them in the meantime; cd is never mutated in place. +func persistSpecTracking(flaggerClient clientset.Interface, cd *flaggerv1.Canary, snap targetSnapshot) error { + name, ns := cd.GetName(), cd.GetNamespace() + basedOnApplied := cd.Status.LastAppliedSpec + basedOnPromoted := cd.Status.LastPromotedSpec + err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + fresh, err := flaggerClient.FlaggerV1beta1().Canaries(ns).Get(context.TODO(), name, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) + } + if fresh.Status.LastAppliedSpec != basedOnApplied { + // another writer updated the tracked spec; the next reconcile + // will re-make the decision against the fresh status + return nil + } + if fresh.Status.LastAppliedSpec == snap.hash && fresh.Status.LastTrackedRevision == snap.fence { + // already persisted + return nil + } + + cdCopy := fresh.DeepCopy() + cdCopy.Status.LastAppliedSpec = snap.hash + cdCopy.Status.LastTrackedRevision = snap.fence + if basedOnPromoted != "" && basedOnPromoted == basedOnApplied { + cdCopy.Status.LastPromotedSpec = snap.hash + } + + return updateStatusWithUpgrade(flaggerClient, cdCopy) + }) + if err != nil { + return fmt.Errorf("failed after retries: %w", err) + } + return nil +} + +// refreshTrackedRevision updates lastTrackedRevision after Flagger's own +// writes to the target (scaling), which advance the server change counter +// without altering the normalized spec. The fence only moves while the +// recorded lastAppliedSpec still matches the hash of the written object: a +// mismatch means a concurrent change is pending (or the status predates the +// tracking format) and the stale fence must remain in place so the change is +// detected by hash comparison instead of being absorbed. +func refreshTrackedRevision(flaggerClient clientset.Interface, cd *flaggerv1.Canary, snap targetSnapshot) error { + name, ns := cd.GetName(), cd.GetNamespace() + err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + fresh, err := flaggerClient.FlaggerV1beta1().Canaries(ns).Get(context.TODO(), name, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) + } + if fresh.Status.LastAppliedSpec != snap.hash || fresh.Status.LastTrackedRevision == snap.fence { + return nil + } + + cdCopy := fresh.DeepCopy() + cdCopy.Status.LastTrackedRevision = snap.fence + + return updateStatusWithUpgrade(flaggerClient, cdCopy) + }) + if err != nil { + return fmt.Errorf("failed after retries: %w", err) + } + return nil +} + func setStatusFailedChecks(flaggerClient clientset.Interface, cd *flaggerv1.Canary, val int) error { firstTry := true name, ns := cd.GetName(), cd.GetNamespace() From f5ed947d5efb18155fc841a3a1d5fb6147674c47 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 2 Jul 2026 12:25:32 +0300 Subject: [PATCH 4/5] Sync lastTrackedRevision in scheduler after initialization The freshly initialized canary status is copied back into the local object before shouldAdvance runs; without the fence the next evaluation would take the migration branch spuriously. Signed-off-by: Stefan Prodan --- pkg/controller/scheduler.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 55802dde9..d18e92eb1 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -1045,9 +1045,11 @@ func (c *Controller) setPhaseInitialized(cd *flaggerv1.Canary, canaryController if err != nil { return fmt.Errorf("failed to get canary %s.%s: %w", cd.Name, cd.Namespace, err) } - // We need to sync the LastAppliedSpec and TrackedConfigs of the `cd` Canary object as it - // is used later to determine whether target revision has changed in `shouldAdvance()`. + // We need to sync the LastAppliedSpec, LastTrackedRevision and TrackedConfigs of the + // `cd` Canary object as they are used later to determine whether target revision + // has changed in `shouldAdvance()`. cd.Status.LastAppliedSpec = canary.Status.LastAppliedSpec + cd.Status.LastTrackedRevision = canary.Status.LastTrackedRevision cd.Status.TrackedConfigs = canary.Status.TrackedConfigs c.recorder.SetStatus(cd, flaggerv1.CanaryPhaseInitialized) From ba973e43744237129151e5ae044ee10a6a047eaa Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 2 Jul 2026 12:25:32 +0300 Subject: [PATCH 5/5] Add unit tests for the spec content hash and change fence Covers the decision table per rule (uid gate, drift absorption with lastPromotedSpec lockstep, replicas-only fence refresh, revision backfill, manual rollback, secondary revision guards, incapable and volatile fences), the phase-aware migration incl. the image comparison heuristic and its failure mode, persistence freshness checks under conflict retries, persist-failure verdict stability, the DaemonSet scale-to-zero normalization edge and golden hash values pinning the canonical JSON representation against future k8s.io/api upgrades. The Knative change detection test now exercises the v2 path instead of passing vacuously through the migration branch. Signed-off-by: Stefan Prodan --- pkg/canary/knative_controller_test.go | 51 ++- pkg/canary/spec_test.go | 554 ++++++++++++++++++++++++++ 2 files changed, 604 insertions(+), 1 deletion(-) create mode 100644 pkg/canary/spec_test.go diff --git a/pkg/canary/knative_controller_test.go b/pkg/canary/knative_controller_test.go index f7467c77f..29fa13b7e 100644 --- a/pkg/canary/knative_controller_test.go +++ b/pkg/canary/knative_controller_test.go @@ -67,13 +67,62 @@ func TestKnativeController_HasTargetChanged(t *testing.T) { service, err := mocks.knativeClient.ServingV1().Services("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) require.NoError(t, err) - mocks.canary.Status.LastAppliedSpec = ComputeHash(service.Status.LatestCreatedRevisionName) + mocks.canary.Status.Phase = v1beta1.CanaryPhaseSucceeded + mocks.canary.Status.LastAppliedSpec = ComputeStringHash(service.Status.LatestCreatedRevisionName) + mocks.canary.Status.LastTrackedRevision = encodeFence(service.UID, service.Status.LatestCreatedRevisionName, "") + // same revision, no change + ok, err := mocks.controller.HasTargetChanged(mocks.canary) + require.NoError(t, err) + assert.False(t, ok) + + // a new revision was created service.Status.LatestCreatedRevisionName = "latest-revision" _, err = mocks.knativeClient.ServingV1().Services("default").UpdateStatus(context.TODO(), service, metav1.UpdateOptions{}) require.NoError(t, err) + ok, err = mocks.controller.HasTargetChanged(mocks.canary) + require.NoError(t, err) + assert.True(t, ok) +} + +func TestKnativeController_HasTargetChanged_Migration(t *testing.T) { + // a canary tracked by a previous Flagger version adopts the current + // revision as baseline when it matches the promoted one + mocks := newKnativeServiceFixture("podinfo") + _, err := mocks.controller.Initialize(mocks.canary) + require.NoError(t, err) + + mocks.canary.Status.Phase = v1beta1.CanaryPhaseSucceeded + mocks.canary.Status.LastAppliedSpec = ComputeHash("podinfo-00001") // legacy spew hash + _, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").UpdateStatus(context.TODO(), mocks.canary, metav1.UpdateOptions{}) + require.NoError(t, err) + ok, err := mocks.controller.HasTargetChanged(mocks.canary) require.NoError(t, err) + assert.False(t, ok) + + migrated, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) + require.NoError(t, err) + assert.Contains(t, migrated.Status.LastAppliedSpec, "v2:") + assert.NotEmpty(t, migrated.Status.LastTrackedRevision) + + // a revision created while Flagger was not running must trigger instead + // of being adopted + stale := newKnativeServiceFixture("podinfo") + _, err = stale.controller.Initialize(stale.canary) + require.NoError(t, err) + + service, err := stale.knativeClient.ServingV1().Services("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) + require.NoError(t, err) + service.Status.LatestCreatedRevisionName = "podinfo-00002" + _, err = stale.knativeClient.ServingV1().Services("default").UpdateStatus(context.TODO(), service, metav1.UpdateOptions{}) + require.NoError(t, err) + + stale.canary.Status.Phase = v1beta1.CanaryPhaseSucceeded + stale.canary.Status.LastAppliedSpec = ComputeHash("podinfo-00001") + + ok, err = stale.controller.HasTargetChanged(stale.canary) + require.NoError(t, err) assert.True(t, ok) } diff --git a/pkg/canary/spec_test.go b/pkg/canary/spec_test.go new file mode 100644 index 000000000..004121f83 --- /dev/null +++ b/pkg/canary/spec_test.go @@ -0,0 +1,554 @@ +/* +Copyright 2020 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package canary + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + k8stesting "k8s.io/client-go/testing" + + flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" + clientset "github.com/fluxcd/flagger/pkg/client/clientset/versioned" + fakeFlagger "github.com/fluxcd/flagger/pkg/client/clientset/versioned/fake" +) + +func specTestFixture(status flaggerv1.CanaryStatus) (*flaggerv1.Canary, clientset.Interface, *zap.SugaredLogger) { + cd := &flaggerv1.Canary{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "podinfo"}, + Spec: flaggerv1.CanarySpec{ + TargetRef: flaggerv1.LocalObjectReference{Name: "podinfo", Kind: "Deployment"}, + }, + Status: status, + } + return cd, fakeFlagger.NewSimpleClientset(cd), zap.NewNop().Sugar() +} + +func getSpecTestCanary(t *testing.T, client clientset.Interface) *flaggerv1.Canary { + t.Helper() + fresh, err := client.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) + require.NoError(t, err) + return fresh +} + +func TestComputeSpecHash_Golden(t *testing.T) { + // pinned expected hashes: if this test fails after a k8s.io/api upgrade, + // the canonical JSON representation moved and unchanged workloads would + // be re-hashed differently in production — investigate before releasing + template := corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "podinfo"}}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "podinfo", Image: "ghcr.io/stefanprodan/podinfo:6.0.0"}, + }, + }, + } + hash, err := ComputeSpecHash(&template) + require.NoError(t, err) + assert.Equal(t, "v2:2941eab5be2776bc", hash) + + svcSpec := corev1.ServiceSpec{ + Selector: map[string]string{"app": "podinfo"}, + Ports: []corev1.ServicePort{{Name: "http", Port: 9898}}, + } + svcHash, err := ComputeSpecHash(&svcSpec) + require.NoError(t, err) + assert.Equal(t, "v2:a426cf88d6c4eb4a", svcHash) + + assert.Equal(t, "v2:ec91372f5617da08", ComputeStringHash("podinfo-00042")) +} + +func TestComputeSpecHash_IgnoresUnsetFields(t *testing.T) { + // a nil optional pointer field and an omitted one must hash identically: + // this is the property that keeps hashes stable across k8s.io/api upgrades + withNil := corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "app", Image: "img:1"}}, + TerminationGracePeriodSeconds: nil, + NodeSelector: nil, + }, + } + withEmpty := corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "app", Image: "img:1"}}, + NodeSelector: map[string]string{}, + }, + } + a, err := ComputeSpecHash(&withNil) + require.NoError(t, err) + b, err := ComputeSpecHash(&withEmpty) + require.NoError(t, err) + assert.Equal(t, a, b) +} + +func TestHasSpecChanged_FirstSync(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{}) + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/5"}, nil) + require.NoError(t, err) + assert.True(t, changed) +} + +func TestHasSpecChanged_UIDGate(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // same hash, but the target was deleted and recreated + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-2/1"}, nil) + require.NoError(t, err) + assert.True(t, changed) +} + +func TestHasSpecChanged_AbsorbsDriftOnFenceMatch(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // hash drifted (simulated algorithm/schema change) while the fence + // proves the spec did not change + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/5"}, nil) + require.NoError(t, err) + assert.False(t, changed) + + // the re-baseline is persisted with lastPromotedSpec in lockstep + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:bbbb", fresh.Status.LastAppliedSpec) + assert.Equal(t, "v2:bbbb", fresh.Status.LastPromotedSpec) + assert.Equal(t, "uid-1/5", fresh.Status.LastTrackedRevision) + + // the in-memory object is never mutated (Failed-phase re-sync asymmetry) + assert.Equal(t, "v2:aaaa", cd.Status.LastAppliedSpec) +} + +func TestHasSpecChanged_NoLockstepDuringRun(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseProgressing, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:pppp", + LastTrackedRevision: "uid-1/5", + }) + + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/5"}, nil) + require.NoError(t, err) + assert.False(t, changed) + + // lastPromotedSpec pointed at another spec and must not move + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:bbbb", fresh.Status.LastAppliedSpec) + assert.Equal(t, "v2:pppp", fresh.Status.LastPromotedSpec) +} + +func TestHasSpecChanged_ReplicasOnlyChange(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // generation advanced (scale) but the template hash is unchanged + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/7"}, nil) + require.NoError(t, err) + assert.False(t, changed) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "uid-1/7", fresh.Status.LastTrackedRevision) +} + +func TestHasSpecChanged_RevisionBackfill(t *testing.T) { + // the fence was stored without a revision component (deployment + // controller lagging at write time); once the revision becomes + // trustworthy it must be persisted even though the counter is unchanged, + // or the secondary absorb stays unavailable + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/5/3"}, nil) + require.NoError(t, err) + assert.False(t, changed) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "uid-1/5/3", fresh.Status.LastTrackedRevision) +} + +func TestHasSpecChanged_ManualRollback(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:cccc", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // the target was rolled back to the promoted spec + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/9"}, nil) + require.NoError(t, err) + assert.False(t, changed) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:aaaa", fresh.Status.LastAppliedSpec) + assert.Equal(t, "v2:aaaa", fresh.Status.LastPromotedSpec) + assert.Equal(t, "uid-1/9", fresh.Status.LastTrackedRevision) +} + +func TestHasSpecChanged_RealChange(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/6"}, nil) + require.NoError(t, err) + assert.True(t, changed) + + // nothing is persisted on a real change; SyncStatus records it later + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:aaaa", fresh.Status.LastAppliedSpec) + assert.Equal(t, "uid-1/5", fresh.Status.LastTrackedRevision) +} + +func TestHasSpecChanged_SecondaryRevisionAbsorbs(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5/3", + }) + + // generation advanced (e.g. HPA scale) AND the hash drifted, but the + // deployment revision proves the template is unchanged + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/7/3"}, nil) + require.NoError(t, err) + assert.False(t, changed) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:bbbb", fresh.Status.LastAppliedSpec) +} + +func TestHasSpecChanged_SecondaryRevisionUnavailable(t *testing.T) { + // when either side lacks a trusted revision (paused deployment or the + // deployment controller lagging), scale+drift must trigger, not absorb + for _, fences := range [][2]string{ + {"uid-1/5/3", "uid-1/7"}, // current revision untrusted + {"uid-1/5", "uid-1/7/3"}, // stored revision untrusted + {"uid-1/5/3", "uid-1/7/4"}, // revision moved: rollout happened + } { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: fences[0], + }) + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:bbbb", fence: fences[1]}, nil) + require.NoError(t, err) + assert.True(t, changed, "stored %s current %s", fences[0], fences[1]) + } +} + +func TestHasSpecChanged_IncapableFenceNeverAbsorbs(t *testing.T) { + // generation 0 means the server does not maintain the counter (e.g. fake + // clients); the fence must not prove anything + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/0", + }) + + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/0"}, nil) + require.NoError(t, err) + assert.True(t, changed) +} + +func TestMigration_SettledPhaseAdopts(t *testing.T) { + for _, phase := range []flaggerv1.CanaryPhase{flaggerv1.CanaryPhaseInitialized, flaggerv1.CanaryPhaseSucceeded} { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: phase, + LastAppliedSpec: "6849403941", // legacy spew hash, no fence + LastPromotedSpec: "6849403941", + }) + + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/5"}, nil) + require.NoError(t, err) + assert.False(t, changed, string(phase)) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:aaaa", fresh.Status.LastAppliedSpec, string(phase)) + assert.Equal(t, "v2:aaaa", fresh.Status.LastPromotedSpec, string(phase)) + assert.Equal(t, "uid-1/5", fresh.Status.LastTrackedRevision, string(phase)) + + // second evaluation with the migrated status takes the normal path + // and stays silent: migration happens at most once per canary + migrated := fresh.DeepCopy() + changedAgain, err := hasSpecChanged(logger, client, migrated, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/5"}, nil) + require.NoError(t, err) + assert.False(t, changedAgain, string(phase)) + } +} + +func TestMigration_ImageDiffTriggers(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "6849403941", + LastPromotedSpec: "6849403941", + }) + + // canary images differ from primary: a rollout landed while Flagger was + // down; adopt would swallow it, so this must trigger + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/5"}, + func() (bool, error) { return true, nil }) + require.NoError(t, err) + assert.True(t, changed) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "6849403941", fresh.Status.LastAppliedSpec) +} + +func TestMigration_ImageDiffErrorBlocks(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "6849403941", + LastPromotedSpec: "6849403941", + }) + + // on a failed comparison the migration must neither adopt nor trigger: + // the error defers the decision to the next reconcile + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/5"}, + func() (bool, error) { return false, errors.New("primary not found") }) + require.Error(t, err) + assert.False(t, changed) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "6849403941", fresh.Status.LastAppliedSpec) + assert.Empty(t, fresh.Status.LastTrackedRevision) +} + +func TestMigration_ActivePhasesTrigger(t *testing.T) { + for _, phase := range []flaggerv1.CanaryPhase{flaggerv1.CanaryPhaseProgressing, flaggerv1.CanaryPhaseWaitingPromotion, flaggerv1.CanaryPhaseFailed} { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: phase, + LastAppliedSpec: "6849403941", + }) + + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/5"}, nil) + require.NoError(t, err) + assert.True(t, changed, string(phase)) + } +} + +func TestPersistSpecTracking_AbortsOnConcurrentWriter(t *testing.T) { + cd, client, _ := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // another writer updated the tracked spec after the decision was made + stale := cd.DeepCopy() + stale.Status.LastAppliedSpec = "v2:old-decision-basis" + + require.NoError(t, persistSpecTracking(client, stale, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/9"})) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:aaaa", fresh.Status.LastAppliedSpec) + assert.Equal(t, "uid-1/5", fresh.Status.LastTrackedRevision) +} + +func TestHasSpecChanged_PersistFailureKeepsVerdict(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // all status writes fail: the absorb verdict must stand (no error, no + // trigger) and the re-baseline is simply retried on the next reconcile + fakeClient := client.(*fakeFlagger.Clientset) + fakeClient.PrependReactor("update", "canaries", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("etcd is down") + }) + + changed, err := hasSpecChanged(logger, client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/5"}, nil) + require.NoError(t, err) + assert.False(t, changed) +} + +func TestHasSpecChanged_VolatileCounterSkipsFenceRefresh(t *testing.T) { + cd, client, logger := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastPromotedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/500", + }) + + // a Service resourceVersion moves on writes unrelated to the spec; + // chasing it would cost one status write per reconcile + changed, err := hasSpecChanged(logger, client, cd, + targetSnapshot{hash: "v2:aaaa", fence: "uid-1/501", volatileCounter: true}, nil) + require.NoError(t, err) + assert.False(t, changed) + + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "uid-1/500", fresh.Status.LastTrackedRevision) +} + +func TestPersistSpecTracking_FreshnessRecheckedOnConflict(t *testing.T) { + cd, client, _ := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // first status update hits a conflict; the retried GET observes a + // concurrent writer's lastAppliedSpec — the retry must re-evaluate the + // freshness check and abort instead of overwriting it + conflicted := false + injectConcurrentGet := false + fakeClient := client.(*fakeFlagger.Clientset) + fakeClient.PrependReactor("update", "canaries", func(k8stesting.Action) (bool, runtime.Object, error) { + if !conflicted { + conflicted = true + injectConcurrentGet = true + return true, nil, apierrors.NewConflict( + schema.GroupResource{Group: "flagger.app", Resource: "canaries"}, "podinfo", errors.New("conflict")) + } + return false, nil, nil + }) + fakeClient.PrependReactor("get", "canaries", func(k8stesting.Action) (bool, runtime.Object, error) { + if injectConcurrentGet { + concurrent := cd.DeepCopy() + concurrent.Status.LastAppliedSpec = "v2:concurrent" + concurrent.Status.LastTrackedRevision = "uid-1/9" + return true, concurrent, nil + } + return false, nil, nil + }) + + require.NoError(t, persistSpecTracking(client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/7"})) + injectConcurrentGet = false + + // the stored canary was never overwritten by the aborted retry + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "v2:aaaa", fresh.Status.LastAppliedSpec) + assert.Equal(t, "uid-1/5", fresh.Status.LastTrackedRevision) +} + +func TestRefreshTrackedRevision(t *testing.T) { + cd, client, _ := specTestFixture(flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseSucceeded, + LastAppliedSpec: "v2:aaaa", + LastTrackedRevision: "uid-1/5", + }) + + // hash matches the tracked spec: only the fence moves + require.NoError(t, refreshTrackedRevision(client, cd, targetSnapshot{hash: "v2:aaaa", fence: "uid-1/7"})) + fresh := getSpecTestCanary(t, client) + assert.Equal(t, "uid-1/7", fresh.Status.LastTrackedRevision) + + // hash differs (concurrent template change captured by the scaling + // write): the fence must stay put so the change is detected by hash + require.NoError(t, refreshTrackedRevision(client, cd, targetSnapshot{hash: "v2:bbbb", fence: "uid-1/8"})) + fresh = getSpecTestCanary(t, client) + assert.Equal(t, "uid-1/7", fresh.Status.LastTrackedRevision) + assert.Equal(t, "v2:aaaa", fresh.Status.LastAppliedSpec) +} + +func TestDaemonSetSnapshot_ScaleToZeroSelectorIgnored(t *testing.T) { + base := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: "podinfo", Namespace: "default", UID: "uid-1", Generation: 3}, + Spec: appsv1.DaemonSetSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "podinfo", Image: "img:1"}}, + }, + }, + }, + } + + scaledDown := base.DeepCopy() + scaledDown.Spec.Template.Spec.NodeSelector = map[string]string{"flagger.app/scale-to-zero": "true"} + + baseSnap, err := daemonSetSnapshot(base) + require.NoError(t, err) + downSnap, err := daemonSetSnapshot(scaledDown) + require.NoError(t, err) + assert.Equal(t, baseSnap.hash, downSnap.hash) + + // user-provided node selectors are still part of the spec + withSelector := base.DeepCopy() + withSelector.Spec.Template.Spec.NodeSelector = map[string]string{"kubernetes.io/os": "linux"} + selectorSnap, err := daemonSetSnapshot(withSelector) + require.NoError(t, err) + assert.NotEqual(t, baseSnap.hash, selectorSnap.hash) +} + +func TestPodImagesDiffer(t *testing.T) { + a := corev1.PodSpec{ + InitContainers: []corev1.Container{{Name: "setup", Image: "init:1"}}, + Containers: []corev1.Container{{Name: "app", Image: "img:1"}}, + } + + b := *a.DeepCopy() + assert.False(t, podImagesDiffer(a, b)) + + b.Containers[0].Image = "img:2" + assert.True(t, podImagesDiffer(a, b)) + + c := *a.DeepCopy() + c.InitContainers[0].Image = "init:2" + assert.True(t, podImagesDiffer(a, c)) + + d := *a.DeepCopy() + d.Containers = append(d.Containers, corev1.Container{Name: "sidecar", Image: "sc:1"}) + assert.True(t, podImagesDiffer(a, d)) +} + +func TestFenceEncoding(t *testing.T) { + assert.Equal(t, "uid-1/5", encodeFence("uid-1", "5", "")) + assert.Equal(t, "uid-1/5/3", encodeFence("uid-1", "5", "3")) + + uid, counter, revision := parseFence("uid-1/5/3") + assert.Equal(t, []string{"uid-1", "5", "3"}, []string{uid, counter, revision}) + + uid, counter, revision = parseFence("uid-1/5") + assert.Equal(t, []string{"uid-1", "5", ""}, []string{uid, counter, revision}) + + uid, counter, revision = parseFence("") + assert.Equal(t, []string{"", "", ""}, []string{uid, counter, revision}) +}