diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index adbb7d1..0aa8aa1 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -8,7 +8,8 @@ on: jobs: - build: + test: + name: Test runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 @@ -20,3 +21,29 @@ jobs: - name: Test run: go test -v ./... + + lint: + name: Lint + runs-on: ubuntu-latest + container: golangci/golangci-lint:v1.42 + steps: + - uses: actions/checkout@v2 + + - name: Check linters are up to date + run: make test-update-linters + + - name: golangci-lint + uses: golangci/golangci-lint-action@v2 + with: + version: v1.42 + + codespell: + name: Spell Check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: codespell-project/actions-codespell@master + with: + skip: .git + check_filenames: true + check_hidden: true diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..6587bdd --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,116 @@ +output: + sort-results: true + +issues: + exclude-use-default: false + max-same-issues: 0 + max-issues-per-linter: 0 + exclude-rules: + # False positive: https://github.com/kunwardeep/paralleltest/issues/8. + - linters: + - paralleltest + text: "does not use range value in test Run" + +linters-settings: + errcheck: + check-type-assertions: true + check-blank: true + gci: + local-prefixes: github.com/flatcar-linux/fleetlock + godot: + capital: true + gofumpt: + extra-rules: true + govet: + enable-all: true + disable: + - fieldalignment + - shadow + makezero: + always: true + nolintlint: + allow-leading-space: false + require-explanation: true + require-specific: true + wsl: + force-err-cuddling: true + +linters: + disable-all: false + disable: + # We do not have clearly defined error types yet. + - goerr113 + # We do not require all used structs to have all fields initialized. + - exhaustivestruct + # This linters has been deprecated. + - interfacer + - maligned + - golint + enable: + - asciicheck + - bodyclose + - cyclop + - deadcode + - depguard + - dogsled + - dupl + - durationcheck + - errcheck + - errname + - errorlint + - exhaustive + - exportloopref + - forbidigo + - forcetypeassert + - funlen + - gci + - gochecknoglobals + - gochecknoinits + - gocognit + - goconst + - gocritic + - gocyclo + - godot + - godox + - gofmt + - gofumpt + - goheader + - goimports + - gomnd + - gomoddirectives + - gomodguard + - goprintffuncname + - ifshort + - importas + - ineffassign + - lll + - makezero + - misspell + - nakedret + - nestif + - nilerr + - nlreturn + - noctx + - nolintlint + - paralleltest + - prealloc + - predeclared + - promlinter + - revive + - rowserrcheck + - scopelint + - sqlclosecheck + - structcheck + - stylecheck + - tagliatelle + - testpackage + - thelper + - tparallel + - typecheck + - unconvert + - unparam + - varcheck + - wastedassign + - whitespace + - wrapcheck + - wsl diff --git a/Makefile b/Makefile index 39d13f8..128d336 100644 --- a/Makefile +++ b/Makefile @@ -1,2 +1,24 @@ -build: +GOLANGCILINT := go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.42.1 + +.PHONY: build +build: ## Compiles CLI binary. @go build -o ./fleetlockctl main.go + +.PHONY: test-working-tree-clean +test-working-tree-clean: ## Checks if working directory is clean. + @test -z "$$(git status --porcelain)" || (echo "Commit all changes before running this target"; exit 1) + +.PHONY: update-linters +update-linters: ## Updates list of linters in .golangci.yml file based on currently installed golangci-lint binary. + # Remove all enabled linters. + sed -i '/^ enable:/q0' .golangci.yml + # Then add all possible linters to config. + $(GOLANGCILINT) linters | grep -E '^\S+:' | cut -d: -f1 | sort | sed 's/^/ - /g' | grep -v -E "($$(grep '^ disable:' -A 100 .golangci.yml | grep -E ' - \S+$$' | awk '{print $$2}' | tr \\n '|' | sed 's/|$$//g'))" >> .golangci.yml + +.PHONY: test-update-linters +test-update-linters: test-working-tree-clean update-linters ## Verifies that list of linters in .golangci.yml file is up to date. + @test -z "$$(git status --porcelain)" || (echo "Linter configuration outdated. Run 'make update-linters' and commit generated changes to fix."; exit 1) + +.PHONY: help +help: ## Prints help message. + @grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' diff --git a/README.md b/README.md index 4841d9c..cc6de15 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -## Fleetlock client +## FleetLock client [![Go Reference](https://pkg.go.dev/badge/github.com/flatcar-linux/fleetlock.svg)](https://pkg.go.dev/github.com/flatcar-linux/fleetlock) [![Go](https://github.com/flatcar-linux/fleetlock/actions/workflows/go.yml/badge.svg?branch=main)](https://github.com/flatcar-linux/fleetlock/actions/workflows/go.yml) diff --git a/cmd/lock.go b/cmd/lock.go index bc20f32..5b920b8 100644 --- a/cmd/lock.go +++ b/cmd/lock.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "fmt" "net/http" @@ -9,22 +10,23 @@ import ( "github.com/flatcar-linux/fleetlock/pkg/client" ) -var ( - lock = &cobra.Command{ - Use: "recursive-lock", +func lock(group, id, url *string) *cobra.Command { + return &cobra.Command{ + Use: "recursive-lock", + Short: "Try to reserve (lock) a slot for rebooting", RunE: func(cmd *cobra.Command, args []string) error { httpClient := http.DefaultClient - c, err := client.New(url, group, id, httpClient) + c, err := client.New(*url, *group, *id, httpClient) if err != nil { return fmt.Errorf("building the client: %w", err) } - if err := c.RecursiveLock(); err != nil { + if err := c.RecursiveLock(context.Background()); err != nil { return fmt.Errorf("locking: %w", err) } return nil }, } -) +} diff --git a/cmd/root.go b/cmd/root.go index dd65678..37b9ec7 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -1,20 +1,22 @@ +// Package cmd implements fleetlockctl CLI. package cmd import ( "github.com/spf13/cobra" ) -var ( - group, id, url string +// Command returns CLI command to be executed. +func Command() *cobra.Command { + cli := &cobra.Command{Use: "fleetlockctl"} - Root = &cobra.Command{Use: "fleetlockctl"} -) + var group, id, url string + + cli.PersistentFlags().StringVarP(&group, "group", "g", "default", "FleetLock group") + cli.PersistentFlags().StringVarP(&id, "id", "i", "", "FleetLock instance ID (e.g. content of /etc/machine-id file)") + cli.PersistentFlags().StringVarP(&url, "url", "u", "", "FleetLock endpoint URL") -func init() { - Root.PersistentFlags().StringVarP(&group, "group", "g", "default", "Fleet-Lock group") - Root.PersistentFlags().StringVarP(&id, "id", "i", "", "Fleet-Lock instance ID (/etc/machine-id for example)") - Root.PersistentFlags().StringVarP(&url, "url", "u", "", "Fleet-Lock endpoint URL") + cli.AddCommand(lock(&group, &id, &url)) + cli.AddCommand(unlock(&group, &id, &url)) - Root.AddCommand(lock) - Root.AddCommand(unlock) + return cli } diff --git a/cmd/unlock.go b/cmd/unlock.go index 19656c3..7e82ef9 100644 --- a/cmd/unlock.go +++ b/cmd/unlock.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "fmt" "net/http" @@ -9,22 +10,23 @@ import ( "github.com/flatcar-linux/fleetlock/pkg/client" ) -var ( - unlock = &cobra.Command{ - Use: "unlock-if-held", +func unlock(group, id, url *string) *cobra.Command { + return &cobra.Command{ + Use: "unlock-if-held", + Short: "Try to release (unlock) a slot that it was previously holding", RunE: func(cmd *cobra.Command, args []string) error { httpClient := http.DefaultClient - c, err := client.New(url, group, id, httpClient) + c, err := client.New(*url, *group, *id, httpClient) if err != nil { return fmt.Errorf("building the client: %w", err) } - if err := c.UnlockIfHeld(); err != nil { + if err := c.UnlockIfHeld(context.Background()); err != nil { return fmt.Errorf("unlocking: %w", err) } return nil }, } -) +} diff --git a/main.go b/main.go index 28fd09e..909182a 100644 --- a/main.go +++ b/main.go @@ -1,14 +1,13 @@ package main import ( - "fmt" "os" "github.com/flatcar-linux/fleetlock/cmd" ) func main() { - if err := cmd.Root.Execute(); err != nil { - fmt.Fprintf(os.Stderr, "unable to execute command: %v", err) + if err := cmd.Command().Execute(); err != nil { + os.Exit(1) } } diff --git a/pkg/client/client.go b/pkg/client/client.go index 7568648..3b35b6e 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -1,8 +1,10 @@ +// Package client implements FleetLock client. package client import ( "bufio" "bytes" + "context" "encoding/json" "fmt" "io/ioutil" @@ -22,30 +24,76 @@ type HTTPClient interface { type Payload struct { // ClientParams holds the parameters specific to the // FleetLock client. - ClientParams *ClientParams `json:"client_params"` + // + //nolint:tagliatelle // FleetLock protocol requires exactly 'client_params' field. + ClientParams *Params `json:"client_params"` } -// Client params is the object holding the +// Params is the object holding the // ID and the group for each client. -type ClientParams struct { - // ID is the client identifer. (e.g node name or UUID) +type Params struct { + // ID is the client identifier. (e.g node name or UUID) ID string `json:"id"` // Group is the reboot-group of the client. Group string `json:"group"` } // Client holds the params related to the host -// in order to interact with the Fleet-Lock URL. +// in order to interact with the FleetLock server URL. type Client struct { - URL string - group string - id string - http HTTPClient + baseServerURL string + group string + id string + http HTTPClient } -func (c *Client) generateRequest(endpoint string) (*http.Request, error) { +// New builds a FleetLock client. +func New(baseServerURL, group, id string, c HTTPClient) (*Client, error) { + if _, err := url.ParseRequestURI(baseServerURL); err != nil { + return nil, fmt.Errorf("parsing URL: %w", err) + } + + return &Client{ + baseServerURL: baseServerURL, + http: c, + group: group, + id: id, + }, nil +} + +// RecursiveLock tries to reserve (lock) a slot for rebooting. +func (c *Client) RecursiveLock(ctx context.Context) error { + req, err := c.generateRequest(ctx, "v1/pre-reboot") + if err != nil { + return fmt.Errorf("generating request: %w", err) + } + + resp, err := c.http.Do(req) + if err != nil { + return fmt.Errorf("doing the request: %w", err) + } + + return handleResponse(resp) +} + +// UnlockIfHeld tries to release (unlock) a slot that it was previously holding. +func (c *Client) UnlockIfHeld(ctx context.Context) error { + req, err := c.generateRequest(ctx, "v1/steady-state") + if err != nil { + return fmt.Errorf("generating request: %w", err) + } + + resp, err := c.http.Do(req) + if err != nil { + return fmt.Errorf("doing the request: %w", err) + } + + return handleResponse(resp) +} + +func (c *Client) generateRequest(ctx context.Context, endpoint string) (*http.Request, error) { payload := &Payload{ - ClientParams: &ClientParams{ + ClientParams: &Params{ ID: c.id, Group: c.group, }, @@ -57,7 +105,10 @@ func (c *Client) generateRequest(endpoint string) (*http.Request, error) { } j := bytes.NewReader(body) - req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/%s", c.URL, endpoint), j) + + targetURL := fmt.Sprintf("%s/%s", c.baseServerURL, endpoint) + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, targetURL, j) if err != nil { return nil, fmt.Errorf("building request: %w", err) } @@ -70,74 +121,30 @@ func (c *Client) generateRequest(endpoint string) (*http.Request, error) { } func handleResponse(resp *http.Response) error { - statusType := resp.StatusCode / 100 + maxHTTPErrorCode := 600 - switch statusType { - case 2: + switch code := resp.StatusCode; { + case code >= 200 && code < 300: return nil - case 3, 4, 5: + case code >= 300 && code < maxHTTPErrorCode: // We try to extract an eventual error. r := bufio.NewReader(resp.Body) + body, err := ioutil.ReadAll(r) if err != nil { return fmt.Errorf("reading body: %w", err) } + //nolint:errcheck // We do it best effort and at least stdlib client never returns error here. resp.Body.Close() e := &Error{} if err := json.Unmarshal(body, &e); err != nil { - return fmt.Errorf("unmarshalling error: %v", err) + return fmt.Errorf("unmarshalling error: %w", err) } return fmt.Errorf("fleetlock error: %s", e.String()) default: return fmt.Errorf("unexpected status code: %d", resp.StatusCode) } - - return nil -} - -// RecursiveLock tries to reserve (lock) a slot for rebooting -func (c *Client) RecursiveLock() error { - req, err := c.generateRequest("v1/pre-reboot") - if err != nil { - return fmt.Errorf("generating request: %w", err) - } - - resp, err := c.http.Do(req) - if err != nil { - return fmt.Errorf("doing the request: %w", err) - } - - return handleResponse(resp) -} - -// UnlockIfHeld tries to release (unlock) a slot that it was previously holding -func (c *Client) UnlockIfHeld() error { - req, err := c.generateRequest("v1/steady-state") - if err != nil { - return fmt.Errorf("generating request: %w", err) - } - - resp, err := c.http.Do(req) - if err != nil { - return fmt.Errorf("doing the request: %w", err) - } - - return handleResponse(resp) -} - -// New builds a Fleet-Lock client. -func New(URL, group, ID string, c HTTPClient) (*Client, error) { - if _, err := url.ParseRequestURI(URL); err != nil { - return nil, fmt.Errorf("parsing URL: %w", err) - } - - return &Client{ - URL: URL, - http: c, - group: group, - id: ID, - }, nil } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index ae48200..ede89cc 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -2,8 +2,9 @@ package client_test import ( "bytes" + "context" "errors" - "io" + "fmt" "io/ioutil" "net/http" "testing" @@ -12,35 +13,40 @@ import ( ) type httpClient struct { - resp *http.Response - r *http.Request - doErr error + do func(req *http.Request) (*http.Response, error) + r *http.Request } -func (h *httpClient) Do(req *http.Request) (*http.Response, error) { - h.r = req - return h.resp, h.doErr +func (m *httpClient) Do(req *http.Request) (*http.Response, error) { + m.r = req + + return m.do(req) } func TestBadURL(t *testing.T) { + t.Parallel() + _, err := client.New("this is not an URL", "default", "1234", nil) if err == nil { t.Fatalf("should get an error") } - if err != nil && err.Error() != "parsing URL: parse \"this is not an URL\": invalid URI for request" { - t.Fatalf("should have %s for the error, got: %s", "parsing URL: parse \"this is not an URL\": invalid URI for request", err.Error()) + expectedError := "parsing URL: parse \"this is not an URL\": invalid URI for request" + if err != nil && err.Error() != expectedError { + t.Fatalf("should have %s for the error, got: %s", expectedError, err.Error()) } } -func TestRecursiveLock(t *testing.T) { +//nolint:funlen // Just many test cases. +func TestClient(t *testing.T) { + t.Parallel() - expURL := "http://1.2.3.4/v1/pre-reboot" + ctx := context.Background() for _, test := range []struct { statusCode int expErr error - body io.ReadCloser + body []byte doErr error }{ { @@ -49,12 +55,12 @@ func TestRecursiveLock(t *testing.T) { { statusCode: 500, expErr: errors.New("fleetlock error: this is an error (error_kind)"), - body: ioutil.NopCloser(bytes.NewReader([]byte(`{"kind":"error_kind","value":"this is an error"}`))), + body: []byte(`{"kind":"error_kind","value":"this is an error"}`), }, { statusCode: 500, expErr: errors.New("unmarshalling error: invalid character '\"' after object key:value pair"), - body: ioutil.NopCloser(bytes.NewReader([]byte(`{"kind":"error_kind" "value":"this is an error"}`))), + body: []byte(`{"kind":"error_kind" "value":"this is an error"}`), }, { statusCode: 100, @@ -65,76 +71,92 @@ func TestRecursiveLock(t *testing.T) { doErr: errors.New("connection refused"), }, } { - h := &httpClient{ - resp: &http.Response{ - StatusCode: test.statusCode, - Body: test.body, - }, - doErr: test.doErr, + test := test + + newClient := func(statusCode int, body []byte, doErr error) (*httpClient, *client.Client) { + h := &httpClient{ + do: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: statusCode, + Body: ioutil.NopCloser(bytes.NewReader(body)), + }, doErr + }, + } + + c, err := client.New("http://1.2.3.4", "default", "1234", h) + if err != nil { + t.Fatalf("Unexpected error creating client: %v", err) + } + + return h, c } - c, _ := client.New("http://1.2.3.4", "default", "1234", h) + t.Run(fmt.Sprintf("UnlockIfHeld_%d", test.statusCode), func(t *testing.T) { + t.Parallel() - err := c.RecursiveLock() - if err != nil && err.Error() != test.expErr.Error() { - t.Fatalf("should have %v for err, got: %v", test.expErr, err) - } + h, c := newClient(test.statusCode, test.body, test.doErr) - if h.r.URL.String() != expURL { - t.Fatalf("should have %s for URL, got: %s", expURL, h.r.URL.String()) - } + err := c.UnlockIfHeld(ctx) + if err != nil && err.Error() != test.expErr.Error() { + t.Fatalf("should have %v for err, got: %v", test.expErr, err) + } + + expURL := "http://1.2.3.4/v1/steady-state" + + if h.r.URL.String() != expURL { + t.Fatalf("should have %s for URL, got: %s", expURL, h.r.URL.String()) + } + }) + + t.Run(fmt.Sprintf("RecursiveLock_%d", test.statusCode), func(t *testing.T) { + t.Parallel() + + h, c := newClient(test.statusCode, test.body, test.doErr) + + err := c.RecursiveLock(ctx) + if err != nil && err.Error() != test.expErr.Error() { + t.Fatalf("should have %v for err, got: %v", test.expErr, err) + } + + expURL := "http://1.2.3.4/v1/pre-reboot" + + if h.r.URL.String() != expURL { + t.Fatalf("should have %s for URL, got: %s", expURL, h.r.URL.String()) + } + }) } } -func TestUnlockIfHeld(t *testing.T) { +func Test_Client_use_given_context_for_requests(t *testing.T) { + t.Parallel() - expURL := "http://1.2.3.4/v1/steady-state" + key := struct{}{} + value := "bar" - for _, test := range []struct { - statusCode int - expErr error - body io.ReadCloser - doErr error - }{ - { - statusCode: 200, - }, - { - statusCode: 500, - expErr: errors.New("fleetlock error: this is an error (error_kind)"), - body: ioutil.NopCloser(bytes.NewReader([]byte(`{"kind":"error_kind","value":"this is an error"}`))), - }, - { - statusCode: 500, - expErr: errors.New("unmarshalling error: invalid character '\"' after object key:value pair"), - body: ioutil.NopCloser(bytes.NewReader([]byte(`{"kind":"error_kind" "value":"this is an error"}`))), - }, - { - statusCode: 100, - expErr: errors.New("unexpected status code: 100"), - }, - { - expErr: errors.New("doing the request: connection refused"), - doErr: errors.New("connection refused"), + h := &httpClient{ + do: func(req *http.Request) (*http.Response, error) { + if req.Context().Value(key) == nil { + t.Fatalf("Expected request to use given context") + } + + return &http.Response{ + StatusCode: 200, + }, nil }, - } { - h := &httpClient{ - resp: &http.Response{ - StatusCode: test.statusCode, - Body: test.body, - }, - doErr: test.doErr, - } + } - c, _ := client.New("http://1.2.3.4", "default", "1234", h) + c, err := client.New("http://1.2.3.4", "default", "1234", h) + if err != nil { + t.Fatalf("Unexpected error creating client: %v", err) + } - err := c.UnlockIfHeld() - if err != nil && err.Error() != test.expErr.Error() { - t.Fatalf("should have %v for err, got: %v", test.expErr, err) - } + ctx := context.WithValue(context.Background(), key, value) - if h.r.URL.String() != expURL { - t.Fatalf("should have %s for URL, got: %s", expURL, h.r.URL.String()) - } + if err := c.RecursiveLock(ctx); err != nil { + t.Fatalf("Unexpected error while doing recursive lock: %v", err) + } + + if err := c.UnlockIfHeld(ctx); err != nil { + t.Fatalf("Unexpected error while unlocking: %v", err) } }