Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pkg/server/api: Return 405s for non-GET requests #240

Merged
merged 1 commit into from
Dec 19, 2018
Merged

pkg/server/api: Return 405s for non-GET requests #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 25 additions & 3 deletions pkg/server/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,14 @@ func NewServerAPIHandler(s Server) *APIHandler {
// ServeHTTP handles the requests for the machine config server
// API handler.
func (sh *APIHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodGet && r.Method != http.MethodHead {
w.Header().Set("Content-Length", "0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point to to a reason why we need to set this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you point to to a reason why we need to set this?

If people start HEADing this server, it seems like they might be interested in getting valid Content-Length back. I just started setting it everywhere to make it easier to test that we get it right when we actually care about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So looking at 5f0a6a7

The defaultHandler gives us our usual empty 404s for requests that
don't match our other handlers.  Without it, we'd have returned Go's
default:

  404 page not found

body, and had unit-test failures like:

  --- FAIL: TestHandler (0.00s)
      --- FAIL: TestHandler/GET_/does-not-exist (0.00s)
          api_test.go:93: response body length 19 does not match Content-Length -1

I makes me go back to to the original thing that we should not setting/expecting Content-Length when we only care about the StatusCode.

So maybe can we remove all the w.Header().Set("Content-Length", "0") and only set it when we care, ie HEAD/GET success result.

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes me go back to to the original thing that we should not setting/expecting Content-Length when we only care about the StatusCode.

If we want to return 404 page not found\n (with Content-Type: text/plain; charset=utf-8?) for all of our 404s, I'm ok with that (although I don't see a need to do it). But I would like to be consistent among our 404s, and not have some that return with no body and some that return with the text/plain body.

Copy link
Contributor

Choose a reason for hiding this comment

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

lets keep it as is for now..

w.WriteHeader(http.StatusMethodNotAllowed)
return
}

if r.URL.Path == "" {
w.Header().Set("Content-Length", "0")
w.WriteHeader(http.StatusBadRequest)
return
}
Expand All @@ -92,18 +98,34 @@ func (sh *APIHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {

conf, err := sh.server.GetConfig(cr)
if err != nil {
w.Header().Set("Content-Length", "0")
w.WriteHeader(http.StatusInternalServerError)
glog.Errorf("couldn't get config for req: %v, error: %v", cr, err)
return
}
if conf == nil && err == nil {
w.Header().Set("Content-Length", "0")
w.WriteHeader(http.StatusNotFound)
return
}

encoder := json.NewEncoder(w)
if err := encoder.Encode(conf); err != nil {
data, err := json.Marshal(conf)
if err != nil {
w.Header().Set("Content-Length", "0")
w.WriteHeader(http.StatusInternalServerError)
glog.Errorf("couldn't encode the config for req: %v, error: %v", cr, err)
glog.Errorf("failed to marshal %v config: %v", cr, err)
return
}

w.Header().Set("Content-Length", fmt.Sprintf("%d", len(data)))
w.Header().Set("Content-Type", "application/json")
if r.Method == http.MethodHead {
w.WriteHeader(http.StatusOK)
return
}

_, err = w.Write(data)
if err != nil {
glog.Errorf("failed to write %v response: %v", cr, err)
}
}
127 changes: 106 additions & 21 deletions pkg/server/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package server

import (
"fmt"
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"
Expand All @@ -17,50 +18,134 @@ func (ms *mockServer) GetConfig(pr poolRequest) (*ignv2_2types.Config, error) {
return ms.GetConfigFn(pr)
}

type checkResponse func(t *testing.T, response *http.Response)

type scenario struct {
name string
expectedStatus int
serverFunc func(poolRequest) (*ignv2_2types.Config, error)
name string
request *http.Request
serverFunc func(poolRequest) (*ignv2_2types.Config, error)
checkResponse checkResponse
}

func TestAPIHandler(t *testing.T) {
scenarios := []scenario{
{
name: "not-found",
abhinavdahiya marked this conversation as resolved.
Show resolved Hide resolved
expectedStatus: http.StatusNotFound,
name: "get non-config path that does not exist",
request: httptest.NewRequest(http.MethodGet, "http://testrequest/does-not-exist", nil),
serverFunc: func(poolRequest) (*ignv2_2types.Config, error) {
return nil, nil
},
checkResponse: func(t *testing.T, response *http.Response) {
checkStatus(t, response, http.StatusNotFound)
checkContentLength(t, response, 0)
checkBodyLength(t, response, 0)
},
},
{
name: "internal-server",
expectedStatus: http.StatusInternalServerError,
name: "get config path that does not exist",
request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/does-not-exist", nil),
serverFunc: func(poolRequest) (*ignv2_2types.Config, error) {
return new(ignv2_2types.Config), fmt.Errorf("not acceptable")
},
checkResponse: func(t *testing.T, response *http.Response) {
checkStatus(t, response, http.StatusInternalServerError)
checkContentLength(t, response, 0)
checkBodyLength(t, response, 0)
},
},
{
name: "get config path that exists",
request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/master", nil),
serverFunc: func(poolRequest) (*ignv2_2types.Config, error) {
return new(ignv2_2types.Config), nil
},
checkResponse: func(t *testing.T, response *http.Response) {
checkStatus(t, response, http.StatusOK)
checkContentType(t, response, "application/json")
checkContentLength(t, response, 114)
checkBodyLength(t, response, 114)
},
},
{
name: "head config path that exists",
request: httptest.NewRequest(http.MethodHead, "http://testrequest/config/master", nil),
serverFunc: func(poolRequest) (*ignv2_2types.Config, error) {
return new(ignv2_2types.Config), nil
},
checkResponse: func(t *testing.T, response *http.Response) {
checkStatus(t, response, http.StatusOK)
checkContentType(t, response, "application/json")
checkContentLength(t, response, 114)
checkBodyLength(t, response, 0)
},
},
{
name: "post non-config path that does not exist",
request: httptest.NewRequest(http.MethodPost, "http://testrequest/post", nil),
serverFunc: func(poolRequest) (*ignv2_2types.Config, error) {
return nil, nil
},
checkResponse: func(t *testing.T, response *http.Response) {
checkStatus(t, response, http.StatusMethodNotAllowed)
checkContentLength(t, response, 0)
checkBodyLength(t, response, 0)
},
},
{
name: "success",
expectedStatus: http.StatusOK,
name: "post config path that exists",
request: httptest.NewRequest(http.MethodPost, "http://testrequest/config/master", nil),
serverFunc: func(poolRequest) (*ignv2_2types.Config, error) {
return new(ignv2_2types.Config), nil
},
checkResponse: func(t *testing.T, response *http.Response) {
checkStatus(t, response, http.StatusMethodNotAllowed)
checkContentLength(t, response, 0)
checkBodyLength(t, response, 0)
},
},
}

for i := range scenarios {
req := httptest.NewRequest("GET", "http://testrequest/", nil)
w := httptest.NewRecorder()
ms := &mockServer{
GetConfigFn: scenarios[i].serverFunc,
}
handler := NewServerAPIHandler(ms)
handler.ServeHTTP(w, req)
for _, scenario := range scenarios {
t.Run(scenario.name, func(t *testing.T) {
w := httptest.NewRecorder()
ms := &mockServer{
GetConfigFn: scenario.serverFunc,
}
handler := NewServerAPIHandler(ms)
handler.ServeHTTP(w, scenario.request)

resp := w.Result()
resp := w.Result()
defer resp.Body.Close()
scenario.checkResponse(t, resp)
})
}
}

if resp.StatusCode != scenarios[i].expectedStatus {
t.Errorf("API Handler test failed for: %s, expected: %d, received: %d", scenarios[i].name, scenarios[i].expectedStatus, resp.StatusCode)
}
func checkStatus(t *testing.T, response *http.Response, expected int) {
if response.StatusCode != expected {
t.Errorf("expected response status %d, received %d", expected, response.StatusCode)
}
}

func checkContentType(t *testing.T, response *http.Response, expected string) {
actual := response.Header.Get("Content-Type")
if actual != expected {
t.Errorf("expected response Content-Type %q, received %q", expected, actual)
}
}

func checkContentLength(t *testing.T, response *http.Response, l int) {
if int(response.ContentLength) != l {
t.Errorf("expected response Content-Length %d, received %d", l, int(response.ContentLength))
}
}

func checkBodyLength(t *testing.T, response *http.Response, l int) {
body, err := ioutil.ReadAll(response.Body)
if err != nil {
t.Fatal(err)
}
if len(body) != l {
t.Errorf("expected response body length %d, received %d", l, len(body))
}
}