Add keyring http endpoints #2509

kyhavlov · 2016-11-15T02:56:42Z

First pass at adding the keyring endpoints for #2502

kyhavlov · 2016-11-15T02:58:08Z

api/operator.go

@@ -79,3 +99,61 @@ func (op *Operator) RaftRemovePeerByAddress(address string, q *WriteOptions) err
 	resp.Body.Close()
 	return nil
 }
+
+// KeyringInstall is used to install a new gossip encryption key into the cluster
+func (op *Operator) KeyringInstall(key string) error {


Should these functions take an argument for overriding the ACL token used? Some of the other api functions do, but some don't so I wasn't sure.

No, but I'd q *QueryOptions and q *WriteOptionsarguments to these methods and get the token from there. It kind of sucks that there's some extra fields in those that don't matter here, but that's kind of the common signature for these. We could add a helper that errors if there are things that are not supported, but we can just document for now.

kyhavlov · 2016-11-15T02:59:55Z

command/agent/operator_endpoint.go

+		if response.Error != "" {
+			return nil, fmt.Errorf(response.Error)
+		}
+	}


Is just returning the first error fine for these KeyringResponses? I thought about using multierror but it didn't seem useful because you'd just get a bunch of duplicate errors ("key must be 16/24/32 length" and the like)

Yeah that makes sense. We could dedup them into a multierror maybe so you can tell if you have a handful of problems vs. having to run several times, since a short list would be super useful.

slackpad

Looks good - put a few comments in the review!

slackpad · 2016-11-22T00:08:15Z

command/agent/operator_endpoint.go

+		if response.Error != "" {
+			return nil, fmt.Errorf(response.Error)
+		}
+	}


Yeah that makes sense. We could dedup them into a multierror maybe so you can tell if you have a handful of problems vs. having to run several times, since a short list would be super useful.

slackpad · 2016-11-22T00:22:15Z

command/agent/http.go

@@ -291,6 +291,10 @@ func (s *HTTPServer) registerHandlers(enableDebug bool) {
 	s.handleFuncMetrics("/v1/kv/", s.wrap(s.KVSEndpoint))
 	s.handleFuncMetrics("/v1/operator/raft/configuration", s.wrap(s.OperatorRaftConfiguration))
 	s.handleFuncMetrics("/v1/operator/raft/peer", s.wrap(s.OperatorRaftPeer))
+	s.handleFuncMetrics("/v1/operator/keyring/install", s.wrap(s.OperatorKeyringInstall))


I'm thinking we can get rid of these /verb suffixes in the URLs and use HTTP verbs for everything:

GET /v1/operator/keyring -> list keys
POST /v1/operator/keyring -> Install a key
PUT /v1/operator/keyring -> use a key
DELETE /v1/operator/keyring -> remove a key

Still not super REST-y since we don't have the key in the path, but looks more REST-y and keeps the API a little smaller.

slackpad · 2016-11-22T00:30:02Z

command/agent/operator_endpoint.go

+		return nil, nil
+	}
+
+	var args structs.KeyringRequest


This has extra fields that we wouldn't ever want to plumb directly (there's currently a panic() in the internal endpoint handler that we should change to an error if the Op isn't understood), so I'd make a simple unexported struct (define in here is fine) and a local string for the token and use those. There's stuff we don't use in this big struct so it's confusing to accept that here.

slackpad · 2016-11-22T00:34:30Z

command/agent/operator_endpoint_test.go

+			t.Fatalf("err: %s", err)
+		}
+
+		for _, response := range listResponse.Responses {


Length check here?

slackpad · 2016-11-22T00:35:13Z

command/agent/operator_endpoint_test.go

+			t.Fatalf("err: %v", !ok)
+		}
+
+		// Check that we get both a LAN and WAN response, and that they both only


Didn't check a WAN vs. LAN (prolly count and compare at the end).

slackpad · 2016-11-22T00:36:53Z

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

+Available in Consul 0.7.2 and later, the keyring remove endpoint supports the
+`PUT` method.
+
+#### PUT Method


This is a DELETE.

slackpad · 2016-11-22T00:42:54Z

api/operator.go

@@ -43,6 +43,26 @@ type RaftConfiguration struct {
 	Index uint64
 }

