From 9e056efc641d77cf26a71492632d709c08f27e08 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Wed, 29 Nov 2023 13:05:10 -0700 Subject: [PATCH 01/11] Add SupportedVersion condition --- conformance/provisioner/provisioner.yaml | 7 + deploy/helm-chart/templates/rbac.yaml | 7 + deploy/manifests/nginx-gateway.yaml | 7 + internal/framework/conditions/conditions.go | 85 ++++++- .../conditions/conditions_test.go | 6 +- .../controller/predicate/annotation.go | 39 ++++ .../controller/predicate/annotation_test.go | 220 ++++++++++++++++++ internal/framework/controller/reconciler.go | 16 +- internal/framework/controller/register.go | 20 +- internal/framework/gatewayclass/validate.go | 82 +++++++ .../framework/gatewayclass/validate_test.go | 124 ++++++++++ internal/mode/provisioner/handler.go | 18 +- internal/mode/provisioner/handler_test.go | 149 ++++++++++-- internal/mode/provisioner/manager.go | 28 +++ internal/mode/provisioner/store.go | 7 + internal/mode/static/build_statuses.go | 10 +- internal/mode/static/manager.go | 29 +++ internal/mode/static/manager_test.go | 13 ++ .../mode/static/state/change_processor.go | 63 ++--- .../static/state/change_processor_test.go | 89 ++++++- .../mode/static/state/changed_predicate.go | 81 +++++++ .../static/state/changed_predicate_test.go | 211 +++++++++++++++++ .../static/state/conditions/conditions.go | 32 --- .../mode/static/state/graph/gatewayclass.go | 33 ++- .../static/state/graph/gatewayclass_test.go | 48 +++- internal/mode/static/state/graph/graph.go | 5 +- internal/mode/static/state/store.go | 103 ++++---- .../overview/gateway-api-compatibility.md | 3 + 28 files changed, 1369 insertions(+), 166 deletions(-) rename internal/{mode/static/state => framework}/conditions/conditions_test.go (86%) create mode 100644 internal/framework/controller/predicate/annotation.go create mode 100644 internal/framework/controller/predicate/annotation_test.go create mode 100644 internal/framework/gatewayclass/validate.go create mode 100644 internal/framework/gatewayclass/validate_test.go create mode 100644 internal/mode/static/state/changed_predicate.go create mode 100644 internal/mode/static/state/changed_predicate_test.go diff --git a/conformance/provisioner/provisioner.yaml b/conformance/provisioner/provisioner.yaml index 5a58889274..5862ee37ec 100644 --- a/conformance/provisioner/provisioner.yaml +++ b/conformance/provisioner/provisioner.yaml @@ -30,6 +30,13 @@ rules: - gatewayclasses/status verbs: - update +- apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - list + - watch --- kind: ClusterRoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/deploy/helm-chart/templates/rbac.yaml b/deploy/helm-chart/templates/rbac.yaml index 405c34525e..ac49069f32 100644 --- a/deploy/helm-chart/templates/rbac.yaml +++ b/deploy/helm-chart/templates/rbac.yaml @@ -80,6 +80,13 @@ rules: - get - update {{- end }} +- apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - list + - watch --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 48e1f6accf..80fc304c05 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -89,6 +89,13 @@ rules: - create - get - update +- apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - list + - watch --- # Source: nginx-gateway-fabric/templates/rbac.yaml apiVersion: rbac.authorization.k8s.io/v1 diff --git a/internal/framework/conditions/conditions.go b/internal/framework/conditions/conditions.go index 2dfc21fdf7..e13f21d2c4 100644 --- a/internal/framework/conditions/conditions.go +++ b/internal/framework/conditions/conditions.go @@ -1,6 +1,8 @@ package conditions import ( + "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "sigs.k8s.io/gateway-api/apis/v1" ) @@ -24,7 +26,40 @@ type Condition struct { Message string } -// NewDefaultGatewayClassConditions returns the default Conditions that must be present in the status of a GatewayClass. +// DeduplicateConditions removes duplicate conditions based on the condition type. +// The last condition wins. The order of conditions is preserved. +func DeduplicateConditions(conds []Condition) []Condition { + type elem struct { + cond Condition + reverseIdx int + } + + uniqueElems := make(map[string]elem) + + idx := 0 + for i := len(conds) - 1; i >= 0; i-- { + if _, exist := uniqueElems[conds[i].Type]; exist { + continue + } + + uniqueElems[conds[i].Type] = elem{ + cond: conds[i], + reverseIdx: idx, + } + idx++ + } + + result := make([]Condition, len(uniqueElems)) + + for _, el := range uniqueElems { + result[len(result)-el.reverseIdx-1] = el.cond + } + + return result +} + +// NewDefaultGatewayClassConditions returns Conditions that indicate that the GatewayClass is accepted and that the +// Gateway API CRD versions are supported. func NewDefaultGatewayClassConditions() []Condition { return []Condition{ { @@ -33,6 +68,54 @@ func NewDefaultGatewayClassConditions() []Condition { Reason: string(v1.GatewayClassReasonAccepted), Message: "GatewayClass is accepted", }, + { + Type: string(v1.GatewayClassConditionStatusSupportedVersion), + Status: metav1.ConditionTrue, + Reason: string(v1.GatewayClassReasonSupportedVersion), + Message: "Gateway API CRD versions are supported", + }, + } +} + +// NewGatewayClassSupportedVersionBestEffort returns a Condition that indicates that the GatewayClass is accepted, +// but the Gateway API CRD versions are not supported. This means NGF will attempt to generate configuration, +// but it does not guarantee support. +func NewGatewayClassSupportedVersionBestEffort(recommendedVersion string) []Condition { + return []Condition{ + { + Type: string(v1.GatewayClassConditionStatusSupportedVersion), + Status: metav1.ConditionFalse, + Reason: string(v1.GatewayClassReasonUnsupportedVersion), + Message: fmt.Sprintf( + "Gateway API CRD versions are not recommended. Recommended version is %s", + recommendedVersion, + ), + }, + } +} + +// NewGatewayClassUnsupportedVersion returns Conditions that indicate that the GatewayClass is not accepted because +// the Gateway API CRD versions are not supported. NGF will not generate configuration in this case. +func NewGatewayClassUnsupportedVersion(recommendedVersion string) []Condition { + return []Condition{ + { + Type: string(v1.GatewayClassConditionStatusAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1.GatewayClassReasonUnsupportedVersion), + Message: fmt.Sprintf( + "Gateway API CRD versions are not supported. Please install version %s", + recommendedVersion, + ), + }, + { + Type: string(v1.GatewayClassConditionStatusSupportedVersion), + Status: metav1.ConditionFalse, + Reason: string(v1.GatewayClassReasonUnsupportedVersion), + Message: fmt.Sprintf( + "Gateway API CRD versions are not supported. Please install version %s", + recommendedVersion, + ), + }, } } diff --git a/internal/mode/static/state/conditions/conditions_test.go b/internal/framework/conditions/conditions_test.go similarity index 86% rename from internal/mode/static/state/conditions/conditions_test.go rename to internal/framework/conditions/conditions_test.go index 8729994c29..de7e31cb1e 100644 --- a/internal/mode/static/state/conditions/conditions_test.go +++ b/internal/framework/conditions/conditions_test.go @@ -5,14 +5,12 @@ import ( . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" ) func TestDeduplicateConditions(t *testing.T) { g := NewWithT(t) - conds := []conditions.Condition{ + conds := []Condition{ { Type: "Type1", Status: metav1.ConditionTrue, @@ -40,7 +38,7 @@ func TestDeduplicateConditions(t *testing.T) { }, } - expected := []conditions.Condition{ + expected := []Condition{ { Type: "Type1", Status: metav1.ConditionFalse, diff --git a/internal/framework/controller/predicate/annotation.go b/internal/framework/controller/predicate/annotation.go new file mode 100644 index 0000000000..fdf1fd696f --- /dev/null +++ b/internal/framework/controller/predicate/annotation.go @@ -0,0 +1,39 @@ +package predicate + +import ( + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +// AnnotationPredicate implements a predicate function based on the Annotation. +// +// This predicate will skip the following events: +// 1. Create events that do not contain the Annotation. +// 2. Update events where the Annotation value has not changed. +type AnnotationPredicate struct { + predicate.Funcs + Annotation string +} + +// Create filters CreateEvents based on the Annotation. +func (cp AnnotationPredicate) Create(e event.CreateEvent) bool { + if e.Object == nil { + return false + } + + _, ok := e.Object.GetAnnotations()[cp.Annotation] + return ok +} + +// Update filters UpdateEvents based on the Annotation. +func (cp AnnotationPredicate) Update(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + // this case should not happen + return false + } + + oldAnnotationVal := e.ObjectOld.GetAnnotations()[cp.Annotation] + newAnnotationVal := e.ObjectNew.GetAnnotations()[cp.Annotation] + + return oldAnnotationVal != newAnnotationVal +} diff --git a/internal/framework/controller/predicate/annotation_test.go b/internal/framework/controller/predicate/annotation_test.go new file mode 100644 index 0000000000..6fe18b3a28 --- /dev/null +++ b/internal/framework/controller/predicate/annotation_test.go @@ -0,0 +1,220 @@ +package predicate + +import ( + "testing" + + . "github.com/onsi/gomega" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" +) + +func TestAnnotationPredicate_Create(t *testing.T) { + annotation := "test" + + tests := []struct { + event event.CreateEvent + name string + expUpdate bool + }{ + { + name: "object has annotation", + event: event.CreateEvent{ + Object: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + }, + expUpdate: true, + }, + { + name: "object does not have annotation", + event: event.CreateEvent{ + Object: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "diff": "one", + }, + }, + }, + }, + expUpdate: false, + }, + { + name: "object does not have any annotations", + event: event.CreateEvent{Object: &apiext.CustomResourceDefinition{}}, + expUpdate: false, + }, + { + name: "object is nil", + event: event.CreateEvent{Object: nil}, + expUpdate: false, + }, + } + + p := AnnotationPredicate{Annotation: annotation} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + update := p.Create(test.event) + g.Expect(update).To(Equal(test.expUpdate)) + }) + } +} + +func TestAnnotationPredicate_Update(t *testing.T) { + annotation := "test" + + tests := []struct { + event event.UpdateEvent + name string + expUpdate bool + }{ + { + name: "annotation changed", + event: event.UpdateEvent{ + ObjectOld: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + ObjectNew: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "two", + }, + }, + }, + }, + expUpdate: true, + }, + { + name: "annotation deleted", + event: event.UpdateEvent{ + ObjectOld: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + ObjectNew: &apiext.CustomResourceDefinition{}, + }, + expUpdate: true, + }, + { + name: "annotation added", + event: event.UpdateEvent{ + ObjectOld: &apiext.CustomResourceDefinition{}, + ObjectNew: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + }, + expUpdate: true, + }, + { + name: "annotation has not changed", + event: event.UpdateEvent{ + ObjectOld: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + ObjectNew: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + }, + expUpdate: false, + }, + { + name: "different annotation changed", + event: event.UpdateEvent{ + ObjectOld: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "diff": "one", + }, + }, + }, + ObjectNew: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "diff": "two", + }, + }, + }, + }, + expUpdate: false, + }, + { + name: "no annotations", + event: event.UpdateEvent{ + ObjectOld: &apiext.CustomResourceDefinition{}, + ObjectNew: &apiext.CustomResourceDefinition{}, + }, + expUpdate: false, + }, + { + name: "old object is nil", + event: event.UpdateEvent{ + ObjectOld: nil, + ObjectNew: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + }, + expUpdate: false, + }, + { + name: "new object is nil", + event: event.UpdateEvent{ + ObjectOld: &apiext.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotation: "one", + }, + }, + }, + ObjectNew: nil, + }, + expUpdate: false, + }, + { + name: "both objects are nil", + event: event.UpdateEvent{ + ObjectOld: nil, + ObjectNew: nil, + }, + expUpdate: false, + }, + } + + p := AnnotationPredicate{Annotation: annotation} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + update := p.Update(test.event) + g.Expect(update).To(Equal(test.expUpdate)) + }) + } +} diff --git a/internal/framework/controller/reconciler.go b/internal/framework/controller/reconciler.go index 43a5ac2002..5d90891da5 100644 --- a/internal/framework/controller/reconciler.go +++ b/internal/framework/controller/reconciler.go @@ -6,6 +6,7 @@ import ( "reflect" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -28,6 +29,8 @@ type ReconcilerConfig struct { EventCh chan<- interface{} // NamespacedNameFilter filters resources the controller will process. Can be nil. NamespacedNameFilter NamespacedNameFilterFunc + // OnlyMetadata indicates that this controller for this resource is only caching metadata for the resource. + OnlyMetadata bool } // Reconciler reconciles Kubernetes resources of a specific type. @@ -49,12 +52,18 @@ func NewReconciler(cfg ReconcilerConfig) *Reconciler { } } -func newObject(objectType client.Object) client.Object { +func (r *Reconciler) newObject(objectType client.Object) client.Object { + if r.cfg.OnlyMetadata { + partialObj := &metav1.PartialObjectMetadata{} + partialObj.SetGroupVersionKind(objectType.GetObjectKind().GroupVersionKind()) + + return partialObj + } + // without Elem(), t will be a pointer to the type. For example, *v1.Gateway, not v1.Gateway t := reflect.TypeOf(objectType).Elem() // We could've used objectType.DeepCopyObject() here, but it's a bit slower confirmed by benchmarks. - return reflect.New(t).Interface().(client.Object) } @@ -73,7 +82,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } } - obj := newObject(r.cfg.ObjectType) + obj := r.newObject(r.cfg.ObjectType) + if err := r.cfg.Getter.Get(ctx, req.NamespacedName, obj); err != nil { if !apierrors.IsNotFound(err) { logger.Error(err, "Failed to get the resource") diff --git a/internal/framework/controller/register.go b/internal/framework/controller/register.go index b48fd2258a..085b1d148c 100644 --- a/internal/framework/controller/register.go +++ b/internal/framework/controller/register.go @@ -6,6 +6,7 @@ import ( "time" ctlr "sigs.k8s.io/controller-runtime" + ctlrBuilder "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -23,6 +24,7 @@ type config struct { k8sPredicate predicate.Predicate fieldIndices index.FieldIndices newReconciler NewReconcilerFunc + onlyMetadata bool } // NewReconcilerFunc defines a function that creates a new Reconciler. Used for unit-testing. @@ -59,6 +61,16 @@ func WithNewReconciler(newReconciler NewReconcilerFunc) Option { } } +// WithOnlyMetadata tells the controller to only cache metadata, and to watch the API server in metadata-only form. +// If using this option, you must set the GroupVersionKind on the ObjectType you pass into the Register function. +// If watching a resource with OnlyMetadata, for example the v1.Pod, you must not Get and List using the v1.Pod type. +// Instead, you must use the special metav1.PartialObjectMetadata type. +func WithOnlyMetadata() Option { + return func(cfg *config) { + cfg.onlyMetadata = true + } +} + func defaultConfig() config { return config{ newReconciler: NewReconciler, @@ -93,7 +105,12 @@ func Register( } } - builder := ctlr.NewControllerManagedBy(mgr).For(objectType) + var forOpts []ctlrBuilder.ForOption + if cfg.onlyMetadata { + forOpts = append(forOpts, ctlrBuilder.OnlyMetadata) + } + + builder := ctlr.NewControllerManagedBy(mgr).For(objectType, forOpts...) if cfg.k8sPredicate != nil { builder = builder.WithEventFilter(cfg.k8sPredicate) @@ -104,6 +121,7 @@ func Register( ObjectType: objectType, EventCh: eventCh, NamespacedNameFilter: cfg.namespacedNameFilter, + OnlyMetadata: cfg.onlyMetadata, } if err := builder.Complete(cfg.newReconciler(recCfg)); err != nil { diff --git a/internal/framework/gatewayclass/validate.go b/internal/framework/gatewayclass/validate.go new file mode 100644 index 0000000000..a18f8ab730 --- /dev/null +++ b/internal/framework/gatewayclass/validate.go @@ -0,0 +1,82 @@ +package gatewayclass + +import ( + "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" +) + +const ( + BundleVersionAnnotation = "gateway.networking.k8s.io/bundle-version" + RecommendedVersion = "v1.0.0" +) + +type apiVersion struct { + major string + minor string +} + +func ValidateCRDVersions( + crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata, +) ([]conditions.Condition, bool) { + installedAPIVersions := getBundleVersions(crdMetadata) + supportedAPIVersion := parseVersionString(RecommendedVersion) + + unsupported := false + bestEffort := false + + for _, version := range installedAPIVersions { + if version.major != supportedAPIVersion.major { + unsupported = true + } else if version.minor != supportedAPIVersion.minor { + bestEffort = true + } + } + + if unsupported { + return conditions.NewGatewayClassUnsupportedVersion(RecommendedVersion), false + } + + if bestEffort { + return conditions.NewGatewayClassSupportedVersionBestEffort(RecommendedVersion), true + } + + return nil, true +} + +func parseVersionString(version string) apiVersion { + versionBits := strings.Split(version, ".") + if len(versionBits) != 3 { + return apiVersion{} + } + + major, _ := strings.CutPrefix(versionBits[0], "v") + + return apiVersion{ + major: major, + minor: versionBits[1], + } +} + +func getBundleVersions(crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata) map[string]apiVersion { + gatewayCRDs := map[string]apiVersion{ + "gatewayclasses.gateway.networking.k8s.io": {}, + "gateways.gateway.networking.k8s.io": {}, + "httproutes.gateway.networking.k8s.io": {}, + "referencegrants.gateway.networking.k8s.io": {}, + } + + versions := make(map[string]apiVersion) + + for nsname, md := range crdMetadata { + if _, ok := gatewayCRDs[nsname.Name]; ok { + bundleVersion := md.Annotations[BundleVersionAnnotation] + versions[bundleVersion] = parseVersionString(bundleVersion) + } + } + + return versions +} diff --git a/internal/framework/gatewayclass/validate_test.go b/internal/framework/gatewayclass/validate_test.go new file mode 100644 index 0000000000..ecc55376e8 --- /dev/null +++ b/internal/framework/gatewayclass/validate_test.go @@ -0,0 +1,124 @@ +package gatewayclass_test + +import ( + "strings" + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" +) + +func TestValidateCRDVersions(t *testing.T) { + createCRDMetadata := func(version string) *metav1.PartialObjectMetadata { + return &metav1.PartialObjectMetadata{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + gatewayclass.BundleVersionAnnotation: version, + }, + }, + } + } + + // Adding patch version to RecommendedVersion to try and avoid having to update these tests with every release. + fields := strings.Split(gatewayclass.RecommendedVersion, ".") + fields[2] = "99" + + validVersionWithPatch := createCRDMetadata(strings.Join(fields, ".")) + bestEffortVersion := createCRDMetadata("v1.99.99") + unsupportedVersion := createCRDMetadata("v99.0.0") + + tests := []struct { + crds map[types.NamespacedName]*metav1.PartialObjectMetadata + name string + expConds []conditions.Condition + valid bool + }{ + { + name: "valid; all supported versions", + crds: map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gatewayclasses.gateway.networking.k8s.io"}: validVersionWithPatch, + {Name: "gateways.gateway.networking.k8s.io"}: validVersionWithPatch, + {Name: "httproutes.gateway.networking.k8s.io"}: validVersionWithPatch, + {Name: "referencegrants.gateway.networking.k8s.io"}: validVersionWithPatch, + {Name: "some.other.crd"}: unsupportedVersion, /* should ignore */ + }, + valid: true, + expConds: nil, + }, + { + name: "valid; only one Gateway API CRD exists but it's a supported version", + crds: map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gatewayclasses.gateway.networking.k8s.io"}: validVersionWithPatch, + {Name: "some.other.crd"}: unsupportedVersion, /* should ignore */ + }, + valid: true, + expConds: nil, + }, + { + name: "valid; all best effort (supported major version)", + crds: map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gatewayclasses.gateway.networking.k8s.io"}: bestEffortVersion, + {Name: "gateways.gateway.networking.k8s.io"}: bestEffortVersion, + {Name: "httproutes.gateway.networking.k8s.io"}: bestEffortVersion, + {Name: "referencegrants.gateway.networking.k8s.io"}: bestEffortVersion, + }, + valid: true, + expConds: conditions.NewGatewayClassSupportedVersionBestEffort(gatewayclass.RecommendedVersion), + }, + { + name: "valid; mix of supported and best effort versions", + crds: map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gatewayclasses.gateway.networking.k8s.io"}: validVersionWithPatch, + {Name: "gateways.gateway.networking.k8s.io"}: bestEffortVersion, + {Name: "httproutes.gateway.networking.k8s.io"}: validVersionWithPatch, + {Name: "referencegrants.gateway.networking.k8s.io"}: validVersionWithPatch, + }, + valid: true, + expConds: conditions.NewGatewayClassSupportedVersionBestEffort(gatewayclass.RecommendedVersion), + }, + { + name: "invalid; all unsupported versions", + crds: map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gatewayclasses.gateway.networking.k8s.io"}: unsupportedVersion, + {Name: "gateways.gateway.networking.k8s.io"}: unsupportedVersion, + {Name: "httproutes.gateway.networking.k8s.io"}: unsupportedVersion, + {Name: "referencegrants.gateway.networking.k8s.io"}: unsupportedVersion, + }, + valid: false, + expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + }, + { + name: "invalid; mix unsupported and best effort versions", + crds: map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gatewayclasses.gateway.networking.k8s.io"}: unsupportedVersion, + {Name: "gateways.gateway.networking.k8s.io"}: bestEffortVersion, + {Name: "httproutes.gateway.networking.k8s.io"}: unsupportedVersion, + {Name: "referencegrants.gateway.networking.k8s.io"}: bestEffortVersion, + }, + valid: false, + expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + }, + { + name: "invalid; bad version string", + crds: map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gatewayclasses.gateway.networking.k8s.io"}: createCRDMetadata("v"), + }, + valid: false, + expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + conds, valid := gatewayclass.ValidateCRDVersions(test.crds) + g.Expect(valid).To(Equal(test.valid)) + g.Expect(conds).To(Equal(test.expConds)) + }) + } +} diff --git a/internal/mode/provisioner/handler.go b/internal/mode/provisioner/handler.go index 0495e176a7..853e814f1b 100644 --- a/internal/mode/provisioner/handler.go +++ b/internal/mode/provisioner/handler.go @@ -11,6 +11,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" ) @@ -56,20 +57,29 @@ func (h *eventHandler) setGatewayClassStatuses(ctx context.Context) { } var gcExists bool + for nsname, gc := range h.store.gatewayClasses { - var conds []conditions.Condition + // The order of conditions matters. Default conditions are added first so that any additional conditions will + // override them, which is ensured by DeduplicateConditions. + conds := conditions.NewDefaultGatewayClassConditions() + if gc.Name == h.gcName { gcExists = true - conds = conditions.NewDefaultGatewayClassConditions() } else { - conds = []conditions.Condition{conditions.NewGatewayClassConflict()} + conds = append(conds, conditions.NewGatewayClassConflict()) } + // We ignore the boolean return value here because the provisioner only sets status, + // it does not generate config. + supportedVersionConds, _ := gatewayclass.ValidateCRDVersions(h.store.crdMetadata) + conds = append(conds, supportedVersionConds...) + statuses.GatewayClassStatuses[nsname] = status.GatewayClassStatus{ - Conditions: conds, + Conditions: conditions.DeduplicateConditions(conds), ObservedGeneration: gc.Generation, } } + if !gcExists { panic(fmt.Errorf("GatewayClass %s must exist", h.gcName)) } diff --git a/internal/mode/provisioner/handler_test.go b/internal/mode/provisioner/handler_test.go index ada3cce1db..9a6de3b39c 100644 --- a/internal/mode/provisioner/handler_test.go +++ b/internal/mode/provisioner/handler_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/ginkgo/v2" v1 "k8s.io/api/apps/v1" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -19,6 +20,7 @@ import ( embeddedfiles "github.com/nginxinc/nginx-gateway-fabric" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes" @@ -34,6 +36,8 @@ var _ = Describe("handler", func() { statusUpdater status.Updater k8sclient client.Client + crd *metav1.PartialObjectMetadata + gc *gatewayv1.GatewayClass ) BeforeEach(OncePerOrdered, func() { @@ -41,6 +45,7 @@ var _ = Describe("handler", func() { Expect(gatewayv1.AddToScheme(scheme)).Should(Succeed()) Expect(v1.AddToScheme(scheme)).Should(Succeed()) + Expect(apiext.AddToScheme(scheme)).Should(Succeed()) k8sclient = fake.NewClientBuilder(). WithScheme(scheme). @@ -62,6 +67,29 @@ var _ = Describe("handler", func() { GatewayClassName: gcName, UpdateGatewayClassStatus: true, }) + + // Add GatewayClass CRD to the cluster + crd = &metav1.PartialObjectMetadata{ + TypeMeta: metav1.TypeMeta{ + Kind: "CustomResourceDefinition", + APIVersion: "apiextensions.k8s.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "gatewayclasses.gateway.networking.k8s.io", + Annotations: map[string]string{ + gatewayclass.BundleVersionAnnotation: gatewayclass.RecommendedVersion, + }, + }, + } + + err := k8sclient.Create(context.Background(), crd) + Expect(err).ShouldNot(HaveOccurred()) + + gc = &gatewayv1.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: gcName, + }, + } }) createGateway := func(gwNsName types.NamespacedName) *gatewayv1.Gateway { @@ -79,21 +107,18 @@ var _ = Describe("handler", func() { itShouldUpsertGatewayClass := func() { // Add GatewayClass to the cluster - gc := &gatewayv1.GatewayClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: gcName, - }, - } - err := k8sclient.Create(context.Background(), gc) Expect(err).ShouldNot(HaveOccurred()) - // UpsertGatewayClass + // UpsertGatewayClass and CRD batch := []interface{}{ &events.UpsertEvent{ Resource: gc, }, + &events.UpsertEvent{ + Resource: crd, + }, } handler.HandleEventBatch(context.Background(), zap.New(), batch) @@ -113,6 +138,14 @@ var _ = Describe("handler", func() { Reason: "Accepted", Message: "GatewayClass is accepted", }, + { + Type: string(gatewayv1.GatewayClassReasonSupportedVersion), + Status: metav1.ConditionTrue, + ObservedGeneration: 0, + LastTransitionTime: fakeClockTime, + Reason: "SupportedVersion", + Message: "Gateway API CRD versions are supported", + }, } Expect(clusterGc.Status.Conditions).To(Equal(expectedConditions)) @@ -147,6 +180,73 @@ var _ = Describe("handler", func() { Expect(dep.Spec.Template.Spec.Containers[0].Args).To(ContainElement(expectedLockFlag)) } + itShouldUpsertCRD := func(version string, accepted bool) { + updatedCRD := crd + updatedCRD.Annotations[gatewayclass.BundleVersionAnnotation] = version + + err := k8sclient.Update(context.Background(), updatedCRD) + Expect(err).ShouldNot(HaveOccurred()) + + batch := []interface{}{ + &events.UpsertEvent{ + Resource: updatedCRD, + }, + } + + handler.HandleEventBatch(context.Background(), zap.New(), batch) + + updatedGC := &gatewayv1.GatewayClass{} + + err = k8sclient.Get(context.Background(), client.ObjectKeyFromObject(gc), updatedGC) + Expect(err).ShouldNot(HaveOccurred()) + + var expConds []metav1.Condition + if !accepted { + expConds = []metav1.Condition{ + { + Type: string(gatewayv1.GatewayClassConditionStatusAccepted), + Status: metav1.ConditionFalse, + ObservedGeneration: 0, + LastTransitionTime: fakeClockTime, + Reason: string(gatewayv1.GatewayClassReasonUnsupportedVersion), + Message: fmt.Sprintf("Gateway API CRD versions are not supported. "+ + "Please install version %s", gatewayclass.RecommendedVersion), + }, + { + Type: string(gatewayv1.GatewayClassReasonSupportedVersion), + Status: metav1.ConditionFalse, + ObservedGeneration: 0, + LastTransitionTime: fakeClockTime, + Reason: string(gatewayv1.GatewayClassReasonUnsupportedVersion), + Message: fmt.Sprintf("Gateway API CRD versions are not supported. "+ + "Please install version %s", gatewayclass.RecommendedVersion), + }, + } + } else { + expConds = []metav1.Condition{ + { + Type: string(gatewayv1.GatewayClassConditionStatusAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 0, + LastTransitionTime: fakeClockTime, + Reason: string(gatewayv1.GatewayClassReasonAccepted), + Message: "GatewayClass is accepted", + }, + { + Type: string(gatewayv1.GatewayClassReasonSupportedVersion), + Status: metav1.ConditionFalse, + ObservedGeneration: 0, + LastTransitionTime: fakeClockTime, + Reason: string(gatewayv1.GatewayClassReasonUnsupportedVersion), + Message: fmt.Sprintf("Gateway API CRD versions are not recommended. "+ + "Recommended version is %s", gatewayclass.RecommendedVersion), + }, + } + } + + Expect(updatedGC.Status.Conditions).To(Equal(expConds)) + } + itShouldPanicWhenUpsertingGateway := func(gwNsName types.NamespacedName) { batch := []interface{}{ &events.UpsertEvent{ @@ -276,7 +376,7 @@ var _ = Describe("handler", func() { When("upserting GatewayClass that is not set in command-line argument", func() { It("should set the proper status if this controller is referenced", func() { - gc := &gatewayv1.GatewayClass{ + newGC := &gatewayv1.GatewayClass{ ObjectMeta: metav1.ObjectMeta{ Name: "unknown-gc", }, @@ -284,35 +384,58 @@ var _ = Describe("handler", func() { ControllerName: "test.example.com", }, } - err := k8sclient.Create(context.Background(), gc) + + err := k8sclient.Create(context.Background(), newGC) Expect(err).ShouldNot(HaveOccurred()) batch := []interface{}{ &events.UpsertEvent{ - Resource: gc, + Resource: newGC, + }, + &events.UpsertEvent{ + Resource: crd, }, } handler.HandleEventBatch(context.Background(), zap.New(), batch) unknownGC := &gatewayv1.GatewayClass{} - err = k8sclient.Get(context.Background(), client.ObjectKeyFromObject(gc), unknownGC) + err = k8sclient.Get(context.Background(), client.ObjectKeyFromObject(newGC), unknownGC) Expect(err).ShouldNot(HaveOccurred()) expectedConditions := []metav1.Condition{ + { + Type: string(gatewayv1.GatewayClassReasonSupportedVersion), + Status: metav1.ConditionTrue, + ObservedGeneration: 0, + LastTransitionTime: fakeClockTime, + Reason: "SupportedVersion", + Message: "Gateway API CRD versions are supported", + }, { Type: string(gatewayv1.GatewayClassConditionStatusAccepted), Status: metav1.ConditionFalse, ObservedGeneration: 0, LastTransitionTime: fakeClockTime, Reason: string(conditions.GatewayClassReasonGatewayClassConflict), - Message: string(conditions.GatewayClassMessageGatewayClassConflict), + Message: conditions.GatewayClassMessageGatewayClassConflict, }, } - Expect(unknownGC.Status.Conditions).To(Equal(expectedConditions)) }) }) + + When("upserting Gateway API CRD that is not a supported major version", func() { + It("should set the SupportedVersion and Accepted statuses to false on GatewayClass", func() { + itShouldUpsertCRD("v99.0.0", false /* accepted */) + }) + }) + + When("upserting Gateway API CRD that is not a supported minor version", func() { + It("should set the SupportedVersion status to false and Accepted status to true on GatewayClass", func() { + itShouldUpsertCRD("1.99.0", true /* accepted */) + }) + }) }) Describe("Edge cases", func() { diff --git a/internal/mode/provisioner/manager.go b/internal/mode/provisioner/manager.go index 1454111578..1bca844bac 100644 --- a/internal/mode/provisioner/manager.go +++ b/internal/mode/provisioner/manager.go @@ -5,8 +5,10 @@ import ( "github.com/go-logr/logr" v1 "k8s.io/api/apps/v1" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" utilruntime "k8s.io/apimachinery/pkg/util/runtime" ctlr "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -17,6 +19,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/predicate" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" ) @@ -39,6 +42,7 @@ func StartManager(cfg Config) error { scheme := runtime.NewScheme() utilruntime.Must(gatewayv1.AddToScheme(scheme)) utilruntime.Must(v1.AddToScheme(scheme)) + utilruntime.Must(apiext.AddToScheme(scheme)) options := manager.Options{ Scheme: scheme, @@ -51,6 +55,11 @@ func StartManager(cfg Config) error { return fmt.Errorf("cannot build runtime manager: %w", err) } + crdWithGVK := apiext.CustomResourceDefinition{} + crdWithGVK.SetGroupVersionKind( + schema.GroupVersionKind{Group: apiext.GroupName, Version: "v1", Kind: "CustomResourceDefinition"}, + ) + // Note: for any new object type or a change to the existing one, // make sure to also update firstBatchPreparer creation below controllerRegCfgs := []struct { @@ -66,6 +75,15 @@ func StartManager(cfg Config) error { { objectType: &gatewayv1.Gateway{}, }, + { + objectType: &crdWithGVK, + options: []controller.Option{ + controller.WithOnlyMetadata(), + controller.WithK8sPredicate( + predicate.AnnotationPredicate{Annotation: gatewayclass.BundleVersionAnnotation}, + ), + }, + }, } ctx := ctlr.SetupSignalHandler() @@ -83,6 +101,15 @@ func StartManager(cfg Config) error { } } + partialObjectMetadataList := &metav1.PartialObjectMetadataList{} + partialObjectMetadataList.SetGroupVersionKind( + schema.GroupVersionKind{ + Group: apiext.GroupName, + Version: "v1", + Kind: "CustomResourceDefinition", + }, + ) + firstBatchPreparer := events.NewFirstEventBatchPreparerImpl( mgr.GetCache(), []client.Object{ @@ -90,6 +117,7 @@ func StartManager(cfg Config) error { }, []client.ObjectList{ &gatewayv1.GatewayList{}, + partialObjectMetadataList, }, ) diff --git a/internal/mode/provisioner/store.go b/internal/mode/provisioner/store.go index a3e521f4e2..6585f21017 100644 --- a/internal/mode/provisioner/store.go +++ b/internal/mode/provisioner/store.go @@ -3,6 +3,7 @@ package provisioner import ( "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -14,12 +15,14 @@ import ( type store struct { gatewayClasses map[types.NamespacedName]*v1.GatewayClass gateways map[types.NamespacedName]*v1.Gateway + crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata } func newStore() *store { return &store{ gatewayClasses: make(map[types.NamespacedName]*v1.GatewayClass), gateways: make(map[types.NamespacedName]*v1.Gateway), + crdMetadata: make(map[types.NamespacedName]*metav1.PartialObjectMetadata), } } @@ -32,6 +35,8 @@ func (s *store) update(batch events.EventBatch) { s.gatewayClasses[client.ObjectKeyFromObject(obj)] = obj case *v1.Gateway: s.gateways[client.ObjectKeyFromObject(obj)] = obj + case *metav1.PartialObjectMetadata: + s.crdMetadata[client.ObjectKeyFromObject(obj)] = obj default: panic(fmt.Errorf("unknown resource type %T", e.Resource)) } @@ -41,6 +46,8 @@ func (s *store) update(batch events.EventBatch) { delete(s.gatewayClasses, e.NamespacedName) case *v1.Gateway: delete(s.gateways, e.NamespacedName) + case *metav1.PartialObjectMetadata: + delete(s.crdMetadata, e.NamespacedName) default: panic(fmt.Errorf("unknown resource type %T", e.Type)) } diff --git a/internal/mode/static/build_statuses.go b/internal/mode/static/build_statuses.go index 5589922161..a5ec222cdd 100644 --- a/internal/mode/static/build_statuses.go +++ b/internal/mode/static/build_statuses.go @@ -61,7 +61,7 @@ func buildGatewayAPIStatuses( parentStatuses = append(parentStatuses, status.ParentStatus{ GatewayNsName: ref.Gateway, SectionName: routeRef.SectionName, - Conditions: staticConds.DeduplicateConditions(allConds), + Conditions: conditions.DeduplicateConditions(allConds), }) } @@ -91,7 +91,7 @@ func buildGatewayClassStatuses( conds = append(conds, gc.Conditions...) statuses[client.ObjectKeyFromObject(gc.Source)] = status.GatewayClassStatus{ - Conditions: staticConds.DeduplicateConditions(conds), + Conditions: conditions.DeduplicateConditions(conds), ObservedGeneration: gc.Source.Generation, } } @@ -136,7 +136,7 @@ func buildGatewayStatus( ) status.GatewayStatus { if !gateway.Valid { return status.GatewayStatus{ - Conditions: staticConds.DeduplicateConditions(gateway.Conditions), + Conditions: conditions.DeduplicateConditions(gateway.Conditions), ObservedGeneration: gateway.Source.Generation, } } @@ -163,7 +163,7 @@ func buildGatewayStatus( listenerStatuses[name] = status.ListenerStatus{ AttachedRoutes: int32(len(l.Routes)), - Conditions: staticConds.DeduplicateConditions(conds), + Conditions: conditions.DeduplicateConditions(conds), SupportedKinds: l.SupportedKinds, } } @@ -183,7 +183,7 @@ func buildGatewayStatus( } return status.GatewayStatus{ - Conditions: staticConds.DeduplicateConditions(gwConds), + Conditions: conditions.DeduplicateConditions(gwConds), ListenerStatuses: listenerStatuses, Addresses: gwAddresses, ObservedGeneration: gateway.Source.Generation, diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index e293291663..9667283226 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -9,9 +9,11 @@ import ( "github.com/prometheus/client_golang/prometheus" apiv1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/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" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -31,6 +33,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/predicate" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors" @@ -57,6 +60,7 @@ func init() { utilruntime.Must(apiv1.AddToScheme(scheme)) utilruntime.Must(discoveryV1.AddToScheme(scheme)) utilruntime.Must(ngfAPI.AddToScheme(scheme)) + utilruntime.Must(apiext.AddToScheme(scheme)) } func StartManager(cfg config.Config) error { @@ -244,6 +248,11 @@ func registerControllers( options []controller.Option } + crdWithGVK := apiext.CustomResourceDefinition{} + crdWithGVK.SetGroupVersionKind( + schema.GroupVersionKind{Group: apiext.GroupName, Version: "v1", Kind: "CustomResourceDefinition"}, + ) + // Note: for any new object type or a change to the existing one, // make sure to also update prepareFirstEventBatchPreparerArgs() controllerRegCfgs := []ctlrCfg{ @@ -304,6 +313,15 @@ func registerControllers( { objectType: &gatewayv1beta1.ReferenceGrant{}, }, + { + objectType: &crdWithGVK, + options: []controller.Option{ + controller.WithOnlyMetadata(), + controller.WithK8sPredicate( + predicate.AnnotationPredicate{Annotation: gatewayclass.BundleVersionAnnotation}, + ), + }, + }, } if cfg.ConfigName != "" { @@ -346,6 +364,16 @@ func prepareFirstEventBatchPreparerArgs( objects := []client.Object{ &gatewayv1.GatewayClass{ObjectMeta: metav1.ObjectMeta{Name: gcName}}, } + + partialObjectMetadataList := &metav1.PartialObjectMetadataList{} + partialObjectMetadataList.SetGroupVersionKind( + schema.GroupVersionKind{ + Group: apiext.GroupName, + Version: "v1", + Kind: "CustomResourceDefinition", + }, + ) + objectLists := []client.ObjectList{ &apiv1.ServiceList{}, &apiv1.SecretList{}, @@ -353,6 +381,7 @@ func prepareFirstEventBatchPreparerArgs( &discoveryV1.EndpointSliceList{}, &gatewayv1.HTTPRouteList{}, &gatewayv1beta1.ReferenceGrantList{}, + partialObjectMetadataList, } if gwNsName == nil { diff --git a/internal/mode/static/manager_test.go b/internal/mode/static/manager_test.go index fd8e540adb..3191a2e01d 100644 --- a/internal/mode/static/manager_test.go +++ b/internal/mode/static/manager_test.go @@ -6,7 +6,9 @@ import ( . "github.com/onsi/gomega" apiv1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -19,6 +21,15 @@ import ( func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { const gcName = "nginx" + partialObjectMetadataList := &metav1.PartialObjectMetadataList{} + partialObjectMetadataList.SetGroupVersionKind( + schema.GroupVersionKind{ + Group: apiext.GroupName, + Version: "v1", + Kind: "CustomResourceDefinition", + }, + ) + tests := []struct { name string gwNsName *types.NamespacedName @@ -39,6 +50,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &gatewayv1.HTTPRouteList{}, &gatewayv1.GatewayList{}, &gatewayv1beta1.ReferenceGrantList{}, + partialObjectMetadataList, }, }, { @@ -58,6 +70,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &discoveryV1.EndpointSliceList{}, &gatewayv1.HTTPRouteList{}, &gatewayv1beta1.ReferenceGrantList{}, + partialObjectMetadataList, }, }, } diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 42d92f8fce..670435a84e 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -7,6 +7,8 @@ import ( "github.com/go-logr/logr" apiv1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -18,6 +20,7 @@ import ( gwapivalidation "sigs.k8s.io/gateway-api/apis/v1/validation" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" @@ -95,6 +98,7 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { Namespaces: make(map[types.NamespacedName]*apiv1.Namespace), ReferenceGrants: make(map[types.NamespacedName]*v1beta1.ReferenceGrant), Secrets: make(map[types.NamespacedName]*apiv1.Secret), + CRDMetadata: make(map[types.NamespacedName]*metav1.PartialObjectMetadata), } extractGVK := func(obj client.Object) schema.GroupVersionKind { @@ -110,54 +114,59 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { clusterState: clusterStore, } - triggerStateChange := func(objType client.Object, nsname types.NamespacedName) bool { - return processor.latestGraph != nil && processor.latestGraph.IsReferenced(objType, nsname) + isReferenced := func(obj client.Object) bool { + nsname := types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()} + return processor.latestGraph != nil && processor.latestGraph.IsReferenced(obj, nsname) } trackingUpdater := newChangeTrackingUpdater( cfg.RelationshipCapturer, - triggerStateChange, extractGVK, []changeTrackingUpdaterObjectTypeCfg{ { - gvk: extractGVK(&v1.GatewayClass{}), - store: newObjectStoreMapAdapter(clusterStore.GatewayClasses), - trackUpsertDelete: true, + gvk: extractGVK(&v1.GatewayClass{}), + store: newObjectStoreMapAdapter(clusterStore.GatewayClasses), + predicate: generationChangedPredicate{}, }, { - gvk: extractGVK(&v1.Gateway{}), - store: newObjectStoreMapAdapter(clusterStore.Gateways), - trackUpsertDelete: true, + gvk: extractGVK(&v1.Gateway{}), + store: newObjectStoreMapAdapter(clusterStore.Gateways), + predicate: generationChangedPredicate{}, }, { - gvk: extractGVK(&v1.HTTPRoute{}), - store: newObjectStoreMapAdapter(clusterStore.HTTPRoutes), - trackUpsertDelete: true, + gvk: extractGVK(&v1.HTTPRoute{}), + store: newObjectStoreMapAdapter(clusterStore.HTTPRoutes), + predicate: generationChangedPredicate{}, }, { - gvk: extractGVK(&v1beta1.ReferenceGrant{}), - store: newObjectStoreMapAdapter(clusterStore.ReferenceGrants), - trackUpsertDelete: true, + gvk: extractGVK(&v1beta1.ReferenceGrant{}), + store: newObjectStoreMapAdapter(clusterStore.ReferenceGrants), + predicate: generationChangedPredicate{}, }, { - gvk: extractGVK(&apiv1.Namespace{}), - store: newObjectStoreMapAdapter(clusterStore.Namespaces), - trackUpsertDelete: false, + gvk: extractGVK(&apiv1.Namespace{}), + store: newObjectStoreMapAdapter(clusterStore.Namespaces), + predicate: nil, }, { - gvk: extractGVK(&apiv1.Service{}), - store: newObjectStoreMapAdapter(clusterStore.Services), - trackUpsertDelete: false, + gvk: extractGVK(&apiv1.Service{}), + store: newObjectStoreMapAdapter(clusterStore.Services), + predicate: nil, }, { - gvk: extractGVK(&discoveryV1.EndpointSlice{}), - store: nil, - trackUpsertDelete: false, + gvk: extractGVK(&discoveryV1.EndpointSlice{}), + store: nil, + predicate: nil, }, { - gvk: extractGVK(&apiv1.Secret{}), - store: newObjectStoreMapAdapter(clusterStore.Secrets), - trackUpsertDelete: false, + gvk: extractGVK(&apiv1.Secret{}), + store: newObjectStoreMapAdapter(clusterStore.Secrets), + predicate: newStateChangedPredicateFuncs(isReferenced), + }, + { + gvk: extractGVK(&apiext.CustomResourceDefinition{}), + store: newObjectStoreMapAdapter(clusterStore.CRDMetadata), + predicate: annotationChangedPredicate{annotation: gatewayclass.BundleVersionAnnotation}, }, }, ) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 1e50371b2b..f04234de13 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -6,6 +6,7 @@ import ( "github.com/onsi/gomega/format" apiv1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -19,6 +20,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" @@ -186,6 +188,7 @@ func createScheme() *runtime.Scheme { utilruntime.Must(v1beta1.AddToScheme(scheme)) utilruntime.Must(apiv1.AddToScheme(scheme)) utilruntime.Must(discoveryV1.AddToScheme(scheme)) + utilruntime.Must(apiext.AddToScheme(scheme)) return scheme } @@ -270,14 +273,15 @@ var _ = Describe("ChangeProcessor", func() { Describe("Process gateway resources", Ordered, func() { var ( - gcUpdated *v1.GatewayClass - diffNsTLSSecret, sameNsTLSSecret *apiv1.Secret - hr1, hr1Updated, hr2 *v1.HTTPRoute - gw1, gw1Updated, gw2 *v1.Gateway - refGrant1, refGrant2 *v1beta1.ReferenceGrant - expGraph *graph.Graph - expRouteHR1, expRouteHR2 *graph.Route - hr1Name, hr2Name types.NamespacedName + gcUpdated *v1.GatewayClass + diffNsTLSSecret, sameNsTLSSecret *apiv1.Secret + hr1, hr1Updated, hr2 *v1.HTTPRoute + gw1, gw1Updated, gw2 *v1.Gateway + refGrant1, refGrant2 *v1beta1.ReferenceGrant + expGraph *graph.Graph + expRouteHR1, expRouteHR2 *graph.Route + hr1Name, hr2Name types.NamespacedName + gatewayAPICRD, gatewayAPICRDUpdated *metav1.PartialObjectMetadata ) BeforeAll(func() { gcUpdated = gc.DeepCopy() @@ -375,6 +379,22 @@ var _ = Describe("ChangeProcessor", func() { gw1Updated.Generation++ gw2 = createGatewayWithTLSListener("gateway-2", sameNsTLSSecret) + + gatewayAPICRD = &metav1.PartialObjectMetadata{ + TypeMeta: metav1.TypeMeta{ + Kind: "CustomResourceDefinition", + APIVersion: "apiextensions.k8s.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "gatewayclasses.gateway.networking.k8s.io", + Annotations: map[string]string{ + gatewayclass.BundleVersionAnnotation: gatewayclass.RecommendedVersion, + }, + }, + } + + gatewayAPICRDUpdated = gatewayAPICRD.DeepCopy() + gatewayAPICRDUpdated.Annotations[gatewayclass.BundleVersionAnnotation] = "v1.99.0" }) BeforeEach(func() { expRouteHR1 = &graph.Route{ @@ -479,7 +499,6 @@ var _ = Describe("ChangeProcessor", func() { ReferencedSecrets: map[types.NamespacedName]*graph.Secret{}, } }) - When("no upsert has occurred", func() { It("returns nil graph", func() { changed, graphCfg := processor.Process() @@ -488,6 +507,15 @@ var _ = Describe("ChangeProcessor", func() { }) }) When("GatewayClass doesn't exist", func() { + When("Gateway API CRD is added", func() { + It("returns empty graph", func() { + processor.CaptureUpsertChange(gatewayAPICRD) + + changed, graphCfg := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(&graph.Graph{}, graphCfg)).To(BeEmpty()) + }) + }) When("Gateways don't exist", func() { When("the first HTTPRoute is upserted", func() { It("returns empty graph", func() { @@ -606,7 +634,6 @@ var _ = Describe("ChangeProcessor", func() { Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) }) }) - When("the ReferenceGrant allowing the hr1 to reference the Service in different ns is upserted", func() { It("returns updated graph", func() { processor.CaptureUpsertChange(refGrant2) @@ -620,6 +647,48 @@ var _ = Describe("ChangeProcessor", func() { Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) }) }) + When("the Gateway API CRD with bundle version annotation change is processed", func() { + It("returns updated graph", func() { + processor.CaptureUpsertChange(gatewayAPICRDUpdated) + + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } + + expGraph.GatewayClass.Conditions = conditions.NewGatewayClassSupportedVersionBestEffort( + gatewayclass.RecommendedVersion, + ) + + changed, graphCfg := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) + }) + }) + When("the Gateway API CRD without bundle version annotation change is processed", func() { + It("returns nil graph", func() { + gatewayAPICRDSameVersion := gatewayAPICRDUpdated.DeepCopy() + + processor.CaptureUpsertChange(gatewayAPICRDSameVersion) + + changed, graphCfg := processor.Process() + Expect(changed).To(BeFalse()) + Expect(graphCfg).To(BeNil()) + }) + }) + When("the Gateway API CRD with bundle version annotation change is processed", func() { + It("returns updated graph", func() { + // change back to supported version + processor.CaptureUpsertChange(gatewayAPICRD) + + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } + + changed, graphCfg := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) + }) + }) When("the first HTTPRoute without a generation changed is processed", func() { It("returns nil graph", func() { hr1UpdatedSameGen := hr1.DeepCopy() diff --git a/internal/mode/static/state/changed_predicate.go b/internal/mode/static/state/changed_predicate.go new file mode 100644 index 0000000000..d52fbdc941 --- /dev/null +++ b/internal/mode/static/state/changed_predicate.go @@ -0,0 +1,81 @@ +package state + +import "sigs.k8s.io/controller-runtime/pkg/client" + +// stateChangedPredicate determines whether upsert and delete events constitute a change in state. +type stateChangedPredicate interface { + // upsert returns true if the newObject changes state. + upsert(oldObject, newObject client.Object) bool + // delete returns true if the deletion of the object changes state. + delete(object client.Object) bool +} + +// funcs is a function that implements stateChangedPredicate. +type funcs struct { + upsertStateChangeFunc func(oldObject, newObject client.Object) bool + deleteStateChangeFunc func(object client.Object) bool +} + +func (f funcs) upsert(oldObject, newObject client.Object) bool { + return f.upsertStateChangeFunc(oldObject, newObject) +} + +func (f funcs) delete(object client.Object) bool { + return f.deleteStateChangeFunc(object) +} + +// newStateChangedPredicateFuncs returns a predicate funcs that applies the given function on calls to upsert and +// delete. +func newStateChangedPredicateFuncs(stateChangedFunc func(object client.Object) bool) funcs { + return funcs{ + upsertStateChangeFunc: func(oldObject, newObject client.Object) bool { + return stateChangedFunc(newObject) + }, + deleteStateChangeFunc: func(object client.Object) bool { + return stateChangedFunc(object) + }, + } +} + +// generationChangedPredicate implements stateChangedPredicate based on the generation of the object. +// This predicate will return true on upsert if the object's generation has changed. +// It always returns true on delete. +type generationChangedPredicate struct{} + +func (generationChangedPredicate) delete(_ client.Object) bool { return true } + +func (generationChangedPredicate) upsert(oldObject, newObject client.Object) bool { + if oldObject == nil { + return true + } + + if newObject == nil { + panic("Cannot determine if generation has changed on upsert because new object is nil") + } + + return newObject.GetGeneration() != oldObject.GetGeneration() +} + +// annotationChangedPredicate implements stateChangedPredicate based on the value of the annotation provided. +// This predicate will return true on upsert if the annotation's value has changed. +// It always returns true on delete. +type annotationChangedPredicate struct { + annotation string +} + +func (a annotationChangedPredicate) upsert(oldObject, newObject client.Object) bool { + if oldObject == nil { + return true + } + + if newObject == nil { + panic("Cannot determine if annotation has changed on upsert because new object is nil") + } + + oldAnnotation := oldObject.GetAnnotations()[a.annotation] + newAnnotation := newObject.GetAnnotations()[a.annotation] + + return oldAnnotation != newAnnotation +} + +func (a annotationChangedPredicate) delete(_ client.Object) bool { return true } diff --git a/internal/mode/static/state/changed_predicate_test.go b/internal/mode/static/state/changed_predicate_test.go new file mode 100644 index 0000000000..e161f9ea30 --- /dev/null +++ b/internal/mode/static/state/changed_predicate_test.go @@ -0,0 +1,211 @@ +package state + +import ( + "testing" + + . "github.com/onsi/gomega" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestFuncsPredicate(t *testing.T) { + alwaysTrueFunc := func(object client.Object) bool { return true } + + p := newStateChangedPredicateFuncs(alwaysTrueFunc) + + g := NewWithT(t) + + g.Expect(p.delete(nil)).To(BeTrue()) + g.Expect(p.upsert(nil, nil)).To(BeTrue()) +} + +func TestGenerationChangedPredicate_Delete(t *testing.T) { + p := generationChangedPredicate{} + + g := NewWithT(t) + g.Expect(p.delete(nil)).To(BeTrue()) +} + +func TestGenerationChangedPredicate_Update(t *testing.T) { + tests := []struct { + oldObj client.Object + newObj client.Object + name string + stateChanged bool + expPanic bool + }{ + { + name: "generation has changed", + oldObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + }, + newObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + }, + }, + stateChanged: true, + }, + { + name: "generation has not changed", + oldObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + }, + newObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + }, + stateChanged: false, + }, + { + name: "old object is nil", + oldObj: nil, + newObj: &v1.Pod{}, + stateChanged: true, + }, + { + name: "new object is nil", + oldObj: &v1.Pod{}, + newObj: nil, + expPanic: true, + }, + } + + p := generationChangedPredicate{} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + if test.expPanic { + upsert := func() { + p.upsert(test.oldObj, test.newObj) + } + g.Expect(upsert).Should(Panic()) + } else { + g.Expect(p.upsert(test.oldObj, test.newObj)).To(Equal(test.stateChanged)) + } + }) + } +} + +func TestAnnotationChangedPredicate_Delete(t *testing.T) { + p := annotationChangedPredicate{} + + g := NewWithT(t) + g.Expect(p.delete(nil)).To(BeTrue()) +} + +func TestAnnotationChangedPredicate_Update(t *testing.T) { + annotation := "test" + + tests := []struct { + oldObj client.Object + newObj client.Object + name string + stateChanged bool + expPanic bool + }{ + { + name: "annotation has changed", + oldObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{annotation: "one"}, + }, + }, + newObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{annotation: "two"}, + }, + }, + stateChanged: true, + }, + { + name: "annotation added", + oldObj: &v1.Pod{}, + newObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{annotation: "one"}, + }, + }, + stateChanged: true, + }, + { + name: "annotation deleted", + oldObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{annotation: "one"}, + }, + }, + newObj: &v1.Pod{}, + stateChanged: true, + }, + { + name: "old object is nil", + oldObj: nil, + newObj: &v1.Pod{}, + stateChanged: true, + }, + { + name: "diff annotation changed", + oldObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"diff": "one"}, + }, + }, + newObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"diff": "two"}, + }, + }, + stateChanged: false, + }, + { + name: "no annotations", + oldObj: &v1.Pod{}, + newObj: &v1.Pod{}, + stateChanged: false, + }, + { + name: "annotation has not changed", + oldObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{annotation: "one"}, + }, + }, + newObj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{annotation: "one"}, + }, + }, + stateChanged: false, + }, + { + name: "new object is nil", + oldObj: &v1.Pod{}, + newObj: nil, + expPanic: true, + }, + } + + p := annotationChangedPredicate{annotation: annotation} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + if test.expPanic { + upsert := func() { + p.upsert(test.oldObj, test.newObj) + } + g.Expect(upsert).Should(Panic()) + } else { + g.Expect(p.upsert(test.oldObj, test.newObj)).To(Equal(test.stateChanged)) + } + }) + } +} diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 2703da1efc..25ade0b204 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -59,38 +59,6 @@ const ( "is programmed again" ) -// DeduplicateConditions removes duplicate conditions based on the condition type. -// The last condition wins. The order of conditions is preserved. -func DeduplicateConditions(conds []conditions.Condition) []conditions.Condition { - type elem struct { - cond conditions.Condition - reverseIdx int - } - - uniqueElems := make(map[string]elem) - - idx := 0 - for i := len(conds) - 1; i >= 0; i-- { - if _, exist := uniqueElems[conds[i].Type]; exist { - continue - } - - uniqueElems[conds[i].Type] = elem{ - cond: conds[i], - reverseIdx: idx, - } - idx++ - } - - result := make([]conditions.Condition, len(uniqueElems)) - - for _, el := range uniqueElems { - result[len(result)-el.reverseIdx-1] = el.cond - } - - return result -} - // NewTODO returns a Condition that can be used as a placeholder for a condition that is not yet implemented. func NewTODO(msg string) conditions.Condition { return conditions.Condition{ diff --git a/internal/mode/static/state/graph/gatewayclass.go b/internal/mode/static/state/graph/gatewayclass.go index 929bf1611b..fc3313f269 100644 --- a/internal/mode/static/state/graph/gatewayclass.go +++ b/internal/mode/static/state/graph/gatewayclass.go @@ -1,12 +1,14 @@ package graph import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) @@ -56,30 +58,39 @@ func processGatewayClasses( return processedGwClasses, gcExists } -func buildGatewayClass(gc *v1.GatewayClass) *GatewayClass { +func buildGatewayClass( + gc *v1.GatewayClass, + crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, +) *GatewayClass { if gc == nil { return nil } - var conds []conditions.Condition - - valErr := validateGatewayClass(gc) - if valErr != nil { - conds = append(conds, staticConds.NewGatewayClassInvalidParameters(valErr.Error())) - } + conds, valid := validateGatewayClass(gc, crdVersions) return &GatewayClass{ Source: gc, - Valid: valErr == nil, + Valid: valid, Conditions: conds, } } -func validateGatewayClass(gc *v1.GatewayClass) error { +func validateGatewayClass( + gc *v1.GatewayClass, + crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, +) ([]conditions.Condition, bool) { + var conds []conditions.Condition + + valid := true + if gc.Spec.ParametersRef != nil { path := field.NewPath("spec").Child("parametersRef") - return field.Forbidden(path, "parametersRef is not supported") + err := field.Forbidden(path, "parametersRef is not supported") + conds = append(conds, staticConds.NewGatewayClassInvalidParameters(err.Error())) + valid = false } - return nil + supportedVersionConds, versionsValid := gatewayclass.ValidateCRDVersions(crdVersions) + + return append(conds, supportedVersionConds...), valid && versionsValid } diff --git a/internal/mode/static/state/graph/gatewayclass_test.go b/internal/mode/static/state/graph/gatewayclass_test.go index 4ad77bd3c9..99cfed6bd5 100644 --- a/internal/mode/static/state/graph/gatewayclass_test.go +++ b/internal/mode/static/state/graph/gatewayclass_test.go @@ -10,6 +10,7 @@ import ( v1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) @@ -127,13 +128,35 @@ func TestBuildGatewayClass(t *testing.T) { }, } + validCRDs := map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gateways.gateway.networking.k8s.io"}: { + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + gatewayclass.BundleVersionAnnotation: gatewayclass.RecommendedVersion, + }, + }, + }, + } + + invalidCRDs := map[types.NamespacedName]*metav1.PartialObjectMetadata{ + {Name: "gateways.gateway.networking.k8s.io"}: { + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + gatewayclass.BundleVersionAnnotation: "v99.0.0", + }, + }, + }, + } + tests := []struct { - gc *v1.GatewayClass - expected *GatewayClass - name string + gc *v1.GatewayClass + crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata + expected *GatewayClass + name string }{ { - gc: validGC, + gc: validGC, + crdMetadata: validCRDs, expected: &GatewayClass{ Source: validGC, Valid: true, @@ -146,7 +169,8 @@ func TestBuildGatewayClass(t *testing.T) { name: "no gatewayclass", }, { - gc: invalidGC, + gc: invalidGC, + crdMetadata: validCRDs, expected: &GatewayClass{ Source: invalidGC, Valid: false, @@ -156,7 +180,17 @@ func TestBuildGatewayClass(t *testing.T) { ), }, }, - name: "invalid gatewayclass", + name: "invalid gatewayclass; parameters ref", + }, + { + gc: validGC, + crdMetadata: invalidCRDs, + expected: &GatewayClass{ + Source: validGC, + Valid: false, + Conditions: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + }, + name: "invalid gatewayclass; unsupported version", }, } @@ -164,7 +198,7 @@ func TestBuildGatewayClass(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - result := buildGatewayClass(test.gc) + result := buildGatewayClass(test.gc, test.crdMetadata) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) }) } diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index 8e370011e3..ae9f5f8e34 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -2,6 +2,7 @@ package graph import ( v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -19,6 +20,7 @@ type ClusterState struct { Namespaces map[types.NamespacedName]*v1.Namespace ReferenceGrants map[types.NamespacedName]*v1beta1.ReferenceGrant Secrets map[types.NamespacedName]*v1.Secret + CRDMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata } // Graph is a Graph-like representation of Gateway API resources. @@ -76,7 +78,8 @@ func BuildGraph( // configured GatewayClass does not reference this controller return &Graph{} } - gc := buildGatewayClass(processedGwClasses.Winner) + + gc := buildGatewayClass(processedGwClasses.Winner, state.CRDMetadata) secretResolver := newSecretResolver(state.Secrets) diff --git a/internal/mode/static/state/store.go b/internal/mode/static/state/store.go index 0c6adf4731..278a8b6654 100644 --- a/internal/mode/static/state/store.go +++ b/internal/mode/static/state/store.go @@ -20,7 +20,7 @@ type Updater interface { // objectStore is a store of client.Object type objectStore interface { - get(nsname types.NamespacedName) (client.Object, bool) + get(nsname types.NamespacedName) client.Object upsert(obj client.Object) delete(nsname types.NamespacedName) } @@ -37,9 +37,19 @@ func newObjectStoreMapAdapter[T client.Object](objects map[types.NamespacedName] } } -func (m *objectStoreMapAdapter[T]) get(nsname types.NamespacedName) (client.Object, bool) { +func (m *objectStoreMapAdapter[T]) get(nsname types.NamespacedName) client.Object { + var zeroVal client.Object + obj, exist := m.objects[nsname] - return obj, exist + if !exist { + // we return zeroVal here instead of obj because we need to be able to compare the return value to nil. + // obj has a value of nil, but its type is a pointer to the concrete type (e.g. *v1.Pod). + // This means that obj != nil. + // However, the value **and** type of zeroVal are nil, which means zeroVal == nil. + return zeroVal + } + + return obj } func (m *objectStoreMapAdapter[T]) upsert(obj client.Object) { @@ -92,7 +102,7 @@ func (m *multiObjectStore) mustFindStoreForObj(obj client.Object) objectStore { return store } -func (m *multiObjectStore) get(objType client.Object, nsname types.NamespacedName) (client.Object, bool) { +func (m *multiObjectStore) get(objType client.Object, nsname types.NamespacedName) client.Object { return m.mustFindStoreForObj(objType).get(nsname) } @@ -107,16 +117,12 @@ func (m *multiObjectStore) delete(objType client.Object, nsname types.Namespaced type changeTrackingUpdaterObjectTypeCfg struct { // store holds the objects of the gvk. If the store is nil, the objects of the gvk are not persisted. store objectStore - gvk schema.GroupVersionKind - // trackUpsertDelete indicates whether an upsert or delete of an object with the gvk results into a change to - // the changeTrackingUpdater's store. Note that for an upsert, the generation of a new object must be different - // from the generation of the previous version, otherwise such an upsert is not considered a change. - trackUpsertDelete bool + // predicate determines if upsert or delete event should trigger a change. + // If predicate is nil, then no upsert or delete event for this object will trigger a change. + predicate stateChangedPredicate + gvk schema.GroupVersionKind } -// triggerStateChangeFunc triggers a change to the changeTrackingUpdater's store for the given object. -type triggerStateChangeFunc func(objType client.Object, nsname types.NamespacedName) bool - // changeTrackingUpdater is an Updater that tracks changes to the cluster state in the multiObjectStore. // // It only works with objects with the GVKs registered in changeTrackingUpdaterObjectTypeCfg. Otherwise, it panics. @@ -126,39 +132,37 @@ type triggerStateChangeFunc func(objType client.Object, nsname types.NamespacedN // that its generation changed. // - An object is upserted or deleted, and it is related to another object, based on the decision by // the relationship capturer. -// - An object is upserted or deleted and triggerStateChange returns true for the object. +// - An object is upserted or deleted and the stateChangedPredicate for that object returns true. type changeTrackingUpdater struct { - store *multiObjectStore - capturer relationship.Capturer - triggerStateChange triggerStateChangeFunc + store *multiObjectStore + capturer relationship.Capturer + stateChangedPredicates map[schema.GroupVersionKind]stateChangedPredicate - extractGVK extractGVKFunc - supportedGVKs gvkList - trackedUpsertDeleteGVKs gvkList - persistedGVKs gvkList + extractGVK extractGVKFunc + supportedGVKs gvkList + persistedGVKs gvkList changed bool } func newChangeTrackingUpdater( capturer relationship.Capturer, - triggerStateChange triggerStateChangeFunc, extractGVK extractGVKFunc, objectTypeCfgs []changeTrackingUpdaterObjectTypeCfg, ) *changeTrackingUpdater { var ( - supportedGVKs gvkList - trackedUpsertDeleteGVKs gvkList - persistedGVKs gvkList + supportedGVKs gvkList + persistedGVKs gvkList - stores = make(map[schema.GroupVersionKind]objectStore) + stores = make(map[schema.GroupVersionKind]objectStore) + stateChangedPredicates = make(map[schema.GroupVersionKind]stateChangedPredicate) ) for _, cfg := range objectTypeCfgs { supportedGVKs = append(supportedGVKs, cfg.gvk) - if cfg.trackUpsertDelete { - trackedUpsertDeleteGVKs = append(trackedUpsertDeleteGVKs, cfg.gvk) + if cfg.predicate != nil { + stateChangedPredicates[cfg.gvk] = cfg.predicate } if cfg.store != nil { @@ -168,13 +172,12 @@ func newChangeTrackingUpdater( } return &changeTrackingUpdater{ - store: newMultiObjectStore(stores, extractGVK), - extractGVK: extractGVK, - supportedGVKs: supportedGVKs, - trackedUpsertDeleteGVKs: trackedUpsertDeleteGVKs, - persistedGVKs: persistedGVKs, - capturer: capturer, - triggerStateChange: triggerStateChange, + store: newMultiObjectStore(stores, extractGVK), + extractGVK: extractGVK, + supportedGVKs: supportedGVKs, + persistedGVKs: persistedGVKs, + capturer: capturer, + stateChangedPredicates: stateChangedPredicates, } } @@ -185,18 +188,22 @@ func (s *changeTrackingUpdater) assertSupportedGVK(gvk schema.GroupVersionKind) } func (s *changeTrackingUpdater) upsert(obj client.Object) (changed bool) { - if !s.persistedGVKs.contains(s.extractGVK(obj)) { + objTypeGVK := s.extractGVK(obj) + + if !s.persistedGVKs.contains(objTypeGVK) { return false } - oldObj, exist := s.store.get(obj, client.ObjectKeyFromObject(obj)) + oldObj := s.store.get(obj, client.ObjectKeyFromObject(obj)) + s.store.upsert(obj) - if !s.trackedUpsertDeleteGVKs.contains(s.extractGVK(obj)) { + stateChanged, ok := s.stateChangedPredicates[objTypeGVK] + if !ok { return false } - return !exist || obj.GetGeneration() != oldObj.GetGeneration() + return stateChanged.upsert(oldObj, obj) } func (s *changeTrackingUpdater) Upsert(obj client.Object) { @@ -209,14 +216,12 @@ func (s *changeTrackingUpdater) Upsert(obj client.Object) { relationshipExists := s.capturer.Exists(obj, client.ObjectKeyFromObject(obj)) - forceChanged := s.triggerStateChange(obj, client.ObjectKeyFromObject(obj)) - // FIXME(pleshakov): Check generation in all cases to minimize the number of Graph regeneration. // s.changed can be true even if the generation of the object did not change, because // capturer and triggerStateChange don't take the generation into account. // See https://github.com/nginxinc/nginx-gateway-fabric/issues/825 - s.changed = s.changed || changingUpsert || relationshipExisted || relationshipExists || forceChanged + s.changed = s.changed || changingUpsert || relationshipExisted || relationshipExists } func (s *changeTrackingUpdater) delete(objType client.Object, nsname types.NamespacedName) (changed bool) { @@ -226,13 +231,19 @@ func (s *changeTrackingUpdater) delete(objType client.Object, nsname types.Names return false } - _, exist := s.store.get(objType, nsname) - if !exist { + obj := s.store.get(objType, nsname) + if obj == nil { return false } + s.store.delete(objType, nsname) - return s.trackedUpsertDeleteGVKs.contains(objTypeGVK) + stateChanged, ok := s.stateChangedPredicates[objTypeGVK] + if !ok { + return false + } + + return stateChanged.delete(obj) } func (s *changeTrackingUpdater) Delete(objType client.Object, nsname types.NamespacedName) { @@ -240,9 +251,7 @@ func (s *changeTrackingUpdater) Delete(objType client.Object, nsname types.Names changingDelete := s.delete(objType, nsname) - forceChanged := s.triggerStateChange(objType, nsname) - - s.changed = s.changed || changingDelete || s.capturer.Exists(objType, nsname) || forceChanged + s.changed = s.changed || changingDelete || s.capturer.Exists(objType, nsname) s.capturer.Remove(objType, nsname) } diff --git a/site/content/overview/gateway-api-compatibility.md b/site/content/overview/gateway-api-compatibility.md index c748ab37fb..a7e983d839 100644 --- a/site/content/overview/gateway-api-compatibility.md +++ b/site/content/overview/gateway-api-compatibility.md @@ -66,8 +66,11 @@ Fields: - `conditions` - supported (Condition/Status/Reason): - `Accepted/True/Accepted` - `Accepted/False/InvalidParameters` + - `Accepted/False/UnsupportedVersion` - `Accepted/False/GatewayClassConflict`: Custom reason for when the GatewayClass references this controller, but a different GatewayClass name is provided to the controller via the command-line argument. + - `SupportedVersion/True/SupportedVersion` + - `SupportedVersion/False/UnsupportedVersion` ### Gateway From b340ebb52f3e56800899b054535dc76f4a74d2e1 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Thu, 30 Nov 2023 15:18:19 -0700 Subject: [PATCH 02/11] Update release process doc --- docs/developer/release-process.md | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/developer/release-process.md b/docs/developer/release-process.md index 966512eb8e..081b4a4052 100644 --- a/docs/developer/release-process.md +++ b/docs/developer/release-process.md @@ -30,8 +30,10 @@ To create a new release, follow these steps: 3. Test the main branch for release-readiness. For that, use the `edge` containers, which are built from the main branch, and the [example applications](/examples). 4. If a problem is found, prepare a fix PR, merge it into the main branch and return to the previous step. -5. Create a release branch with a name that follows the `release-X.Y` format. -6. Prepare and merge a PR into the release branch to update the repo files for the release: +5. If the supported Gateway API minor version has changed since the last release, test NGF with the previous version of the Gateway API CRDs. +6. If a problem is found, add a note to the release notes explaining that the previous version is not supported. +7. Create a release branch with a name that follows the `release-X.Y` format. +8. Prepare and merge a PR into the release branch to update the repo files for the release: 1. Update the Helm [Chart.yaml](/deploy/helm-chart/Chart.yaml): the `appVersion` to `X.Y.Z`, the icon and source URLs to point at `vX.Y.Z`, and bump the `version`. 2. Adjust the `VERSION` variable in the [Makefile](/Makefile) and the `TAG` in the @@ -52,17 +54,17 @@ To create a new release, follow these steps: draft of the full changelog. This draft can be found under the [GitHub releases](https://github.com/nginxinc/nginx-gateway-fabric/releases) after the release branch is created. Use the previous changelog entries for formatting and content guidance. -7. Create and push the release tag in the format `vX.Y.Z`. As a result, the CI/CD pipeline will: +9. Create and push the release tag in the format `vX.Y.Z`. As a result, the CI/CD pipeline will: - Build NGF container images with the release tag `X.Y.Z` and push it to the registry. - Package and publish the Helm chart to the registry. - Create a GitHub release with an autogenerated changelog and attached release artifacts. -8. Prepare and merge a PR into the main branch to update the [README](/README.md) to include the information about - the latest release and also the [changelog](/CHANGELOG.md). Also update any installation instructions to ensure - that the supported Gateway API and NGF versions are correct. Specifically, helm README and `site/content/includes/installation/install-gateway-api-resources.md`. -9. Close the issue created in Step 1. -10. Ensure that the [associated milestone](https://github.com/nginxinc/nginx-gateway-fabric/milestones) is closed. -11. Verify that published artifacts in the release can be installed properly. -12. Submit the `conformance-profile.yaml` artifact from the release to the [Gateway API repo](https://github.com/kubernetes-sigs/gateway-api/tree/main/conformance/reports). +10. Prepare and merge a PR into the main branch to update the [README](/README.md) to include the information about + the latest release and also the [changelog](/CHANGELOG.md). Also update any installation instructions to ensure + that the supported Gateway API and NGF versions are correct. Specifically, helm README and `site/content/includes/installation/install-gateway-api-resources.md`. +11. Close the issue created in Step 1. +12. Ensure that the [associated milestone](https://github.com/nginxinc/nginx-gateway-fabric/milestones) is closed. +13. Verify that published artifacts in the release can be installed properly. +14. Submit the `conformance-profile.yaml` artifact from the release to the [Gateway API repo](https://github.com/kubernetes-sigs/gateway-api/tree/main/conformance/reports). - Create a fork of the repository - Name the file `nginxinc-nginx-gateway-fabric.yaml` and set `gatewayAPIVersion` in the file to the supported version by NGF. Also update the site source if necessary (see following example). From 974f50341b2b55c66be7bc650bbb0004a1819d81 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 1 Dec 2023 13:47:03 -0700 Subject: [PATCH 03/11] ShouldNot -> ToNot; code review --- .../controller/index/endpointslice_test.go | 2 +- internal/framework/gatewayclass/validate.go | 17 ++++++------ internal/mode/provisioner/handler_test.go | 26 +++++++++---------- .../mode/static/nginx/config/execute_test.go | 2 +- .../mode/static/nginx/config/servers_test.go | 2 +- .../mode/static/nginx/file/manager_test.go | 14 +++++----- .../static/state/resolver/resolver_test.go | 2 +- internal/mode/static/state/store.go | 8 +----- 8 files changed, 33 insertions(+), 40 deletions(-) diff --git a/internal/framework/controller/index/endpointslice_test.go b/internal/framework/controller/index/endpointslice_test.go index 749f53828d..23e09dbf41 100644 --- a/internal/framework/controller/index/endpointslice_test.go +++ b/internal/framework/controller/index/endpointslice_test.go @@ -51,7 +51,7 @@ func TestServiceNameIndexFunc(t *testing.T) { func TestServiceNameIndexFuncPanics(t *testing.T) { defer func() { g := NewWithT(t) - g.Expect(recover()).ShouldNot(BeNil()) + g.Expect(recover()).ToNot(BeNil()) }() ServiceNameIndexFunc(&v1.Namespace{}) diff --git a/internal/framework/gatewayclass/validate.go b/internal/framework/gatewayclass/validate.go index a18f8ab730..4ffb766ba4 100644 --- a/internal/framework/gatewayclass/validate.go +++ b/internal/framework/gatewayclass/validate.go @@ -14,6 +14,13 @@ const ( RecommendedVersion = "v1.0.0" ) +var gatewayCRDs = map[string]apiVersion{ + "gatewayclasses.gateway.networking.k8s.io": {}, + "gateways.gateway.networking.k8s.io": {}, + "httproutes.gateway.networking.k8s.io": {}, + "referencegrants.gateway.networking.k8s.io": {}, +} + type apiVersion struct { major string minor string @@ -25,8 +32,7 @@ func ValidateCRDVersions( installedAPIVersions := getBundleVersions(crdMetadata) supportedAPIVersion := parseVersionString(RecommendedVersion) - unsupported := false - bestEffort := false + var unsupported, bestEffort bool for _, version := range installedAPIVersions { if version.major != supportedAPIVersion.major { @@ -62,13 +68,6 @@ func parseVersionString(version string) apiVersion { } func getBundleVersions(crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata) map[string]apiVersion { - gatewayCRDs := map[string]apiVersion{ - "gatewayclasses.gateway.networking.k8s.io": {}, - "gateways.gateway.networking.k8s.io": {}, - "httproutes.gateway.networking.k8s.io": {}, - "referencegrants.gateway.networking.k8s.io": {}, - } - versions := make(map[string]apiVersion) for nsname, md := range crdMetadata { diff --git a/internal/mode/provisioner/handler_test.go b/internal/mode/provisioner/handler_test.go index 9a6de3b39c..a674b53387 100644 --- a/internal/mode/provisioner/handler_test.go +++ b/internal/mode/provisioner/handler_test.go @@ -83,7 +83,7 @@ var _ = Describe("handler", func() { } err := k8sclient.Create(context.Background(), crd) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) gc = &gatewayv1.GatewayClass{ ObjectMeta: metav1.ObjectMeta{ @@ -108,7 +108,7 @@ var _ = Describe("handler", func() { // Add GatewayClass to the cluster err := k8sclient.Create(context.Background(), gc) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) // UpsertGatewayClass and CRD @@ -127,7 +127,7 @@ var _ = Describe("handler", func() { clusterGc := &gatewayv1.GatewayClass{} err = k8sclient.Get(context.Background(), client.ObjectKeyFromObject(gc), clusterGc) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) expectedConditions := []metav1.Condition{ { @@ -168,7 +168,7 @@ var _ = Describe("handler", func() { dep := &v1.Deployment{} err := k8sclient.Get(context.Background(), depNsName, dep) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(dep.ObjectMeta.Namespace).To(Equal("nginx-gateway")) Expect(dep.ObjectMeta.Name).To(Equal(depNsName.Name)) @@ -185,7 +185,7 @@ var _ = Describe("handler", func() { updatedCRD.Annotations[gatewayclass.BundleVersionAnnotation] = version err := k8sclient.Update(context.Background(), updatedCRD) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) batch := []interface{}{ &events.UpsertEvent{ @@ -198,7 +198,7 @@ var _ = Describe("handler", func() { updatedGC := &gatewayv1.GatewayClass{} err = k8sclient.Get(context.Background(), client.ObjectKeyFromObject(gc), updatedGC) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) var expConds []metav1.Condition if !accepted { @@ -320,7 +320,7 @@ var _ = Describe("handler", func() { err := k8sclient.List(context.Background(), deps) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(deps.Items).To(HaveLen(1)) Expect(deps.Items[0].ObjectMeta.Name).To(Equal("nginx-gateway-2")) }) @@ -341,7 +341,7 @@ var _ = Describe("handler", func() { err := k8sclient.List(context.Background(), deps) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(deps.Items).To(HaveLen(0)) }) }) @@ -369,7 +369,7 @@ var _ = Describe("handler", func() { deps := &v1.DeploymentList{} err := k8sclient.List(context.Background(), deps) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(deps.Items).To(HaveLen(0)) }) }) @@ -386,7 +386,7 @@ var _ = Describe("handler", func() { } err := k8sclient.Create(context.Background(), newGC) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) batch := []interface{}{ &events.UpsertEvent{ @@ -401,7 +401,7 @@ var _ = Describe("handler", func() { unknownGC := &gatewayv1.GatewayClass{} err = k8sclient.Get(context.Background(), client.ObjectKeyFromObject(newGC), unknownGC) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) expectedConditions := []metav1.Condition{ { @@ -497,7 +497,7 @@ var _ = Describe("handler", func() { } err := k8sclient.Create(context.Background(), dep) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) itShouldPanicWhenUpsertingGateway(gwNsName) }) @@ -518,7 +518,7 @@ var _ = Describe("handler", func() { } err := k8sclient.Delete(context.Background(), dep) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) batch := []interface{}{ &events.DeleteEvent{ diff --git a/internal/mode/static/nginx/config/execute_test.go b/internal/mode/static/nginx/config/execute_test.go index 4456c7370a..61538973c3 100644 --- a/internal/mode/static/nginx/config/execute_test.go +++ b/internal/mode/static/nginx/config/execute_test.go @@ -20,7 +20,7 @@ func TestExecute(t *testing.T) { func TestExecutePanics(t *testing.T) { defer func() { g := NewWithT(t) - g.Expect(recover()).ShouldNot(BeNil()) + g.Expect(recover()).ToNot(BeNil()) }() _ = execute(serversTemplate, "not-correct-data") diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index ef9477f739..3cb1a1c5ca 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -440,7 +440,7 @@ func TestCreateServers(t *testing.T) { expectedMatchString := func(m []httpMatch) string { g := NewWithT(t) b, err := json.Marshal(m) - g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(err).ToNot(HaveOccurred()) return string(b) } diff --git a/internal/mode/static/nginx/file/manager_test.go b/internal/mode/static/nginx/file/manager_test.go index 07f0478ae9..12f51a0f4f 100644 --- a/internal/mode/static/nginx/file/manager_test.go +++ b/internal/mode/static/nginx/file/manager_test.go @@ -23,7 +23,7 @@ var _ = Describe("EventHandler", func() { ensureFiles := func(files []file.File) { entries, err := os.ReadDir(tmpDir) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(entries).Should(HaveLen(len(files))) entriesMap := make(map[string]os.DirEntry) @@ -36,7 +36,7 @@ var _ = Describe("EventHandler", func() { Expect(ok).Should(BeTrue()) info, err := os.Stat(f.Path) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(info.IsDir()).To(BeFalse()) @@ -47,7 +47,7 @@ var _ = Describe("EventHandler", func() { } bytes, err := os.ReadFile(f.Path) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(bytes).To(Equal(f.Content)) } } @@ -89,7 +89,7 @@ var _ = Describe("EventHandler", func() { files := []file.File{regular1, regular2, secret} err := mgr.ReplaceFiles(files) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) ensureFiles(files) }) @@ -102,7 +102,7 @@ var _ = Describe("EventHandler", func() { } err := mgr.ReplaceFiles(files) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) ensureFiles(files) ensureNotExist(regular1) @@ -110,7 +110,7 @@ var _ = Describe("EventHandler", func() { It("should remove all files", func() { err := mgr.ReplaceFiles(nil) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) ensureNotExist(regular2, regular3, secret) }) @@ -181,7 +181,7 @@ var _ = Describe("EventHandler", func() { // to kick off removing, we need to successfully write files beforehand if fakeOSMgr.RemoveStub != nil { err := mgr.ReplaceFiles(files) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) } err := mgr.ReplaceFiles(files) diff --git a/internal/mode/static/state/resolver/resolver_test.go b/internal/mode/static/state/resolver/resolver_test.go index baedda7f96..411ca190e9 100644 --- a/internal/mode/static/state/resolver/resolver_test.go +++ b/internal/mode/static/state/resolver/resolver_test.go @@ -126,7 +126,7 @@ func TestGetServicePort(t *testing.T) { // ports exist for _, p := range []int32{80, 81, 82} { port, err := getServicePort(svc, p) - g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(port.Port).To(Equal(p)) } diff --git a/internal/mode/static/state/store.go b/internal/mode/static/state/store.go index 278a8b6654..41378e9f21 100644 --- a/internal/mode/static/state/store.go +++ b/internal/mode/static/state/store.go @@ -38,15 +38,9 @@ func newObjectStoreMapAdapter[T client.Object](objects map[types.NamespacedName] } func (m *objectStoreMapAdapter[T]) get(nsname types.NamespacedName) client.Object { - var zeroVal client.Object - obj, exist := m.objects[nsname] if !exist { - // we return zeroVal here instead of obj because we need to be able to compare the return value to nil. - // obj has a value of nil, but its type is a pointer to the concrete type (e.g. *v1.Pod). - // This means that obj != nil. - // However, the value **and** type of zeroVal are nil, which means zeroVal == nil. - return zeroVal + return nil } return obj From 1b34ab10360eb009350adb2d81fca019167ad09c Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 1 Dec 2023 14:23:43 -0700 Subject: [PATCH 04/11] Add panic for GVK --- internal/framework/controller/register.go | 3 + .../framework/controller/register_test.go | 62 ++++++++++++++----- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/internal/framework/controller/register.go b/internal/framework/controller/register.go index 085b1d148c..fbb5a7e712 100644 --- a/internal/framework/controller/register.go +++ b/internal/framework/controller/register.go @@ -107,6 +107,9 @@ func Register( var forOpts []ctlrBuilder.ForOption if cfg.onlyMetadata { + if objectType.GetObjectKind().GroupVersionKind().Empty() { + panic("the object must have its GVK set") + } forOpts = append(forOpts, ctlrBuilder.OnlyMetadata) } diff --git a/internal/framework/controller/register_test.go b/internal/framework/controller/register_test.go index 3a591b9e6d..ac4e4c61c9 100644 --- a/internal/framework/controller/register_test.go +++ b/internal/framework/controller/register_test.go @@ -10,8 +10,10 @@ import ( "github.com/onsi/gomega/gcustom" gtypes "github.com/onsi/gomega/types" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -50,14 +52,24 @@ func TestRegister(t *testing.T) { testError := errors.New("test error") + objectTypeWithGVK := &v1.HTTPRoute{} + objectTypeWithGVK.SetGroupVersionKind( + schema.GroupVersionKind{Group: v1.GroupName, Version: "v1", Kind: "HTTPRoute"}, + ) + + objectTypeNoGVK := &v1.HTTPRoute{} + tests := []struct { fakes fakes + objectType client.Object expectedErr error msg string expectedMgrAddCallCount int + expectPanic bool }{ { fakes: getDefaultFakes(), + objectType: objectTypeWithGVK, expectedErr: nil, expectedMgrAddCallCount: 1, msg: "normal case", @@ -67,6 +79,7 @@ func TestRegister(t *testing.T) { f.indexer.IndexFieldReturns(testError) return f }(getDefaultFakes()), + objectType: objectTypeWithGVK, expectedErr: testError, expectedMgrAddCallCount: 0, msg: "preparing index fails", @@ -76,13 +89,20 @@ func TestRegister(t *testing.T) { f.mgr.AddReturns(testError) return f }(getDefaultFakes()), + objectType: objectTypeWithGVK, expectedErr: testError, expectedMgrAddCallCount: 1, msg: "building controller fails", }, + { + fakes: getDefaultFakes(), + objectType: objectTypeNoGVK, + expectPanic: true, + expectedMgrAddCallCount: 0, + msg: "adding OnlyMetadata option panics", + }, } - objectType := &v1.HTTPRoute{} nsNameFilter := func(nsname types.NamespacedName) (bool, string) { return true, "" } @@ -104,28 +124,36 @@ func TestRegister(t *testing.T) { newReconciler := func(c controller.ReconcilerConfig) *controller.Reconciler { g.Expect(c.Getter).To(BeIdenticalTo(test.fakes.mgr.GetClient())) - g.Expect(c.ObjectType).To(BeIdenticalTo(objectType)) + g.Expect(c.ObjectType).To(BeIdenticalTo(test.objectType)) g.Expect(c.EventCh).To(BeIdenticalTo(eventCh)) g.Expect(c.NamespacedNameFilter).Should(beSameFunctionPointer(nsNameFilter)) return controller.NewReconciler(c) } - err := controller.Register( - context.Background(), - objectType, - test.fakes.mgr, - eventCh, - controller.WithNamespacedNameFilter(nsNameFilter), - controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}), - controller.WithFieldIndices(fieldIndexes), - controller.WithNewReconciler(newReconciler), - ) - - if test.expectedErr == nil { - g.Expect(err).To(BeNil()) + register := func() error { + return controller.Register( + context.Background(), + test.objectType, + test.fakes.mgr, + eventCh, + controller.WithNamespacedNameFilter(nsNameFilter), + controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}), + controller.WithFieldIndices(fieldIndexes), + controller.WithNewReconciler(newReconciler), + controller.WithOnlyMetadata(), + ) + } + + if test.expectPanic { + g.Expect(func() { _ = register() }).To(Panic()) } else { - g.Expect(err).To(MatchError(test.expectedErr)) + err := register() + if test.expectedErr == nil { + g.Expect(err).To(BeNil()) + } else { + g.Expect(err).To(MatchError(test.expectedErr)) + } } indexCallCount := test.fakes.indexer.IndexFieldCallCount() @@ -134,7 +162,7 @@ func TestRegister(t *testing.T) { _, objType, field, indexFunc := test.fakes.indexer.IndexFieldArgsForCall(0) - g.Expect(objType).To(BeIdenticalTo(objectType)) + g.Expect(objType).To(BeIdenticalTo(test.objectType)) g.Expect(field).To(BeIdenticalTo(index.KubernetesServiceNameIndexField)) expectedIndexFunc := fieldIndexes[index.KubernetesServiceNameIndexField] From 7a544bad7f629d789f7202d0ce58b82e181ba5ba Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 1 Dec 2023 14:31:11 -0700 Subject: [PATCH 05/11] Add comments and change RecommendedVersion -> SupportedVersion --- internal/framework/gatewayclass/validate.go | 10 ++++++---- internal/framework/gatewayclass/validate_test.go | 14 +++++++------- internal/mode/provisioner/handler_test.go | 8 ++++---- .../mode/static/state/change_processor_test.go | 4 ++-- .../mode/static/state/graph/gatewayclass_test.go | 4 ++-- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/internal/framework/gatewayclass/validate.go b/internal/framework/gatewayclass/validate.go index 4ffb766ba4..ff40525608 100644 --- a/internal/framework/gatewayclass/validate.go +++ b/internal/framework/gatewayclass/validate.go @@ -10,8 +10,10 @@ import ( ) const ( + // BundleVersionAnnotation is the annotation on Gateway API CRDs that contains the installed version. BundleVersionAnnotation = "gateway.networking.k8s.io/bundle-version" - RecommendedVersion = "v1.0.0" + // SupportedVersion is the supported version of the Gateway API CRDs. + SupportedVersion = "v1.0.0" ) var gatewayCRDs = map[string]apiVersion{ @@ -30,7 +32,7 @@ func ValidateCRDVersions( crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata, ) ([]conditions.Condition, bool) { installedAPIVersions := getBundleVersions(crdMetadata) - supportedAPIVersion := parseVersionString(RecommendedVersion) + supportedAPIVersion := parseVersionString(SupportedVersion) var unsupported, bestEffort bool @@ -43,11 +45,11 @@ func ValidateCRDVersions( } if unsupported { - return conditions.NewGatewayClassUnsupportedVersion(RecommendedVersion), false + return conditions.NewGatewayClassUnsupportedVersion(SupportedVersion), false } if bestEffort { - return conditions.NewGatewayClassSupportedVersionBestEffort(RecommendedVersion), true + return conditions.NewGatewayClassSupportedVersionBestEffort(SupportedVersion), true } return nil, true diff --git a/internal/framework/gatewayclass/validate_test.go b/internal/framework/gatewayclass/validate_test.go index ecc55376e8..8359365477 100644 --- a/internal/framework/gatewayclass/validate_test.go +++ b/internal/framework/gatewayclass/validate_test.go @@ -23,8 +23,8 @@ func TestValidateCRDVersions(t *testing.T) { } } - // Adding patch version to RecommendedVersion to try and avoid having to update these tests with every release. - fields := strings.Split(gatewayclass.RecommendedVersion, ".") + // Adding patch version to SupportedVersion to try and avoid having to update these tests with every release. + fields := strings.Split(gatewayclass.SupportedVersion, ".") fields[2] = "99" validVersionWithPatch := createCRDMetadata(strings.Join(fields, ".")) @@ -67,7 +67,7 @@ func TestValidateCRDVersions(t *testing.T) { {Name: "referencegrants.gateway.networking.k8s.io"}: bestEffortVersion, }, valid: true, - expConds: conditions.NewGatewayClassSupportedVersionBestEffort(gatewayclass.RecommendedVersion), + expConds: conditions.NewGatewayClassSupportedVersionBestEffort(gatewayclass.SupportedVersion), }, { name: "valid; mix of supported and best effort versions", @@ -78,7 +78,7 @@ func TestValidateCRDVersions(t *testing.T) { {Name: "referencegrants.gateway.networking.k8s.io"}: validVersionWithPatch, }, valid: true, - expConds: conditions.NewGatewayClassSupportedVersionBestEffort(gatewayclass.RecommendedVersion), + expConds: conditions.NewGatewayClassSupportedVersionBestEffort(gatewayclass.SupportedVersion), }, { name: "invalid; all unsupported versions", @@ -89,7 +89,7 @@ func TestValidateCRDVersions(t *testing.T) { {Name: "referencegrants.gateway.networking.k8s.io"}: unsupportedVersion, }, valid: false, - expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.SupportedVersion), }, { name: "invalid; mix unsupported and best effort versions", @@ -100,7 +100,7 @@ func TestValidateCRDVersions(t *testing.T) { {Name: "referencegrants.gateway.networking.k8s.io"}: bestEffortVersion, }, valid: false, - expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.SupportedVersion), }, { name: "invalid; bad version string", @@ -108,7 +108,7 @@ func TestValidateCRDVersions(t *testing.T) { {Name: "gatewayclasses.gateway.networking.k8s.io"}: createCRDMetadata("v"), }, valid: false, - expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + expConds: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.SupportedVersion), }, } diff --git a/internal/mode/provisioner/handler_test.go b/internal/mode/provisioner/handler_test.go index a674b53387..0faa894a9a 100644 --- a/internal/mode/provisioner/handler_test.go +++ b/internal/mode/provisioner/handler_test.go @@ -77,7 +77,7 @@ var _ = Describe("handler", func() { ObjectMeta: metav1.ObjectMeta{ Name: "gatewayclasses.gateway.networking.k8s.io", Annotations: map[string]string{ - gatewayclass.BundleVersionAnnotation: gatewayclass.RecommendedVersion, + gatewayclass.BundleVersionAnnotation: gatewayclass.SupportedVersion, }, }, } @@ -210,7 +210,7 @@ var _ = Describe("handler", func() { LastTransitionTime: fakeClockTime, Reason: string(gatewayv1.GatewayClassReasonUnsupportedVersion), Message: fmt.Sprintf("Gateway API CRD versions are not supported. "+ - "Please install version %s", gatewayclass.RecommendedVersion), + "Please install version %s", gatewayclass.SupportedVersion), }, { Type: string(gatewayv1.GatewayClassReasonSupportedVersion), @@ -219,7 +219,7 @@ var _ = Describe("handler", func() { LastTransitionTime: fakeClockTime, Reason: string(gatewayv1.GatewayClassReasonUnsupportedVersion), Message: fmt.Sprintf("Gateway API CRD versions are not supported. "+ - "Please install version %s", gatewayclass.RecommendedVersion), + "Please install version %s", gatewayclass.SupportedVersion), }, } } else { @@ -239,7 +239,7 @@ var _ = Describe("handler", func() { LastTransitionTime: fakeClockTime, Reason: string(gatewayv1.GatewayClassReasonUnsupportedVersion), Message: fmt.Sprintf("Gateway API CRD versions are not recommended. "+ - "Recommended version is %s", gatewayclass.RecommendedVersion), + "Recommended version is %s", gatewayclass.SupportedVersion), }, } } diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index f04234de13..9456aa6084 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -388,7 +388,7 @@ var _ = Describe("ChangeProcessor", func() { ObjectMeta: metav1.ObjectMeta{ Name: "gatewayclasses.gateway.networking.k8s.io", Annotations: map[string]string{ - gatewayclass.BundleVersionAnnotation: gatewayclass.RecommendedVersion, + gatewayclass.BundleVersionAnnotation: gatewayclass.SupportedVersion, }, }, } @@ -656,7 +656,7 @@ var _ = Describe("ChangeProcessor", func() { } expGraph.GatewayClass.Conditions = conditions.NewGatewayClassSupportedVersionBestEffort( - gatewayclass.RecommendedVersion, + gatewayclass.SupportedVersion, ) changed, graphCfg := processor.Process() diff --git a/internal/mode/static/state/graph/gatewayclass_test.go b/internal/mode/static/state/graph/gatewayclass_test.go index 99cfed6bd5..11c4feb321 100644 --- a/internal/mode/static/state/graph/gatewayclass_test.go +++ b/internal/mode/static/state/graph/gatewayclass_test.go @@ -132,7 +132,7 @@ func TestBuildGatewayClass(t *testing.T) { {Name: "gateways.gateway.networking.k8s.io"}: { ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - gatewayclass.BundleVersionAnnotation: gatewayclass.RecommendedVersion, + gatewayclass.BundleVersionAnnotation: gatewayclass.SupportedVersion, }, }, }, @@ -188,7 +188,7 @@ func TestBuildGatewayClass(t *testing.T) { expected: &GatewayClass{ Source: validGC, Valid: false, - Conditions: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.RecommendedVersion), + Conditions: conditions.NewGatewayClassUnsupportedVersion(gatewayclass.SupportedVersion), }, name: "invalid gatewayclass; unsupported version", }, From 512876344ef974e04255f6ff3a72582471990a89 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 1 Dec 2023 14:32:06 -0700 Subject: [PATCH 06/11] Name return args --- internal/framework/gatewayclass/validate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/framework/gatewayclass/validate.go b/internal/framework/gatewayclass/validate.go index ff40525608..d0d7d083f7 100644 --- a/internal/framework/gatewayclass/validate.go +++ b/internal/framework/gatewayclass/validate.go @@ -30,7 +30,7 @@ type apiVersion struct { func ValidateCRDVersions( crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata, -) ([]conditions.Condition, bool) { +) (conds []conditions.Condition, valid bool) { installedAPIVersions := getBundleVersions(crdMetadata) supportedAPIVersion := parseVersionString(SupportedVersion) From fc6c68c6675487c9bc482b7117fad56a798516ae Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 1 Dec 2023 14:36:37 -0700 Subject: [PATCH 07/11] Change from map to list --- internal/framework/gatewayclass/validate.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/framework/gatewayclass/validate.go b/internal/framework/gatewayclass/validate.go index d0d7d083f7..266e6ca5bc 100644 --- a/internal/framework/gatewayclass/validate.go +++ b/internal/framework/gatewayclass/validate.go @@ -69,13 +69,13 @@ func parseVersionString(version string) apiVersion { } } -func getBundleVersions(crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata) map[string]apiVersion { - versions := make(map[string]apiVersion) +func getBundleVersions(crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata) []apiVersion { + versions := make([]apiVersion, 0, len(gatewayCRDs)) for nsname, md := range crdMetadata { if _, ok := gatewayCRDs[nsname.Name]; ok { bundleVersion := md.Annotations[BundleVersionAnnotation] - versions[bundleVersion] = parseVersionString(bundleVersion) + versions = append(versions, parseVersionString(bundleVersion)) } } From 75499dbcf5e0660f5efefa3d7b128378ef958563 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 1 Dec 2023 14:39:59 -0700 Subject: [PATCH 08/11] Remove trackUpsertDelete comment --- internal/mode/static/state/store.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/mode/static/state/store.go b/internal/mode/static/state/store.go index 41378e9f21..fb383d6acb 100644 --- a/internal/mode/static/state/store.go +++ b/internal/mode/static/state/store.go @@ -122,11 +122,7 @@ type changeTrackingUpdaterObjectTypeCfg struct { // It only works with objects with the GVKs registered in changeTrackingUpdaterObjectTypeCfg. Otherwise, it panics. // // A change is tracked when: -// - An object with a GVK with a non-nil store and trackUpsertDelete set to 'true' is upserted or deleted, provided -// that its generation changed. -// - An object is upserted or deleted, and it is related to another object, based on the decision by -// the relationship capturer. -// - An object is upserted or deleted and the stateChangedPredicate for that object returns true. +// - An object with a GVK with a non-nil store and the stateChangedPredicate for that object returns true. type changeTrackingUpdater struct { store *multiObjectStore capturer relationship.Capturer From b9bc464c9984fd30921e5d1220580b116d9861ee Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Mon, 4 Dec 2023 10:27:17 -0700 Subject: [PATCH 09/11] Style suggestions --- docs/developer/release-process.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/developer/release-process.md b/docs/developer/release-process.md index 081b4a4052..92c1df5f28 100644 --- a/docs/developer/release-process.md +++ b/docs/developer/release-process.md @@ -30,9 +30,9 @@ To create a new release, follow these steps: 3. Test the main branch for release-readiness. For that, use the `edge` containers, which are built from the main branch, and the [example applications](/examples). 4. If a problem is found, prepare a fix PR, merge it into the main branch and return to the previous step. -5. If the supported Gateway API minor version has changed since the last release, test NGF with the previous version of the Gateway API CRDs. -6. If a problem is found, add a note to the release notes explaining that the previous version is not supported. -7. Create a release branch with a name that follows the `release-X.Y` format. +5. If the supported Gateway API minor version has changed since the last release, test NGINX Gateway Fabric with the previous version of the Gateway API CRDs. +6. If a compatibility issue is found, add a note to the release notes explaining that the previous version is not supported. +7. Create a release branch following the `release-X.Y` naming convention. 8. Prepare and merge a PR into the release branch to update the repo files for the release: 1. Update the Helm [Chart.yaml](/deploy/helm-chart/Chart.yaml): the `appVersion` to `X.Y.Z`, the icon and source URLs to point at `vX.Y.Z`, and bump the `version`. From 32bc60d6c28157b3520dd25ff4c4b54a1f0cd19c Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Mon, 4 Dec 2023 13:33:09 -0700 Subject: [PATCH 10/11] Add back in comment about relationship capturer --- internal/mode/static/state/store.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/mode/static/state/store.go b/internal/mode/static/state/store.go index fb383d6acb..351717c244 100644 --- a/internal/mode/static/state/store.go +++ b/internal/mode/static/state/store.go @@ -123,6 +123,8 @@ type changeTrackingUpdaterObjectTypeCfg struct { // // A change is tracked when: // - An object with a GVK with a non-nil store and the stateChangedPredicate for that object returns true. +// - An object is upserted or deleted, and it is related to another object, +// based on the decision by the relationship capturer. type changeTrackingUpdater struct { store *multiObjectStore capturer relationship.Capturer From 8b458266d7bf8cb66bc1481947de52b340bfd3e8 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Mon, 4 Dec 2023 13:41:51 -0700 Subject: [PATCH 11/11] Change predicate name --- .../mode/static/state/change_processor.go | 2 +- .../mode/static/state/changed_predicate.go | 29 +++++-------------- .../static/state/changed_predicate_test.go | 4 +-- 3 files changed, 11 insertions(+), 24 deletions(-) diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 670435a84e..d2ba8eed35 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -161,7 +161,7 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: extractGVK(&apiv1.Secret{}), store: newObjectStoreMapAdapter(clusterStore.Secrets), - predicate: newStateChangedPredicateFuncs(isReferenced), + predicate: funcPredicate{stateChanged: isReferenced}, }, { gvk: extractGVK(&apiext.CustomResourceDefinition{}), diff --git a/internal/mode/static/state/changed_predicate.go b/internal/mode/static/state/changed_predicate.go index d52fbdc941..6b253d968e 100644 --- a/internal/mode/static/state/changed_predicate.go +++ b/internal/mode/static/state/changed_predicate.go @@ -10,31 +10,18 @@ type stateChangedPredicate interface { delete(object client.Object) bool } -// funcs is a function that implements stateChangedPredicate. -type funcs struct { - upsertStateChangeFunc func(oldObject, newObject client.Object) bool - deleteStateChangeFunc func(object client.Object) bool +// funcPredicate applies the stateChanged function on upsert and delete. On upsert, the newObject is passed. +// Implements stateChangedPredicate. +type funcPredicate struct { + stateChanged func(object client.Object) bool } -func (f funcs) upsert(oldObject, newObject client.Object) bool { - return f.upsertStateChangeFunc(oldObject, newObject) +func (f funcPredicate) upsert(_, newObject client.Object) bool { + return f.stateChanged(newObject) } -func (f funcs) delete(object client.Object) bool { - return f.deleteStateChangeFunc(object) -} - -// newStateChangedPredicateFuncs returns a predicate funcs that applies the given function on calls to upsert and -// delete. -func newStateChangedPredicateFuncs(stateChangedFunc func(object client.Object) bool) funcs { - return funcs{ - upsertStateChangeFunc: func(oldObject, newObject client.Object) bool { - return stateChangedFunc(newObject) - }, - deleteStateChangeFunc: func(object client.Object) bool { - return stateChangedFunc(object) - }, - } +func (f funcPredicate) delete(object client.Object) bool { + return f.stateChanged(object) } // generationChangedPredicate implements stateChangedPredicate based on the generation of the object. diff --git a/internal/mode/static/state/changed_predicate_test.go b/internal/mode/static/state/changed_predicate_test.go index e161f9ea30..0e5142a9ff 100644 --- a/internal/mode/static/state/changed_predicate_test.go +++ b/internal/mode/static/state/changed_predicate_test.go @@ -9,10 +9,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func TestFuncsPredicate(t *testing.T) { +func TestFuncPredicate(t *testing.T) { alwaysTrueFunc := func(object client.Object) bool { return true } - p := newStateChangedPredicateFuncs(alwaysTrueFunc) + p := funcPredicate{stateChanged: alwaysTrueFunc} g := NewWithT(t)