From cc1d316ceedcde3ed1d56c34681ebff2c0dc5321 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Wed, 23 Sep 2026 05:45:13 -0700 Subject: [PATCH] Restore original HTTPRoute spec on Gateway API canary finalize Motivation: When a Canary using the Gateway API provider (gatewayapi:v1) is deleted with spec.revertOnDeletion: true, Flagger reverts the target Deployment and the Kubernetes Service, but GatewayAPIRouter.Finalize was a no-op that returned nil without touching the HTTPRoute. The HTTPRoute was left pointing at the Flagger-managed *-primary/*-canary backends and carrying the session-affinity cookie rule, both of which reference Services that are deleted during finalization. This leaves the route with unresolved backends, causing request failures until an operator manually restores it. Approach: Apply the same annotation-based revert pattern already used by IstioRouter.Finalize (VirtualService) and KubernetesDefaultRouter.Finalize (Service) to GatewayAPIRouter: - In Reconcile, the first time Flagger overwrites a pre-existing HTTPRoute's spec, the original spec is preserved: either the kubectl.kubernetes.io/last-applied-configuration annotation already carries it (when the route was applied with kubectl), or Flagger stores it under its own flagger.kubernetes.io/original-configuration annotation before the first update. - Finalize now looks up either annotation and, if found, restores the HTTPRoute's spec from it. If neither annotation is present (e.g. the HTTPRoute was created by Flagger itself and there is nothing to revert to), it logs a warning and leaves the route untouched, as before. This change is scoped to pkg/router/gateway_api.go (the gatewayapi:v1 provider named in the report). GatewayAPIV1Beta1Router (gateway_api_v1beta1.go) is a separate, independently implemented type with its own copy of Reconcile/Finalize and has the same gap, but is left untouched here to keep this change minimal and surgical. Validation: - go build ./pkg/router/... - go test ./pkg/router/... (all tests pass) - go vet ./pkg/router/... (clean) - gofmt -l on both changed files (clean) - Added TestGatewayAPIRouter_Finalize with four subtests: HTTPRoute not found returns an error; no stored annotation is a graceful no-op; restoring from the flagger config annotation; restoring from the kubectl annotation. Verified these are a genuine regression test by stashing the gateway_api.go change and re-running: 3 of 4 subtests fail against the previous no-op Finalize, and all 4 pass with the fix applied. - Interface compliance (router.Interface.Finalize) is verified by go build ./pkg/router/..., since pkg/router/factory.go assigns &GatewayAPIRouter{} to that interface within the same package. - Could not run a full-repo `go build ./...` or the make-based CI target in this sandbox due to available disk space being exhausted by unrelated heavy dependencies (client-go informers, cloud SDKs) pulled in by other packages; the change itself is fully contained to pkg/router and was validated at that scope. Report: https://github.com/fluxcd/flagger/issues/1974 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- pkg/router/gateway_api.go | 58 ++++++++++++++++++- pkg/router/gateway_api_test.go | 103 +++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-) diff --git a/pkg/router/gateway_api.go b/pkg/router/gateway_api.go index df325caae..e8542ba40 100644 --- a/pkg/router/gateway_api.go +++ b/pkg/router/gateway_api.go @@ -18,6 +18,7 @@ package router import ( "context" + "encoding/json" "fmt" "reflect" "slices" @@ -241,6 +242,24 @@ func (gwr *GatewayAPIRouter) Reconcile(canary *flaggerv1.Canary) error { hrClone.Spec = httpRouteSpec hrClone.ObjectMeta.Annotations = mergedAnnotations hrClone.ObjectMeta.Labels = newMetadata.Labels + + // If the kubectl annotation is present it will be used to restore the HTTPRoute on Finalize, + // so there is no need to duplicate the original spec. Otherwise, store it so that it can be + // restored once the canary is deleted with revertOnDeletion enabled. + if _, ok := hrClone.Annotations[kubectlAnnotation]; !ok && specDiff != "" { + if _, ok := hrClone.Annotations[configAnnotation]; !ok { + b, err := json.Marshal(httpRoute.Spec) + if err != nil { + gwr.logger.Warnf("Unable to marshal HTTPRoute %s for orig-configuration annotation", httpRoute.Name) + } else { + if hrClone.ObjectMeta.Annotations == nil { + hrClone.ObjectMeta.Annotations = make(map[string]string) + } + hrClone.ObjectMeta.Annotations[configAnnotation] = string(b) + } + } + } + _, err := gwr.gatewayAPIClient.GatewayapiV1().HTTPRoutes(hrNamespace). Update(context.TODO(), hrClone, metav1.UpdateOptions{}) if err != nil { @@ -443,7 +462,44 @@ func (gwr *GatewayAPIRouter) SetRoutes( return nil } -func (gwr *GatewayAPIRouter) Finalize(_ *flaggerv1.Canary) error { +// Finalize restores the HTTPRoute to the state it was in before Flagger took it over, +// so that traffic keeps flowing to the target service after the canary is deleted. +func (gwr *GatewayAPIRouter) Finalize(canary *flaggerv1.Canary) error { + apexSvcName, _, _ := canary.GetServiceNames() + hrNamespace := canary.Namespace + + httpRoute, err := gwr.gatewayAPIClient.GatewayapiV1().HTTPRoutes(hrNamespace).Get( + context.TODO(), apexSvcName, metav1.GetOptions{}, + ) + if err != nil { + return fmt.Errorf("HTTPRoute %s.%s get query error: %w", apexSvcName, hrNamespace, err) + } + + var storedSpec v1.HTTPRouteSpec + if a, ok := httpRoute.Annotations[kubectlAnnotation]; ok { + var storedHTTPRoute v1.HTTPRoute + if err := json.Unmarshal([]byte(a), &storedHTTPRoute); err != nil { + return fmt.Errorf("HTTPRoute %s.%s failed to unmarshal annotation %s", + apexSvcName, hrNamespace, kubectlAnnotation) + } + storedSpec = storedHTTPRoute.Spec + } else if a, ok := httpRoute.Annotations[configAnnotation]; ok { + if err := json.Unmarshal([]byte(a), &storedSpec); err != nil { + return fmt.Errorf("HTTPRoute %s.%s failed to unmarshal annotation %s", + apexSvcName, hrNamespace, configAnnotation) + } + } else { + gwr.logger.Warnf("HTTPRoute %s.%s original configuration not found, unable to revert", apexSvcName, hrNamespace) + return nil + } + + clone := httpRoute.DeepCopy() + clone.Spec = storedSpec + + _, err = gwr.gatewayAPIClient.GatewayapiV1().HTTPRoutes(hrNamespace).Update(context.TODO(), clone, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("HTTPRoute %s.%s update error: %w", apexSvcName, hrNamespace, err) + } return nil } diff --git a/pkg/router/gateway_api_test.go b/pkg/router/gateway_api_test.go index b9923f288..eac21a4f5 100644 --- a/pkg/router/gateway_api_test.go +++ b/pkg/router/gateway_api_test.go @@ -18,6 +18,7 @@ package router import ( "context" + "encoding/json" "fmt" "strings" "testing" @@ -708,3 +709,105 @@ func TestGatewayAPIRouter_GetRoutes(t *testing.T) { assert.False(t, mirrored) }) } + +func TestGatewayAPIRouter_Finalize(t *testing.T) { + canary := newTestGatewayAPICanary() + + originalSpec := v1.HTTPRouteSpec{ + Hostnames: []v1.Hostname{"podinfo.example.com"}, + Rules: []v1.HTTPRouteRule{ + { + BackendRefs: []v1.HTTPBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Name: "podinfo", + }, + }, + }, + }, + }, + }, + } + + t.Run("errors when the HTTPRoute is not found", func(t *testing.T) { + mocks := newFixture(canary) + router := &GatewayAPIRouter{ + gatewayAPIClient: mocks.meshClient, + kubeClient: mocks.kubeClient, + logger: mocks.logger, + } + + err := router.Finalize(canary) + require.Error(t, err) + }) + + t.Run("does not error when no original configuration is stored", func(t *testing.T) { + mocks := newFixture(canary) + router := &GatewayAPIRouter{ + gatewayAPIClient: mocks.meshClient, + kubeClient: mocks.kubeClient, + logger: mocks.logger, + } + require.NoError(t, router.Reconcile(canary)) + + require.NoError(t, router.Finalize(canary)) + }) + + t.Run("restores the spec stored in the flagger config annotation", func(t *testing.T) { + mocks := newFixture(canary) + router := &GatewayAPIRouter{ + gatewayAPIClient: mocks.meshClient, + kubeClient: mocks.kubeClient, + logger: mocks.logger, + } + require.NoError(t, router.Reconcile(canary)) + + httpRoute, err := router.gatewayAPIClient.GatewayapiV1().HTTPRoutes(canary.Namespace). + Get(context.TODO(), canary.Name, metav1.GetOptions{}) + require.NoError(t, err) + + b, err := json.Marshal(originalSpec) + require.NoError(t, err) + httpRoute.Annotations = map[string]string{configAnnotation: string(b)} + _, err = router.gatewayAPIClient.GatewayapiV1().HTTPRoutes(canary.Namespace). + Update(context.TODO(), httpRoute, metav1.UpdateOptions{}) + require.NoError(t, err) + + require.NoError(t, router.Finalize(canary)) + + httpRoute, err = router.gatewayAPIClient.GatewayapiV1().HTTPRoutes(canary.Namespace). + Get(context.TODO(), canary.Name, metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, originalSpec, httpRoute.Spec) + }) + + t.Run("restores the spec embedded in the kubectl annotation", func(t *testing.T) { + mocks := newFixture(canary) + router := &GatewayAPIRouter{ + gatewayAPIClient: mocks.meshClient, + kubeClient: mocks.kubeClient, + logger: mocks.logger, + } + require.NoError(t, router.Reconcile(canary)) + + httpRoute, err := router.gatewayAPIClient.GatewayapiV1().HTTPRoutes(canary.Namespace). + Get(context.TODO(), canary.Name, metav1.GetOptions{}) + require.NoError(t, err) + + stored := v1.HTTPRoute{Spec: originalSpec} + b, err := json.Marshal(stored) + require.NoError(t, err) + httpRoute.Annotations = map[string]string{kubectlAnnotation: string(b)} + _, err = router.gatewayAPIClient.GatewayapiV1().HTTPRoutes(canary.Namespace). + Update(context.TODO(), httpRoute, metav1.UpdateOptions{}) + require.NoError(t, err) + + require.NoError(t, router.Finalize(canary)) + + httpRoute, err = router.gatewayAPIClient.GatewayapiV1().HTTPRoutes(canary.Namespace). + Get(context.TODO(), canary.Name, metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, originalSpec, httpRoute.Spec) + }) +}