+// KeyringOpts is used for performing Keyring operations
+type KeyringOpts struct {


I'd make this an unexported struct keyringRequest since it's not part of your interface for the actual API functions.

slackpad · 2016-11-22T00:45:23Z

api/operator.go

@@ -79,3 +99,61 @@ func (op *Operator) RaftRemovePeerByAddress(address string, q *WriteOptions) err
 	resp.Body.Close()
 	return nil
 }
+
+// KeyringInstall is used to install a new gossip encryption key into the cluster
+func (op *Operator) KeyringInstall(key string) error {


No, but I'd q *QueryOptions and q *WriteOptionsarguments to these methods and get the token from there. It kind of sucks that there's some extra fields in those that don't matter here, but that's kind of the common signature for these. We could add a helper that errors if there are things that are not supported, but we can just document for now.

slackpad · 2016-11-22T00:49:15Z

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

+
+A JSON body is returned that looks like this:
+
+```javascript


It might be nice to add an endpoint here (or in the API client based on this) that summarizes the keys that have been successfully installed everywhere and are safe to switch to. Probably not in this pass, but it would be nice to know as an operator in a very high level way "this key is good to go".

kyhavlov · 2016-11-22T23:32:23Z

Addressed comments, should be ready for another look

slackpad

Just a couple trivial things and this is good to commit.

slackpad · 2016-11-22T23:44:40Z

api/operator.go

@@ -43,6 +43,26 @@ type RaftConfiguration struct {
 	Index uint64
 }

+// keyringRequest is used for performing Keyring operations
+type keyringRequest struct {
+	Key string `json:",omitempty"`


Do you need the omitempty for anything?

I can't remember why I added this - it's the only field in the struct and we only ever decode to it, so there's no reason for it to be in there

slackpad · 2016-11-22T23:48:18Z

command/agent/operator_endpoint_test.go

+			t.Fatalf("bad: %d", len(responses))
+		}
+
+		// WAN


Is this order deterministic?

Yes, it's consistent; here we call executeKeyringOp with wan = true, then with wan = false, and in this test we always get back the wan reply first.

slackpad · 2016-11-22T23:50:25Z

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

+into the cluster. There is more information on gossip encryption available
+[here](/docs/agent/encryption.html#gossip-encryption).
+
+The register endpoint expects a JSON request body to be PUT. The request


These descriptions all say PUT - need to update with the right verbs.

slackpad · 2016-11-23T01:02:14Z

LGTM

Add keyring http endpoints

24942f5

kyhavlov commented Nov 15, 2016

View reviewed changes

kyhavlov added 2 commits November 15, 2016 15:33

Add keyring api website doc sections

babe41c

Add version specific note to keyring http docs

3b8b6ae

slackpad added this to the 0.7.2 milestone Nov 18, 2016

slackpad suggested changes Nov 22, 2016

View reviewed changes

slackpad reviewed Nov 22, 2016

View reviewed changes

Combine keyring endpoints into one

727dc87

slackpad suggested changes Nov 22, 2016

View reviewed changes

Fix keyring doc method wording

567f188

slackpad approved these changes Nov 23, 2016

View reviewed changes

kyhavlov merged commit dcdadd0 into master Nov 23, 2016

kyhavlov deleted the f-keyring-api branch November 23, 2016 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keyring http endpoints #2509

Add keyring http endpoints #2509

kyhavlov commented Nov 15, 2016 •

edited

Loading

kyhavlov Nov 15, 2016 •

edited

Loading

slackpad Nov 22, 2016

kyhavlov Nov 15, 2016

slackpad Nov 22, 2016

slackpad left a comment

slackpad Nov 22, 2016

slackpad Nov 22, 2016

slackpad Nov 22, 2016

slackpad Nov 22, 2016

slackpad Nov 22, 2016

slackpad Nov 22, 2016

slackpad Nov 22, 2016

slackpad Nov 22, 2016

slackpad Nov 22, 2016

kyhavlov commented Nov 22, 2016

slackpad left a comment

slackpad Nov 22, 2016

kyhavlov Nov 22, 2016

slackpad Nov 22, 2016

kyhavlov Nov 22, 2016

slackpad Nov 22, 2016

slackpad commented Nov 23, 2016

Add keyring http endpoints #2509

Add keyring http endpoints #2509

Conversation

kyhavlov commented Nov 15, 2016 • edited Loading

kyhavlov Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slackpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyhavlov commented Nov 22, 2016

slackpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slackpad commented Nov 23, 2016

kyhavlov commented Nov 15, 2016 •

edited

Loading

kyhavlov Nov 15, 2016 •

edited

Loading