added tests that the API doesn't leak Node.SecretID

added more documentation on JSON encoding to the contributing guide

api/event_stream_test.go

@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/nomad/api/internal/testutil"
+	"github.com/mitchellh/mapstructure"
 	"github.com/stretchr/testify/require"
 )
 
@@ -148,9 +149,24 @@ func TestEventStream_PayloadValue(t *testing.T) {
 			require.NoError(t, err)
 		}
 		for _, e := range event.Events {
+			// verify that we get a node
 			n, err := e.Node()
 			require.NoError(t, err)
 			require.NotEqual(t, "", n.ID)
+
+			// raw decoding to verify that the node did not contain SecretID
+			raw := make(map[string]map[string]interface{}, 0)
+			cfg := &mapstructure.DecoderConfig{
+				Result: &raw,
+			}
+
+			dec, err := mapstructure.NewDecoder(cfg)
+			require.NoError(t, err)
+			require.NoError(t, dec.Decode(e.Payload))
+			require.Contains(t, raw, "Node")
+			rawNode := raw["Node"]
+			require.Equal(t, n.ID, rawNode["ID"])
+			require.NotContains(t, rawNode, "SecretID")
 		}
 	case <-time.After(5 * time.Second):
 		require.Fail(t, "failed waiting for event stream event")

api/nodes_test.go

@@ -178,6 +178,38 @@ func TestNodes_Info(t *testing.T) {
 	}
 }
 
