From ee48f022af50d3fb51ddb5a74a15917b43da4e5a Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 16 Mar 2022 18:04:39 -0700 Subject: [PATCH 1/6] Support Config Generation This commit adds a new component - Generator - for generating NGINX config. --- internal/events/loop.go | 46 +-- internal/events/loop_test.go | 38 ++- internal/helpers/helpers.go | 10 + internal/manager/manager.go | 4 +- .../config/configfakes/fake_generator.go | 112 ++++++++ internal/nginx/config/generator.go | 98 +++++++ internal/nginx/config/generator_test.go | 267 ++++++++++++++++++ internal/nginx/config/http.go | 11 + internal/nginx/config/template.go | 47 +++ internal/nginx/config/template_test.go | 57 ++++ internal/state/configuration_test.go | 14 +- 11 files changed, 648 insertions(+), 56 deletions(-) create mode 100644 internal/nginx/config/configfakes/fake_generator.go create mode 100644 internal/nginx/config/generator.go create mode 100644 internal/nginx/config/generator_test.go create mode 100644 internal/nginx/config/http.go create mode 100644 internal/nginx/config/template.go create mode 100644 internal/nginx/config/template_test.go diff --git a/internal/events/loop.go b/internal/events/loop.go index 0c4991b3ef..4b6ca08fc2 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -5,10 +5,10 @@ import ( "fmt" "github.com/go-logr/logr" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" apiv1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -16,17 +16,19 @@ import ( type EventLoop struct { conf state.Configuration serviceStore state.ServiceStore + generator config.Generator eventCh <-chan interface{} logger logr.Logger statusUpdater status.Updater } // NewEventLoop creates a new EventLoop. -func NewEventLoop(conf state.Configuration, serviceStore state.ServiceStore, eventCh <-chan interface{}, - statusUpdater status.Updater, logger logr.Logger) *EventLoop { +func NewEventLoop(conf state.Configuration, serviceStore state.ServiceStore, generator config.Generator, + eventCh <-chan interface{}, statusUpdater status.Updater, logger logr.Logger) *EventLoop { return &EventLoop{ conf: conf, serviceStore: serviceStore, + generator: generator, eventCh: eventCh, statusUpdater: statusUpdater, logger: logger.WithName("eventLoop"), @@ -114,40 +116,10 @@ func (el *EventLoop) processChangesAndStatusUpdates(ctx context.Context, changes fmt.Printf("%+v\n", c) if c.Op == state.Upsert { - // The code below resolves service backend refs into their cluster IPs - // TO-DO: this code will be removed once we have the component that generates NGINX config and - // uses the ServiceStore to resolve services. - for _, g := range c.Host.PathRouteGroups { - for _, r := range g.Routes { - for _, b := range r.Source.Spec.Rules[r.RuleIdx].BackendRefs { - if b.BackendRef.Kind == nil || *b.BackendRef.Kind == "Service" { - ns := r.Source.Namespace - if b.BackendRef.Namespace != nil { - ns = string(*b.BackendRef.Namespace) - } - - address, err := el.serviceStore.Resolve(types.NamespacedName{ - Namespace: ns, - Name: string(b.BackendRef.Name), - }) - - if err != nil { - fmt.Printf("Service %s/%s error: %v\n", ns, b.BackendRef.Name, err) - continue - } - - var port int32 = 80 - if b.BackendRef.Port != nil { - port = int32(*b.BackendRef.Port) - } - - address = fmt.Sprintf("%s:%d", address, port) - - fmt.Printf("Service %s/%s: %s\n", ns, b.BackendRef.Name, address) - } - } - } - } + cfg := el.generator.GenerateForHost(c.Host) + // TO-DO: for now, we only print the generated config, without writing it on the file system + // and reloading NGINX. + fmt.Println(string(cfg)) } } diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index c1dc6514af..5c0c99e325 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -4,6 +4,7 @@ import ( "context" "github.com/nginxinc/nginx-gateway-kubernetes/internal/events" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config/configfakes" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state/statefakes" "github.com/nginxinc/nginx-gateway-kubernetes/internal/status/statusfakes" @@ -35,6 +36,7 @@ var _ = Describe("EventLoop", func() { var fakeConf *statefakes.FakeConfiguration var fakeUpdater *statusfakes.FakeUpdater var fakeServiceStore *statefakes.FakeServiceStore + var fakeGenerator *configfakes.FakeGenerator var cancel context.CancelFunc var eventCh chan interface{} var errorCh chan error @@ -44,7 +46,8 @@ var _ = Describe("EventLoop", func() { eventCh = make(chan interface{}) fakeUpdater = &statusfakes.FakeUpdater{} fakeServiceStore = &statefakes.FakeServiceStore{} - ctrl = events.NewEventLoop(fakeConf, fakeServiceStore, eventCh, fakeUpdater, zap.New()) + fakeGenerator = &configfakes.FakeGenerator{} + ctrl = events.NewEventLoop(fakeConf, fakeServiceStore, fakeGenerator, eventCh, fakeUpdater, zap.New()) var ctx context.Context @@ -66,15 +69,19 @@ var _ = Describe("EventLoop", func() { }) It("should process upsert event", func() { + fakeChanges := []state.Change{ + { + Op: state.Upsert, + Host: state.Host{}, + }, + } fakeStatusUpdates := []state.StatusUpdate{ { NamespacedName: types.NamespacedName{}, Status: nil, }, } - // for now, we pass nil, because we don't need to test how EventLoop processes changes yet. We will start - // testing once we have NGINX Configuration Manager component. - fakeConf.UpsertHTTPRouteReturns(nil, fakeStatusUpdates) + fakeConf.UpsertHTTPRouteReturns(fakeChanges, fakeStatusUpdates) hr := &v1alpha2.HTTPRoute{} @@ -87,23 +94,32 @@ var _ = Describe("EventLoop", func() { return fakeConf.UpsertHTTPRouteArgsForCall(0) }).Should(Equal(hr)) - Eventually(fakeUpdater.ProcessStatusUpdatesCallCount()).Should(Equal(1)) + Eventually(fakeUpdater.ProcessStatusUpdatesCallCount).Should(Equal(1)) Eventually(func() []state.StatusUpdate { _, updates := fakeUpdater.ProcessStatusUpdatesArgsForCall(0) return updates }).Should(Equal(fakeStatusUpdates)) + + Eventually(fakeGenerator.GenerateForHostCallCount).Should(Equal(1)) + Eventually(func() state.Host { + return fakeGenerator.GenerateForHostArgsForCall(0) + }).Should(Equal(fakeChanges[0].Host)) }) It("should process delete event", func() { + fakeChanges := []state.Change{ + { + Op: state.Delete, + Host: state.Host{}, + }, + } fakeStatusUpdates := []state.StatusUpdate{ { NamespacedName: types.NamespacedName{}, Status: nil, }, } - // for now, we pass nil, because we don't need to test how EventLoop processes changes yet. We will start - // testing once we have NGINX Configuration Manager component. - fakeConf.DeleteHTTPRouteReturns(nil, fakeStatusUpdates) + fakeConf.DeleteHTTPRouteReturns(fakeChanges, fakeStatusUpdates) nsname := types.NamespacedName{Namespace: "test", Name: "route"} @@ -117,11 +133,15 @@ var _ = Describe("EventLoop", func() { return fakeConf.DeleteHTTPRouteArgsForCall(0) }).Should(Equal(nsname)) - Eventually(fakeUpdater.ProcessStatusUpdatesCallCount()).Should(Equal(1)) + Eventually(fakeUpdater.ProcessStatusUpdatesCallCount).Should(Equal(1)) Eventually(func() []state.StatusUpdate { _, updates := fakeUpdater.ProcessStatusUpdatesArgsForCall(0) return updates }).Should(Equal(fakeStatusUpdates)) + + // TO-DO: + // once we have a component that processes host deletion, ensure that + // its corresponding method is eventually called }) }) diff --git a/internal/helpers/helpers.go b/internal/helpers/helpers.go index 3196bdd0ce..a38009f787 100644 --- a/internal/helpers/helpers.go +++ b/internal/helpers/helpers.go @@ -13,3 +13,13 @@ func Diff(x, y interface{}) string { } return r } + +// GetStringPointer takes a string and returns a pointer to it. Useful in unit tests when initializing structs. +func GetStringPointer(s string) *string { + return &s +} + +// GetInt32Pointer takes an int32 and returns a pointer to it. Useful in unit tests when initializing structs. +func GetInt32Pointer(i int32) *int32 { + return &i +} diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 67b435dc64..0c09d0e1c9 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -11,6 +11,7 @@ import ( gcfg "github.com/nginxinc/nginx-gateway-kubernetes/internal/implementations/gatewayconfig" hr "github.com/nginxinc/nginx-gateway-kubernetes/internal/implementations/httproute" svc "github.com/nginxinc/nginx-gateway-kubernetes/internal/implementations/service" + ngxcfg "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" nginxgwv1alpha1 "github.com/nginxinc/nginx-gateway-kubernetes/pkg/apis/gateway/v1alpha1" @@ -76,7 +77,8 @@ func Start(cfg config.Config) error { conf := state.NewConfiguration(cfg.GatewayCtlrName, state.NewRealClock()) serviceStore := state.NewServiceStore() reporter := status.NewUpdater(mgr.GetClient(), cfg.Logger) - eventLoop := events.NewEventLoop(conf, serviceStore, eventCh, reporter, cfg.Logger) + configGenerator := ngxcfg.NewGeneratorImpl(serviceStore) + eventLoop := events.NewEventLoop(conf, serviceStore, configGenerator, eventCh, reporter, cfg.Logger) err = mgr.Add(eventLoop) if err != nil { diff --git a/internal/nginx/config/configfakes/fake_generator.go b/internal/nginx/config/configfakes/fake_generator.go new file mode 100644 index 0000000000..7d3faa0f71 --- /dev/null +++ b/internal/nginx/config/configfakes/fake_generator.go @@ -0,0 +1,112 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package configfakes + +import ( + "sync" + + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" +) + +type FakeGenerator struct { + GenerateForHostStub func(state.Host) []byte + generateForHostMutex sync.RWMutex + generateForHostArgsForCall []struct { + arg1 state.Host + } + generateForHostReturns struct { + result1 []byte + } + generateForHostReturnsOnCall map[int]struct { + result1 []byte + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeGenerator) GenerateForHost(arg1 state.Host) []byte { + fake.generateForHostMutex.Lock() + ret, specificReturn := fake.generateForHostReturnsOnCall[len(fake.generateForHostArgsForCall)] + fake.generateForHostArgsForCall = append(fake.generateForHostArgsForCall, struct { + arg1 state.Host + }{arg1}) + stub := fake.GenerateForHostStub + fakeReturns := fake.generateForHostReturns + fake.recordInvocation("GenerateForHost", []interface{}{arg1}) + fake.generateForHostMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenerator) GenerateForHostCallCount() int { + fake.generateForHostMutex.RLock() + defer fake.generateForHostMutex.RUnlock() + return len(fake.generateForHostArgsForCall) +} + +func (fake *FakeGenerator) GenerateForHostCalls(stub func(state.Host) []byte) { + fake.generateForHostMutex.Lock() + defer fake.generateForHostMutex.Unlock() + fake.GenerateForHostStub = stub +} + +func (fake *FakeGenerator) GenerateForHostArgsForCall(i int) state.Host { + fake.generateForHostMutex.RLock() + defer fake.generateForHostMutex.RUnlock() + argsForCall := fake.generateForHostArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeGenerator) GenerateForHostReturns(result1 []byte) { + fake.generateForHostMutex.Lock() + defer fake.generateForHostMutex.Unlock() + fake.GenerateForHostStub = nil + fake.generateForHostReturns = struct { + result1 []byte + }{result1} +} + +func (fake *FakeGenerator) GenerateForHostReturnsOnCall(i int, result1 []byte) { + fake.generateForHostMutex.Lock() + defer fake.generateForHostMutex.Unlock() + fake.GenerateForHostStub = nil + if fake.generateForHostReturnsOnCall == nil { + fake.generateForHostReturnsOnCall = make(map[int]struct { + result1 []byte + }) + } + fake.generateForHostReturnsOnCall[i] = struct { + result1 []byte + }{result1} +} + +func (fake *FakeGenerator) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.generateForHostMutex.RLock() + defer fake.generateForHostMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeGenerator) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ config.Generator = new(FakeGenerator) diff --git a/internal/nginx/config/generator.go b/internal/nginx/config/generator.go new file mode 100644 index 0000000000..d57d724a9d --- /dev/null +++ b/internal/nginx/config/generator.go @@ -0,0 +1,98 @@ +package config + +import ( + "fmt" + + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +// nginx502Server is used as a backend for services that cannot be resolved (have no IP address). +const nginx502Server = "unix:/var/lib/nginx/nginx-502-server.sock" + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Generator + +// Generator generates NGINX configuration. +type Generator interface { + // GenerateForHost generates configuration for a host. + GenerateForHost(host state.Host) []byte +} + +// GeneratorImpl is an implementation of Generator +type GeneratorImpl struct { + executor *templateExecutor + serviceStore state.ServiceStore +} + +// NewGeneratorImpl creates a new GeneratorImpl. +func NewGeneratorImpl(serviceStore state.ServiceStore) *GeneratorImpl { + return &GeneratorImpl{ + executor: newTemplateExecutor(), + serviceStore: serviceStore, + } +} + +func (g *GeneratorImpl) GenerateForHost(host state.Host) []byte { + server := generate(host, g.serviceStore) + return g.executor.ExecuteForServer(server) +} + +func generate(host state.Host, serviceStore state.ServiceStore) server { + var locs []location + + for _, g := range host.PathRouteGroups { + // number of routes in a group is always at least 1 + // otherwise, it is a bug in the state.Configuration code, so it is OK to panic here + r := g.Routes[0] // TO-DO: for now, we only handle the first route in case there are multiple routes + address := getBackendAddress(r.Source.Spec.Rules[r.RuleIdx].BackendRefs, r.Source.Namespace, serviceStore) + + loc := location{ + Path: g.Path, + ProxyPass: generateProxyPass(address), + } + locs = append(locs, loc) + } + + return server{ + ServerName: host.Value, + Locations: locs, + } +} + +func generateProxyPass(address string) string { + if address == "" { + return "http://" + nginx502Server + } + return "http://" + address +} + +func getBackendAddress(refs []v1alpha2.HTTPBackendRef, parentNS string, serviceStore state.ServiceStore) string { + // TO-DO: make sure the warnings are generated and reported to the user fot the edge cases + if len(refs) == 0 { + return "" + } + + // TO-DO: for now, we only support a single backend reference + ref := refs[0].BackendRef + + if ref.Kind != nil && *ref.Kind != "Service" { + return "" + } + + ns := parentNS + if ref.Namespace != nil { + ns = string(*ref.Namespace) + } + + address, err := serviceStore.Resolve(types.NamespacedName{Namespace: ns, Name: string(ref.Name)}) + if err != nil { + return "" + } + + if ref.Port == nil { + return "" + } + + return fmt.Sprintf("%s:%d", address, *ref.Port) +} diff --git a/internal/nginx/config/generator_test.go b/internal/nginx/config/generator_test.go new file mode 100644 index 0000000000..de9a363620 --- /dev/null +++ b/internal/nginx/config/generator_test.go @@ -0,0 +1,267 @@ +package config + +import ( + "errors" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/helpers" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state/statefakes" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +func TestGenerateForHost(t *testing.T) { + generator := NewGeneratorImpl(&statefakes.FakeServiceStore{}) + + host := state.Host{Value: "example.com"} + + cfg := generator.GenerateForHost(host) + + if len(cfg) == 0 { + t.Errorf("GenerateForHost() generated empty config") + } +} + +func TestGenerate(t *testing.T) { + hr := &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + Spec: v1alpha2.HTTPRouteSpec{ + Hostnames: []v1alpha2.Hostname{ + "cafe.example.com", + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + Matches: []v1alpha2.HTTPRouteMatch{ + { + Path: &v1alpha2.HTTPPathMatch{ + Value: helpers.GetStringPointer("/"), + }, + }, + }, + BackendRefs: []v1alpha2.HTTPBackendRef{ + { + BackendRef: v1alpha2.BackendRef{ + BackendObjectReference: v1alpha2.BackendObjectReference{ + Name: "service1", + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Port: (*v1alpha2.PortNumber)(helpers.GetInt32Pointer(80)), + }, + }, + }, + }, + }, + }, + }, + } + + host := state.Host{ + Value: "example.com", + PathRouteGroups: []state.PathRouteGroup{ + { + Path: "/", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr, + }, + }, + }, + }, + } + + fakeServiceStore := &statefakes.FakeServiceStore{} + fakeServiceStore.ResolveReturns("10.0.0.1", nil) + + expected := server{ + ServerName: "example.com", + Locations: []location{ + { + Path: "/", + ProxyPass: "http://10.0.0.1:80", + }, + }, + } + + result := generate(host, fakeServiceStore) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generate() mismatch (-want +got):\n%s", diff) + } +} + +func TestGenerateProxyPass(t *testing.T) { + expected := "http://10.0.0.1:80" + + result := generateProxyPass("10.0.0.1:80") + if result != expected { + t.Errorf("generateProxyPass() returned %s but expected %s", result, expected) + } + + expected = "http://" + nginx502Server + + result = generateProxyPass("") + if result != expected { + t.Errorf("generateProxyPass() returned %s but expected %s", result, expected) + } +} + +func TestGetBackendAddress(t *testing.T) { + getNormalRefs := func() []v1alpha2.HTTPBackendRef { + return []v1alpha2.HTTPBackendRef{ + { + BackendRef: v1alpha2.BackendRef{ + BackendObjectReference: v1alpha2.BackendObjectReference{ + Group: (*v1alpha2.Group)(helpers.GetStringPointer("networking.k8s.io")), + Kind: (*v1alpha2.Kind)(helpers.GetStringPointer("Service")), + Name: "service1", + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Port: (*v1alpha2.PortNumber)(helpers.GetInt32Pointer(80)), + }, + }, + }, + } + } + + getModifiedRefs := func(mod func([]v1alpha2.HTTPBackendRef) []v1alpha2.HTTPBackendRef) []v1alpha2.HTTPBackendRef { + return mod(getNormalRefs()) + } + + tests := []struct { + refs []v1alpha2.HTTPBackendRef + parentNS string + storeAddress string + storeErr error + expectedResolverCallCount int + expectedNsName types.NamespacedName + expectedAddress string + msg string + }{ + { + refs: getNormalRefs(), + parentNS: "test", + storeAddress: "10.0.0.1", + storeErr: nil, + expectedResolverCallCount: 1, + expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, + expectedAddress: "10.0.0.1:80", + msg: "normal case", + }, + { + refs: getModifiedRefs(func(refs []v1alpha2.HTTPBackendRef) []v1alpha2.HTTPBackendRef { + refs[0].BackendRef.Namespace = nil + return refs + }), + parentNS: "test", + storeAddress: "10.0.0.1", + storeErr: nil, + expectedResolverCallCount: 1, + expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, + expectedAddress: "10.0.0.1:80", + msg: "normal case with implicit namespace", + }, + { + refs: getModifiedRefs(func(refs []v1alpha2.HTTPBackendRef) []v1alpha2.HTTPBackendRef { + refs[0].BackendRef.Group = nil + refs[0].BackendRef.Kind = nil + return refs + }), + parentNS: "test", + storeAddress: "10.0.0.1", + storeErr: nil, + expectedResolverCallCount: 1, + expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, + expectedAddress: "10.0.0.1:80", + msg: "normal case with implicit service", + }, + { + refs: getModifiedRefs(func(refs []v1alpha2.HTTPBackendRef) []v1alpha2.HTTPBackendRef { + secondRef := refs[0].DeepCopy() + secondRef.Name = "service2" + return append(refs, *secondRef) + }), + parentNS: "test", + storeAddress: "10.0.0.1", + storeErr: nil, + expectedResolverCallCount: 1, + expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, + expectedAddress: "10.0.0.1:80", + msg: "first backend ref is used", + }, + { + refs: getModifiedRefs(func(refs []v1alpha2.HTTPBackendRef) []v1alpha2.HTTPBackendRef { + refs[0].BackendRef.Kind = (*v1alpha2.Kind)(helpers.GetStringPointer("NotService")) + return refs + }), + parentNS: "test", + storeAddress: "10.0.0.1", + storeErr: nil, + expectedResolverCallCount: 0, + expectedNsName: types.NamespacedName{}, + expectedAddress: "", + msg: "not a service Kind", + }, + { + refs: nil, + parentNS: "test", + storeAddress: "10.0.0.1", + storeErr: nil, + expectedResolverCallCount: 0, + expectedNsName: types.NamespacedName{}, + expectedAddress: "", + msg: "no refs", + }, + { + refs: getModifiedRefs(func(refs []v1alpha2.HTTPBackendRef) []v1alpha2.HTTPBackendRef { + refs[0].BackendRef.Port = nil + return refs + }), + parentNS: "test", + storeAddress: "10.0.0.1", + storeErr: nil, + expectedResolverCallCount: 1, + expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, + expectedAddress: "", + msg: "no port", + }, + { + refs: getNormalRefs(), + parentNS: "test", + storeAddress: "", + storeErr: errors.New(""), + expectedResolverCallCount: 1, + expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, + expectedAddress: "", + msg: "service doesn't exist", + }, + } + + for _, test := range tests { + fakeServiceStore := &statefakes.FakeServiceStore{} + fakeServiceStore.ResolveReturns(test.storeAddress, test.storeErr) + + result := getBackendAddress(test.refs, test.parentNS, fakeServiceStore) + if result != test.expectedAddress { + t.Errorf("getBackendAddress() returned %s but expected %s for case %s", result, test.expectedAddress, test.msg) + } + + callCount := fakeServiceStore.ResolveCallCount() + if callCount != test.expectedResolverCallCount { + t.Errorf("getBackendAddress() called fakeServiceStore.Resolve %d times but expected %d for case %s", callCount, test.expectedResolverCallCount, test.msg) + } + + if test.expectedResolverCallCount == 0 { + continue + } + + nsname := fakeServiceStore.ResolveArgsForCall(0) + if nsname != test.expectedNsName { + t.Errorf("getBackendAddress() called fakeServiceStore.Resolve with %v but expected %v for case %s", nsname, test.expectedNsName, test.msg) + } + } +} diff --git a/internal/nginx/config/http.go b/internal/nginx/config/http.go new file mode 100644 index 0000000000..0e8b7c96a5 --- /dev/null +++ b/internal/nginx/config/http.go @@ -0,0 +1,11 @@ +package config + +type server struct { + ServerName string + Locations []location +} + +type location struct { + Path string + ProxyPass string +} diff --git a/internal/nginx/config/template.go b/internal/nginx/config/template.go new file mode 100644 index 0000000000..3b805b25b4 --- /dev/null +++ b/internal/nginx/config/template.go @@ -0,0 +1,47 @@ +package config + +import ( + "bytes" + "fmt" + "text/template" +) + +var serverTemplate = `server { + server_name {{ .ServerName }}; + + {{ range $l := .Locations }} + location {{ $l.Path }} { + proxy_set_header Host $host; + proxy_pass {{ $l.ProxyPass }}; + } + {{ end }} +} +` + +// templateExecutor generates NGINX configuration using a template. +// Template parsing or executing errors can only occur if there is a bug in the template, so they are handled with panics. +// TO-DO: for now, we only generate configuration with NGINX server. +// We will also need to generate the main NGINX configuration file, upstreams. +type templateExecutor struct { + serverTemplate *template.Template +} + +func newTemplateExecutor() *templateExecutor { + t, err := template.New("server").Parse(serverTemplate) + if err != nil { + panic(fmt.Errorf("failed to parse server template: %w", err)) + } + + return &templateExecutor{serverTemplate: t} +} + +func (e *templateExecutor) ExecuteForServer(s server) []byte { + var buf bytes.Buffer + + err := e.serverTemplate.Execute(&buf, s) + if err != nil { + panic(fmt.Errorf("failed to execute server template: %w", err)) + } + + return buf.Bytes() +} diff --git a/internal/nginx/config/template_test.go b/internal/nginx/config/template_test.go new file mode 100644 index 0000000000..bb03edad9e --- /dev/null +++ b/internal/nginx/config/template_test.go @@ -0,0 +1,57 @@ +package config + +import ( + "testing" + "text/template" +) + +func TestExecuteForServer(t *testing.T) { + executor := newTemplateExecutor() + + server := server{ + ServerName: "example.com", + Locations: []location{ + { + Path: "/", + ProxyPass: "http://10.0.0.1", + }, + }, + } + + cfg := executor.ExecuteForServer(server) + // we only do a sanity check here. + // the config generation logic is tested in the Generator tests. + if len(cfg) == 0 { + t.Error("ExecuteForServer() returned 0-length config") + } +} + +func TestNewTemplateExecutorPanics(t *testing.T) { + defer func() { + r := recover() + if r == nil { + t.Error("newTemplateExecutor() didn't panic") + } + }() + + serverTemplate = "{{ end }}" // invalid template + newTemplateExecutor() +} + +func TestExecuteForServerPanics(t *testing.T) { + defer func() { + r := recover() + if r == nil { + t.Error("ExecuteForServer() didn't panic") + } + }() + + tmpl, err := template.New("test").Parse("{{ .NonExistingField }}") + if err != nil { + t.Fatalf("Failed to parse template: %v", err) + } + + executor := &templateExecutor{serverTemplate: tmpl} + + _ = executor.ExecuteForServer(server{}) +} diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go index b37ac40596..0c58240cdc 100644 --- a/internal/state/configuration_test.go +++ b/internal/state/configuration_test.go @@ -46,7 +46,7 @@ var _ = Describe("Configuration", func() { Matches: []v1alpha2.HTTPRouteMatch{ { Path: &v1alpha2.HTTPPathMatch{ - Value: getStringPointer("/coffee"), + Value: helpers.GetStringPointer("/coffee"), }, }, }, @@ -56,7 +56,7 @@ var _ = Describe("Configuration", func() { } updatedHRWithSameGen = hr.DeepCopy() - updatedHRWithSameGen.Spec.Rules[1].Matches[0].Path.Value = getStringPointer("/tea") + updatedHRWithSameGen.Spec.Rules[1].Matches[0].Path.Value = helpers.GetStringPointer("/tea") updatedHRWithIncrementedGen = updatedHRWithSameGen.DeepCopy() updatedHRWithIncrementedGen.Generation++ @@ -275,7 +275,7 @@ var _ = Describe("Configuration", func() { Matches: []v1alpha2.HTTPRouteMatch{ { Path: &v1alpha2.HTTPPathMatch{ - Value: getStringPointer("/coffee"), + Value: helpers.GetStringPointer("/coffee"), }, }, }, @@ -285,7 +285,7 @@ var _ = Describe("Configuration", func() { } hr2Updated = hr2.DeepCopy() - hr2Updated.Spec.Rules[0].Matches[0].Path.Value = getStringPointer("/tea") + hr2Updated.Spec.Rules[0].Matches[0].Path.Value = helpers.GetStringPointer("/tea") hr2Updated.Generation++ }) @@ -643,7 +643,7 @@ var _ = Describe("Configuration", func() { Matches: []v1alpha2.HTTPRouteMatch{ { Path: &v1alpha2.HTTPPathMatch{ - Value: getStringPointer("/"), + Value: helpers.GetStringPointer("/"), }, }, }, @@ -881,7 +881,3 @@ var _ = Describe("Configuration", func() { }) }) }) - -func getStringPointer(s string) *string { - return &s -} From 932b7786c502f8af4df5b1615315a93713ec5acf Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 23 Mar 2022 15:45:32 -0700 Subject: [PATCH 2/6] Split long param list into multiple lines --- internal/events/loop.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/events/loop.go b/internal/events/loop.go index 4b6ca08fc2..9b157f4cec 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -23,8 +23,14 @@ type EventLoop struct { } // NewEventLoop creates a new EventLoop. -func NewEventLoop(conf state.Configuration, serviceStore state.ServiceStore, generator config.Generator, - eventCh <-chan interface{}, statusUpdater status.Updater, logger logr.Logger) *EventLoop { +func NewEventLoop( + conf state.Configuration, + serviceStore state.ServiceStore, + generator config.Generator, + eventCh <-chan interface{}, + statusUpdater status.Updater, + logger logr.Logger, +) *EventLoop { return &EventLoop{ conf: conf, serviceStore: serviceStore, From 03a2374accdd517b505b30400f82676319eaa248 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 23 Mar 2022 15:50:26 -0700 Subject: [PATCH 3/6] Expect right away instead of unnecessary eventually --- internal/events/loop_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index 5c0c99e325..0b2b2d2129 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -101,9 +101,7 @@ var _ = Describe("EventLoop", func() { }).Should(Equal(fakeStatusUpdates)) Eventually(fakeGenerator.GenerateForHostCallCount).Should(Equal(1)) - Eventually(func() state.Host { - return fakeGenerator.GenerateForHostArgsForCall(0) - }).Should(Equal(fakeChanges[0].Host)) + Expect(fakeGenerator.GenerateForHostArgsForCall(0)).Should(Equal(fakeChanges[0].Host)) }) It("should process delete event", func() { From 941f783cf087156340942a713361bf481a7358df Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 23 Mar 2022 15:52:54 -0700 Subject: [PATCH 4/6] Help the case to stand out --- internal/nginx/config/generator_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/nginx/config/generator_test.go b/internal/nginx/config/generator_test.go index de9a363620..dc74ea3110 100644 --- a/internal/nginx/config/generator_test.go +++ b/internal/nginx/config/generator_test.go @@ -247,12 +247,12 @@ func TestGetBackendAddress(t *testing.T) { result := getBackendAddress(test.refs, test.parentNS, fakeServiceStore) if result != test.expectedAddress { - t.Errorf("getBackendAddress() returned %s but expected %s for case %s", result, test.expectedAddress, test.msg) + t.Errorf("getBackendAddress() returned %s but expected %s for case %q", result, test.expectedAddress, test.msg) } callCount := fakeServiceStore.ResolveCallCount() if callCount != test.expectedResolverCallCount { - t.Errorf("getBackendAddress() called fakeServiceStore.Resolve %d times but expected %d for case %s", callCount, test.expectedResolverCallCount, test.msg) + t.Errorf("getBackendAddress() called fakeServiceStore.Resolve %d times but expected %d for case %q", callCount, test.expectedResolverCallCount, test.msg) } if test.expectedResolverCallCount == 0 { @@ -261,7 +261,7 @@ func TestGetBackendAddress(t *testing.T) { nsname := fakeServiceStore.ResolveArgsForCall(0) if nsname != test.expectedNsName { - t.Errorf("getBackendAddress() called fakeServiceStore.Resolve with %v but expected %v for case %s", nsname, test.expectedNsName, test.msg) + t.Errorf("getBackendAddress() called fakeServiceStore.Resolve with %v but expected %v for case %q", nsname, test.expectedNsName, test.msg) } } } From a262b5080b98025c5c51db0ce0fa39381f673306 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 23 Mar 2022 16:14:21 -0700 Subject: [PATCH 5/6] Optimize slice initialization --- internal/nginx/config/generator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/nginx/config/generator.go b/internal/nginx/config/generator.go index d57d724a9d..36024a4c3c 100644 --- a/internal/nginx/config/generator.go +++ b/internal/nginx/config/generator.go @@ -39,7 +39,7 @@ func (g *GeneratorImpl) GenerateForHost(host state.Host) []byte { } func generate(host state.Host, serviceStore state.ServiceStore) server { - var locs []location + locs := make([]location, 0, len(host.PathRouteGroups)) // TO-DO: expand with g.Routes for _, g := range host.PathRouteGroups { // number of routes in a group is always at least 1 From 13642af26cbba6a29d0a27b50ac133b943cc2ec1 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 23 Mar 2022 17:49:52 -0700 Subject: [PATCH 6/6] Report warnings --- internal/events/loop.go | 13 +++- .../config/configfakes/fake_generator.go | 23 +++++--- internal/nginx/config/generator.go | 32 ++++++---- internal/nginx/config/generator_test.go | 59 ++++++++++++++++++- internal/nginx/config/warnings.go | 24 ++++++++ internal/nginx/config/warnings_test.go | 46 +++++++++++++++ 6 files changed, 171 insertions(+), 26 deletions(-) create mode 100644 internal/nginx/config/warnings.go create mode 100644 internal/nginx/config/warnings_test.go diff --git a/internal/events/loop.go b/internal/events/loop.go index 9b157f4cec..47a3e9e1de 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -122,10 +122,21 @@ func (el *EventLoop) processChangesAndStatusUpdates(ctx context.Context, changes fmt.Printf("%+v\n", c) if c.Op == state.Upsert { - cfg := el.generator.GenerateForHost(c.Host) + cfg, warnings := el.generator.GenerateForHost(c.Host) // TO-DO: for now, we only print the generated config, without writing it on the file system // and reloading NGINX. fmt.Println(string(cfg)) + + for obj, objWarnings := range warnings { + for _, w := range objWarnings { + // TO-DO: report warnings via Object status + el.logger.Info("got warning while generating config", + "kind", obj.GetObjectKind().GroupVersionKind().Kind, + "namespace", obj.GetNamespace(), + "name", obj.GetName(), + "warning", w) + } + } } } diff --git a/internal/nginx/config/configfakes/fake_generator.go b/internal/nginx/config/configfakes/fake_generator.go index 7d3faa0f71..ff7a05b688 100644 --- a/internal/nginx/config/configfakes/fake_generator.go +++ b/internal/nginx/config/configfakes/fake_generator.go @@ -9,22 +9,24 @@ import ( ) type FakeGenerator struct { - GenerateForHostStub func(state.Host) []byte + GenerateForHostStub func(state.Host) ([]byte, config.Warnings) generateForHostMutex sync.RWMutex generateForHostArgsForCall []struct { arg1 state.Host } generateForHostReturns struct { result1 []byte + result2 config.Warnings } generateForHostReturnsOnCall map[int]struct { result1 []byte + result2 config.Warnings } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } -func (fake *FakeGenerator) GenerateForHost(arg1 state.Host) []byte { +func (fake *FakeGenerator) GenerateForHost(arg1 state.Host) ([]byte, config.Warnings) { fake.generateForHostMutex.Lock() ret, specificReturn := fake.generateForHostReturnsOnCall[len(fake.generateForHostArgsForCall)] fake.generateForHostArgsForCall = append(fake.generateForHostArgsForCall, struct { @@ -38,9 +40,9 @@ func (fake *FakeGenerator) GenerateForHost(arg1 state.Host) []byte { return stub(arg1) } if specificReturn { - return ret.result1 + return ret.result1, ret.result2 } - return fakeReturns.result1 + return fakeReturns.result1, fakeReturns.result2 } func (fake *FakeGenerator) GenerateForHostCallCount() int { @@ -49,7 +51,7 @@ func (fake *FakeGenerator) GenerateForHostCallCount() int { return len(fake.generateForHostArgsForCall) } -func (fake *FakeGenerator) GenerateForHostCalls(stub func(state.Host) []byte) { +func (fake *FakeGenerator) GenerateForHostCalls(stub func(state.Host) ([]byte, config.Warnings)) { fake.generateForHostMutex.Lock() defer fake.generateForHostMutex.Unlock() fake.GenerateForHostStub = stub @@ -62,27 +64,30 @@ func (fake *FakeGenerator) GenerateForHostArgsForCall(i int) state.Host { return argsForCall.arg1 } -func (fake *FakeGenerator) GenerateForHostReturns(result1 []byte) { +func (fake *FakeGenerator) GenerateForHostReturns(result1 []byte, result2 config.Warnings) { fake.generateForHostMutex.Lock() defer fake.generateForHostMutex.Unlock() fake.GenerateForHostStub = nil fake.generateForHostReturns = struct { result1 []byte - }{result1} + result2 config.Warnings + }{result1, result2} } -func (fake *FakeGenerator) GenerateForHostReturnsOnCall(i int, result1 []byte) { +func (fake *FakeGenerator) GenerateForHostReturnsOnCall(i int, result1 []byte, result2 config.Warnings) { fake.generateForHostMutex.Lock() defer fake.generateForHostMutex.Unlock() fake.GenerateForHostStub = nil if fake.generateForHostReturnsOnCall == nil { fake.generateForHostReturnsOnCall = make(map[int]struct { result1 []byte + result2 config.Warnings }) } fake.generateForHostReturnsOnCall[i] = struct { result1 []byte - }{result1} + result2 config.Warnings + }{result1, result2} } func (fake *FakeGenerator) Invocations() map[string][][]interface{} { diff --git a/internal/nginx/config/generator.go b/internal/nginx/config/generator.go index 36024a4c3c..c103a2ea34 100644 --- a/internal/nginx/config/generator.go +++ b/internal/nginx/config/generator.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" @@ -16,7 +17,7 @@ const nginx502Server = "unix:/var/lib/nginx/nginx-502-server.sock" // Generator generates NGINX configuration. type Generator interface { // GenerateForHost generates configuration for a host. - GenerateForHost(host state.Host) []byte + GenerateForHost(host state.Host) ([]byte, Warnings) } // GeneratorImpl is an implementation of Generator @@ -33,19 +34,24 @@ func NewGeneratorImpl(serviceStore state.ServiceStore) *GeneratorImpl { } } -func (g *GeneratorImpl) GenerateForHost(host state.Host) []byte { - server := generate(host, g.serviceStore) - return g.executor.ExecuteForServer(server) +func (g *GeneratorImpl) GenerateForHost(host state.Host) ([]byte, Warnings) { + server, warnings := generate(host, g.serviceStore) + return g.executor.ExecuteForServer(server), warnings } -func generate(host state.Host, serviceStore state.ServiceStore) server { +func generate(host state.Host, serviceStore state.ServiceStore) (server, Warnings) { + warnings := newWarnings() + locs := make([]location, 0, len(host.PathRouteGroups)) // TO-DO: expand with g.Routes for _, g := range host.PathRouteGroups { // number of routes in a group is always at least 1 // otherwise, it is a bug in the state.Configuration code, so it is OK to panic here r := g.Routes[0] // TO-DO: for now, we only handle the first route in case there are multiple routes - address := getBackendAddress(r.Source.Spec.Rules[r.RuleIdx].BackendRefs, r.Source.Namespace, serviceStore) + address, err := getBackendAddress(r.Source.Spec.Rules[r.RuleIdx].BackendRefs, r.Source.Namespace, serviceStore) + if err != nil { + warnings.AddWarning(r.Source, err.Error()) + } loc := location{ Path: g.Path, @@ -57,7 +63,7 @@ func generate(host state.Host, serviceStore state.ServiceStore) server { return server{ ServerName: host.Value, Locations: locs, - } + }, warnings } func generateProxyPass(address string) string { @@ -67,17 +73,17 @@ func generateProxyPass(address string) string { return "http://" + address } -func getBackendAddress(refs []v1alpha2.HTTPBackendRef, parentNS string, serviceStore state.ServiceStore) string { +func getBackendAddress(refs []v1alpha2.HTTPBackendRef, parentNS string, serviceStore state.ServiceStore) (string, error) { // TO-DO: make sure the warnings are generated and reported to the user fot the edge cases if len(refs) == 0 { - return "" + return "", errors.New("empty backend refs") } // TO-DO: for now, we only support a single backend reference ref := refs[0].BackendRef if ref.Kind != nil && *ref.Kind != "Service" { - return "" + return "", fmt.Errorf("unsupported kind %s", *ref.Kind) } ns := parentNS @@ -87,12 +93,12 @@ func getBackendAddress(refs []v1alpha2.HTTPBackendRef, parentNS string, serviceS address, err := serviceStore.Resolve(types.NamespacedName{Namespace: ns, Name: string(ref.Name)}) if err != nil { - return "" + return "", fmt.Errorf("service %s/%s cannot be resolved: %w", ns, ref.Name, err) } if ref.Port == nil { - return "" + return "", errors.New("port is nil") } - return fmt.Sprintf("%s:%d", address, *ref.Port) + return fmt.Sprintf("%s:%d", address, *ref.Port), nil } diff --git a/internal/nginx/config/generator_test.go b/internal/nginx/config/generator_test.go index dc74ea3110..ca959660fc 100644 --- a/internal/nginx/config/generator_test.go +++ b/internal/nginx/config/generator_test.go @@ -18,11 +18,14 @@ func TestGenerateForHost(t *testing.T) { host := state.Host{Value: "example.com"} - cfg := generator.GenerateForHost(host) + cfg, warnings := generator.GenerateForHost(host) if len(cfg) == 0 { t.Errorf("GenerateForHost() generated empty config") } + if len(warnings) > 0 { + t.Errorf("GenerateForHost() returned unexpected warnings: %v", warnings) + } } func TestGenerate(t *testing.T) { @@ -56,6 +59,16 @@ func TestGenerate(t *testing.T) { }, }, }, + { + Matches: []v1alpha2.HTTPRouteMatch{ + { + Path: &v1alpha2.HTTPPathMatch{ + Value: helpers.GetStringPointer("/test"), + }, + }, + }, + BackendRefs: nil, // no backend refs will cause warnings + }, }, }, } @@ -73,6 +86,16 @@ func TestGenerate(t *testing.T) { }, }, }, + { + Path: "/test", + Routes: []state.Route{ + { + MatchIdx: 0, + RuleIdx: 1, + Source: hr, + }, + }, + }, }, } @@ -86,13 +109,24 @@ func TestGenerate(t *testing.T) { Path: "/", ProxyPass: "http://10.0.0.1:80", }, + { + Path: "/test", + ProxyPass: "http://" + nginx502Server, + }, }, } + expectedWarnings := Warnings{ + hr: []string{"empty backend refs"}, + } + + result, warnings := generate(host, fakeServiceStore) - result := generate(host, fakeServiceStore) if diff := cmp.Diff(expected, result); diff != "" { t.Errorf("generate() mismatch (-want +got):\n%s", diff) } + if diff := cmp.Diff(expectedWarnings, warnings); diff != "" { + t.Errorf("generate() mismatch on warnings (-want +got):\n%s", diff) + } } func TestGenerateProxyPass(t *testing.T) { @@ -140,6 +174,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount int expectedNsName types.NamespacedName expectedAddress string + expectErr bool msg string }{ { @@ -150,6 +185,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 1, expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, expectedAddress: "10.0.0.1:80", + expectErr: false, msg: "normal case", }, { @@ -163,6 +199,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 1, expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, expectedAddress: "10.0.0.1:80", + expectErr: false, msg: "normal case with implicit namespace", }, { @@ -177,6 +214,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 1, expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, expectedAddress: "10.0.0.1:80", + expectErr: false, msg: "normal case with implicit service", }, { @@ -191,6 +229,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 1, expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, expectedAddress: "10.0.0.1:80", + expectErr: false, msg: "first backend ref is used", }, { @@ -204,6 +243,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 0, expectedNsName: types.NamespacedName{}, expectedAddress: "", + expectErr: true, msg: "not a service Kind", }, { @@ -214,6 +254,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 0, expectedNsName: types.NamespacedName{}, expectedAddress: "", + expectErr: true, msg: "no refs", }, { @@ -227,6 +268,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 1, expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, expectedAddress: "", + expectErr: true, msg: "no port", }, { @@ -237,6 +279,7 @@ func TestGetBackendAddress(t *testing.T) { expectedResolverCallCount: 1, expectedNsName: types.NamespacedName{Namespace: "test", Name: "service1"}, expectedAddress: "", + expectErr: true, msg: "service doesn't exist", }, } @@ -245,11 +288,21 @@ func TestGetBackendAddress(t *testing.T) { fakeServiceStore := &statefakes.FakeServiceStore{} fakeServiceStore.ResolveReturns(test.storeAddress, test.storeErr) - result := getBackendAddress(test.refs, test.parentNS, fakeServiceStore) + result, err := getBackendAddress(test.refs, test.parentNS, fakeServiceStore) if result != test.expectedAddress { t.Errorf("getBackendAddress() returned %s but expected %s for case %q", result, test.expectedAddress, test.msg) } + if test.expectErr { + if err == nil { + t.Errorf("getBackendAddress() didn't return any error for case %q", test.msg) + } + } else { + if err != nil { + t.Errorf("getBackendAddress() returned unexpected error %v for case %q", err, test.msg) + } + } + callCount := fakeServiceStore.ResolveCallCount() if callCount != test.expectedResolverCallCount { t.Errorf("getBackendAddress() called fakeServiceStore.Resolve %d times but expected %d for case %q", callCount, test.expectedResolverCallCount, test.msg) diff --git a/internal/nginx/config/warnings.go b/internal/nginx/config/warnings.go new file mode 100644 index 0000000000..a1925cd865 --- /dev/null +++ b/internal/nginx/config/warnings.go @@ -0,0 +1,24 @@ +package config + +import ( + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Warnings stores a list of warnings for a given object. +type Warnings map[client.Object][]string + +func newWarnings() Warnings { + return make(map[client.Object][]string) +} + +// AddWarningf adds a warning for the specified object using the provided format and arguments. +func (w Warnings) AddWarningf(obj client.Object, msgFmt string, args ...interface{}) { + w[obj] = append(w[obj], fmt.Sprintf(msgFmt, args...)) +} + +// AddWarning adds a warning for the specified object. +func (w Warnings) AddWarning(obj client.Object, msg string) { + w[obj] = append(w[obj], msg) +} diff --git a/internal/nginx/config/warnings_test.go b/internal/nginx/config/warnings_test.go new file mode 100644 index 0000000000..2d20a61a73 --- /dev/null +++ b/internal/nginx/config/warnings_test.go @@ -0,0 +1,46 @@ +package config + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +func TestAddWarningf(t *testing.T) { + warnings := newWarnings() + obj := &v1alpha2.HTTPRoute{} + + expected := Warnings{ + obj: []string{ + "simple", + "advanced 1", + }, + } + + warnings.AddWarningf(obj, "simple") + warnings.AddWarningf(obj, "advanced %d", 1) + + if diff := cmp.Diff(expected, warnings); diff != "" { + t.Errorf("AddWarningf mismatch (-want +got):\n%s", diff) + } +} + +func TestAddWarning(t *testing.T) { + warnings := newWarnings() + obj := &v1alpha2.HTTPRoute{} + + expected := Warnings{ + obj: []string{ + "first", + "second", + }, + } + + warnings.AddWarning(obj, "first") + warnings.AddWarning(obj, "second") + + if diff := cmp.Diff(expected, warnings); diff != "" { + t.Errorf("AddWarning mismatch (-want +got):\n%s", diff) + } +}