Merge pull request #909 from hashicorp/f-create

Support ACL upsert behavior

command/agent/acl_endpoint.go

@@ -2,9 +2,10 @@ package agent
 
 import (
 	"fmt"
-	"github.com/hashicorp/consul/consul/structs"
 	"net/http"
 	"strings"
+
+	"github.com/hashicorp/consul/consul/structs"
 )
 
 // aclCreateResponse is used to wrap the ACL ID
@@ -80,14 +81,8 @@ func (s *HTTPServer) aclSet(resp http.ResponseWriter, req *http.Request, update
 		}
 	}
 
-	// Ensure there is no ID set for create
+	// Ensure there is an ID set for update. ID is optional for
-	if !update && args.ACL.ID != "" {
+	// create, as one will be generated if not provided.
-		resp.WriteHeader(400)
-		resp.Write([]byte(fmt.Sprintf("ACL ID cannot be set")))
-		return nil, nil
-	}
-
-	// Ensure there is an ID set for update
 	if update && args.ACL.ID == "" {
 		resp.WriteHeader(400)
 		resp.Write([]byte(fmt.Sprintf("ACL ID must be set")))

command/agent/acl_endpoint_test.go

@@ -3,10 +3,11 @@ package agent
 import (
 	"bytes"
 	"encoding/json"
-	"github.com/hashicorp/consul/consul/structs"
 	"net/http"
 	"net/http/httptest"
 	"testing"
+
+	"github.com/hashicorp/consul/consul/structs"
 )
 
 func makeTestACL(t *testing.T, srv *HTTPServer) string {
@@ -62,6 +63,34 @@ func TestACLUpdate(t *testing.T) {
 	})
 }
 
+func TestACLUpdate_Upsert(t *testing.T) {
+	httpTest(t, func(srv *HTTPServer) {
+		body := bytes.NewBuffer(nil)
+		enc := json.NewEncoder(body)
+		raw := map[string]interface{}{
+			"ID":    "my-old-id",
+			"Name":  "User Token 2",
+			"Type":  "client",
+			"Rules": "",
+		}
+		enc.Encode(raw)
+
+		req, err := http.NewRequest("PUT", "/v1/acl/update?token=root", body)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		resp := httptest.NewRecorder()
+		obj, err := srv.ACLUpdate(resp, req)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		aclResp := obj.(aclCreateResponse)
+		if aclResp.ID != "my-old-id" {
+			t.Fatalf("bad: %v", aclResp)
+		}
+	})
+}
+
 func TestACLDestroy(t *testing.T) {
 	httpTest(t, func(srv *HTTPServer) {
 		id := makeTestACL(t, srv)

consul/acl_endpoint.go

@@ -55,22 +55,12 @@ func (a *ACL) Apply(args *structs.ACLRequest, reply *string) error {
 			return fmt.Errorf("ACL rule compilation failed: %v", err)
 		}
 
-		// Check if this is an update
+		// If no ID is provided, generate a new ID. This must
-		state := a.srv.fsm.State()
-		var existing *structs.ACL
-		if args.ACL.ID != "" {
-			_, existing, err = state.ACLGet(args.ACL.ID)
-			if err != nil {
-				a.srv.logger.Printf("[ERR] consul.acl: ACL lookup failed: %v", err)
-				return err
-			}
-		}
-
-		// If this is a create, generate a new ID. This must
 		// be done prior to appending to the raft log, because the ID is not
 		// deterministic. Once the entry is in the log, the state update MUST
 		// be deterministic or the followers will not converge.
-		if existing == nil {
+		if args.ACL.ID == "" {
+			state := a.srv.fsm.State()
 			for {
 				args.ACL.ID = generateUUID()
 				_, acl, err := state.ACLGet(args.ACL.ID)

consul/acl_endpoint_test.go

@@ -148,6 +148,53 @@ func TestACLEndpoint_Update_PurgeCache(t *testing.T) {
 	}
 }
 
+func TestACLEndpoint_Apply_CustomID(t *testing.T) {
+	dir1, s1 := testServerWithConfig(t, func(c *Config) {
+		c.ACLDatacenter = "dc1"
+		c.ACLMasterToken = "root"
+	})
+	defer os.RemoveAll(dir1)
+	defer s1.Shutdown()
+	client := rpcClient(t, s1)
+	defer client.Close()
+
+	testutil.WaitForLeader(t, client.Call, "dc1")
+
+	arg := structs.ACLRequest{
+		Datacenter: "dc1",
+		Op:         structs.ACLSet,
+		ACL: structs.ACL{
+			ID:   "foobarbaz", // Specify custom ID, does not exist
+			Name: "User token",
+			Type: structs.ACLTypeClient,
+		},
+		WriteRequest: structs.WriteRequest{Token: "root"},
+	}
+	var out string
+	if err := client.Call("ACL.Apply", &arg, &out); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if out != "foobarbaz" {
+		t.Fatalf("bad token ID: %s", out)
+	}
+
+	// Verify
+	state := s1.fsm.State()
+	_, s, err := state.ACLGet(out)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if s == nil {
+		t.Fatalf("should not be nil")
+	}
+	if s.ID != out {
+		t.Fatalf("bad: %v", s)
+	}
+	if s.Name != "User token" {
+		t.Fatalf("bad: %v", s)
+	}
+}
+
 func TestACLEndpoint_Apply_Denied(t *testing.T) {
 	dir1, s1 := testServerWithConfig(t, func(c *Config) {
 		c.ACLDatacenter = "dc1"

website/source/docs/agent/http/acl.html.markdown

@@ -54,6 +54,10 @@ None of the fields are mandatory. In fact, no body needs to be PUT if the
 defaults are to be used. The `Name` and `Rules` fields default to being
 blank, and the `Type` defaults to "client".
 
+The `ID` field may be provided, and if omitted a random UUID will be generated.
+The security of the ACL system depends on the difficulty of guessing the token.
+Tokens should not be generated in a predictable manner or with too little entropy.
+
 The format of the `Rules` property is [documented here](/docs/internals/acl.html).
 
 A successful response body will return the `ID` of the newly created ACL, like so:
@@ -69,8 +73,8 @@ A successful response body will return the `ID` of the newly created ACL, like s
 The update endpoint is used to modify the policy for a given ACL token. It
 is very similar to the create endpoint; however, instead of generating a new
 token ID, the `ID` field must be provided. As with [`/v1/acl/create`](#acl_create),
-requests to this endpoint must be made with a management
+requests to this endpoint must be made with a management token. If the ID does not
-token.
+exist, the ACL will be upserted. In this sense, create and update are identical.
 
 In any Consul cluster, only a single datacenter is authoritative for ACLs, so
 all requests are automatically routed to that datacenter regardless