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

wking · 2018-12-16T07:57:50Z

We don't want folks POSTing, etc. to this server. With this pull-request, we'll return the appropriate response status code if they do.

abhinavdahiya · 2018-12-16T15:26:12Z

Can you add tests to api_test.go

wking · 2018-12-16T15:36:15Z

I'd have to extend scenario. Narrowly to add a method property? Broadly to add a request property (which would also allow testing of #239)?

abhinavdahiya · 2018-12-16T16:30:48Z

/hold

Each sub-handler would be switching on Path anyway, and we don't have so many paths for the muxer's extra level of path switching to help much. And it's nice to only have to put things like [1]'s method guard in one spot (vs. repeating in each of the muxer's sub-handlers). [1]: openshift#240

wking · 2018-12-17T05:02:17Z

Rebased onto #242 and added a test with a9e8294 -> 98d3fa2.

ashcrow

👍

wking · 2018-12-17T19:03:18Z

With #242 closed, I've rebased this onto master again and added a unit test based on Requests with 98d3fa2 -> 2dc59a3.

pkg/server/api.go

abhinavdahiya · 2018-12-17T19:43:37Z

/hold cancel

wking · 2018-12-17T20:16:26Z

e2e-aws:

level=error msg="1 error occurred:"
level=error msg="\t* module.vpc.aws_vpc_endpoint.s3: 1 error occurred:"
level=error msg="\t* aws_vpc_endpoint.s3: Error creating VPC Endpoint: VpcEndpointLimitExceeded: The maximum number of VPC endpoints has been reached."
level=error msg="\tstatus code: 400, request id: e86bbbe2-9fc6-4177-ba9c-ad2a98f8030b"

James is working on getting that limit bumped, but until then it's just luck ;). I'll wait until the queue calms down tonight before retesting, since this is a fairly minor change.

wking · 2018-12-17T21:40:59Z

Our limit has been bumped.

/retest

abhinavdahiya · 2018-12-17T22:41:38Z

pkg/server/api.go

@@ -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")


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

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.

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.

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.

lets keep it as is for now..

wking · 2018-12-17T23:12:53Z

images failure was openshift/release#2070.

/retest

pkg/server/api_test.go

We don't want folks POSTing, etc. to this server. If they do, this commit will return the appropriate response status code. Also add support for HEAD requests and start setting Content-Length and Content-Type ourselves instead of leaning on Go's autodetection [1]. The application/json media type is from [2]. [1]: https://golang.org/pkg/net/http/#ResponseWriter [2]: https://tools.ietf.org/html/rfc8259#section-1.2

abhinavdahiya · 2018-12-18T19:01:40Z

/lgtm

/retest

openshift-ci-robot · 2018-12-18T19:01:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ashcrow · 2018-12-18T22:20:45Z

/test e2e-aws

openshift-bot · 2018-12-19T01:15:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

…streams-from-progressing Bug 1811143: purge removed imagestreams as a part of upgrade from progessing/impor…

openshift-ci-robot requested review from abhinavdahiya and jlebon December 16, 2018 07:57

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 16, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2018

wking mentioned this pull request Dec 16, 2018

pkg/server/api: Add /healthz for load-balancer health checks #239

Closed

wking mentioned this pull request Dec 17, 2018

pkg/server/api: Drop the muxer #242

Closed

wking force-pushed the server-405-for-non-get-requests branch from a9e8294 to 98d3fa2 Compare December 17, 2018 05:01

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 17, 2018

ashcrow approved these changes Dec 17, 2018

View reviewed changes

wking force-pushed the server-405-for-non-get-requests branch from 98d3fa2 to 2dc59a3 Compare December 17, 2018 19:00

abhinavdahiya reviewed Dec 17, 2018

View reviewed changes

pkg/server/api.go Outdated Show resolved Hide resolved

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2018

wking mentioned this pull request Dec 17, 2018

pkg/server: Trim public API to return *http.Server #246

Closed

wking force-pushed the server-405-for-non-get-requests branch from 2dc59a3 to 22d0c45 Compare December 17, 2018 22:37

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2018

abhinavdahiya reviewed Dec 17, 2018

View reviewed changes

pkg/server/api_test.go Show resolved Hide resolved

abhinavdahiya reviewed Dec 17, 2018

View reviewed changes

pkg/server/api_test.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Dec 17, 2018

View reviewed changes

pkg/server/api_test.go Outdated Show resolved Hide resolved

wking force-pushed the server-405-for-non-get-requests branch 3 times, most recently from 0eebfed to 124fd90 Compare December 18, 2018 02:27

wking force-pushed the server-405-for-non-get-requests branch from 124fd90 to 01d22e1 Compare December 18, 2018 18:18

openshift-ci-robot assigned abhinavdahiya Dec 18, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2018

openshift-merge-robot merged commit 1268da7 into openshift:master Dec 19, 2018

wking deleted the server-405-for-non-get-requests branch December 19, 2018 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wking commented Dec 16, 2018

abhinavdahiya commented Dec 16, 2018

wking commented Dec 16, 2018 •

edited

Loading

abhinavdahiya commented Dec 16, 2018

wking commented Dec 17, 2018 •

edited

Loading

ashcrow left a comment

wking commented Dec 17, 2018

abhinavdahiya commented Dec 17, 2018

wking commented Dec 17, 2018

wking commented Dec 17, 2018

abhinavdahiya Dec 17, 2018

wking Dec 17, 2018

abhinavdahiya Dec 18, 2018

wking Dec 18, 2018

abhinavdahiya Dec 18, 2018

wking commented Dec 17, 2018

abhinavdahiya commented Dec 18, 2018

openshift-ci-robot commented Dec 18, 2018

ashcrow commented Dec 18, 2018

openshift-bot commented Dec 19, 2018

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

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

Conversation

wking commented Dec 16, 2018

abhinavdahiya commented Dec 16, 2018

wking commented Dec 16, 2018 • edited Loading

abhinavdahiya commented Dec 16, 2018

wking commented Dec 17, 2018 • edited Loading

ashcrow left a comment

Choose a reason for hiding this comment

wking commented Dec 17, 2018

abhinavdahiya commented Dec 17, 2018

wking commented Dec 17, 2018

wking commented Dec 17, 2018

abhinavdahiya Dec 17, 2018

Choose a reason for hiding this comment

wking Dec 17, 2018

Choose a reason for hiding this comment

abhinavdahiya Dec 18, 2018

Choose a reason for hiding this comment

wking Dec 18, 2018

Choose a reason for hiding this comment

abhinavdahiya Dec 18, 2018

Choose a reason for hiding this comment

wking commented Dec 17, 2018

abhinavdahiya commented Dec 18, 2018

openshift-ci-robot commented Dec 18, 2018

ashcrow commented Dec 18, 2018

openshift-bot commented Dec 19, 2018

wking commented Dec 16, 2018 •

edited

Loading

wking commented Dec 17, 2018 •

edited

Loading