Skip to content

Commit

Permalink
agent: consolidate handling of 405 Method Not Allowed (#3405)
Browse files Browse the repository at this point in the history 
* agent: consolidate http method not allowed checks

This patch uses the error handling of the http handlers to handle HTTP
method not allowed errors across all available endpoints. It also adds a
test for testing whether the endpoints respond with the correct status
code.

* agent: do not panic on metrics tests

* agent: drop other tests for MethodNotAllowed

* agent: align /agent/join with reality

/agent/join uses PUT instead of GET as documented.

* agent: align /agent/check/{fail,warn,pass} with reality

/agent/check/{fail,warn,pass} uses PUT instead of GET as documented.

* fix some tests

* Drop more tests for method not allowed

* Align TestAgent_RegisterService_InvalidAddress with reality

* Changes API client join to use PUT instead of GET.

* Fixes agent endpoint verbs and removes obsolete tests.

* Updates the change log.
  • Loading branch information
@magiconair @slackpad
magiconair authored and slackpad committed Sep 26, 2017
1 parent 5143218 commit 1e46111
Show file tree
Hide file tree
Showing 27 changed files with 447 additions and 265 deletions.
43 changes: 42 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,48 @@

BREAKING CHANGES:

* **Raft Protocol Defaults to 3:** The [`-raft-protocol`](https://www.consul.io/docs/agent/options.html#_raft_protocol) default has been changed from 2 to 3, enabling all Autopilot features by default. Version 3 requires Consul running 0.8.0 or newer on all servers in order to work, so if you are upgrading with older servers in a cluster then you will need to set this back to 2 in order to upgrade. See [Raft Protocol Version Compatibility](https://www.consul.io/docs/upgrade-specific.html#raft-protocol-version-compatibility) for more details. Also the format of `peers.json` used for outage recovery is different when running with the lastest Raft protocol. See [Manual Recovery Using peers.json](https://www.consul.io/docs/guides/outage.html#manual-recovery-using-peers-json) for a description of the required format.
* **Raft Protocol Defaults to 3:** The [`-raft-protocol`](https://www.consul.io/docs/agent/options.html#_raft_protocol) default has been changed from 2 to 3, enabling all Autopilot features by default. Version 3 requires Consul running 0.8.0 or newer on all servers in order to work, so if you are upgrading with older servers in a cluster then you will need to set this back to 2 in order to upgrade. See [Raft Protocol Version Compatibility](https://www.consul.io/docs/upgrade-specific.html#raft-protocol-version-compatibility) for more details. Also the format of `peers.json` used for outage recovery is different when running with the lastest Raft protocol. See [Manual Recovery Using peers.json](https://www.consul.io/docs/guides/outage.html#manual-recovery-using-peers-json) for a description of the required format. [GH-3477]
* **HTTP Verb Enforcement:** Many endpoints in the HTTP API that previously took any HTTP verb now check for specific HTTP verbs and enforce them. This may break clients relying on the old behavior. The table below has the endpoints that were updated. [GH-3405]
| Endpoint | Required Verbs |
| -------- | -------------- |
| /v1/acl/info | GET |
| /v1/acl/list | GET |
| /v1/acl/replication | GET |
| /v1/agent/check/deregister | PUT |
| /v1/agent/check/fail | PUT |
| /v1/agent/check/pass | PUT |
| /v1/agent/check/register | PUT |
| /v1/agent/check/warn | PUT |
| /v1/agent/checks | GET |
| /v1/agent/force-leave | PUT |
| /v1/agent/join | PUT |
| /v1/agent/members | GET |
| /v1/agent/metrics | GET |
| /v1/agent/self | GET |
| /v1/agent/service/register | PUT |
| /v1/agent/service/deregister | PUT |
| /v1/agent/services | GET |
| /v1/catalog/datacenters | GET |
| /v1/catalog/deregister | PUT |
| /v1/catalog/node | GET |
| /v1/catalog/nodes | GET |
| /v1/catalog/register | PUT |
| /v1/catalog/service | GET |
| /v1/catalog/services | GET |
| /v1/coordinate/datacenters | GET |
| /v1/coordinate/nodes | GET |
| /v1/health/checks | GET |
| /v1/health/node | GET |
| /v1/health/service | GET |
| /v1/health/state | GET |
| /v1/internal/ui/node | GET |
| /v1/internal/ui/nodes | GET |
| /v1/internal/ui/services | GET |
| /v1/session/info | GET |
| /v1/session/list | GET |
| /v1/session/node | GET |
| /v1/status/leader | GET |
| /v1/status/peers | GET |

FEATURES:

Expand Down
33 changes: 22 additions & 11 deletions agent/acl_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ func ACLDisabled(resp http.ResponseWriter, req *http.Request) (interface{}, erro
// a cluster to get the first management token.
func (s *HTTPServer) ACLBootstrap(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

args := structs.DCSpecificRequest{
Expand All @@ -49,10 +48,8 @@ func (s *HTTPServer) ACLBootstrap(resp http.ResponseWriter, req *http.Request) (
}

func (s *HTTPServer) ACLDestroy(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
// Mandate a PUT request
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

args := structs.ACLRequest{
Expand All @@ -77,18 +74,22 @@ func (s *HTTPServer) ACLDestroy(resp http.ResponseWriter, req *http.Request) (in
}

func (s *HTTPServer) ACLCreate(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}
return s.aclSet(resp, req, false)
}

func (s *HTTPServer) ACLUpdate(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}
return s.aclSet(resp, req, true)
}

func (s *HTTPServer) aclSet(resp http.ResponseWriter, req *http.Request, update bool) (interface{}, error) {
// Mandate a PUT request
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

args := structs.ACLRequest{
Expand Down Expand Up @@ -128,10 +129,8 @@ func (s *HTTPServer) aclSet(resp http.ResponseWriter, req *http.Request, update
}

func (s *HTTPServer) ACLClone(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
// Mandate a PUT request
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

args := structs.ACLSpecificRequest{
Expand Down Expand Up @@ -182,6 +181,10 @@ func (s *HTTPServer) ACLClone(resp http.ResponseWriter, req *http.Request) (inte
}

func (s *HTTPServer) ACLGet(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

args := structs.ACLSpecificRequest{
Datacenter: s.agent.config.ACLDatacenter,
}
Expand Down Expand Up @@ -212,6 +215,10 @@ func (s *HTTPServer) ACLGet(resp http.ResponseWriter, req *http.Request) (interf
}

func (s *HTTPServer) ACLList(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

args := structs.DCSpecificRequest{
Datacenter: s.agent.config.ACLDatacenter,
}
Expand All @@ -234,6 +241,10 @@ func (s *HTTPServer) ACLList(resp http.ResponseWriter, req *http.Request) (inter
}

func (s *HTTPServer) ACLReplicationStatus(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

// Note that we do not forward to the ACL DC here. This is a query for
// any DC that's doing replication.
args := structs.DCSpecificRequest{}
Expand Down
1 change: 0 additions & 1 deletion agent/acl_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ func TestACL_Bootstrap(t *testing.T) {
code int
token bool
}{
{"bad method", "GET", http.StatusMethodNotAllowed, false},
{"bootstrap", "PUT", http.StatusOK, true},
{"not again", "PUT", http.StatusForbidden, false},
}
Expand Down
80 changes: 63 additions & 17 deletions agent/agent_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ type Self struct {
}

func (s *HTTPServer) AgentSelf(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

var cs lib.CoordinateSet
if !s.agent.config.DisableCoordinates {
var err error
Expand Down Expand Up @@ -58,6 +62,10 @@ func (s *HTTPServer) AgentSelf(resp http.ResponseWriter, req *http.Request) (int
}

func (s *HTTPServer) AgentMetrics(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

// Fetch the ACL token, if any, and enforce agent policy.
var token string
s.parseToken(req, &token)
Expand All @@ -74,8 +82,7 @@ func (s *HTTPServer) AgentMetrics(resp http.ResponseWriter, req *http.Request) (

func (s *HTTPServer) AgentReload(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

// Fetch the ACL token, if any, and enforce agent policy.
Expand Down Expand Up @@ -107,6 +114,10 @@ func (s *HTTPServer) AgentReload(resp http.ResponseWriter, req *http.Request) (i
}

func (s *HTTPServer) AgentServices(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

// Fetch the ACL token, if any.
var token string
s.parseToken(req, &token)
Expand All @@ -127,6 +138,10 @@ func (s *HTTPServer) AgentServices(resp http.ResponseWriter, req *http.Request)
}

func (s *HTTPServer) AgentChecks(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

// Fetch the ACL token, if any.
var token string
s.parseToken(req, &token)
Expand All @@ -147,6 +162,10 @@ func (s *HTTPServer) AgentChecks(resp http.ResponseWriter, req *http.Request) (i
}

func (s *HTTPServer) AgentMembers(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "GET" {
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

// Fetch the ACL token, if any.
var token string
s.parseToken(req, &token)
Expand Down Expand Up @@ -192,6 +211,10 @@ func (s *HTTPServer) AgentMembers(resp http.ResponseWriter, req *http.Request) (
}

func (s *HTTPServer) AgentJoin(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

// Fetch the ACL token, if any, and enforce agent policy.
var token string
s.parseToken(req, &token)
Expand Down Expand Up @@ -221,8 +244,7 @@ func (s *HTTPServer) AgentJoin(resp http.ResponseWriter, req *http.Request) (int

func (s *HTTPServer) AgentLeave(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

// Fetch the ACL token, if any, and enforce agent policy.
Expand All @@ -243,6 +265,10 @@ func (s *HTTPServer) AgentLeave(resp http.ResponseWriter, req *http.Request) (in
}

func (s *HTTPServer) AgentForceLeave(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

// Fetch the ACL token, if any, and enforce agent policy.
var token string
s.parseToken(req, &token)
Expand Down Expand Up @@ -270,6 +296,10 @@ func (s *HTTPServer) syncChanges() {
const invalidCheckMessage = "Must provide TTL or Script/DockerContainerID/HTTP/TCP and Interval"

func (s *HTTPServer) AgentRegisterCheck(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

var args structs.CheckDefinition
// Fixup the type decode of TTL or Interval.
decodeCB := func(raw interface{}) error {
Expand Down Expand Up @@ -321,6 +351,10 @@ func (s *HTTPServer) AgentRegisterCheck(resp http.ResponseWriter, req *http.Requ
}

func (s *HTTPServer) AgentDeregisterCheck(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/deregister/"))

// Get the provided token, if any, and vet against any ACL policies.
Expand All @@ -338,6 +372,10 @@ func (s *HTTPServer) AgentDeregisterCheck(resp http.ResponseWriter, req *http.Re
}

func (s *HTTPServer) AgentCheckPass(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/pass/"))
note := req.URL.Query().Get("note")

Expand All @@ -356,6 +394,10 @@ func (s *HTTPServer) AgentCheckPass(resp http.ResponseWriter, req *http.Request)
}

func (s *HTTPServer) AgentCheckWarn(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/warn/"))
note := req.URL.Query().Get("note")

Expand All @@ -374,6 +416,10 @@ func (s *HTTPServer) AgentCheckWarn(resp http.ResponseWriter, req *http.Request)
}

func (s *HTTPServer) AgentCheckFail(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/fail/"))
note := req.URL.Query().Get("note")

Expand Down Expand Up @@ -408,8 +454,7 @@ type checkUpdate struct {
// APIs.
func (s *HTTPServer) AgentCheckUpdate(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

var update checkUpdate
Expand Down Expand Up @@ -452,6 +497,10 @@ func (s *HTTPServer) AgentCheckUpdate(resp http.ResponseWriter, req *http.Reques
}

func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

var args structs.ServiceDefinition
// Fixup the type decode of TTL or Interval if a check if provided.
decodeCB := func(raw interface{}) error {
Expand Down Expand Up @@ -535,6 +584,10 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re
}

func (s *HTTPServer) AgentDeregisterService(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/deregister/")

// Get the provided token, if any, and vet against any ACL policies.
Expand All @@ -552,10 +605,8 @@ func (s *HTTPServer) AgentDeregisterService(resp http.ResponseWriter, req *http.
}

func (s *HTTPServer) AgentServiceMaintenance(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
// Only PUT supported
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

// Ensure we have a service ID
Expand Down Expand Up @@ -608,10 +659,8 @@ func (s *HTTPServer) AgentServiceMaintenance(resp http.ResponseWriter, req *http
}

func (s *HTTPServer) AgentNodeMaintenance(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
// Only PUT supported
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

// Ensure we have some action
Expand Down Expand Up @@ -651,10 +700,8 @@ func (s *HTTPServer) AgentNodeMaintenance(resp http.ResponseWriter, req *http.Re
}

func (s *HTTPServer) AgentMonitor(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
// Only GET supported.
if req.Method != "GET" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"GET"}}
}

// Fetch the ACL token, if any, and enforce agent policy.
Expand Down Expand Up @@ -741,8 +788,7 @@ func (h *httpLogHandler) HandleLog(log string) {

func (s *HTTPServer) AgentToken(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if req.Method != "PUT" {
resp.WriteHeader(http.StatusMethodNotAllowed)
return nil, nil
return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
}

// Fetch the ACL token, if any, and enforce agent policy.
Expand Down
Loading

0 comments on commit 1e46111

Please sign in to comment.