api: add PeeeringList, polish (#12934)

agent/peering_endpoint_test.go

@@ -305,9 +305,6 @@ func TestHTTP_Peering_Delete(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
-	// Insert peerings directly to state store.
-	// Note that the state store holds reference to the underlying
-	// variables; do not modify them after writing.
 	foo := &pbpeering.PeeringWriteRequest{
 		Peering: &pbpeering.Peering{
 			Name:                "foo",
@@ -357,7 +354,6 @@ func TestHTTP_Peering_Delete(t *testing.T) {
 		resp := httptest.NewRecorder()
 		a.srv.h.ServeHTTP(resp, req)
 
-		// TODO(peering): it may be a security concern, but do we want to say 404 here?
 		require.Equal(t, http.StatusOK, resp.Code)
 	})
 }

api/peering.go

@@ -9,14 +9,12 @@ import (
 type PeeringState int32
 
 const (
-	// Undefined represents an unset value for PeeringState during
+	// PeeringStateUndefined represents an unset value for PeeringState during
 	// writes.
-	UNDEFINED PeeringState = 0
+	PeeringStateUndefined PeeringState = 0
-	// INITIAL Initial means a Peering has been initialized and is awaiting
+	// PeeringStateInitial means a Peering has been initialized and is awaiting
 	// acknowledgement from a remote peer.
-	INITIAL PeeringState = 1
+	PeeringStateInitial PeeringState = 1
-	// Active means that the peering connection is active and healthy.
-	// ACTIVE PeeringState = 2
 )
 
 type Peering struct {
@@ -44,9 +42,13 @@ type Peering struct {
 	ModifyIndex uint64
 }
 
-// PeeringRequest is used for Read and Delete HTTP calls.
+type PeeringReadRequest struct {
-// The PeeringReadRequest and PeeringDeleteRequest look the same, so we treat them the same for now
+	Name       string
-type PeeringRequest struct {
+	Partition  string `json:"Partition,omitempty"`
+	Datacenter string
+}
+
+type PeeringDeleteRequest struct {
 	Name       string
 	Partition  string `json:"Partition,omitempty"`
 	Datacenter string
@@ -87,6 +89,10 @@ type PeeringInitiateResponse struct {
 	Status uint32
 }
 
+type PeeringListRequest struct {
+	// future proofing in case we extend List functionality
+}
+
 type Peerings struct {
 	c *Client
 }
@@ -96,14 +102,15 @@ func (c *Client) Peerings() *Peerings {
 	return &Peerings{c: c}
 }
 
-func (p *Peerings) Read(ctx context.Context, name string, q *QueryOptions) (*Peering, *QueryMeta, error) {
+func (p *Peerings) Read(ctx context.Context, peeringReq PeeringReadRequest, q *QueryOptions) (*Peering, *QueryMeta, error) {
-	if name == "" {
+	if peeringReq.Name == "" {
 		return nil, nil, fmt.Errorf("peering name cannot be empty")
 	}
 
-	req := p.c.newRequest("GET", fmt.Sprintf("/v1/peering/%s", name))
+	req := p.c.newRequest("GET", fmt.Sprintf("/v1/peering/%s", peeringReq.Name))
 	req.setQueryOptions(q)
 	req.ctx = ctx
+	req.obj = peeringReq
 
 	rtt, resp, err := p.c.doRequest(req)
 	if err != nil {
@@ -126,7 +133,7 @@ func (p *Peerings) Read(ctx context.Context, name string, q *QueryOptions) (*Pee
 	return &out, qm, nil
 }
 
-func (p *Peerings) Delete(ctx context.Context, peeringReq PeeringRequest, q *QueryOptions) (*PeeringDeleteResponse, *QueryMeta, error) {
+func (p *Peerings) Delete(ctx context.Context, peeringReq PeeringDeleteRequest, q *QueryOptions) (*PeeringDeleteResponse, *QueryMeta, error) {
 	if peeringReq.Name == "" {
 		return nil, nil, fmt.Errorf("peering name cannot be empty")
 	}
@@ -211,3 +218,31 @@ func (p *Peerings) Initiate(ctx context.Context, i PeeringInitiateRequest, wq *W
 
 	return &out, wm, nil
 }
+
+func (p *Peerings) List(ctx context.Context, plr PeeringListRequest, q *QueryOptions) ([]*Peering, *QueryMeta, error) {
+
+	req := p.c.newRequest("GET", "/v1/peerings")
+	req.setQueryOptions(q)
+	req.ctx = ctx
+	req.obj = plr
+
+	rtt, resp, err := p.c.doRequest(req)
+	if err != nil {
+		return nil, nil, err
+	}
+	defer closeResponseBody(resp)
+	if err := requireOK(resp); err != nil {
+		return nil, nil, err
+	}
+
+	qm := &QueryMeta{}
+	parseQueryMeta(resp, qm)
+	qm.RequestTime = rtt
+
+	var out []*Peering
+	if err := decodeBody(resp, &out); err != nil {
+		return nil, nil, err
+	}
+
+	return out, qm, nil
+}

api/peering_test.go

@@ -2,14 +2,134 @@ package api
 
 import (
 	"context"
+	"reflect"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
 
 	"github.com/hashicorp/consul/sdk/testutil"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
 )
 
+const DefaultCtxDuration = 15 * time.Second
+
+func peerExistsInPeerListings(peer *Peering, peerings []*Peering) bool {
+	for _, aPeer := range peerings {
+		isEqual := (peer.PeerID == aPeer.PeerID) &&
+			(reflect.DeepEqual(peer.PeerCAPems, aPeer.PeerCAPems)) &&
+			(peer.PeerServerName == aPeer.PeerServerName) &&
+			(peer.Partition == aPeer.Partition) &&
+			(peer.Name == aPeer.Name) &&
+			(reflect.DeepEqual(peer.PeerServerAddresses, aPeer.PeerServerAddresses)) &&
+			(peer.State == aPeer.State) &&
+			(peer.CreateIndex == aPeer.CreateIndex) &&
+			(peer.ModifyIndex) == aPeer.ModifyIndex
+
+		if isEqual {
+			return true
+		}
+	}
+
+	return false
+}
+
+func TestAPI_Peering_Read_ErrorHandling(t *testing.T) {
+	t.Parallel()
+	c, s := makeClientWithCA(t)
+	defer s.Stop()
+	s.WaitForSerfCheck(t)
+	ctx, cancel := context.WithTimeout(context.Background(), DefaultCtxDuration)
+	defer cancel()
+	peerings := c.Peerings()
+
+	t.Run("call Read with no name", func(t *testing.T) {
+		resp, qm, err := peerings.Read(ctx, PeeringReadRequest{}, nil)
+
+		// basic checks
+		require.EqualError(t, err, "peering name cannot be empty")
+		require.Empty(t, qm)
+		require.Empty(t, resp)
+	})
+
+	t.Run("read peer that does not exist on server", func(t *testing.T) {
+		resp, qm, err := peerings.Read(ctx, PeeringReadRequest{Name: "peer1"}, nil)
+
+		// basic checks
+		require.NotNil(t, err) // 404
+		require.Empty(t, qm)
+		require.Empty(t, resp)
+	})
+}
+
+// TestAPI_Peering_List
+func TestAPI_Peering_List(t *testing.T) {
+	t.Parallel()
+	c, s := makeClientWithCA(t)
+	defer s.Stop()
+	s.WaitForSerfCheck(t)
+	ctx, cancel := context.WithTimeout(context.Background(), DefaultCtxDuration)
+	defer cancel()
+	peerings := c.Peerings()
+
+	// "call List when no peers should exist"
+	resp, qm, err := peerings.List(ctx, PeeringListRequest{}, nil)
+
+	// basic checks
+	require.NoError(t, err)
+	require.NotEmpty(t, qm)
+
+	require.Empty(t, resp) // no peerings so this should be empty
+
+	// "call List when peers are present"
+	resp2, wm, err := peerings.GenerateToken(ctx, PeeringGenerateTokenRequest{PeerName: "peer1"}, nil)
+	require.NoError(t, err)
+	require.NotEmpty(t, wm)
+	require.NotEmpty(t, resp2)
+
+	resp3, wm, err := peerings.GenerateToken(ctx, PeeringGenerateTokenRequest{PeerName: "peer2"}, nil)
+	require.NoError(t, err)
+	require.NotEmpty(t, wm)
+	require.NotEmpty(t, resp3)
+
+	peering1, qm, err2 := peerings.Read(ctx, PeeringReadRequest{Name: "peer1"}, nil)
+	require.NoError(t, err2)
+	require.NotEmpty(t, qm)
+	require.NotEmpty(t, peering1)
+	peering2, qm, err2 := peerings.Read(ctx, PeeringReadRequest{Name: "peer2"}, nil)
+	require.NoError(t, err2)
+	require.NotEmpty(t, qm)
+	require.NotEmpty(t, peering2)
+
+	peeringsList, qm, err := peerings.List(ctx, PeeringListRequest{}, nil)
+	require.NoError(t, err)
+	require.NotEmpty(t, qm)
+
+	require.Equal(t, 2, len(peeringsList))
+	require.True(t, peerExistsInPeerListings(peering1, peeringsList), "expected to find peering in list response")
+	require.True(t, peerExistsInPeerListings(peering2, peeringsList), "expected to find peering in list response")
+
+}
+
+func TestAPI_Peering_GenerateToken(t *testing.T) {
+	t.Parallel()
+	c, s := makeClientWithCA(t)
+	defer s.Stop()
+	s.WaitForSerfCheck(t)
+	ctx, cancel := context.WithTimeout(context.Background(), DefaultCtxDuration)
+	defer cancel()
+	peerings := c.Peerings()
+
+	t.Run("cannot have GenerateToken forward DC requests", func(t *testing.T) {
+		// Try to generate a token in dc2
+		resp, wm, err := peerings.GenerateToken(ctx, PeeringGenerateTokenRequest{PeerName: "peer2", Datacenter: "dc2"}, nil)
+
+		require.Error(t, err)
+		require.Empty(t, wm)
+		require.Empty(t, resp)
+	})
+}
+
 // TODO(peering): cover the following test cases: bad/ malformed input, peering with wrong token,
 // peering with the wrong PeerName
 
@@ -22,7 +142,8 @@ func TestAPI_Peering_GenerateToken_Read_Initiate_Delete(t *testing.T) {
 	defer s.Stop()
 	s.WaitForSerfCheck(t)
 	options := &WriteOptions{Datacenter: "dc1"}
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), DefaultCtxDuration)
+	defer cancel()
 	peerings := c.Peerings()
 
 	p1 := PeeringGenerateTokenRequest{
@@ -38,7 +159,7 @@ func TestAPI_Peering_GenerateToken_Read_Initiate_Delete(t *testing.T) {
 	require.NotEmpty(t, resp)
 
 	// Read token generated on server
-	resp2, qm, err2 := peerings.Read(ctx, "peer1", nil)
+	resp2, qm, err2 := peerings.Read(ctx, PeeringReadRequest{Name: "peer1"}, nil)
 
 	// basic ok checking
 	require.NoError(t, err2)
@@ -47,10 +168,7 @@ func TestAPI_Peering_GenerateToken_Read_Initiate_Delete(t *testing.T) {
 
 	// token specific assertions on the "server"
 	require.Equal(t, "peer1", resp2.Name)
-
+	require.Equal(t, PeeringStateInitial, resp2.State)
-	// TODO(peering) -- split in OSS/ ENT test for "default" vs ""; or revisit PartitionOrEmpty vs PartitionOrDefault
-	// require.Equal(t, "default", resp2.Partition)
-	require.Equal(t, INITIAL, resp2.State)
 
 	// Initiate peering
 
@@ -73,28 +191,26 @@ func TestAPI_Peering_GenerateToken_Read_Initiate_Delete(t *testing.T) {
 	require.NotEmpty(t, wm3)
 
 	// at first the token will be undefined
-	require.Equal(t, UNDEFINED, PeeringState(respi.Status))
+	require.Equal(t, PeeringStateUndefined, PeeringState(respi.Status))
 
 	// wait for the peering backend to finish the peering connection
 	time.Sleep(2 * time.Second)
 
-	respr, qm2, err4 := c2.Peerings().Read(ctx, "peer1", nil)
+	retry.Run(t, func(r *retry.R) {
+		respr, qm2, err4 := c2.Peerings().Read(ctx, PeeringReadRequest{Name: "peer1"}, nil)
 
 		// basic ok checking
-	require.NoError(t, err4)
+		require.NoError(r, err4)
-	require.NotEmpty(t, qm2)
+		require.NotEmpty(r, qm2)
 
 		// require that the peering state is not undefined
-	require.Equal(t, INITIAL, respr.State)
+		require.Equal(r, PeeringStateInitial, respr.State)
 
 		// TODO(peering) -- let's go all the way and test in code either here or somewhere else that PeeringState does move to Active
-	// require.Equal(t, PeeringState_ACTIVE, respr.State)
+	})
 
 	// Delete the token on server 1
-	p := PeeringRequest{
+	resp4, qm3, err5 := peerings.Delete(ctx, PeeringDeleteRequest{Name: "peer1"}, nil)
-		Name: "peer1",
-	}
-	resp4, qm3, err5 := peerings.Delete(ctx, p, nil)
 
 	require.NoError(t, err5)
 	require.NotEmpty(t, qm3)
@@ -103,7 +219,7 @@ func TestAPI_Peering_GenerateToken_Read_Initiate_Delete(t *testing.T) {
 	require.Empty(t, resp4)
 
 	// Read to see if the token is "gone"
-	resp5, qm4, err6 := peerings.Read(ctx, "peer1", nil)
+	resp5, qm4, err6 := peerings.Read(ctx, PeeringReadRequest{Name: "peer1"}, nil)
 
 	// basic checks
 	require.NotNil(t, err6)