From 745bd15b153f56d86ed4ae93a968e60e2efcda7e Mon Sep 17 00:00:00 2001 From: FFMMM Date: Thu, 5 May 2022 14:15:42 -0700 Subject: [PATCH] api: add PeeeringList, polish (#12934) --- agent/peering_endpoint_test.go | 4 - api/peering.go | 61 ++++++++++--- api/peering_test.go | 156 ++++++++++++++++++++++++++++----- 3 files changed, 184 insertions(+), 37 deletions(-) diff --git a/agent/peering_endpoint_test.go b/agent/peering_endpoint_test.go index f44060a7a3..f7f9a2397b 100644 --- a/agent/peering_endpoint_test.go +++ b/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) }) } diff --git a/api/peering.go b/api/peering.go index 21b73cc0b3..37c14e2c85 100644 --- a/api/peering.go +++ b/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 - // INITIAL Initial means a Peering has been initialized and is awaiting + PeeringStateUndefined PeeringState = 0 + // PeeringStateInitial means a Peering has been initialized and is awaiting // acknowledgement from a remote peer. - INITIAL PeeringState = 1 - // Active means that the peering connection is active and healthy. - // ACTIVE PeeringState = 2 + PeeringStateInitial PeeringState = 1 ) type Peering struct { @@ -44,9 +42,13 @@ type Peering struct { ModifyIndex uint64 } -// PeeringRequest is used for Read and Delete HTTP calls. -// The PeeringReadRequest and PeeringDeleteRequest look the same, so we treat them the same for now -type PeeringRequest struct { +type PeeringReadRequest struct { + Name string + 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) { - if name == "" { +func (p *Peerings) Read(ctx context.Context, peeringReq PeeringReadRequest, q *QueryOptions) (*Peering, *QueryMeta, error) { + 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 +} diff --git a/api/peering_test.go b/api/peering_test.go index b3cc97084d..d5efec8758 100644 --- a/api/peering_test.go +++ b/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) - - // 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) + require.Equal(t, PeeringStateInitial, 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.NotEmpty(t, qm2) + // basic ok checking + require.NoError(r, err4) + require.NotEmpty(r, qm2) - // require that the peering state is not undefined - require.Equal(t, INITIAL, respr.State) + // require that the peering state is not undefined + 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) + // TODO(peering) -- let's go all the way and test in code either here or somewhere else that PeeringState does move to Active + }) // Delete the token on server 1 - p := PeeringRequest{ - Name: "peer1", - } - resp4, qm3, err5 := peerings.Delete(ctx, p, nil) + resp4, qm3, err5 := peerings.Delete(ctx, PeeringDeleteRequest{Name: "peer1"}, 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)