Add unit tests; remove logger

Author: sjberman

internal/mode/static/handler.go

@@ -2,6 +2,7 @@ package static
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 	"time"
@@ -183,9 +184,9 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 		h.setLatestConfiguration(&cfg)
 
 		if h.cfg.plus {
-			err = h.updateUpstreamServers(logger, cfg)
+			err = h.updateUpstreamServers(cfg)
 		} else {
-			err = h.updateNginxConf(ctx, logger, cfg)
+			err = h.updateNginxConf(ctx, cfg)
 		}
 	case state.ClusterStateChange:
 		h.version++
@@ -198,7 +199,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 
 		h.setLatestConfiguration(&cfg)
 
-		err = h.updateNginxConf(ctx, logger, cfg)
+		err = h.updateNginxConf(ctx, cfg)
 	}
 
 	var nginxReloadRes status.NginxReloadResult
@@ -305,7 +306,6 @@ func (h *eventHandlerImpl) parseAndCaptureEvent(ctx context.Context, logger logr
 // updateNginxConf updates nginx conf files and reloads nginx.
 func (h *eventHandlerImpl) updateNginxConf(
 	ctx context.Context,
-	logger logr.Logger,
 	conf dataplane.Configuration,
 ) error {
 	files := h.cfg.generator.Generate(conf)
@@ -318,7 +318,7 @@ func (h *eventHandlerImpl) updateNginxConf(
 	}
 
 	// If using NGINX Plus, update upstream servers using the API.
-	if err := h.updateUpstreamServers(logger, conf); err != nil {
+	if err := h.updateUpstreamServers(conf); err != nil {
 		return fmt.Errorf("failed to update upstream servers: %w", err)
 	}
 
@@ -327,7 +327,7 @@ func (h *eventHandlerImpl) updateNginxConf(
 
 // updateUpstreamServers determines which servers have changed and uses the NGINX Plus API to update them.
 // Only applicable when using NGINX Plus.
-func (h *eventHandlerImpl) updateUpstreamServers(logger logr.Logger, conf dataplane.Configuration) error {
+func (h *eventHandlerImpl) updateUpstreamServers(conf dataplane.Configuration) error {
 	if !h.cfg.plus {
 		return nil
 	}
@@ -375,19 +375,22 @@ func (h *eventHandlerImpl) updateUpstreamServers(logger logr.Logger, conf datapl
 		}
 	}
 
+	var updateErr error
 	for _, upstream := range upstreams {
 		if err := h.cfg.nginxRuntimeMgr.UpdateHTTPServers(upstream.name, upstream.servers); err != nil {
-			logger.Error(err, "couldn't update upstream via the API", "upstreamName", upstream.name)
+			updateErr = errors.Join(updateErr, fmt.Errorf(
+				"couldn't update upstream %q via the API: %w", upstream.name, err))
 		}
 	}
 
 	for _, upstream := range streamUpstreams {
 		if err := h.cfg.nginxRuntimeMgr.UpdateStreamServers(upstream.name, upstream.servers); err != nil {
-			logger.Error(err, "couldn't update stream upstream via the API", "upstreamName", upstream.name)
+			updateErr = errors.Join(updateErr, fmt.Errorf(
+				"couldn't update stream upstream %q via the API: %w", upstream.name, err))
 		}
 	}
 
-	return nil
+	return updateErr
 }
 
 // serversEqual accepts lists of either UpstreamServer/Peer or StreamUpstreamServer/StreamPeer and determines

internal/mode/static/handler_test.go

@@ -505,19 +505,29 @@ var _ = Describe("eventHandler", func() {
 			})
 
 			It("should update servers using the NGINX Plus API", func() {
-				Expect(handler.updateUpstreamServers(ctlrZap.New(), conf)).To(Succeed())
+				Expect(handler.updateUpstreamServers(conf)).To(Succeed())
 				Expect(fakeNginxRuntimeMgr.UpdateHTTPServersCallCount()).To(Equal(1))
 			})
 
 			It("should return error when GET API returns an error", func() {
 				fakeNginxRuntimeMgr.GetUpstreamsReturns(nil, nil, errors.New("error"))
-				Expect(handler.updateUpstreamServers(ctlrZap.New(), conf)).ToNot(Succeed())
+				Expect(handler.updateUpstreamServers(conf)).ToNot(Succeed())
+			})
+
+			It("should return error when UpdateHTTPServers API returns an error", func() {
+				fakeNginxRuntimeMgr.UpdateHTTPServersReturns(errors.New("error"))
+				Expect(handler.updateUpstreamServers(conf)).ToNot(Succeed())
+			})
+
+			It("should return error when UpdateStreamServers API returns an error", func() {
+				fakeNginxRuntimeMgr.UpdateStreamServersReturns(errors.New("error"))
+				Expect(handler.updateUpstreamServers(conf)).ToNot(Succeed())
 			})
 		})
 
 		When("not running NGINX Plus", func() {
 			It("should not do anything", func() {
-				Expect(handler.updateUpstreamServers(ctlrZap.New(), conf)).To(Succeed())
+				Expect(handler.updateUpstreamServers(conf)).To(Succeed())
 
 				Expect(fakeNginxRuntimeMgr.UpdateHTTPServersCallCount()).To(Equal(0))
 			})

internal/mode/static/nginx/runtime/manager_test.go

@@ -224,6 +224,17 @@ var _ = Describe("NGINX Runtime Manager", func() {
 			Expect(streamUpstreams).To(BeNil())
 		})
 
+		It("returns an error when GetUpstreams returns nil", func() {
+			ngxPlusClient.GetUpstreamsReturns(nil, nil)
+
+			upstreams, streamUpstreams, err := manager.GetUpstreams()
+
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError("GET upstreams returned nil value"))
+			Expect(upstreams).To(BeNil())
+			Expect(streamUpstreams).To(BeNil())
+		})
+
 		It("returns an error when GetStreamUpstreams fails", func() {
 			ngxPlusClient.GetUpstreamsReturns(&ngxclient.Upstreams{}, nil)
 			ngxPlusClient.GetStreamUpstreamsReturns(nil, errors.New("failed to get upstreams"))
@@ -235,6 +246,18 @@ var _ = Describe("NGINX Runtime Manager", func() {
 			Expect(upstreams).To(BeNil())
 			Expect(streamUpstreams).To(BeNil())
 		})
+
+		It("returns an error when GetStreamUpstreams returns nil", func() {
+			ngxPlusClient.GetUpstreamsReturns(&ngxclient.Upstreams{}, nil)
+			ngxPlusClient.GetStreamUpstreamsReturns(nil, nil)
+
+			upstreams, streamUpstreams, err := manager.GetUpstreams()
+
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(MatchError("GET stream upstreams returned nil value"))
+			Expect(upstreams).To(BeNil())
+			Expect(streamUpstreams).To(BeNil())
+		})
 	})
 
 	When("not running NGINX plus", func() {