From 8f06fc0fc0131fd4238ca4929b542ff40b80ed21 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Thu, 4 Sep 2025 13:52:08 -0300 Subject: [PATCH 1/2] OCPBUGS-61228: Bump Route generation when spec is updated Kubernetes resources should bump their generation every time a field on spec is changed. This allows controllers to keep track of the desired state and the current state. This change: * initializes the generation of a resource if it does not exist * updates the generation of a resource when a field on spec is changed * adds some unit tests to assure the desired behavior --- .../apiserver/registry/route/strategy.go | 9 ++++ .../apiserver/registry/route/strategy_test.go | 42 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/pkg/route/apiserver/registry/route/strategy.go b/pkg/route/apiserver/registry/route/strategy.go index 18e0681fc..9ac03a181 100644 --- a/pkg/route/apiserver/registry/route/strategy.go +++ b/pkg/route/apiserver/registry/route/strategy.go @@ -5,6 +5,7 @@ import ( "fmt" authorizationapi "k8s.io/api/authorization/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" @@ -66,6 +67,7 @@ func (s routeStrategy) routeValidationOptions() routecommon.RouteValidationOptio func (s routeStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { route := obj.(*routeapi.Route) route.Status = routeapi.RouteStatus{} + route.Generation = 1 stripEmptyDestinationCACertificate(route) // In kube APIs, disabled fields are stripped from inbound objects. @@ -96,6 +98,13 @@ func (s routeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob route.Spec.TLS.ExternalCertificate = nil } } + + // Any changes to the spec increment the generation number. + // Changes to status, or to any metadata field (eg.: labels and annotations) + // should not impact on the generation + if !apiequality.Semantic.DeepEqual(oldRoute.Spec, route.Spec) { + route.Generation = oldRoute.Generation + 1 + } } func (s routeStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { diff --git a/pkg/route/apiserver/registry/route/strategy_test.go b/pkg/route/apiserver/registry/route/strategy_test.go index 6670fc34f..f3097d15e 100644 --- a/pkg/route/apiserver/registry/route/strategy_test.go +++ b/pkg/route/apiserver/registry/route/strategy_test.go @@ -1112,3 +1112,45 @@ func TestExternalCertRemoval(t *testing.T) { } } } + +func TestRouteGenerationManagement(t *testing.T) { + ctx := apirequest.NewContext() + strategy := NewStrategy(testAllocator{}, &testSAR{allow: true}, &testSecretGetter{}, true) + + simpleRoute := &routeapi.Route{} + strategy.Validate(ctx, simpleRoute) + if simpleRoute.Spec.Host != "mygeneratedhost.com" { + t.Fatalf("Expected host to be allocated, got %s", simpleRoute.Spec.Host) + } + + if simpleRoute.Generation > 0 { + t.Fatalf("Expected generation to not be allocated yet, got %d", simpleRoute.Generation) + } + + // PrepareForCreate should set a generation 1 + strategy.PrepareForCreate(ctx, simpleRoute) + if simpleRoute.Generation != 1 { + t.Fatalf("Expected generation after create to be 1, got %d", simpleRoute.Generation) + } + + newRoute := simpleRoute.DeepCopy() + // Changing annotations and labels should not bump the generation + newRoute.Annotations = map[string]string{ + "someannotation": "novalue", + } + newRoute.Labels = map[string]string{ + "somelabel": "novalue", + } + + strategy.PrepareForUpdate(ctx, newRoute, simpleRoute) + if newRoute.Generation != 1 { + t.Fatalf("Expected generation after metadata update to still be 1, got %d", simpleRoute.Generation) + } + + // Updating the spec should bump the generation + newRoute.Spec.Path = "/xpto" + strategy.PrepareForUpdate(ctx, newRoute, simpleRoute) + if newRoute.Generation != 2 { + t.Fatalf("Expected generation after spec update to be 2, got %d", simpleRoute.Generation) + } +} From 24078787e77f9e8b0bf080998b94c1866de9f554 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Thu, 4 Sep 2025 14:14:48 -0300 Subject: [PATCH 2/2] Fix reviews, fix unit test to reflect new generation field --- pkg/route/apiserver/registry/route/strategy.go | 2 ++ pkg/route/apiserver/registry/route/strategy_test.go | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/route/apiserver/registry/route/strategy.go b/pkg/route/apiserver/registry/route/strategy.go index 9ac03a181..4b55e31ec 100644 --- a/pkg/route/apiserver/registry/route/strategy.go +++ b/pkg/route/apiserver/registry/route/strategy.go @@ -102,6 +102,8 @@ func (s routeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob // Any changes to the spec increment the generation number. // Changes to status, or to any metadata field (eg.: labels and annotations) // should not impact on the generation + // Preserve existing generation unless the Spec changes. + route.Generation = oldRoute.Generation if !apiequality.Semantic.DeepEqual(oldRoute.Spec, route.Spec) { route.Generation = oldRoute.Generation + 1 } diff --git a/pkg/route/apiserver/registry/route/strategy_test.go b/pkg/route/apiserver/registry/route/strategy_test.go index f3097d15e..001202a87 100644 --- a/pkg/route/apiserver/registry/route/strategy_test.go +++ b/pkg/route/apiserver/registry/route/strategy_test.go @@ -120,6 +120,7 @@ func TestEmptyDefaultCACertificate(t *testing.T) { Name: "myroute", UID: types.UID("abc"), ResourceVersion: "1", + Generation: 1, }, Spec: routeapi.RouteSpec{ Host: "myhost.com", @@ -1144,13 +1145,13 @@ func TestRouteGenerationManagement(t *testing.T) { strategy.PrepareForUpdate(ctx, newRoute, simpleRoute) if newRoute.Generation != 1 { - t.Fatalf("Expected generation after metadata update to still be 1, got %d", simpleRoute.Generation) + t.Fatalf("Expected generation after metadata update to still be 1, got %d", newRoute.Generation) } // Updating the spec should bump the generation newRoute.Spec.Path = "/xpto" strategy.PrepareForUpdate(ctx, newRoute, simpleRoute) if newRoute.Generation != 2 { - t.Fatalf("Expected generation after spec update to be 2, got %d", simpleRoute.Generation) + t.Fatalf("Expected generation after spec update to be 2, got %d", newRoute.Generation) } }