Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@
.vscode

dist

coverage.out

10 changes: 9 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,15 @@ export TEST_API_ENDPOINT=http://$(DOCKER_NGINX_PLUS):8080/api
export TEST_API_ENDPOINT_OF_HELPER=http://$(DOCKER_NGINX_PLUS_HELPER):8080/api
export TEST_UNAVAILABLE_STREAM_ADDRESS=$(DOCKER_NGINX_PLUS):8081

test: run-nginx-plus test-run configure-no-stream-block test-run-no-stream-block clean

test: run-nginx-plus test-run configure-no-stream-block test-run-no-stream-block clean ## Run integration tests

unittest:
go test client/*.go -shuffle=on -race -v

cover:
go test -shuffle=on -race -v client/*.go -count=1 -cover -covermode=atomic -coverprofile=coverage.out
go tool cover -html coverage.out

lint:
docker run --pull always --rm -v $(shell pwd):/nginx-plus-go-client -w /nginx-plus-go-client -v $(shell go env GOCACHE):/cache/go -e GOCACHE=/cache/go -e GOLANGCI_LINT_CACHE=/cache/go -v $(shell go env GOPATH)/pkg:/go/pkg golangci/golangci-lint:latest golangci-lint --color always run
Expand Down
72 changes: 67 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,79 @@ This Client works against versions 4 to 8 of the NGINX Plus API. The table below
| 7 | R25 |
| 8 | R27 |

## Using the Client
## Using the client library

1. Import `github.com/nginxinc/nginx-plus-go-client/client` into your go project.
2. Use your favorite vendor tool to add this to your `/vendor` directory in your project.
Import the library using:
```go
import "github.com/nginxinc/nginx-plus-go-client/client"
```

## Creating a client

When creating a new ```client``` it is assumed that NGINX API is reachable. The ```client``` verifies API version of the running NGINX at the time of creating a new client object.

If user wants to create a client without NGINX automatic version check it should use the ```NewDefaultNginxClient``` func to create the client.


### Create a new ```Client``` that works with the latest NGINX API Version:
```go
myHTTPClient := &http.Client{
Timeout: 5*time.Second
}
c, err := NewNginxClient(myHTTPClient, "your-api-endpoint")
Copy link

@tomasohaodha tomasohaodha Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes it's client.New...() (see line 63 for example), sometimes it's just New...() (as in here). Can we keep this consistent?

if err != nil {
// handle error
}
```

### Create a new ```Client``` with specified API version:
```go
myHTTPClient := &http.Client{
Timeout: 5*time.Second
}

c, err := NewNginxClientWithVersion(myHTTPClient, "your-api-endpoint", 7)
if err != nil {
// handle error
}
```

### Create a new default ```Client``` that does not check NGINX API version:
```go
c, err := client.NewDefaultNginxClient("your-api-endpoint")
if err != nil {
// handle error
}
```
### Create a new default client with customized ```http.Client``` and API Version:
```go
myHTTPClient := &http.Client{
Timeout: 60 * time.Second,
}

c, err := client.NewDefaultNginxClient(
"your-api-endpoint",
client.WithHTTPClient(myHTTPClient),
client.WithAPIVersion(7),
)
if err != nil {
// handle error
}
```
Note that:
- default NGINX Plus Client is using ```http.Client``` with specified ```Timeout``` value of 10s
- it is user's responsibility to provide correct NGINX API version as the ```NewDefaultNginxClient``` func does not verify the version

## Testing

### Unit tests
Run unittests
```
$ make unittest
```
Run unittests with coverage report
```
$ cd client
$ go test
$ make cover
```

### Integration tests
Expand Down
54 changes: 54 additions & 0 deletions client/nginx.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,60 @@ type HTTPLimitConnections map[string]LimitConnection
// StreamLimitConnections represents limit connections related stats
type StreamLimitConnections map[string]LimitConnection

type option func(*NginxClient) error

// WithHTTPClient configures NGINX Plus Client to use customized http.Client.
//
//nolint:all
func WithHTTPClient(httpClient *http.Client) option {
return func(nc *NginxClient) error {
if httpClient == nil {
return errors.New("nil http client")
}
nc.httpClient = httpClient
return nil
}
}

// WithAPIVersion configures NGINX Plus Client to use a specific version of the NGINX Plus API.
//
//nolint:all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm struggling to see why these functions are needed and why they can't be "normal" parameters of NewDefaultNginxClient. Also I think we should avoid adding new code that needs a //nolint:all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm struggling to see why these functions are needed and why they can't be "normal" parameters of NewDefaultNginxClient. Also I think we should avoid adding new code that needs a //nolint:all.

Functions in question (functonal options pattern) provide ability to configure created obj by the user of the library. At the same time if user is happy to use a default obj, he is not forced to do not necessary paperwork (creating httpClient to pass it to the func to create the obj, passing explicit api version, etc.). This way the API is cleaner and function signature keep to a minimum. In other words, user can create a minimum usable obj with minimum paperwork involved from his side. If the user wants to pass his own params (in this case http client and version) he can do it with functional options.

Commenting on the //nolinit:all directive:
The linter we use (not staticcheck) issues a message that exported func returns a not exported type. This is intentional as the WithXYZ functions validates params passed to option func. the option type is used internally and is not exported, as the library user won't use it. The golangci-linter we use (with settings) for some reason emits a message. This message would make CI pipeline fail. So, the reason to keep this directive is to allow CI to pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I understand the functional options pattern, I'm just not sure about adding a new NewDefaultNginxClient with just a couple of options, maybe we should do a refactor of the package.
We already have NewNginxClientWithVersion as well where you can pass the version, if anything I'd follow this pattern.

for the unexported-return error, what I usually see is that the options are private but the func is exported, see https://github.com/golang/text/blob/master/secure/precis/options.go#L16 for example.

Copy link
Contributor Author

@jjngx jjngx Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option type is un-exported for a reason. It is used internally in the package and it is not a part of package API. External consumers of the library use WithXYZ functions to configure the client. The main point of this change is to have an option to initialise the NGINX client WITHOUT requiring unnecessary paperwork from user perspective. Chance the NewXYZ func returns a default client configured out of the box. If user WANTS to pass config params it has an option to do so.
The problem that we have with golangci lint is that it complains unnecessarily and we need to suppress this complaint. Otherwise the CI configured currently won't pass. Please not that staticcheck does not complain about exported functions returning not exported types.

The additional functionality of the new func that creates an instance of the NGINX client is to decouple the client from a running NGINX server the client tries to connect to.
We can rename the NewXYZ client however we wish to suit better naming convention we use in the library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what the functionality is about, I just don't agree with the implementation.

I don't think golangci-lint is complaining unnecessarily 🤷‍♂️

Maybe somebody else from the team can weigh in @nginxinc/kic

func WithAPIVersion(version int) option {
return func(nc *NginxClient) error {
if !versionSupported(version) {
return fmt.Errorf("unsupported API version %v", version)
}
nc.version = version
return nil
}
}

// NewDefaultNginxClient creates the client for the latest NGINX plus version
// and configured default HTTP timeout set to 10 seconds. User can configure
// customized http.Client and supported API version by passing options.
//
// NewDefaultNginxClient does not perform external http request to determine
// what NGINX version it talks to. It is responsibility of the caller to make
// sure the library is used with supported versions.
func NewDefaultNginxClient(apiEndpoint string, opts ...option) (*NginxClient, error) {
if apiEndpoint == "" {
return nil, errors.New("api endpoint not specified")
}
nc := NginxClient{
apiEndpoint: apiEndpoint,
httpClient: &http.Client{
Timeout: 10 * time.Second,
Copy link

@tomasohaodha tomasohaodha Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is hard-coding a 10 second timeout here really creating a 'default' nginx client? I would have thought leaving the http.Client timeout it its default is more correct. If the user wants to specify 10 seconds then they can do that with WithHTTPClient()
At the least should we define the timeout with the rest of the constants towards the top of the file, and not use the magic number 10 here?

},
version: APIVersion,
}
for _, opt := range opts {
if err := opt(&nc); err != nil {
return nil, err
}
}
return &nc, nil
}

// NewNginxClient creates an NginxClient with the latest supported version.
func NewNginxClient(httpClient *http.Client, apiEndpoint string) (*NginxClient, error) {
return NewNginxClientWithVersion(httpClient, apiEndpoint, APIVersion)
Expand Down
62 changes: 62 additions & 0 deletions client/nginx_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package client

import (
"net/http"
"reflect"
"testing"
"time"

"github.com/google/go-cmp/cmp"
)

func TestDetermineUpdates(t *testing.T) {
Expand Down Expand Up @@ -518,3 +522,61 @@ func TestHaveSameParametersForStream(t *testing.T) {
}
}
}

func TestClientFailsToCreateOnInvalidAPIVersionInput(t *testing.T) {
t.Parallel()

invalidAPIVersions := []int{-1, 0, 3, 9}
for _, v := range invalidAPIVersions {
_, err := NewDefaultNginxClient("test-endpoint", WithAPIVersion(v))
if err == nil {
t.Errorf("want error on using invalid API version %d, got nil", v)
}
}
}

func TestClientFailsToCreateOnInvalidInputForHTTPClient(t *testing.T) {
t.Parallel()

_, err := NewDefaultNginxClient("test-endpoint", WithHTTPClient(nil))
if err == nil {
t.Error("want error on using nil http client, got nil")
}
}

func TestClientFailsToCreateOnInvalidAPIEndpointInput(t *testing.T) {
t.Parallel()

_, err := NewDefaultNginxClient("")
if err == nil {
t.Error("want error on empty endpoint, got nil")
}
}

func TestNewCreatesDefaultClientOnValidCustomHTTPClientInput(t *testing.T) {
t.Parallel()
ht := &http.Client{
Timeout: 30 * time.Second,
}

client, err := NewDefaultNginxClient("test-enpoint", WithHTTPClient(ht))
if err != nil {
t.Fatal(err)
}
if !cmp.Equal(ht, client.httpClient) {
t.Errorf(cmp.Diff(ht, client.httpClient))
}
}

func TestNewCreatesDefaultClientOnValidAPIVersionInput(t *testing.T) {
t.Parallel()

apiVer := 7
client, err := NewDefaultNginxClient("test-endpoint", WithAPIVersion(apiVer))
if err != nil {
t.Fatal(err)
}
if !cmp.Equal(apiVer, client.version) {
t.Error(cmp.Diff(apiVer, client.version))
}
}
40 changes: 40 additions & 0 deletions examples/default-client/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package main

import (
"fmt"
"log"
"net/http"
"time"

"github.com/nginxinc/nginx-plus-go-client/client"
)

func main() {
// Create a custom HTTP Client
myHTTPClient := &http.Client{
Timeout: 60 * time.Second,
}

// Create NGINX Plus Client for working with version 8
c, err := client.NewDefaultNginxClient(
"https://demo.nginx.com/api",
client.WithHTTPClient(myHTTPClient),
client.WithAPIVersion(8),
)
if err != nil {
// handle error
log.Fatal(err)
}

// Retrieve info about running NGINX
info, err := c.GetNginxInfo()
if err != nil {
// handle error
log.Fatal(err)
}
fmt.Printf("%+v\n", info)

// Prints
// &{Version:1.21.6 Build:nginx-plus-r27 Address:3.125.64.247 Generation:4 LoadTimestamp:2022-09-14T12:45:25.218Z Timestamp:2022-10-10T12:25:16.552Z ProcessID:3640888 ParentProcessID:2945895}

}
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
module github.com/nginxinc/nginx-plus-go-client

go 1.19

require github.com/google/go-cmp v0.5.8
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg=
github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=