From 4224cfc9fef8ca9fcccd7d2e78641a330e3347b8 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 27 Jan 2022 10:05:45 -0800 Subject: [PATCH 1/9] Process HTTPRoutes in Configuration - Add new type -- Configuration, which is an internal representation of the Gateway configuration - Implement methods for Upserting and Deleting HTTPRoutes - Add unit tests - Add github.com/onsi/ginkgo/v2 (the new tests rely on the v2 features) --- go.mod | 7 +- go.sum | 11 +- internal/state/clock.go | 37 ++ internal/state/configuration.go | 451 ++++++++++++++ internal/state/configuration_test.go | 860 +++++++++++++++++++++++++++ internal/state/state_suit_test.go | 13 + 6 files changed, 1369 insertions(+), 10 deletions(-) create mode 100644 internal/state/clock.go create mode 100644 internal/state/configuration.go create mode 100644 internal/state/configuration_test.go create mode 100644 internal/state/state_suit_test.go diff --git a/go.mod b/go.mod index 50f83bc1dc..38c215ba1a 100644 --- a/go.mod +++ b/go.mod @@ -4,8 +4,10 @@ go 1.17 require ( github.com/go-logr/logr v1.2.0 + github.com/google/go-cmp v0.5.6 github.com/onsi/ginkgo v1.16.5 - github.com/onsi/gomega v1.16.0 + github.com/onsi/ginkgo/v2 v2.1.1 + github.com/onsi/gomega v1.17.0 github.com/spf13/pflag v1.0.5 golang.org/x/net v0.0.0-20210813160813-60bc85c4be6d k8s.io/apimachinery v0.23.0-alpha.3 @@ -34,11 +36,9 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.2 // indirect - github.com/google/go-cmp v0.5.6 // indirect github.com/google/gofuzz v1.1.0 // indirect github.com/google/uuid v1.1.2 // indirect github.com/googleapis/gnostic v0.5.5 // indirect - github.com/hashicorp/golang-lru v0.5.4 // indirect github.com/imdario/mergo v0.3.12 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect github.com/josharian/intern v1.0.0 // indirect @@ -56,7 +56,6 @@ require ( github.com/prometheus/common v0.28.0 // indirect github.com/prometheus/procfs v0.6.0 // indirect github.com/spf13/cobra v1.2.1 // indirect - github.com/spf13/pflag v1.0.5 // indirect go.uber.org/atomic v1.9.0 // indirect go.uber.org/multierr v1.7.0 // indirect go.uber.org/zap v1.19.1 // indirect diff --git a/go.sum b/go.sum index 8df48e3c56..5d40650721 100644 --- a/go.sum +++ b/go.sum @@ -247,7 +247,6 @@ github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.1.0 h1:Hsa8mG0dQ46ij8Sl2AYJDUv1oA9/d6Vk+3LG99Oe02g= github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= -github.com/google/martian v2.1.0+incompatible h1:/CP5g8u/VJHijgedC/Legn3BAbAaWPgecwXBIDzw5no= github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs= github.com/google/martian/v3 v3.0.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0= github.com/google/martian/v3 v3.1.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0= @@ -262,6 +261,7 @@ github.com/google/pprof v0.0.0-20201023163331-3e6fc7fc9c4c/go.mod h1:kpwsk12EmLe github.com/google/pprof v0.0.0-20201203190320-1bf35d6f28c2/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/pprof v0.0.0-20210122040257-d980be63207e/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/pprof v0.0.0-20210226084205-cbba55b83ad5/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= +github.com/google/pprof v0.0.0-20210407192527-94a9f03dee38/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= @@ -288,7 +288,6 @@ github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBt github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= -github.com/hashicorp/go-immutable-radix v1.0.0 h1:AKDB1HM5PWEA7i4nhcpwOrO2byshxBjXVn/J/3+z5/0= github.com/hashicorp/go-immutable-radix v1.0.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= @@ -296,12 +295,10 @@ github.com/hashicorp/go-rootcerts v1.0.0/go.mod h1:K6zTfqpRlCUIjkwsN4Z+hiSfzSTQa github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4= github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= -github.com/hashicorp/go-uuid v1.0.1 h1:fv1ep09latC32wFoVwnqcnKJGnMSdBanPczbHAYm1BE= github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go.net v0.0.1/go.mod h1:hjKkEWcCURg++eb33jQU7oqQcI9XDCnUzHA0oac0k90= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= -github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc= github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO+LraFDTW64= @@ -407,13 +404,16 @@ github.com/onsi/ginkgo v1.14.0/go.mod h1:iSB4RoI2tjJc9BBv4NKIKWKya62Rps+oPG/Lv9k github.com/onsi/ginkgo v1.16.4/go.mod h1:dX+/inL/fNMqNlz0e9LfyB9TswhZpCVdJM/Z6Vvnwo0= github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE= github.com/onsi/ginkgo v1.16.5/go.mod h1:+E8gABHa3K6zRBolWtd+ROzc/U5bkGt0FwiG042wbpU= +github.com/onsi/ginkgo/v2 v2.1.1 h1:LCnPB85AvFNr91s0B2aDzEiiIg6MUwLYbryC1NSlWi8= +github.com/onsi/ginkgo/v2 v2.1.1/go.mod h1:vw5CSIxN1JObi/U8gcbwft7ZxR2dgaR70JSE3/PpL4c= github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA= github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY= github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo= github.com/onsi/gomega v1.14.0/go.mod h1:cIuvLEne0aoVhAgh/O6ac0Op8WWw9H6eYCriF+tEHG0= -github.com/onsi/gomega v1.16.0 h1:6gjqkI8iiRHMvdccRJM8rVKjCWk6ZIm6FTm3ddIe4/c= github.com/onsi/gomega v1.16.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAlGdZY= +github.com/onsi/gomega v1.17.0 h1:9Luw4uT5HTjHTN8+aNcSThgH1vdXnmdJ8xIfZ4wyTRE= +github.com/onsi/gomega v1.17.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAlGdZY= github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic= @@ -1027,7 +1027,6 @@ k8s.io/client-go v0.23.0-alpha.3/go.mod h1:33Jk0VCuzz21Piw21Y0GeAN9OjifDFTTO2j/2 k8s.io/code-generator v0.21.3/go.mod h1:K3y0Bv9Cz2cOW2vXUrNZlFbflhuPvuadW6JdnN6gGKo= k8s.io/code-generator v0.22.0/go.mod h1:eV77Y09IopzeXOJzndrDyCI88UBok2h6WxAlBwpxa+o= k8s.io/code-generator v0.22.2/go.mod h1:eV77Y09IopzeXOJzndrDyCI88UBok2h6WxAlBwpxa+o= -k8s.io/code-generator v0.22.3 h1:24xLuKySzFl1XupMarNBkpt10q0N+73R9dF7wzJO/hE= k8s.io/code-generator v0.22.3/go.mod h1:eV77Y09IopzeXOJzndrDyCI88UBok2h6WxAlBwpxa+o= k8s.io/code-generator v0.22.4 h1:h7lBa5IuEUC4OQ45q/gIip/a0iQcML2iwrRmXksau30= k8s.io/code-generator v0.22.4/go.mod h1:qjYl54pQ/emhkT0UxbufbREYJMWsHNNV/jSVwhYZQGw= diff --git a/internal/state/clock.go b/internal/state/clock.go new file mode 100644 index 0000000000..58fb5bb596 --- /dev/null +++ b/internal/state/clock.go @@ -0,0 +1,37 @@ +package state + +import "time" + +// Clock returns the current local time. +type Clock interface { + Now() time.Time +} + +// Real clock returns the current local time. +type RealClock struct { +} + +// NewRealClock creates a new RealClock. +func NewRealClock() *RealClock { + return &RealClock{} +} + +// Now returns the current local time. +func (c *RealClock) Now() time.Time { + return time.Now() +} + +// FakeClock allows you to control the returned time. +type FakeClock struct { + time time.Time +} + +// NewFakeClock creates a FakeClock. The clock will always return the specified time. +func NewFakeClock(time time.Time) *FakeClock { + return &FakeClock{time: time} +} + +// Now is a fake implementation of Now(). +func (c FakeClock) Now() time.Time { + return c.time +} diff --git a/internal/state/configuration.go b/internal/state/configuration.go new file mode 100644 index 0000000000..59dee965bc --- /dev/null +++ b/internal/state/configuration.go @@ -0,0 +1,451 @@ +package state + +import ( + "fmt" + "reflect" + "sort" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +// httpListener defines an HTTP Listener. +type httpListener struct { + // hosts include all Hosts that belong to the listener. + hosts map[string]*Host + // httpRoutes include all HTTPRoute resources that belong to the listener. + httpRoutes map[string]*v1alpha2.HTTPRoute +} + +// Host is the primary configuration unit of the internal representation. +// It corresponds to an NGINX server block with server_name Value; +// See https://nginx.org/en/docs/http/ngx_http_core_module.html#server +type Host struct { + // Value is the host value (hostname). + Value string + // PathRouteGroups include all PathRouteGroups that belong to the Host. + // We use a slice rather than a map to control the order of the routes. + PathRouteGroups []*PathRouteGroup +} + +// PathRouteGroup represents a collection of Routes grouped by a path. +// Among those Routes, there will be routing rules with additional matching criteria. For example, matching of headers. +// The reason we group Routes by Path is how NGINX processes requests: its primary routing rule mechanism is a location block. +// See https://nginx.org/en/docs/http/ngx_http_core_module.html#location +type PathRouteGroup struct { + // Path is the path (URI). + Path string + // Routes include all Routes for that path. + // Routes are sorted based on the creation timestamp and namespace/name of the Route source (HTTPRoute). This way + // the ordering resolves the conflicts among any conflicting rules. + // Sorting is stable so that the Routes retain the order of appearance of the corresponding matches in the corresponding + // HTTPRoute resources. + // The first "fired" Route will win in the NGINX configuration. + Routes []Route +} + +// Route represents a Route, which corresponds to a Match in the HTTPRouteRule. If a rule doesn't define any matches, +// it is assumed that the rule is for "/" path. +type Route struct { + // MatchIdx is the index of the rule in the Rule.Matches or -1 if there are no matches. + MatchIdx int + // RuleIdx is the index of the corresponding rule in the HTTPRoute. + RuleIdx int + // Source is the corresponding HTTPRoute resource. + Source *v1alpha2.HTTPRoute +} + +// Operation defines an operation to be performed for a Host. +type Operation int + +const ( + // Delete the config for the Host. + Delete Operation = iota + // Upsert the config for the Host. + Upsert +) + +// Change represents a change of the Host that needs to be reflected in the NGINX config. +type Change struct { + // Op is the operation to be performed. + Op Operation + // Host is a reference to the Host associated with the Change. + Host *Host +} + +// StatusUpdate represents an update to the status of a resource. +type StatusUpdate struct { + // Object is the resource. + Object runtime.Object + // Status is the status field of the resource + // The Status include only the new conditions. This means that the status reporter component will need to merge + // the new conditions with the existing conditions of the resource. + Status interface{} +} + +// Configuration represents the configuration of the Gateway - a collection of routing rules ready to be transformed +// into NGINX configuration. +type Configuration struct { + // caches of valid resources + httpRoutes map[string]*v1alpha2.HTTPRoute + + // internal representation of Gateway configuration + httpListeners map[string]*httpListener + + gatewayCtlrName string + clock Clock +} + +// NewConfiguration creates a Configuration. +func NewConfiguration(gatewayCtlrName string, clock Clock) *Configuration { + c := &Configuration{ + httpRoutes: make(map[string]*v1alpha2.HTTPRoute), + httpListeners: make(map[string]*httpListener), + gatewayCtlrName: gatewayCtlrName, + clock: clock, + } + + // Until we process the GatewayClass and Gateway resources, we assume the "http" listener always exists. + c.httpListeners["http"] = &httpListener{ + hosts: make(map[string]*Host), + } + + return c +} + +// UpsertHTTPRoute upserts an HTTPRoute into the Configuration. +func (c *Configuration) UpsertHTTPRoute(httpRoute *v1alpha2.HTTPRoute) ([]Change, []StatusUpdate) { + key := getResourceKey(&httpRoute.ObjectMeta) + + oldHR, exist := c.httpRoutes[key] + if exist && compareObjectMetas(&oldHR.ObjectMeta, &httpRoute.ObjectMeta) { + // nothing to do - the resource hasn't changed + return nil, nil + } + + c.httpRoutes[key] = httpRoute + + return c.updateListeners() +} + +// DeleteHTTPRoute deletes an HTTPRoute from the Configuration. +func (c *Configuration) DeleteHTTPRoute(nsname types.NamespacedName) ([]Change, []StatusUpdate) { + delete(c.httpRoutes, nsname.String()) + + return c.updateListeners() +} + +func (c *Configuration) updateListeners() ([]Change, []StatusUpdate) { + var changes []Change + + // for now, we support only one listener + + c.httpListeners["http"], changes = rebuildHTTPListener(c.httpListeners["http"], c.httpRoutes) + + var statusUpdates []StatusUpdate + + listener := c.httpListeners["http"] + + // TO-DO: optimize it so that we only update the status of the affected (changed) httpRoutes + for _, key := range getSortedKeysForHTTPRoutes(listener.httpRoutes) { + update := StatusUpdate{ + Object: listener.httpRoutes[key], + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: v1alpha2.GatewayController(c.gatewayCtlrName), + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: listener.httpRoutes[key].Generation, + LastTransitionTime: metav1.NewTime(c.clock.Now()), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", // TO-DO: figure out a good message + }, + }, + }, + }, + }, + }, + } + statusUpdates = append(statusUpdates, update) + } + + // TO-DO: remove the accepted condition for the excluded (no longer handled) Routes + + return changes, statusUpdates +} + +func rebuildHTTPListener(listener *httpListener, httpRoutes map[string]*v1alpha2.HTTPRoute) (*httpListener, []Change) { + pathRoutesForHosts := buildPathRoutesForHosts(httpRoutes) + + newHosts, newHTTPRoutes := buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts) + + removedHosts, updatedHosts, addedHosts := determineChangesInHosts(listener, newHosts) + + changes := createChanges(removedHosts, updatedHosts, addedHosts, listener.hosts, newHosts) + + newListener := &httpListener{ + hosts: newHosts, + httpRoutes: newHTTPRoutes, + } + + return newListener, changes +} + +func createChanges(removedHosts []string, updatedHosts []string, addedHosts []string, oldHosts map[string]*Host, newHosts map[string]*Host) []Change { + var changes []Change + + for _, h := range removedHosts { + change := Change{ + Op: Delete, + Host: oldHosts[h], + } + changes = append(changes, change) + } + + for _, h := range updatedHosts { + change := Change{ + Op: Upsert, + Host: newHosts[h], + } + changes = append(changes, change) + } + + for _, h := range addedHosts { + change := Change{ + Op: Upsert, + Host: newHosts[h], + } + changes = append(changes, change) + } + + return changes +} + +func determineChangesInHosts(listener *httpListener, newHosts map[string]*Host) (removedHosts []string, updatedHosts []string, addedHosts []string) { + for _, h := range getSortedKeysForHosts(listener.hosts) { + _, exists := newHosts[h] + if !exists { + removedHosts = append(removedHosts, h) + } + } + + for _, h := range getSortedKeysForHosts(newHosts) { + _, exists := listener.hosts[h] + if !exists { + addedHosts = append(addedHosts, h) + } + } + + for _, h := range getSortedKeysForHosts(newHosts) { + oldHost, exists := listener.hosts[h] + if !exists { + continue + } + + if !arePathRoutesEqual(oldHost.PathRouteGroups, newHosts[h].PathRouteGroups) { + updatedHosts = append(updatedHosts, h) + } + } + return removedHosts, updatedHosts, addedHosts +} + +func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]map[string]*PathRouteGroup) (map[string]*Host, map[string]*v1alpha2.HTTPRoute) { + hosts := make(map[string]*Host) + routes := make(map[string]*v1alpha2.HTTPRoute) + + for h, pathRoutes := range pathRoutesForHosts { + host := &Host{ + Value: h, + } + + // This sorting (getSortedKeysForPathRoutes) will mess up the original order of rules in the HTTPRoutes. + // The order of routes can be important when regexes are used + // See https://nginx.org/en/docs/http/ngx_http_core_module.html#location to learn how NGINX searches for + // a location. + // This comment is to be aware of that. However, it is not yet clear whether it is a problem. + for _, path := range getSortedKeysForPathRoutes(pathRoutes) { + pathRoute := pathRoutes[path] + + sortRoutes(pathRoute.Routes) + + host.PathRouteGroups = append(host.PathRouteGroups, pathRoute) + + for _, r := range pathRoute.Routes { + key := getResourceKey(&r.Source.ObjectMeta) + routes[key] = r.Source + } + } + + hosts[h] = host + } + return hosts, routes +} + +func buildPathRoutesForHosts(httpRoutes map[string]*v1alpha2.HTTPRoute) map[string]map[string]*PathRouteGroup { + pathRoutesForHosts := make(map[string]map[string]*PathRouteGroup) + + // for now, we take in all available HTTPRoutes + for _, key := range getSortedKeysForHTTPRoutes(httpRoutes) { + hr := httpRoutes[key] + + // every hostname x every routing rule + for _, h := range hr.Spec.Hostnames { + pathRoutes, exist := pathRoutesForHosts[string(h)] + if !exist { + pathRoutes = make(map[string]*PathRouteGroup) + pathRoutesForHosts[string(h)] = pathRoutes + } + + for i := range hr.Spec.Rules { + rule := &hr.Spec.Rules[i] + + if len(rule.Matches) == 0 { + pathRoute, exist := pathRoutes["/"] + if !exist { + pathRoute = &PathRouteGroup{ + Path: "/", + } + pathRoutes["/"] = pathRoute + } + + pathRoute.Routes = append(pathRoute.Routes, Route{ + MatchIdx: -1, + RuleIdx: i, + Source: hr, + }) + } else { + for j, m := range rule.Matches { + path := "/" + if m.Path != nil && m.Path.Value != nil && *m.Path.Value != "/" { + path = *m.Path.Value + } + + pathRoute, exist := pathRoutes[path] + if !exist { + pathRoute = &PathRouteGroup{ + Path: path, + } + pathRoutes[path] = pathRoute + } + + pathRoute.Routes = append(pathRoute.Routes, Route{ + MatchIdx: j, + RuleIdx: i, + Source: hr, + }) + } + } + } + } + } + + return pathRoutesForHosts +} + +func arePathRoutesEqual(pathRoutes1, pathRoutes2 []*PathRouteGroup) bool { + if len(pathRoutes1) != len(pathRoutes2) { + return false + } + + for i := 0; i < len(pathRoutes1); i++ { + if pathRoutes1[i].Path != pathRoutes2[i].Path { + return false + } + + if len(pathRoutes1[i].Routes) != len(pathRoutes2[i].Routes) { + return false + } + + for j := 0; j < len(pathRoutes1[i].Routes); j++ { + if !compareObjectMetas(&pathRoutes1[i].Routes[j].Source.ObjectMeta, &pathRoutes2[i].Routes[j].Source.ObjectMeta) { + return false + } + + // DeepEqual might not be needed - the comparison above might be enough + idx1 := pathRoutes1[i].Routes[j].RuleIdx + rule1 := pathRoutes1[i].Routes[j].Source.Spec.Rules[idx1] + + idx2 := pathRoutes2[i].Routes[j].RuleIdx + rule2 := pathRoutes2[i].Routes[j].Source.Spec.Rules[idx2] + + if !reflect.DeepEqual(rule1, rule2) { + return false + } + } + } + + return true +} + +func compareObjectMetas(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool { + // Two resources are different if: + // (1) They have different namespaces or names. + // (2) They have the same namespace and name (resources are the same resource) but their specs are different. + // If their specs are different, their Generations are different too. So we only test their Generations. + // note: annotations are not part of the spec, so their update doesn't affect the Generation. + return meta1.Namespace == meta2.Namespace && + meta1.Name == meta2.Name && + meta1.Generation == meta2.Generation +} + +func sortRoutes(routes []Route) { + // stable sort is used so that the order of matches (as defined in each HTTPRoute rule) is preserved + // this is important, because the winning match is the first match to win. + sort.SliceStable(routes, func(i, j int) bool { + return lessObjectMeta(&routes[i].Source.ObjectMeta, &routes[j].Source.ObjectMeta) + }) +} + +func lessObjectMeta(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool { + if meta1.CreationTimestamp.Equal(&meta2.CreationTimestamp) { + return getResourceKey(meta1) < getResourceKey(meta2) + } + + return meta1.CreationTimestamp.Before(&meta2.CreationTimestamp) +} + +func getSortedKeysForPathRoutes(pathRoutes map[string]*PathRouteGroup) []string { + var keys []string + + for k := range pathRoutes { + keys = append(keys, k) + } + + sort.Strings(keys) + + return keys +} + +func getSortedKeysForHTTPRoutes(httpRoutes map[string]*v1alpha2.HTTPRoute) []string { + var keys []string + + for k := range httpRoutes { + keys = append(keys, k) + } + + sort.Strings(keys) + + return keys +} + +func getSortedKeysForHosts(hosts map[string]*Host) []string { + var keys []string + + for k := range hosts { + keys = append(keys, k) + } + + sort.Strings(keys) + + return keys +} + +func getResourceKey(meta *metav1.ObjectMeta) string { + return fmt.Sprintf("%s/%s", meta.Namespace, meta.Name) +} diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go new file mode 100644 index 0000000000..96dd744510 --- /dev/null +++ b/internal/state/configuration_test.go @@ -0,0 +1,860 @@ +package state_test + +import ( + "time" + + "github.com/google/go-cmp/cmp" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + . "github.com/onsi/ginkgo/v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + . "github.com/onsi/gomega" +) + +const gatewayCtlrName = v1alpha2.GatewayController("test-name") + +var _ = Describe("Configuration", func() { + Describe("Processing HTTPRoutes", func() { + var conf *state.Configuration + + constTime := time.Now() + + BeforeEach(OncePerOrdered, func() { + conf = state.NewConfiguration(string(gatewayCtlrName), state.NewFakeClock(constTime)) + }) + + Describe("Process one HTTPRoute with one host", Ordered, func() { + var hr, updatedHRWithSameGen, updatedHRWithIncrementedGen *v1alpha2.HTTPRoute + + BeforeAll(func() { + hr = &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + Spec: v1alpha2.HTTPRouteSpec{ + Hostnames: []v1alpha2.Hostname{ + "cafe.example.com", + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + // mo matches -> "/" + }, + { + Matches: []v1alpha2.HTTPRouteMatch{ + { + Path: &v1alpha2.HTTPPathMatch{ + Value: getStringPointer("/coffee"), + }, + }, + }, + }, + }, + }, + } + + updatedHRWithSameGen = hr.DeepCopy() + updatedHRWithSameGen.Spec.Rules[1].Matches[0].Path.Value = getStringPointer("/tea") + + updatedHRWithIncrementedGen = updatedHRWithSameGen.DeepCopy() + updatedHRWithIncrementedGen.Generation++ + }) + + It("should upsert a host and generate a status update for the new HTTPRoute", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr, + }, + }, + }, + { + Path: "/coffee", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 1, + Source: hr, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(hr) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should not generate changes and status updates for the updated HTTPRoute because it has the same generation as the old one", func() { + changes, statusUpdates := conf.UpsertHTTPRoute(updatedHRWithSameGen) + Expect(changes).To(BeEmpty()) + Expect(statusUpdates).To(BeEmpty()) + }) + + It("should upsert the host changes and generate a status update for the updated HTTPRoute", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: updatedHRWithIncrementedGen, + }, + }, + }, + { + Path: "/tea", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 1, + Source: updatedHRWithIncrementedGen, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: updatedHRWithIncrementedGen, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: updatedHRWithIncrementedGen.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(updatedHRWithIncrementedGen) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should delete the host for the deleted HTTPRoute", func() { + expectedChanges := []state.Change{ + { + Op: state.Delete, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: updatedHRWithIncrementedGen, + }, + }, + }, + { + Path: "/tea", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 1, + Source: updatedHRWithIncrementedGen, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route1"}) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(statusUpdates).To(BeEmpty()) + }) + }) + + It("should allow removing an non-exiting HTTPRoute", func() { + changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "some-route"}) + Expect(changes).To(BeEmpty()) + Expect(statusUpdates).To(BeEmpty()) + }) + + Describe("Processing multiple HTTPRoutes for one host", Ordered, func() { + var hr1, hr2, hr2Updated *v1alpha2.HTTPRoute + + BeforeAll(func() { + hr1 = &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + Spec: v1alpha2.HTTPRouteSpec{ + Hostnames: []v1alpha2.Hostname{ + "cafe.example.com", + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + // mo matches -> "/" + }, + }, + }, + } + + hr2 = &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route2", + }, + Spec: v1alpha2.HTTPRouteSpec{ + Hostnames: []v1alpha2.Hostname{ + "cafe.example.com", + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + Matches: []v1alpha2.HTTPRouteMatch{ + { + Path: &v1alpha2.HTTPPathMatch{ + Value: getStringPointer("/coffee"), + }, + }, + }, + }, + }, + }, + } + + hr2Updated = hr2.DeepCopy() + hr2Updated.Spec.Rules[0].Matches[0].Path.Value = getStringPointer("/tea") + hr2Updated.Generation++ + }) + + It("should upsert a host and generate a status update for the first HTTPRoute", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr1, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr1.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(hr1) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should upsert the same host and generate status updates for both HTTPRoutes after adding the second", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1, + }, + }, + }, + { + Path: "/coffee", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr2, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr1, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr1.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + { + Object: hr2, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr2.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(hr2) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should upsert the host and generate status updates for both HTTPRoutes after updating the second", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1, + }, + }, + }, + { + Path: "/tea", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr2Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr1, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr1.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + { + Object: hr2Updated, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr2Updated.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(hr2Updated) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should upsert the host and generate a status updates for the first HTTPRoute after deleting the second", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr1, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr1.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route2"}) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should delete the host after deleting the first HTTPRoute", func() { + expectedChanges := []state.Change{ + { + Op: state.Delete, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1, + }, + }, + }, + }, + }, + }, + } + changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route1"}) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(statusUpdates).To(BeEmpty()) + }) + }) + + Describe("Processing conflicting HTTPRoutes", Ordered, func() { + var earlier, later metav1.Time + var hr1, hr2, hr1Updated *v1alpha2.HTTPRoute + + BeforeAll(func() { + earlier = metav1.Now() + later = metav1.NewTime(earlier.Add(1 * time.Second)) + + hr1 = &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + CreationTimestamp: earlier, + }, + Spec: v1alpha2.HTTPRouteSpec{ + Hostnames: []v1alpha2.Hostname{ + "cafe.example.com", + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + // mo matches -> "/" + }, + }, + }, + } + + hr2 = &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route2", + CreationTimestamp: earlier, + }, + Spec: v1alpha2.HTTPRouteSpec{ + Hostnames: []v1alpha2.Hostname{ + "cafe.example.com", + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + Matches: []v1alpha2.HTTPRouteMatch{ + { + Path: &v1alpha2.HTTPPathMatch{ + Value: getStringPointer("/"), + }, + }, + }, + }, + }, + }, + } + + hr1Updated = hr1.DeepCopy() + hr1Updated.Generation++ + hr1Updated.CreationTimestamp = later + }) + + It("should upsert a host and generate a status update for the first HTTPRoute", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr1, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr1.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(hr1) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should upsert the host (make the first HTTPRoute the winner for '/' rule) and generate status updates for both HTTPRoutes after adding the second", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1, + }, + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr2, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr1, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr1.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + { + Object: hr2, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr2.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(hr2) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + + It("should upsert the host (make the second HTTPRoute the winner for '/' rule) and generate status updates for both HTTPRoutes after updating the first", func() { + expectedChanges := []state.Change{ + { + Op: state.Upsert, + Host: &state.Host{ + Value: "cafe.example.com", + PathRouteGroups: []*state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr2, + }, + { + MatchIdx: -1, + RuleIdx: 0, + Source: hr1Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatusUpdates := []state.StatusUpdate{ + { + Object: hr1Updated, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr1Updated.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + { + Object: hr2, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr2.Generation, + LastTransitionTime: metav1.NewTime(constTime), + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + changes, statusUpdates := conf.UpsertHTTPRoute(hr1Updated) + Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + }) + }) + }) +}) + +// diff prints the diff between two structs. +// It is useful when the structs are large. In such a case, without diff it will be difficult to pinpoint the difference +// between the two structs. +func diff(x, y interface{}) string { + r := cmp.Diff(x, y) + + if r != "" { + return "(-want +got)\n" + r + } + return r +} + +func getStringPointer(s string) *string { + return &s +} diff --git a/internal/state/state_suit_test.go b/internal/state/state_suit_test.go new file mode 100644 index 0000000000..5be96add9e --- /dev/null +++ b/internal/state/state_suit_test.go @@ -0,0 +1,13 @@ +package state_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "testing" +) + +func TestState(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "State Suite") +} From 3e76537b13f5b3b0861bbbcf53a4998fd37f17d1 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 1 Feb 2022 10:55:15 -0800 Subject: [PATCH 2/9] Add mapper interface and impls --- internal/state/configuration.go | 113 ++++++++++++++------------------ internal/state/sort.go | 34 ++++++++++ 2 files changed, 82 insertions(+), 65 deletions(-) create mode 100644 internal/state/sort.go diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 59dee965bc..a5f6e6d3ff 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -3,7 +3,6 @@ package state import ( "fmt" "reflect" - "sort" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -14,9 +13,33 @@ import ( // httpListener defines an HTTP Listener. type httpListener struct { // hosts include all Hosts that belong to the listener. - hosts map[string]*Host + hosts hosts // httpRoutes include all HTTPRoute resources that belong to the listener. - httpRoutes map[string]*v1alpha2.HTTPRoute + httpRoutes httpRoutes +} + +type hosts map[string]*Host + +func (hs hosts) Keys() []string { + keys := make([]string, 0, len(hs)) + + for k := range hs { + keys = append(keys, k) + } + + return keys +} + +type httpRoutes map[string]*v1alpha2.HTTPRoute + +func (hrs httpRoutes) Keys() []string { + keys := make([]string, 0, len(hrs)) + + for k := range hrs { + keys = append(keys, k) + } + + return keys } // Host is the primary configuration unit of the internal representation. @@ -46,6 +69,18 @@ type PathRouteGroup struct { Routes []Route } +type pathRoutesForHosts map[string]*PathRouteGroup + +func (prs pathRoutesForHosts) Keys() []string { + keys := make([]string, 0, len(prs)) + + for k := range prs { + keys = append(keys, k) + } + + return keys +} + // Route represents a Route, which corresponds to a Match in the HTTPRouteRule. If a rule doesn't define any matches, // it is assumed that the rule is for "/" path. type Route struct { @@ -149,7 +184,7 @@ func (c *Configuration) updateListeners() ([]Change, []StatusUpdate) { listener := c.httpListeners["http"] // TO-DO: optimize it so that we only update the status of the affected (changed) httpRoutes - for _, key := range getSortedKeysForHTTPRoutes(listener.httpRoutes) { + for _, key := range getSortedKeys(listener.httpRoutes) { update := StatusUpdate{ Object: listener.httpRoutes[key], Status: &v1alpha2.HTTPRouteStatus{ @@ -227,22 +262,22 @@ func createChanges(removedHosts []string, updatedHosts []string, addedHosts []st return changes } -func determineChangesInHosts(listener *httpListener, newHosts map[string]*Host) (removedHosts []string, updatedHosts []string, addedHosts []string) { - for _, h := range getSortedKeysForHosts(listener.hosts) { +func determineChangesInHosts(listener *httpListener, newHosts hosts) (removedHosts []string, updatedHosts []string, addedHosts []string) { + for _, h := range getSortedKeys(listener.hosts) { _, exists := newHosts[h] if !exists { removedHosts = append(removedHosts, h) } } - for _, h := range getSortedKeysForHosts(newHosts) { + for _, h := range getSortedKeys(newHosts) { _, exists := listener.hosts[h] if !exists { addedHosts = append(addedHosts, h) } } - for _, h := range getSortedKeysForHosts(newHosts) { + for _, h := range getSortedKeys(newHosts) { oldHost, exists := listener.hosts[h] if !exists { continue @@ -255,7 +290,7 @@ func determineChangesInHosts(listener *httpListener, newHosts map[string]*Host) return removedHosts, updatedHosts, addedHosts } -func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]map[string]*PathRouteGroup) (map[string]*Host, map[string]*v1alpha2.HTTPRoute) { +func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]pathRoutesForHosts) (map[string]*Host, map[string]*v1alpha2.HTTPRoute) { hosts := make(map[string]*Host) routes := make(map[string]*v1alpha2.HTTPRoute) @@ -269,7 +304,7 @@ func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]map[string]* // See https://nginx.org/en/docs/http/ngx_http_core_module.html#location to learn how NGINX searches for // a location. // This comment is to be aware of that. However, it is not yet clear whether it is a problem. - for _, path := range getSortedKeysForPathRoutes(pathRoutes) { + for _, path := range getSortedKeys(pathRoutes) { pathRoute := pathRoutes[path] sortRoutes(pathRoute.Routes) @@ -287,11 +322,11 @@ func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]map[string]* return hosts, routes } -func buildPathRoutesForHosts(httpRoutes map[string]*v1alpha2.HTTPRoute) map[string]map[string]*PathRouteGroup { - pathRoutesForHosts := make(map[string]map[string]*PathRouteGroup) +func buildPathRoutesForHosts(httpRoutes httpRoutes) map[string]pathRoutesForHosts { + pathRoutesForHosts := make(map[string]pathRoutesForHosts) // for now, we take in all available HTTPRoutes - for _, key := range getSortedKeysForHTTPRoutes(httpRoutes) { + for _, key := range getSortedKeys(httpRoutes) { hr := httpRoutes[key] // every hostname x every routing rule @@ -394,58 +429,6 @@ func compareObjectMetas(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool meta1.Generation == meta2.Generation } -func sortRoutes(routes []Route) { - // stable sort is used so that the order of matches (as defined in each HTTPRoute rule) is preserved - // this is important, because the winning match is the first match to win. - sort.SliceStable(routes, func(i, j int) bool { - return lessObjectMeta(&routes[i].Source.ObjectMeta, &routes[j].Source.ObjectMeta) - }) -} - -func lessObjectMeta(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool { - if meta1.CreationTimestamp.Equal(&meta2.CreationTimestamp) { - return getResourceKey(meta1) < getResourceKey(meta2) - } - - return meta1.CreationTimestamp.Before(&meta2.CreationTimestamp) -} - -func getSortedKeysForPathRoutes(pathRoutes map[string]*PathRouteGroup) []string { - var keys []string - - for k := range pathRoutes { - keys = append(keys, k) - } - - sort.Strings(keys) - - return keys -} - -func getSortedKeysForHTTPRoutes(httpRoutes map[string]*v1alpha2.HTTPRoute) []string { - var keys []string - - for k := range httpRoutes { - keys = append(keys, k) - } - - sort.Strings(keys) - - return keys -} - -func getSortedKeysForHosts(hosts map[string]*Host) []string { - var keys []string - - for k := range hosts { - keys = append(keys, k) - } - - sort.Strings(keys) - - return keys -} - func getResourceKey(meta *metav1.ObjectMeta) string { return fmt.Sprintf("%s/%s", meta.Namespace, meta.Name) } diff --git a/internal/state/sort.go b/internal/state/sort.go new file mode 100644 index 0000000000..c97fb6b070 --- /dev/null +++ b/internal/state/sort.go @@ -0,0 +1,34 @@ +package state + +import ( + "sort" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func sortRoutes(routes []Route) { + // stable sort is used so that the order of matches (as defined in each HTTPRoute rule) is preserved + // this is important, because the winning match is the first match to win. + sort.SliceStable(routes, func(i, j int) bool { + return lessObjectMeta(&routes[i].Source.ObjectMeta, &routes[j].Source.ObjectMeta) + }) +} + +func lessObjectMeta(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool { + if meta1.CreationTimestamp.Equal(&meta2.CreationTimestamp) { + return getResourceKey(meta1) < getResourceKey(meta2) + } + + return meta1.CreationTimestamp.Before(&meta2.CreationTimestamp) +} + +type mapper interface { + Keys() []string +} + +func getSortedKeys(m mapper) []string { + keys := m.Keys() + sort.Strings(keys) + + return keys +} From 2a1a63531b6b7c6c3081edc471582786efebd53a Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 3 Feb 2022 13:15:35 -0800 Subject: [PATCH 3/9] Add unit test for FakeClock --- internal/state/clock_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 internal/state/clock_test.go diff --git a/internal/state/clock_test.go b/internal/state/clock_test.go new file mode 100644 index 0000000000..f32e2c22fe --- /dev/null +++ b/internal/state/clock_test.go @@ -0,0 +1,16 @@ +package state + +import ( + "testing" + "time" +) + +func TestFakeClock(t *testing.T) { + time := time.Now() + clock := NewFakeClock(time) + + result := clock.Now() + if result != time { + t.Errorf("Now() returned %v but expected %v", result, time) + } +} From dd4da6906a6d205405922455bec6d7190aab4c0a Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 3 Feb 2022 13:24:04 -0800 Subject: [PATCH 4/9] Replace long forms type with the type declarations --- internal/state/configuration.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/state/configuration.go b/internal/state/configuration.go index a5f6e6d3ff..8c740f9a33 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -124,7 +124,7 @@ type StatusUpdate struct { // into NGINX configuration. type Configuration struct { // caches of valid resources - httpRoutes map[string]*v1alpha2.HTTPRoute + httpRoutes httpRoutes // internal representation of Gateway configuration httpListeners map[string]*httpListener @@ -136,7 +136,7 @@ type Configuration struct { // NewConfiguration creates a Configuration. func NewConfiguration(gatewayCtlrName string, clock Clock) *Configuration { c := &Configuration{ - httpRoutes: make(map[string]*v1alpha2.HTTPRoute), + httpRoutes: make(httpRoutes), httpListeners: make(map[string]*httpListener), gatewayCtlrName: gatewayCtlrName, clock: clock, @@ -144,7 +144,7 @@ func NewConfiguration(gatewayCtlrName string, clock Clock) *Configuration { // Until we process the GatewayClass and Gateway resources, we assume the "http" listener always exists. c.httpListeners["http"] = &httpListener{ - hosts: make(map[string]*Host), + hosts: make(hosts), } return c @@ -215,7 +215,7 @@ func (c *Configuration) updateListeners() ([]Change, []StatusUpdate) { return changes, statusUpdates } -func rebuildHTTPListener(listener *httpListener, httpRoutes map[string]*v1alpha2.HTTPRoute) (*httpListener, []Change) { +func rebuildHTTPListener(listener *httpListener, httpRoutes httpRoutes) (*httpListener, []Change) { pathRoutesForHosts := buildPathRoutesForHosts(httpRoutes) newHosts, newHTTPRoutes := buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts) @@ -232,7 +232,7 @@ func rebuildHTTPListener(listener *httpListener, httpRoutes map[string]*v1alpha2 return newListener, changes } -func createChanges(removedHosts []string, updatedHosts []string, addedHosts []string, oldHosts map[string]*Host, newHosts map[string]*Host) []Change { +func createChanges(removedHosts []string, updatedHosts []string, addedHosts []string, oldHosts hosts, newHosts hosts) []Change { var changes []Change for _, h := range removedHosts { @@ -290,9 +290,9 @@ func determineChangesInHosts(listener *httpListener, newHosts hosts) (removedHos return removedHosts, updatedHosts, addedHosts } -func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]pathRoutesForHosts) (map[string]*Host, map[string]*v1alpha2.HTTPRoute) { - hosts := make(map[string]*Host) - routes := make(map[string]*v1alpha2.HTTPRoute) +func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]pathRoutesForHosts) (hosts, httpRoutes) { + hosts := make(hosts) + routes := make(httpRoutes) for h, pathRoutes := range pathRoutesForHosts { host := &Host{ From 6cf04db6e9a06241eb4949ac33eb6f1637c22503 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 3 Feb 2022 13:26:27 -0800 Subject: [PATCH 5/9] Add a comment about gatewayCtlrName --- internal/state/configuration.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 8c740f9a33..3a236676db 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -134,6 +134,7 @@ type Configuration struct { } // NewConfiguration creates a Configuration. +// It is expected that the client set gatewayCtlrName to a non-empty value. func NewConfiguration(gatewayCtlrName string, clock Clock) *Configuration { c := &Configuration{ httpRoutes: make(httpRoutes), From 2afbd74f1b093ecf989c7b528c3eed02dd92a673 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 3 Feb 2022 13:54:15 -0800 Subject: [PATCH 6/9] Get rid of pointers and rename some vars --- internal/state/configuration.go | 74 ++++++++++++++-------------- internal/state/configuration_test.go | 44 ++++++++--------- 2 files changed, 60 insertions(+), 58 deletions(-) diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 3a236676db..63cf3ca209 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -18,7 +18,7 @@ type httpListener struct { httpRoutes httpRoutes } -type hosts map[string]*Host +type hosts map[string]Host func (hs hosts) Keys() []string { keys := make([]string, 0, len(hs)) @@ -50,7 +50,7 @@ type Host struct { Value string // PathRouteGroups include all PathRouteGroups that belong to the Host. // We use a slice rather than a map to control the order of the routes. - PathRouteGroups []*PathRouteGroup + PathRouteGroups []PathRouteGroup } // PathRouteGroup represents a collection of Routes grouped by a path. @@ -69,9 +69,9 @@ type PathRouteGroup struct { Routes []Route } -type pathRoutesForHosts map[string]*PathRouteGroup +type pathRoutesGroups map[string]PathRouteGroup -func (prs pathRoutesForHosts) Keys() []string { +func (prs pathRoutesGroups) Keys() []string { keys := make([]string, 0, len(prs)) for k := range prs { @@ -107,7 +107,7 @@ type Change struct { // Op is the operation to be performed. Op Operation // Host is a reference to the Host associated with the Change. - Host *Host + Host Host } // StatusUpdate represents an update to the status of a resource. @@ -127,7 +127,7 @@ type Configuration struct { httpRoutes httpRoutes // internal representation of Gateway configuration - httpListeners map[string]*httpListener + httpListeners map[string]httpListener gatewayCtlrName string clock Clock @@ -138,13 +138,13 @@ type Configuration struct { func NewConfiguration(gatewayCtlrName string, clock Clock) *Configuration { c := &Configuration{ httpRoutes: make(httpRoutes), - httpListeners: make(map[string]*httpListener), + httpListeners: make(map[string]httpListener), gatewayCtlrName: gatewayCtlrName, clock: clock, } // Until we process the GatewayClass and Gateway resources, we assume the "http" listener always exists. - c.httpListeners["http"] = &httpListener{ + c.httpListeners["http"] = httpListener{ hosts: make(hosts), } @@ -216,8 +216,8 @@ func (c *Configuration) updateListeners() ([]Change, []StatusUpdate) { return changes, statusUpdates } -func rebuildHTTPListener(listener *httpListener, httpRoutes httpRoutes) (*httpListener, []Change) { - pathRoutesForHosts := buildPathRoutesForHosts(httpRoutes) +func rebuildHTTPListener(listener httpListener, httpRoutes httpRoutes) (httpListener, []Change) { + pathRoutesForHosts := buildPathRoutesGroupsForHosts(httpRoutes) newHosts, newHTTPRoutes := buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts) @@ -225,7 +225,7 @@ func rebuildHTTPListener(listener *httpListener, httpRoutes httpRoutes) (*httpLi changes := createChanges(removedHosts, updatedHosts, addedHosts, listener.hosts, newHosts) - newListener := &httpListener{ + newListener := httpListener{ hosts: newHosts, httpRoutes: newHTTPRoutes, } @@ -263,7 +263,7 @@ func createChanges(removedHosts []string, updatedHosts []string, addedHosts []st return changes } -func determineChangesInHosts(listener *httpListener, newHosts hosts) (removedHosts []string, updatedHosts []string, addedHosts []string) { +func determineChangesInHosts(listener httpListener, newHosts hosts) (removedHosts []string, updatedHosts []string, addedHosts []string) { for _, h := range getSortedKeys(listener.hosts) { _, exists := newHosts[h] if !exists { @@ -291,28 +291,28 @@ func determineChangesInHosts(listener *httpListener, newHosts hosts) (removedHos return removedHosts, updatedHosts, addedHosts } -func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]pathRoutesForHosts) (hosts, httpRoutes) { +func buildHostsAndDetermineHTTPRoutes(routeGroupsForHosts map[string]pathRoutesGroups) (hosts, httpRoutes) { hosts := make(hosts) routes := make(httpRoutes) - for h, pathRoutes := range pathRoutesForHosts { - host := &Host{ + for h, groups := range routeGroupsForHosts { + host := Host{ Value: h, } - // This sorting (getSortedKeysForPathRoutes) will mess up the original order of rules in the HTTPRoutes. + // This sorting (getSortedKeys) will mess up the original order of rules in the HTTPRoutes. // The order of routes can be important when regexes are used // See https://nginx.org/en/docs/http/ngx_http_core_module.html#location to learn how NGINX searches for // a location. // This comment is to be aware of that. However, it is not yet clear whether it is a problem. - for _, path := range getSortedKeys(pathRoutes) { - pathRoute := pathRoutes[path] + for _, path := range getSortedKeys(groups) { + group := groups[path] - sortRoutes(pathRoute.Routes) + sortRoutes(group.Routes) - host.PathRouteGroups = append(host.PathRouteGroups, pathRoute) + host.PathRouteGroups = append(host.PathRouteGroups, group) - for _, r := range pathRoute.Routes { + for _, r := range group.Routes { key := getResourceKey(&r.Source.ObjectMeta) routes[key] = r.Source } @@ -323,8 +323,8 @@ func buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts map[string]pathRoutesFo return hosts, routes } -func buildPathRoutesForHosts(httpRoutes httpRoutes) map[string]pathRoutesForHosts { - pathRoutesForHosts := make(map[string]pathRoutesForHosts) +func buildPathRoutesGroupsForHosts(httpRoutes httpRoutes) map[string]pathRoutesGroups { + routeGroupsForHosts := make(map[string]pathRoutesGroups) // for now, we take in all available HTTPRoutes for _, key := range getSortedKeys(httpRoutes) { @@ -332,29 +332,30 @@ func buildPathRoutesForHosts(httpRoutes httpRoutes) map[string]pathRoutesForHost // every hostname x every routing rule for _, h := range hr.Spec.Hostnames { - pathRoutes, exist := pathRoutesForHosts[string(h)] + groups, exist := routeGroupsForHosts[string(h)] if !exist { - pathRoutes = make(map[string]*PathRouteGroup) - pathRoutesForHosts[string(h)] = pathRoutes + groups = make(pathRoutesGroups) + routeGroupsForHosts[string(h)] = groups } for i := range hr.Spec.Rules { rule := &hr.Spec.Rules[i] if len(rule.Matches) == 0 { - pathRoute, exist := pathRoutes["/"] + group, exist := groups["/"] if !exist { - pathRoute = &PathRouteGroup{ + group = PathRouteGroup{ Path: "/", } - pathRoutes["/"] = pathRoute } - pathRoute.Routes = append(pathRoute.Routes, Route{ + group.Routes = append(group.Routes, Route{ MatchIdx: -1, RuleIdx: i, Source: hr, }) + + groups["/"] = group } else { for j, m := range rule.Matches { path := "/" @@ -362,29 +363,30 @@ func buildPathRoutesForHosts(httpRoutes httpRoutes) map[string]pathRoutesForHost path = *m.Path.Value } - pathRoute, exist := pathRoutes[path] + group, exist := groups[path] if !exist { - pathRoute = &PathRouteGroup{ + group = PathRouteGroup{ Path: path, } - pathRoutes[path] = pathRoute } - pathRoute.Routes = append(pathRoute.Routes, Route{ + group.Routes = append(group.Routes, Route{ MatchIdx: j, RuleIdx: i, Source: hr, }) + + groups[path] = group } } } } } - return pathRoutesForHosts + return routeGroupsForHosts } -func arePathRoutesEqual(pathRoutes1, pathRoutes2 []*PathRouteGroup) bool { +func arePathRoutesEqual(pathRoutes1, pathRoutes2 []PathRouteGroup) bool { if len(pathRoutes1) != len(pathRoutes2) { return false } diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go index 96dd744510..3881bef623 100644 --- a/internal/state/configuration_test.go +++ b/internal/state/configuration_test.go @@ -66,9 +66,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -133,9 +133,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -194,9 +194,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Delete, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -287,9 +287,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -338,9 +338,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -421,9 +421,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -504,9 +504,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -555,9 +555,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Delete, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -637,9 +637,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -688,9 +688,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ @@ -766,9 +766,9 @@ var _ = Describe("Configuration", func() { expectedChanges := []state.Change{ { Op: state.Upsert, - Host: &state.Host{ + Host: state.Host{ Value: "cafe.example.com", - PathRouteGroups: []*state.PathRouteGroup{ + PathRouteGroups: []state.PathRouteGroup{ { Path: "/", Routes: []state.Route{ From 0132176832ea68d044107f3d253f701b4affad4e Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 3 Feb 2022 14:06:50 -0800 Subject: [PATCH 7/9] Add comments about sorting or remove sorting --- internal/state/configuration.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 63cf3ca209..3062443749 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -185,6 +185,7 @@ func (c *Configuration) updateListeners() ([]Change, []StatusUpdate) { listener := c.httpListeners["http"] // TO-DO: optimize it so that we only update the status of the affected (changed) httpRoutes + // getSortedKeys is used to ensure predictable order for unit tests for _, key := range getSortedKeys(listener.httpRoutes) { update := StatusUpdate{ Object: listener.httpRoutes[key], @@ -264,6 +265,8 @@ func createChanges(removedHosts []string, updatedHosts []string, addedHosts []st } func determineChangesInHosts(listener httpListener, newHosts hosts) (removedHosts []string, updatedHosts []string, addedHosts []string) { + // getSortedKeys is used to ensure predictable order for unit tests + for _, h := range getSortedKeys(listener.hosts) { _, exists := newHosts[h] if !exists { @@ -300,11 +303,12 @@ func buildHostsAndDetermineHTTPRoutes(routeGroupsForHosts map[string]pathRoutesG Value: h, } - // This sorting (getSortedKeys) will mess up the original order of rules in the HTTPRoutes. - // The order of routes can be important when regexes are used + // getSortedKeys is used so that the order of locations in the NGINX config doesn't change on every config + // regeneration. However, that sorting will mess up the original order of the rules in the HTTPRoutes. + // The order can be important when regexes are used. // See https://nginx.org/en/docs/http/ngx_http_core_module.html#location to learn how NGINX searches for // a location. - // This comment is to be aware of that. However, it is not yet clear whether it is a problem. + // This comment is to be aware of a potential issue. However, it is not yet clear whether it is an issue. for _, path := range getSortedKeys(groups) { group := groups[path] @@ -327,9 +331,7 @@ func buildPathRoutesGroupsForHosts(httpRoutes httpRoutes) map[string]pathRoutesG routeGroupsForHosts := make(map[string]pathRoutesGroups) // for now, we take in all available HTTPRoutes - for _, key := range getSortedKeys(httpRoutes) { - hr := httpRoutes[key] - + for _, hr := range httpRoutes { // every hostname x every routing rule for _, h := range hr.Spec.Hostnames { groups, exist := routeGroupsForHosts[string(h)] From d6f20b78e5b85e034c1bce6df3cca8100477b7aa Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 3 Feb 2022 14:08:30 -0800 Subject: [PATCH 8/9] Add a to-do to use set data structure --- internal/state/configuration.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 3062443749..9466ae090b 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -267,6 +267,8 @@ func createChanges(removedHosts []string, updatedHosts []string, addedHosts []st func determineChangesInHosts(listener httpListener, newHosts hosts) (removedHosts []string, updatedHosts []string, addedHosts []string) { // getSortedKeys is used to ensure predictable order for unit tests + // TO-DO: consider using a data structure for sets + for _, h := range getSortedKeys(listener.hosts) { _, exists := newHosts[h] if !exists { From bc12c717cc5891ce7b925466db88b23b23f87cb2 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 3 Feb 2022 14:15:39 -0800 Subject: [PATCH 9/9] Creates slices with known capacities --- internal/state/configuration.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 9466ae090b..0c350875a7 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -180,10 +180,10 @@ func (c *Configuration) updateListeners() ([]Change, []StatusUpdate) { c.httpListeners["http"], changes = rebuildHTTPListener(c.httpListeners["http"], c.httpRoutes) - var statusUpdates []StatusUpdate - listener := c.httpListeners["http"] + statusUpdates := make([]StatusUpdate, 0, len(listener.httpRoutes)) + // TO-DO: optimize it so that we only update the status of the affected (changed) httpRoutes // getSortedKeys is used to ensure predictable order for unit tests for _, key := range getSortedKeys(listener.httpRoutes) { @@ -305,6 +305,8 @@ func buildHostsAndDetermineHTTPRoutes(routeGroupsForHosts map[string]pathRoutesG Value: h, } + host.PathRouteGroups = make([]PathRouteGroup, 0, len(groups)) + // getSortedKeys is used so that the order of locations in the NGINX config doesn't change on every config // regeneration. However, that sorting will mess up the original order of the rules in the HTTPRoutes. // The order can be important when regexes are used.