diff --git a/CHANGES.md b/CHANGES.md index 96d6930..28add84 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,9 +4,11 @@ Notable changes between versions. ## Latest +## v0.2.0 + * Add Prometheus `/metrics` endpoint ([#4](https://github.com/poseidon/fleetlock/pull/4)) +* Add JSON error responses ([#9](https://github.com/poseidon/fleetlock/pull/9)) * Fix `-version` command output ([#6](https://github.com/poseidon/fleetlock/pull/6)) -* Use JSON output for error responses to match fleetlock specification ([#9](https://github.com/poseidon/fleetlock/pull/9)) ## v0.1.0 diff --git a/examples/k8s/deployment.yaml b/examples/k8s/deployment.yaml index 67d3265..7d889cf 100644 --- a/examples/k8s/deployment.yaml +++ b/examples/k8s/deployment.yaml @@ -17,7 +17,7 @@ spec: serviceAccountName: fleetlock containers: - name: fleetlock - image: quay.io/poseidon/fleetlock:v0.1.0 + image: quay.io/poseidon/fleetlock:v0.2.0 ports: - name: http containerPort: 8080 diff --git a/internal/encode.go b/internal/encode.go new file mode 100644 index 0000000..ce3c737 --- /dev/null +++ b/internal/encode.go @@ -0,0 +1,56 @@ +package fleetlock + +import ( + "encoding/json" + "fmt" + "net/http" +) + +// List of ReplyKind +const ( + KindMethodNotAllowed ReplyKind = "method_not_allowed" + KindMissingHeader ReplyKind = "missing_header" + KindDecodeError ReplyKind = "decode_error" + KindInternalError ReplyKind = "internal_error" + KindLockHeld ReplyKind = "lock_held" +) + +// ReplyKind is used as a Zincati metrics label. +type ReplyKind string + +// Reply represents a Fleetlock protocol reply. +type Reply struct { + // reply identifier + Kind ReplyKind `json:"kind"` + // human-friendly reply message + Value string `json:"value"` +} + +// NewReply creates an Reply with a specific kind and a formatted message. +func NewReply(kind ReplyKind, format string, a ...interface{}) Reply { + return Reply{ + Kind: kind, + Value: fmt.Sprintf(format, a...), + } +} + +// encodeReply writes response with the given Reply and HTTP code. +func encodeReply(w http.ResponseWriter, reply Reply) error { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Content-Type-Options", "nosniff") + + switch reply.Kind { + case KindMethodNotAllowed: + w.WriteHeader(http.StatusMethodNotAllowed) + case KindDecodeError, KindMissingHeader: + w.WriteHeader(http.StatusBadRequest) + case KindInternalError: + w.WriteHeader(http.StatusInternalServerError) + case KindLockHeld: + w.WriteHeader(http.StatusLocked) + default: + w.WriteHeader(http.StatusOK) + } + + return json.NewEncoder(w).Encode(reply) +} diff --git a/internal/encode_test.go b/internal/encode_test.go new file mode 100644 index 0000000..980cead --- /dev/null +++ b/internal/encode_test.go @@ -0,0 +1,60 @@ +package fleetlock + +import ( + "fmt" + "io/ioutil" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestEncodeReply(t *testing.T) { + cases := []struct { + reply Reply + expectedStatus int + expectedResponse string + }{ + { + reply: NewReply(KindMethodNotAllowed, "required method POST"), + expectedStatus: 405, + expectedResponse: `{"kind": "method_not_allowed", "value": "required method POST"}`, + }, + { + reply: NewReply(KindMissingHeader, "missing required header %s: %s", fleetLockHeaderKey, "true"), + expectedStatus: 400, + expectedResponse: `{"kind": "missing_header", "value": "missing required header fleet-lock-protocol: true"}`, + }, + { + reply: NewReply(KindDecodeError, "error decoding message"), + expectedStatus: 400, + expectedResponse: `{"kind": "decode_error", "value": "error decoding message"}`, + }, + { + reply: NewReply(KindInternalError, "error getting reboot lease"), + expectedStatus: 500, + expectedResponse: `{"kind": "internal_error", "value": "error getting reboot lease"}`, + }, + { + reply: NewReply(KindLockHeld, "reboot lease unavailable, held by %s", "e0f3745b108f471cbd4883c6fbed8cdd"), + expectedStatus: 423, + expectedResponse: `{"kind": "lock_held", "value": "reboot lease unavailable, held by e0f3745b108f471cbd4883c6fbed8cdd"}`, + }, + { + reply: NewReply("other", "message"), + expectedStatus: 200, + expectedResponse: `{"kind": "other", "value": "message"}`, + }, + } + for _, c := range cases { + t.Run(fmt.Sprintf("%v-%v", c.reply.Kind, c.reply.Value), func(t *testing.T) { + w := httptest.NewRecorder() + encodeReply(w, c.reply) + + assert.Equal(t, c.expectedStatus, w.Code, "Expected status code %v", c.expectedStatus) + + body, _ := ioutil.ReadAll(w.Body) + assert.JSONEq(t, c.expectedResponse, string(body), "Unexpected JSON output in response") + }) + } +} diff --git a/internal/handlers.go b/internal/handlers.go index f34fa1f..f25b7c6 100644 --- a/internal/handlers.go +++ b/internal/handlers.go @@ -12,7 +12,7 @@ const ( func POSTHandler(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, req *http.Request) { if req.Method != http.MethodPost { - encodeReply(w, NewReply(ErrorMethodNotAllowed, "required method POST")) + encodeReply(w, NewReply(KindMethodNotAllowed, "required method POST")) return } next.ServeHTTP(w, req) @@ -24,7 +24,7 @@ func POSTHandler(next http.Handler) http.Handler { func HeaderHandler(key, value string, next http.Handler) http.Handler { fn := func(w http.ResponseWriter, req *http.Request) { if req.Header.Get(key) != value { - encodeReply(w, NewReply(ErrorMissingHeader, "missing required header %s: %s", key, value)) + encodeReply(w, NewReply(KindMissingHeader, "missing required header %s: %s", key, value)) return } next.ServeHTTP(w, req) diff --git a/internal/reply.go b/internal/reply.go deleted file mode 100644 index 818a006..0000000 --- a/internal/reply.go +++ /dev/null @@ -1,60 +0,0 @@ -package fleetlock - -import ( - "encoding/json" - "fmt" - "net/http" -) - -// ReplyKind is a reply type identifier. -// -// It is designed to be tracked in Zincati metrics. -// Fleetlock specification states that this MUST have a bounded/small cardinality. -type ReplyKind string - -// List of ReplyKind -const ( - ErrorMethodNotAllowed ReplyKind = "method_not_allowed" - ErrorMissingHeader ReplyKind = "missing_header" - ErrorDecodingRequest ReplyKind = "failed_decoding_request" - ErrorInternal ReplyKind = "internal_error" - ErrorLocked ReplyKind = "locked" -) - -// Reply holds data used for fleetlock replies (currently only used for error replies). -type Reply struct { - // reply type identifier - Kind ReplyKind `json:"kind"` - - // human-friendly reply message - Value string `json:"value"` -} - -// NewReply creates an Reply with a specific kind and a formatted message. -func NewReply(kind ReplyKind, format string, a ...interface{}) Reply { - return Reply{ - Kind: kind, - Value: fmt.Sprintf(format, a...), - } -} - -// encodeReply writes a reply with the specified Reply and inferred http status. -func encodeReply(w http.ResponseWriter, fleetlockErr Reply) { - w.Header().Set("Content-Type", "application/json") - w.Header().Set("X-Content-Type-Options", "nosniff") - - switch fleetlockErr.Kind { - case ErrorMethodNotAllowed: - w.WriteHeader(http.StatusMethodNotAllowed) - case ErrorDecodingRequest, ErrorMissingHeader: - w.WriteHeader(http.StatusBadRequest) - case ErrorInternal: - w.WriteHeader(http.StatusInternalServerError) - case ErrorLocked: - w.WriteHeader(http.StatusLocked) - default: - w.WriteHeader(http.StatusInternalServerError) - } - - json.NewEncoder(w).Encode(fleetlockErr) -} diff --git a/internal/reply_test.go b/internal/reply_test.go deleted file mode 100644 index 373171b..0000000 --- a/internal/reply_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package fleetlock - -import ( - "fmt" - "io/ioutil" - "net/http/httptest" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestEncodeReply(t *testing.T) { - cases := []struct { - reply Reply - expectedStatus int - expectedResponse string - }{ - { - reply: NewReply(ErrorLocked, "reboot lease unavailable, held by %s", "e0f3745b108f471cbd4883c6fbed8cdd"), - expectedStatus: 423, - expectedResponse: `{"kind": "locked", "value": "reboot lease unavailable, held by e0f3745b108f471cbd4883c6fbed8cdd"}`, - }, - { - reply: NewReply(ErrorMissingHeader, "missing required header %s: %s", fleetLockHeaderKey, "true"), - expectedStatus: 400, - expectedResponse: `{"kind": "missing_header", "value": "missing required header fleet-lock-protocol: true"}`, - }, - { - reply: NewReply("other", "message"), // Do not use undefined reply kinds! - expectedStatus: 500, - expectedResponse: `{"kind": "other", "value": "message"}`, - }, - } - for _, c := range cases { - t.Run(fmt.Sprintf("%v-%v", c.reply.Kind, c.reply.Value), func(t *testing.T) { - w := httptest.NewRecorder() - encodeReply(w, c.reply) - - assert.Equal(t, c.expectedStatus, w.Code, "Expected status code %v", c.expectedStatus) - - body, _ := ioutil.ReadAll(w.Body) - assert.JSONEq(t, c.expectedResponse, string(body), "Unexpected JSON output in reply") - }) - } -} diff --git a/internal/server.go b/internal/server.go index 81b618f..7e3ca4e 100644 --- a/internal/server.go +++ b/internal/server.go @@ -104,7 +104,7 @@ func (s *Server) lock(w http.ResponseWriter, req *http.Request) { msg, err := decodeMessage(req) if err != nil { s.log.Errorf("fleetlock: error decoding message: %v", err) - encodeReply(w, NewReply(ErrorDecodingRequest, "error decoding message")) + encodeReply(w, NewReply(KindDecodeError, "error decoding message")) } id := msg.ClientParmas.ID group := msg.ClientParmas.Group @@ -124,7 +124,7 @@ func (s *Server) lock(w http.ResponseWriter, req *http.Request) { if err != nil { if !errors.IsNotFound(err) { s.log.Errorf("fleetlock: error getting reboot lease %s: %v", rebootLease.Name(), err) - encodeReply(w, NewReply(ErrorInternal, "error getting reboot lease")) + encodeReply(w, NewReply(KindInternalError, "error getting reboot lease")) return } } @@ -158,7 +158,7 @@ func (s *Server) lock(w http.ResponseWriter, req *http.Request) { // reboot lease held by different node s.log.WithFields(fields).Info("fleetlock: reboot lease unavailable") - encodeReply(w, NewReply(ErrorLocked, "reboot lease unavailable, held by %s", lock.Holder)) + encodeReply(w, NewReply(KindLockHeld, "reboot lease unavailable, held by %s", lock.Holder)) } // unlock attempts to release a reboot lease lock. @@ -167,7 +167,7 @@ func (s *Server) unlock(w http.ResponseWriter, req *http.Request) { msg, err := decodeMessage(req) if err != nil { s.log.Errorf("fleetlock: error decoding message: %v", err) - encodeReply(w, NewReply(ErrorDecodingRequest, "error decoding message")) + encodeReply(w, NewReply(KindDecodeError, "error decoding message")) return } id := msg.ClientParmas.ID @@ -188,7 +188,7 @@ func (s *Server) unlock(w http.ResponseWriter, req *http.Request) { if err != nil { if !errors.IsNotFound(err) { s.log.Errorf("fleetlock: error getting reboot lease %s: %v", rebootLease.Name(), err) - encodeReply(w, NewReply(ErrorInternal, "error getting reboot lease")) + encodeReply(w, NewReply(KindInternalError, "error getting reboot lease")) return } } @@ -204,7 +204,7 @@ func (s *Server) unlock(w http.ResponseWriter, req *http.Request) { err = rebootLease.Update(ctx, update) if err != nil { s.log.WithFields(fields).Errorf("fleetlock: error unlocking reboot lease: %v", err) - encodeReply(w, NewReply(ErrorInternal, "error unlocking reboot lease")) + encodeReply(w, NewReply(KindInternalError, "error unlocking reboot lease")) return } s.metrics.lockState.With(prometheus.Labels{"group": group}).Set(0)