From 3955897dc678a8040dd70b2c77c12377108b33a5 Mon Sep 17 00:00:00 2001 From: dean-coakley Date: Mon, 27 Aug 2018 16:25:28 +0100 Subject: [PATCH 1/9] Add stream upstream/server zone metrics support * Added support for stream upstream metrics * Added support for stream server zone metrics * Added stream server zone to nginx.conf for testing purposes * Extended GetStats to include stream metrics * Added integration test to validate that stream metrics are returned --- client/nginx.go | 108 +++++++++++++++++++++++++++++++++++++++---- docker/nginx.conf | 8 +++- tests/client_test.go | 69 +++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 11 deletions(-) diff --git a/client/nginx.go b/client/nginx.go index 25083b80..cdca133f 100644 --- a/client/nginx.go +++ b/client/nginx.go @@ -60,11 +60,13 @@ type apiError struct { // Stats represents NGINX Plus stats fetched from the NGINX Plus API. // https://nginx.org/en/docs/http/ngx_http_api_module.html type Stats struct { - Connections Connections - HTTPRequests HTTPRequests - SSL SSL - ServerZones ServerZones - Upstreams Upstreams + Connections Connections + HTTPRequests HTTPRequests + SSL SSL + ServerZones ServerZones + Upstreams Upstreams + StreamServerZones StreamServerZones + StreamUpstreams StreamUpstreams } // Connections represents connection related stats. @@ -101,6 +103,19 @@ type ServerZone struct { Sent uint64 } +// StreamServerZones is map of stream server zone stats by zone name. +type StreamServerZones map[string]StreamServerZone + +// StreamServerZone represents stream server zone related stats. +type StreamServerZone struct { + Processing uint64 + Connections uint64 + Sessions Sessions + Discarded uint64 + Received uint64 + Sent uint64 +} + // Responses represents HTTP reponse related stats. type Responses struct { Responses1xx uint64 `json:"1xx"` @@ -111,6 +126,14 @@ type Responses struct { Total uint64 } +// Sessions represents stream session related stats. +type Sessions struct { + Sessions2xx uint64 `json:"2xx"` + Sessions4xx uint64 `josn:"4xx"` + Sessions5xx uint64 `josn:"5xx"` + Total uint64 +} + // Upstreams is a map of upstream stats by upstream name. type Upstreams map[string]Upstream @@ -123,6 +146,16 @@ type Upstream struct { Queue Queue } +// StreamUpstreams is a map of stream upstream stats by upstream name. +type StreamUpstreams map[string]StreamUpstream + +// StreamUpstream represents stream upstream related stats. +type StreamUpstream struct { + Peers []StreamPeer + Zombies int + Zone string +} + // Queue represents queue related stats for an upstream. type Queue struct { Size int @@ -155,6 +188,31 @@ type Peer struct { ResponseTime uint64 `json:"response_time"` } +// StreamPeer represents peer (stream upstream server) related stats. +type StreamPeer struct { + ID int + Server string + Service string + Name string + Backup bool + Weight int + State string + Active uint64 + MaxConns int `json:"max_conns"` + Connections uint64 + ConnectTime int `json:"connect_time"` + FirstByteTime int `json:"first_byte_time"` + ResponseTime uint64 `json:"response_time"` + Sent uint64 + Received uint64 + Fails uint64 + Unavail uint64 + HealthChecks HealthChecks `json:"health_checks"` + Downtime uint64 + Downstart string + Selected string +} + // HealthChecks represents health check related stats for a peer. type HealthChecks struct { Checks uint64 @@ -594,17 +652,29 @@ func (client *NginxClient) GetStats() (*Stats, error) { return nil, fmt.Errorf("failed to get stats: %v", err) } + streamZones, err := client.getStreamServerZones() + if err != nil { + return nil, fmt.Errorf("failed to get stats: %v", err) + } + upstreams, err := client.getUpstreams() if err != nil { return nil, fmt.Errorf("failed to get stats: %v", err) } + streamUpstreams, err := client.getStreamUpstreams() + if err != nil { + return nil, fmt.Errorf("failed to get stats: %v", err) + } + return &Stats{ - Connections: *cons, - HTTPRequests: *requests, - SSL: *ssl, - ServerZones: *zones, - Upstreams: *upstreams, + Connections: *cons, + HTTPRequests: *requests, + SSL: *ssl, + ServerZones: *zones, + StreamServerZones: *streamZones, + Upstreams: *upstreams, + StreamUpstreams: *streamUpstreams, }, nil } @@ -646,6 +716,15 @@ func (client *NginxClient) getServerZones() (*ServerZones, error) { return &zones, err } +func (client *NginxClient) getStreamServerZones() (*StreamServerZones, error) { + var zones StreamServerZones + err := client.get("stream/server_zones", &zones) + if err != nil { + return nil, fmt.Errorf("failed to get server zones: %v", err) + } + return &zones, err +} + func (client *NginxClient) getUpstreams() (*Upstreams, error) { var upstreams Upstreams err := client.get("http/upstreams", &upstreams) @@ -654,3 +733,12 @@ func (client *NginxClient) getUpstreams() (*Upstreams, error) { } return &upstreams, nil } + +func (client *NginxClient) getStreamUpstreams() (*StreamUpstreams, error) { + var upstreams StreamUpstreams + err := client.get("stream/upstreams", &upstreams) + if err != nil { + return nil, fmt.Errorf("failed to get stream upstreams: %v", err) + } + return &upstreams, nil +} diff --git a/docker/nginx.conf b/docker/nginx.conf index 22f6d4a6..ef7f8358 100644 --- a/docker/nginx.conf +++ b/docker/nginx.conf @@ -33,7 +33,13 @@ http { stream { upstream stream_test { - zone stream 64k; + zone stream_test 64k; + } + + server { + listen 8081; + proxy_pass stream_test; + status_zone stream_test; } } diff --git a/tests/client_test.go b/tests/client_test.go index b415c663..cf86d2a6 100644 --- a/tests/client_test.go +++ b/tests/client_test.go @@ -453,6 +453,75 @@ func TestStats(t *testing.T) { } } +func TestStreamStats(t *testing.T) { + httpClient := &http.Client{} + c, err := client.NewNginxClient(httpClient, "http://127.0.0.1:8080/api") + if err != nil { + t.Fatalf("Error connecting to nginx: %v", err) + } + + // need upstream for stats + server := client.StreamUpstreamServer{ + Server: "127.0.0.1:8080", + } + err = c.AddStreamServer(streamUpstream, server) + if err != nil { + t.Errorf("Error adding stream upstream server: %v", err) + } + + // make request so we have stream server zone stats, expect 5xx error + streamClient := &http.Client{} + _, err = streamClient.Get("http://127.0.0.1:8081") + if err != nil { + t.Errorf("Error making request: %v", err) + } + + stats, err := c.GetStats() + if err != nil { + t.Errorf("Error getting stats: %v", err) + } + + if stats.Connections.Active == 0 { + t.Errorf("Bad connections: %v", stats.Connections) + } + // SSL metrics blank in this example + if len(stats.StreamServerZones) < 1 { + t.Errorf("No StreamServerZone metrics: %v", stats.StreamServerZones) + } + + if streamServerZone, ok := stats.StreamServerZones[streamUpstream]; ok { + if streamServerZone.Connections < 1 { + t.Errorf("StreamServerZone stats missing: %v", streamServerZone) + } + } else { + t.Errorf("StreamServerZone 'stream_test' not found") + } + + if upstream, ok := stats.StreamUpstreams[streamUpstream]; ok { + if len(upstream.Peers) < 1 { + t.Errorf("stream upstream server not visible in stats") + } else { + if upstream.Peers[0].State != "up" { + t.Errorf("stream upstream server state should be 'up'") + } + if upstream.Peers[0].Connections < 0 { + t.Errorf("stream upstream should have connects value") + } + if upstream.Peers[0].HealthChecks.LastPassed { + t.Errorf("stream upstream server health check should report last failed") + } + } + } else { + t.Errorf("Stream upstream 'stream_test' not found") + } + + // cleanup stream upstream servers + _, _, err = c.UpdateStreamServers(streamUpstream, []client.StreamUpstreamServer{}) + if err != nil { + t.Errorf("Couldn't remove stream servers: %v", err) + } +} + func compareUpstreamServers(x []client.UpstreamServer, y []client.UpstreamServer) bool { var xServers []string for _, us := range x { From 0c34234e2cb4fed986806ee087f94dd94b0e9040 Mon Sep 17 00:00:00 2001 From: dean-coakley Date: Mon, 27 Aug 2018 16:38:05 +0100 Subject: [PATCH 2/9] Fix response typo. reponse->response --- client/nginx.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/nginx.go b/client/nginx.go index cdca133f..4f51e794 100644 --- a/client/nginx.go +++ b/client/nginx.go @@ -116,7 +116,7 @@ type StreamServerZone struct { Sent uint64 } -// Responses represents HTTP reponse related stats. +// Responses represents HTTP response related stats. type Responses struct { Responses1xx uint64 `json:"1xx"` Responses2xx uint64 `json:"2xx"` From 2b059ca9d74ea098e9812a1182bf581b7097dd0a Mon Sep 17 00:00:00 2001 From: dean-coakley Date: Tue, 28 Aug 2018 11:42:26 +0100 Subject: [PATCH 3/9] Fix Makefile docker port mapping. Fix comments --- Makefile | 2 +- client/nginx.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index a99b20be..498f0c9e 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,7 @@ docker-build: docker build --build-arg NGINX_PLUS_VERSION=$(NGINX_PLUS_VERSION)~stretch -t $(NGINX_IMAGE) docker run-nginx-plus: - docker run -d --name nginx-plus-test --rm -p 8080:8080 $(NGINX_IMAGE) + docker run -d --name nginx-plus-test --rm -p 8080:8080 -p 8081:8081 $(NGINX_IMAGE) test-run: go test client/* diff --git a/client/nginx.go b/client/nginx.go index 4f51e794..831faa74 100644 --- a/client/nginx.go +++ b/client/nginx.go @@ -516,7 +516,7 @@ func (client *NginxClient) GetStreamServers(upstream string) ([]StreamUpstreamSe return servers, nil } -// AddStreamServer adds the server to the upstream. +// AddStreamServer adds the stream server to the upstream. func (client *NginxClient) AddStreamServer(upstream string, server StreamUpstreamServer) error { id, err := client.getIDOfStreamServer(upstream, server.Server) @@ -630,7 +630,7 @@ func determineStreamUpdates(updatedServers []StreamUpstreamServer, nginxServers return } -// GetStats gets connection, request, ssl, zone, and upstream related stats from the NGINX Plus API. +// GetStats gets connection, request, ssl, zone, stream zone, upstream and stream upstream related stats from the NGINX Plus API. func (client *NginxClient) GetStats() (*Stats, error) { cons, err := client.getConnections() if err != nil { @@ -720,7 +720,7 @@ func (client *NginxClient) getStreamServerZones() (*StreamServerZones, error) { var zones StreamServerZones err := client.get("stream/server_zones", &zones) if err != nil { - return nil, fmt.Errorf("failed to get server zones: %v", err) + return nil, fmt.Errorf("failed to get stream server zones: %v", err) } return &zones, err } From 6adc4f1e7b2df018c32035c2430d8d844b91359c Mon Sep 17 00:00:00 2001 From: isaac Date: Wed, 29 Aug 2018 12:43:19 +0100 Subject: [PATCH 4/9] Handle missing stream block --- Makefile | 15 ++++++++---- client/nginx.go | 28 +++++++++++++++++++++-- docker/nginx_no_stream.conf | 32 ++++++++++++++++++++++++++ docker/test.conf | 2 +- tests/client_no_stream_test.go | 42 ++++++++++++++++++++++++++++++++++ tests/client_test.go | 12 +++++----- 6 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 docker/nginx_no_stream.conf create mode 100644 tests/client_no_stream_test.go diff --git a/Makefile b/Makefile index 498f0c9e..83ff2185 100644 --- a/Makefile +++ b/Makefile @@ -1,8 +1,8 @@ NGINX_PLUS_VERSION=15-2 NGINX_IMAGE=nginxplus:$(NGINX_PLUS_VERSION) -test: docker-build run-nginx-plus test-run clean - +test: docker-build run-nginx-plus test-run config-no-stream test-no-stream clean + docker-build: docker build --build-arg NGINX_PLUS_VERSION=$(NGINX_PLUS_VERSION)~stretch -t $(NGINX_IMAGE) docker @@ -10,8 +10,15 @@ run-nginx-plus: docker run -d --name nginx-plus-test --rm -p 8080:8080 -p 8081:8081 $(NGINX_IMAGE) test-run: - go test client/* - go test tests/client_test.go + GOCACHE=off go test client/* + GOCACHE=off go test tests/client_test.go + +config-no-stream: + docker cp docker/nginx_no_stream.conf nginx-plus-test:/etc/nginx/nginx.conf + docker exec nginx-plus-test nginx -s reload + +test-no-stream: + GOCACHE=off go test tests/client_no_stream_test.go clean: docker kill nginx-plus-test diff --git a/client/nginx.go b/client/nginx.go index 831faa74..bbf2b71a 100644 --- a/client/nginx.go +++ b/client/nginx.go @@ -12,6 +12,8 @@ import ( // APIVersion is a version of NGINX Plus API. const APIVersion = 2 +const streamNotConfiguredCode = "StreamNotConfigured" + // NginxClient lets you access NGINX Plus API. type NginxClient struct { apiEndpoint string @@ -57,6 +59,15 @@ type apiError struct { Code string } +type internalError struct { + apiError + err string +} + +func (internalError *internalError) Error() string { + return internalError.err +} + // Stats represents NGINX Plus stats fetched from the NGINX Plus API. // https://nginx.org/en/docs/http/ngx_http_api_module.html type Stats struct { @@ -273,12 +284,15 @@ func getAPIVersions(httpClient *http.Client, endpoint string) (*versions, error) } func createResponseMismatchError(respBody io.ReadCloser, mainErr error) error { - apiErr, err := readAPIErrorResponse(respBody) + apiErrResp, err := readAPIErrorResponse(respBody) if err != nil { return fmt.Errorf("%v; failed to read the response body: %v", mainErr, err) } - return fmt.Errorf("%v; error: %v", mainErr, apiErr.toString()) + return &internalError{ + err: fmt.Sprintf("%v; error: %v", mainErr, apiErrResp.toString()), + apiError: apiErrResp.Error, + } } func readAPIErrorResponse(respBody io.ReadCloser) (*apiErrorResponse, error) { @@ -720,6 +734,11 @@ func (client *NginxClient) getStreamServerZones() (*StreamServerZones, error) { var zones StreamServerZones err := client.get("stream/server_zones", &zones) if err != nil { + if err, ok := err.(*internalError); ok { + if err.Code == streamNotConfiguredCode { + return &zones, nil + } + } return nil, fmt.Errorf("failed to get stream server zones: %v", err) } return &zones, err @@ -738,6 +757,11 @@ func (client *NginxClient) getStreamUpstreams() (*StreamUpstreams, error) { var upstreams StreamUpstreams err := client.get("stream/upstreams", &upstreams) if err != nil { + if err, ok := err.(*internalError); ok { + if err.Code == streamNotConfiguredCode { + return &upstreams, nil + } + } return nil, fmt.Errorf("failed to get stream upstreams: %v", err) } return &upstreams, nil diff --git a/docker/nginx_no_stream.conf b/docker/nginx_no_stream.conf new file mode 100644 index 00000000..5e076aad --- /dev/null +++ b/docker/nginx_no_stream.conf @@ -0,0 +1,32 @@ + +user nginx; +worker_processes auto; + +error_log /var/log/nginx/error.log notice; +pid /var/run/nginx.pid; + + +events { + worker_connections 1024; +} + + +http { + include /etc/nginx/mime.types; + default_type application/octet-stream; + + log_format main '$remote_addr - $remote_user [$time_local] "$request" ' + '$status $body_bytes_sent "$http_referer" ' + '"$http_user_agent" "$http_x_forwarded_for"'; + + access_log /var/log/nginx/access.log main; + + sendfile on; + #tcp_nopush on; + + keepalive_timeout 65; + + #gzip on; + + include /etc/nginx/conf.d/*.conf; +} diff --git a/docker/test.conf b/docker/test.conf index 81506409..693b6d6a 100644 --- a/docker/test.conf +++ b/docker/test.conf @@ -22,4 +22,4 @@ server { health_check interval=10 fails=3 passes=1; } status_zone test; -} +} \ No newline at end of file diff --git a/tests/client_no_stream_test.go b/tests/client_no_stream_test.go new file mode 100644 index 00000000..0d1ad43c --- /dev/null +++ b/tests/client_no_stream_test.go @@ -0,0 +1,42 @@ +package tests + +import ( + "net/http" + "testing" + + "github.com/nginxinc/nginx-plus-go-sdk/client" +) + +const ( + upstream = "test" + streamUpstream = "stream_test" +) + +// TestStatsNoStream tests the peculiar behavior of getting Stream-related +// stats from the API when there are no stream blocks in the config. +// The API returns a special error code that we can use to determine if the API +// is misconfigured or of the stream block is missing. +func TestStatsNoStream(t *testing.T) { + httpClient := &http.Client{} + c, err := client.NewNginxClient(httpClient, "http://127.0.0.1:8080/api") + if err != nil { + t.Fatalf("Error connecting to nginx: %v", err) + } + + stats, err := c.GetStats() + if err != nil { + t.Errorf("Error getting stats: %v", err) + } + + if stats.Connections.Active == 0 { + t.Errorf("Bad connections: %v", stats.Connections) + } + + if len(stats.StreamServerZones) != 0 { + t.Error("No stream block should result in no StreamServerZones") + } + + if len(stats.StreamUpstreams) != 0 { + t.Error("No stream block should result in no StreamUpstreams") + } +} diff --git a/tests/client_test.go b/tests/client_test.go index cf86d2a6..116fc3f0 100644 --- a/tests/client_test.go +++ b/tests/client_test.go @@ -1,6 +1,7 @@ package tests import ( + "net" "net/http" "reflect" "testing" @@ -179,7 +180,7 @@ func TestStreamUpstreamServerSlowStart(t *testing.T) { } servers, err := c.GetStreamServers(streamUpstream) if err != nil { - t.Errorf("Error getting stream servers: %v", err) + t.Fatalf("Error getting stream servers: %v", err) } if len(servers) != 1 { t.Errorf("Too many servers") @@ -371,7 +372,7 @@ func TestUpstreamServerSlowStart(t *testing.T) { } servers, err := c.GetHTTPServers(upstream) if err != nil { - t.Errorf("Error getting HTTPServers: %v", err) + t.Fatalf("Error getting HTTPServers: %v", err) } if len(servers) != 1 { t.Errorf("Too many servers") @@ -469,11 +470,10 @@ func TestStreamStats(t *testing.T) { t.Errorf("Error adding stream upstream server: %v", err) } - // make request so we have stream server zone stats, expect 5xx error - streamClient := &http.Client{} - _, err = streamClient.Get("http://127.0.0.1:8081") + // make connection so we have stream server zone stats - ignore response + _, err = net.Dial("tcp", "127.0.0.1:8081") if err != nil { - t.Errorf("Error making request: %v", err) + t.Errorf("Error making tcp connection: %v", err) } stats, err := c.GetStats() From 06d8dbebb592384c65eac0db4561272fbbfb3512 Mon Sep 17 00:00:00 2001 From: isaac Date: Wed, 29 Aug 2018 12:52:35 +0100 Subject: [PATCH 5/9] Add stream healthcheck test --- docker/nginx.conf | 1 + tests/client_no_stream_test.go | 5 ----- tests/client_test.go | 10 ++++------ 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/docker/nginx.conf b/docker/nginx.conf index ef7f8358..4bba7aaf 100644 --- a/docker/nginx.conf +++ b/docker/nginx.conf @@ -40,6 +40,7 @@ stream { listen 8081; proxy_pass stream_test; status_zone stream_test; + health_check interval=10 fails=3 passes=1; } } diff --git a/tests/client_no_stream_test.go b/tests/client_no_stream_test.go index 0d1ad43c..b2c7b896 100644 --- a/tests/client_no_stream_test.go +++ b/tests/client_no_stream_test.go @@ -7,11 +7,6 @@ import ( "github.com/nginxinc/nginx-plus-go-sdk/client" ) -const ( - upstream = "test" - streamUpstream = "stream_test" -) - // TestStatsNoStream tests the peculiar behavior of getting Stream-related // stats from the API when there are no stream blocks in the config. // The API returns a special error code that we can use to determine if the API diff --git a/tests/client_test.go b/tests/client_test.go index 116fc3f0..d89e39b9 100644 --- a/tests/client_test.go +++ b/tests/client_test.go @@ -398,7 +398,6 @@ func TestStats(t *testing.T) { t.Fatalf("Error connecting to nginx: %v", err) } - // need upstream for stats server := client.UpstreamServer{ Server: "127.0.0.1:8080", } @@ -461,7 +460,6 @@ func TestStreamStats(t *testing.T) { t.Fatalf("Error connecting to nginx: %v", err) } - // need upstream for stats server := client.StreamUpstreamServer{ Server: "127.0.0.1:8080", } @@ -484,7 +482,7 @@ func TestStreamStats(t *testing.T) { if stats.Connections.Active == 0 { t.Errorf("Bad connections: %v", stats.Connections) } - // SSL metrics blank in this example + if len(stats.StreamServerZones) < 1 { t.Errorf("No StreamServerZone metrics: %v", stats.StreamServerZones) } @@ -504,11 +502,11 @@ func TestStreamStats(t *testing.T) { if upstream.Peers[0].State != "up" { t.Errorf("stream upstream server state should be 'up'") } - if upstream.Peers[0].Connections < 0 { + if upstream.Peers[0].Connections < 1 { t.Errorf("stream upstream should have connects value") } - if upstream.Peers[0].HealthChecks.LastPassed { - t.Errorf("stream upstream server health check should report last failed") + if !upstream.Peers[0].HealthChecks.LastPassed { + t.Errorf("stream upstream server health check should report last passed") } } } else { From 2745a4544b301714ecc2ad1f947c9f81b4b6de84 Mon Sep 17 00:00:00 2001 From: isaac Date: Fri, 31 Aug 2018 09:39:04 +0100 Subject: [PATCH 6/9] Add sleep to wait for health checks to pass - Improve test error clarity --- tests/client_no_stream_test.go | 4 ++-- tests/client_test.go | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/client_no_stream_test.go b/tests/client_no_stream_test.go index b2c7b896..cd530c4c 100644 --- a/tests/client_no_stream_test.go +++ b/tests/client_no_stream_test.go @@ -23,8 +23,8 @@ func TestStatsNoStream(t *testing.T) { t.Errorf("Error getting stats: %v", err) } - if stats.Connections.Active == 0 { - t.Errorf("Bad connections: %v", stats.Connections) + if stats.Connections.Accepted < 1 { + t.Errorf("Stats should report some connections: %v", stats.Connections) } if len(stats.StreamServerZones) != 0 { diff --git a/tests/client_test.go b/tests/client_test.go index d89e39b9..e3eb676b 100644 --- a/tests/client_test.go +++ b/tests/client_test.go @@ -5,6 +5,7 @@ import ( "net/http" "reflect" "testing" + "time" "github.com/nginxinc/nginx-plus-go-sdk/client" ) @@ -474,6 +475,9 @@ func TestStreamStats(t *testing.T) { t.Errorf("Error making tcp connection: %v", err) } + // wait for health checks + time.Sleep(50 * time.Millisecond) + stats, err := c.GetStats() if err != nil { t.Errorf("Error getting stats: %v", err) From 1ae242a042fe21cb313559e5684b9a0a1fba755a Mon Sep 17 00:00:00 2001 From: isaac Date: Fri, 31 Aug 2018 12:22:33 +0100 Subject: [PATCH 7/9] Simplify internal error handling - Add internalError.Wrap for preserving context --- Makefile | 6 +++--- client/nginx.go | 33 +++++++++++++++++++++++---------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index 83ff2185..ebf3abfd 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ NGINX_PLUS_VERSION=15-2 NGINX_IMAGE=nginxplus:$(NGINX_PLUS_VERSION) -test: docker-build run-nginx-plus test-run config-no-stream test-no-stream clean +test: docker-build run-nginx-plus test-run configure-no-stream-block test-run-no-stream-block clean docker-build: docker build --build-arg NGINX_PLUS_VERSION=$(NGINX_PLUS_VERSION)~stretch -t $(NGINX_IMAGE) docker @@ -13,11 +13,11 @@ test-run: GOCACHE=off go test client/* GOCACHE=off go test tests/client_test.go -config-no-stream: +configure-no-stream-block: docker cp docker/nginx_no_stream.conf nginx-plus-test:/etc/nginx/nginx.conf docker exec nginx-plus-test nginx -s reload -test-no-stream: +test-run-no-stream-block: GOCACHE=off go test tests/client_no_stream_test.go clean: diff --git a/client/nginx.go b/client/nginx.go index bbf2b71a..fb54a8f7 100644 --- a/client/nginx.go +++ b/client/nginx.go @@ -64,10 +64,18 @@ type internalError struct { err string } +// Error allows internalError to match the Error interface. func (internalError *internalError) Error() string { return internalError.err } +// Wrap is a way of including current context while preserving previous error information, +// similar to `return fmt.Errof("error doing foo, err: %v", err)` but for our internalError type. +func (internalError *internalError) Wrap(err string) *internalError { + internalError.err = fmt.Sprintf("%v. %v", err, internalError.err) + return internalError +} + // Stats represents NGINX Plus stats fetched from the NGINX Plus API. // https://nginx.org/en/docs/http/ngx_http_api_module.html type Stats struct { @@ -283,14 +291,17 @@ func getAPIVersions(httpClient *http.Client, endpoint string) (*versions, error) return &vers, nil } -func createResponseMismatchError(respBody io.ReadCloser, mainErr error) error { +func createResponseMismatchError(respBody io.ReadCloser) *internalError { apiErrResp, err := readAPIErrorResponse(respBody) if err != nil { - return fmt.Errorf("%v; failed to read the response body: %v", mainErr, err) + return &internalError{ + err: fmt.Sprintf("failed to read the response body: %v", err), + apiError: apiError{}, + } } return &internalError{ - err: fmt.Sprintf("%v; error: %v", mainErr, apiErrResp.toString()), + err: apiErrResp.toString(), apiError: apiErrResp.Error, } } @@ -451,8 +462,9 @@ func (client *NginxClient) get(path string, data interface{}) error { } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - mainErr := fmt.Errorf("expected %v response, got %v", http.StatusOK, resp.StatusCode) - return createResponseMismatchError(resp.Body, mainErr) + return createResponseMismatchError(resp.Body).Wrap(fmt.Sprintf( + "expected %v response, got %v", + http.StatusCreated, resp.StatusCode)) } body, err := ioutil.ReadAll(resp.Body) @@ -481,8 +493,9 @@ func (client *NginxClient) post(path string, input interface{}) error { } defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - mainErr := fmt.Errorf("expected %v response, got %v", http.StatusCreated, resp.StatusCode) - return createResponseMismatchError(resp.Body, mainErr) + return createResponseMismatchError(resp.Body).Wrap(fmt.Sprintf( + "expected %v response, got %v", + http.StatusCreated, resp.StatusCode)) } return nil @@ -503,9 +516,9 @@ func (client *NginxClient) delete(path string) error { defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - mainErr := fmt.Errorf("failed to complete delete request: expected %v response, got %v", - http.StatusOK, resp.StatusCode) - return createResponseMismatchError(resp.Body, mainErr) + return createResponseMismatchError(resp.Body).Wrap(fmt.Sprintf( + "failed to complete delete request: expected %v response, got %v", + http.StatusCreated, resp.StatusCode)) } return nil } From fbe16c56cd1b586088c4adf179d4375bc6ceef3d Mon Sep 17 00:00:00 2001 From: isaac Date: Fri, 31 Aug 2018 13:21:18 +0100 Subject: [PATCH 8/9] Fix copy/paste typos - Fix StatusOK vs StatusCreated copy/paste typo - Re-order stats calls - Implicit empty apiError --- client/nginx.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/client/nginx.go b/client/nginx.go index fb54a8f7..07fdce40 100644 --- a/client/nginx.go +++ b/client/nginx.go @@ -295,8 +295,7 @@ func createResponseMismatchError(respBody io.ReadCloser) *internalError { apiErrResp, err := readAPIErrorResponse(respBody) if err != nil { return &internalError{ - err: fmt.Sprintf("failed to read the response body: %v", err), - apiError: apiError{}, + err: fmt.Sprintf("failed to read the response body: %v", err), } } @@ -464,7 +463,7 @@ func (client *NginxClient) get(path string, data interface{}) error { if resp.StatusCode != http.StatusOK { return createResponseMismatchError(resp.Body).Wrap(fmt.Sprintf( "expected %v response, got %v", - http.StatusCreated, resp.StatusCode)) + http.StatusOK, resp.StatusCode)) } body, err := ioutil.ReadAll(resp.Body) @@ -518,7 +517,7 @@ func (client *NginxClient) delete(path string) error { if resp.StatusCode != http.StatusOK { return createResponseMismatchError(resp.Body).Wrap(fmt.Sprintf( "failed to complete delete request: expected %v response, got %v", - http.StatusCreated, resp.StatusCode)) + http.StatusOK, resp.StatusCode)) } return nil } @@ -679,12 +678,12 @@ func (client *NginxClient) GetStats() (*Stats, error) { return nil, fmt.Errorf("failed to get stats: %v", err) } - streamZones, err := client.getStreamServerZones() + upstreams, err := client.getUpstreams() if err != nil { return nil, fmt.Errorf("failed to get stats: %v", err) } - upstreams, err := client.getUpstreams() + streamZones, err := client.getStreamServerZones() if err != nil { return nil, fmt.Errorf("failed to get stats: %v", err) } From 94ad6a0ce5b7e9cd4e8750b41553d4760e46a260 Mon Sep 17 00:00:00 2001 From: isaac Date: Fri, 31 Aug 2018 13:26:00 +0100 Subject: [PATCH 9/9] Cache client unit tests --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index ebf3abfd..ecd336dc 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ run-nginx-plus: docker run -d --name nginx-plus-test --rm -p 8080:8080 -p 8081:8081 $(NGINX_IMAGE) test-run: - GOCACHE=off go test client/* + go test client/* GOCACHE=off go test tests/client_test.go configure-no-stream-block: