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 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 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/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/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/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}) +} 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() 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)