+func TestNodes_NoSecretID(t *testing.T) {
+	t.Parallel()
+	c, s := makeClient(t, nil, func(c *testutil.TestServerConfig) {
+		c.DevMode = true
+	})
+	defer s.Stop()
+	nodes := c.Nodes()
+
+	// Get the node ID
+	var nodeID string
+	testutil.WaitForResult(func() (bool, error) {
+		out, _, err := nodes.List(nil)
+		if err != nil {
+			return false, err
+		}
+		if n := len(out); n != 1 {
+			return false, fmt.Errorf("expected 1 node, got: %d", n)
+		}
+		nodeID = out[0].ID
+		return true, nil
+	}, func(err error) {
+		t.Fatalf("err: %s", err)
+	})
+
+	// Query the node, ensure that .SecretID was not returned by the HTTP server
+	resp := make(map[string]interface{})
+	_, err := c.query("/v1/node/"+nodeID, &resp, nil)
+	require.NoError(t, err)
+	require.Equal(t, nodeID, resp["ID"])
+	require.Empty(t, resp["SecretID"])
+}
+
 func TestNodes_ToggleDrain(t *testing.T) {
 	t.Parallel()
 	require := require.New(t)

command/agent/http_test.go

@@ -29,6 +29,8 @@ import (
 	"github.com/hashicorp/nomad/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	nomadjson "github.com/hashicorp/nomad/nomad/json/handlers"
 )
 
 // makeHTTPServer returns a test server whose logs will be written to
@@ -312,11 +314,11 @@ func testPrettyPrint(pretty string, prettyFmt bool, t *testing.T) {
 	var expected bytes.Buffer
 	var err error
 	if prettyFmt {
-		enc := codec.NewEncoder(&expected, structs.JsonHandlePretty)
+		enc := codec.NewEncoder(&expected, nomadjson.JsonHandlePretty)
 		err = enc.Encode(r)
 		expected.WriteByte('\n')
 	} else {
-		enc := codec.NewEncoder(&expected, structs.JsonHandle)
+		enc := codec.NewEncoder(&expected, nomadjson.JsonHandle)
 		err = enc.Encode(r)
 	}
 	if err != nil {

contributing/checklist-jobspec.md

@@ -13,10 +13,10 @@
   * Note that analogous struct field names should match with `api/` package
   * Test the structs/fields via methods mentioned above
   * Implement and test other logical methods
-* [ ] Add conversion between `api/` and `nomad/structs` in `command/agent/job_endpoint.go`
+* [ ] Add conversion between `api/` and `nomad/structs/` in `command/agent/job_endpoint.go`
   * Add test for conversion
-  * msgpack [encoding](http://ugorji.net/blog/go-codec-primer#drop-in-replacement-for-encoding-json-json-key-in-struct-tag-supported) only uses the [`codec` tag](https://github.com/hashicorp/nomad/blob/v1.0.0/nomad/structs/structs.go#L10557-L10558);
-    the `json` tag is available for customizing API output when encoding `structs` objects
+* [ ] Determine JSON encoding strategy for responses from RPC (see "JSON Encoding" below)
+  * [ ] Write `nomad/structs/` to `api/` conversions if necessary and write tests
 * [ ] Implement diff logic for new structs/fields in `nomad/structs/diff.go`
   * Note that fields must be listed in alphabetical order in `FieldDiff` slices in `nomad/structs/diff_test.go`
   * Add test for diff of new structs/fields
@@ -42,3 +42,29 @@ required in the original `jobspec` package.
 * [ ] Job JSON API entry https://www.nomadproject.io/api/json-jobs.html
 * [ ] Sample Response output in API https://www.nomadproject.io/api/jobs.html
 * [ ] Consider if it needs a guide https://www.nomadproject.io/guides/index.html
+
+## JSON Encoding
+
+As a general rule, HTTP endpoints (under `command/agent/`) will make RPC calls that return structs belonging to 
+`nomad/structs/`. These handlers ultimately return an object that is encoded by the Nomad HTTP server. The encoded form
+needs to match the Nomad API; specifically, it should have the form of the corresponding struct from `api/`. There are
+a few ways that this can be accomplished:
+* directly return the struct from the RPC call, if it has the same shape as the corresponding struct in `api/`. 
+  This is convenient when possible, resulting in the least work for the developer. 
+  Examples of this approach include [GET `/v1/evaluation/:id`](https://github.com/hashicorp/nomad/blob/v1.0.
+  0/command/agent/eval_endpoint.go#L88).
+* convert the struct from the RPC call to the appropriate `api/` struct.
+  This approach is the most developer effort, but it does have a strong guarantee that the HTTP response matches the 
+  API, due to the explicit conversion (assuming proper implementation, which requires tests).
+  Examples of this approach include [GET `/v1/volume/csi/:id`](https://github.com/hashicorp/nomad/blob/v1.0.0/command/agent/csi_endpoint.go#L108)
+* convert to an intermediate struct with the same shape as the `api/` struct.
+  This approach strikes a balance between the former two approaches. 
+  This conversion can be performed in-situ in the agent HTTP handler, as long as the conversion doesn't need to 
+  appear in other handlers. 
+  Otherwise, it is possible to register an extension on the JSON encoding used by the HTTP agent; these extensions
+  can be put in `nomad/json/extensions.go`.
+  
+Note, for simple transformations to encoding (like renaming or neglecting fields), we can use field tags on the structs
+from `nomad/structs`. Our msgpack [encoding](http://ugorji.net/blog/go-codec-primer#drop-in-replacement-for-encoding-json-json-key-in-struct-tag-supported) 
+only uses the [`codec` tag](https://github.com/hashicorp/nomad/blob/v1.0.0/nomad/structs/structs.go#L10557-L10558).
+Therefore, the `json` tag is available for customizing API output when encoding `structs` objects. See `structs.Node.SecretID`, for example.

contributing/checklist-rpc-endpoint.md

@@ -30,7 +30,7 @@ Prefer adding a new message to changing any existing RPC messages.
 
 * [ ] `nomad/core_sched.go` sends many RPCs
   * `ServersMeetMinimumVersion` asserts that the server cluster is
-    upgraded, so use this to gaurd sending the new RPC, else send the old RPC
+    upgraded, so use this to guard sending the new RPC, else send the old RPC
   * Version must match the actual release version!
 
 ## Docs

nomad/state/events_test.go

@@ -94,59 +94,6 @@ func TestEventFromChange_ACLTokenSecretID(t *testing.T) {
 	require.Empty(t, tokenEvent2.ACLToken.SecretID)
 }
 
-// TestEventFromChange_NodeSecretID ensures that a node's secret ID is not
-// included in a node event
-func TestEventFromChange_NodeSecretID(t *testing.T) {
-	t.Parallel()
-	s := TestStateStoreCfg(t, TestStateStorePublisher(t))
-	defer s.StopEventBroker()
-
-	node := mock.Node()
-	require.NotEmpty(t, node.SecretID)
-
-	// Create
-	changes := Changes{
-		Index:   100,
-		MsgType: structs.NodeRegisterRequestType,
-		Changes: memdb.Changes{
-			{
-				Table:  "nodes",
-				Before: nil,
-				After:  node,
-			},
-		},
-	}
-
-	out := eventsFromChanges(s.db.ReadTxn(), changes)
-	require.Len(t, out.Events, 1)
-
-	_, ok := out.Events[0].Payload.(*structs.NodeStreamEvent)
-	require.True(t, ok)
-	// TODO: cgbaker: do we really want to remove this check?
-	// require.Empty(t, nodeEvent.Node.SecretID)
-
-	// Delete
-	changes = Changes{
-		Index:   100,
-		MsgType: structs.NodeDeregisterRequestType,
-		Changes: memdb.Changes{
-			{
-				Table:  "nodes",
-				Before: node,
-				After:  nil,
-			},
-		},
-	}
-
-	out2 := eventsFromChanges(s.db.ReadTxn(), changes)
-	require.Len(t, out2.Events, 1)
-
-	_, ok = out2.Events[0].Payload.(*structs.NodeStreamEvent)
-	require.True(t, ok)
-	// TODO: cgbaker: do we really want to remove this check?
-	// require.Empty(t, nodeEvent2.Node.SecretID)
-}
-
 func TestEventsFromChanges_DeploymentUpdate(t *testing.T) {
 	t.Parallel()
 	s := TestStateStoreCfg(t, TestStateStorePublisher(t))

nomad/structs/structs.go

@@ -1870,6 +1870,7 @@ type Node struct {
 	ComputedClass string
 
 	// DrainStrategy determines the node's draining behavior.
+	// Will be non-nil only while draining.
 	DrainStrategy *DrainStrategy
 
 	// SchedulingEligibility determines whether this node will receive new