From ca3bdcecd8c21a043bc64940e8ec6a1996426b8e Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 28 Jun 2022 12:43:13 -0400 Subject: [PATCH 01/10] Add DeleteNetworkContainer method to CNS Client DNC needs to delete network containers using CNS, and it currently does so through raw HTTP requests. This instead makes this an official operation within the CNS client so that it can be used there instead. --- cns/client/client.go | 54 ++++++++++++++++ cns/client/client_test.go | 131 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+) diff --git a/cns/client/client.go b/cns/client/client.go index 62d60d76f1..0db01c25f3 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -411,3 +411,57 @@ func (c *Client) GetHTTPServiceData(ctx context.Context) (*restserver.GetHTTPSer return &resp, nil } + +// DeleteNetworkContainer destroys the requested network container matching the +// provided ID. +func (c *Client) DeleteNetworkContainer(ncID string) (cns.DeleteNetworkContainerResponse, error) { + // define a utility function to avoid overly verbose error returns imposed by + // the return signature + die := func(err error) (cns.DeleteNetworkContainerResponse, error) { + return cns.DeleteNetworkContainerResponse{}, err + } + + // the network container ID is required by the API, so ensure that we have + // one before we even make the request + if ncID == "" { + return die(errors.New("no network container ID provided")) + } + + // build the request + dncr := cns.DeleteNetworkContainerRequest{ + NetworkContainerid: ncID, + } + body, err := json.Marshal(dncr) + if err != nil { + return die(errors.Wrap(err, "encoding request body")) + } + u := c.routes[cns.DeleteNetworkContainer] + req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + if err != nil { + return die(errors.Wrap(err, "building HTTP request")) + } + + // submit the request + resp, err := c.client.Do(req) + if err != nil { + return die(errors.Wrap(err, "sending HTTP request")) + } + defer resp.Body.Close() + + // decode the response + var out cns.DeleteNetworkContainerResponse + err = json.NewDecoder(resp.Body).Decode(&out) + if err != nil { + return die(errors.Wrap(err, "decoding response as JSON")) + } + + // if a non-zero response code was received from CNS, it means something went + // wrong and it should be surfaced to the caller as an error + if out.Response.ReturnCode != 0 { + return die(errors.New(out.Response.Message)) + } + + // otherwise return the response, which doesn't have much useful information + // other than the successful response struct. + return out, nil +} diff --git a/cns/client/client_test.go b/cns/client/client_test.go index ab4cace1ce..eaeca70abc 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -23,6 +23,7 @@ import ( "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/Azure/azure-container-networking/log" + "github.com/google/go-cmp/cmp" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -757,6 +758,136 @@ func TestCreateHostNCApipaEndpoint(t *testing.T) { } } +type RequestCapture struct { + Request *http.Request + Next interface { + Do(*http.Request) (*http.Response, error) + } +} + +// Do captures the outgoing HTTP request for later examination within a test. +func (r *RequestCapture) Do(req *http.Request) (*http.Response, error) { + // clone the request to ensure that any downstream handlers can't modify what + // we've captured. Clone requires a non-nil context argument, so use a + // throwaway value. + r.Request = req.Clone(context.Background()) + + // invoke the next handler in the chain and transparently return its results + return r.Next.Do(req) +} + +func TestDeleteNetworkContainer(t *testing.T) { + // the CNS client has to be provided with routes going somewhere, so create a + // bunch of routes mapped to the localhost + emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) + + // define our test cases + deleteNCTests := []struct { + name string + ncID string + response *RequestCapture + expReq *cns.DeleteNetworkContainerRequest + exp cns.DeleteNetworkContainerResponse + shouldErr bool + }{ + { + "empty", + "", + &RequestCapture{ + Next: &mockdo{}, + }, + nil, + cns.DeleteNetworkContainerResponse{}, + true, + }, + { + "with id", + "foo", + &RequestCapture{ + Next: &mockdo{ + httpStatusCodeToReturn: http.StatusOK, + }, + }, + &cns.DeleteNetworkContainerRequest{ + NetworkContainerid: "foo", + }, + cns.DeleteNetworkContainerResponse{}, + false, + }, + { + "unspecified error", + "foo", + &RequestCapture{ + Next: &mockdo{ + errToReturn: nil, + objToReturn: cns.DeleteNetworkContainerResponse{ + Response: cns.Response{ + ReturnCode: types.MalformedSubnet, + }, + }, + httpStatusCodeToReturn: http.StatusBadRequest, + }, + }, + &cns.DeleteNetworkContainerRequest{"foo"}, + cns.DeleteNetworkContainerResponse{}, + true, + }, + } + + for _, test := range deleteNCTests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // create a new client with the mock routes and the mock doer + client := Client{ + client: test.response, + routes: emptyRoutes, + } + + // execute the method under test + got, err := client.DeleteNetworkContainer(test.ncID) + if err != nil && !test.shouldErr { + t.Fatal("unexpected error: err:", err) + } + + if err == nil && test.shouldErr { + t.Fatal("expected test to error, but no error was produced") + } + + // make sure a request was actually sent + if test.expReq != nil && test.response.Request == nil { + t.Fatal("expected a request to be sent, but none was") + } + + // if a request was expected to be sent, decode it and ensure that it + // matches expectations + if test.expReq != nil { + var gotReq cns.DeleteNetworkContainerRequest + err = json.NewDecoder(test.response.Request.Body).Decode(&gotReq) + if err != nil { + t.Fatal("error decoding the received request: err:", err) + } + + // a nil expReq is semantically meaningful (i.e. "no request"), but in + // order for cmp to work properly, the outer types should be identical. + // Thus we have to dereference it explicitly: + expReq := *test.expReq + + // ensure that the received request is what was expected + if !cmp.Equal(gotReq, expReq) { + t.Error("received request differs from expectation: diff", cmp.Diff(gotReq, expReq)) + } + } + + // assert that the response is as was expected + if !cmp.Equal(got, test.exp) { + t.Error("received response differs from expectation: diff:", cmp.Diff(got, test.exp)) + } + }) + } +} + func TestDeleteHostNCApipaEndpoint(t *testing.T) { emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) tests := []struct { From 748d40d829bec528a10dfd2827a90a6a8ede9db7 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 28 Jun 2022 16:12:43 -0400 Subject: [PATCH 02/10] Remove return value from DeleteNetworkContainer The return value from DeleteNetworkContainer was basically unused in DeleteNetworkContainer, since it only contains a CNSResponse type which can only ever be a successful response. Given this, it's sufficient to just return no error as a signifier of success. --- cns/client/client.go | 27 +++++++++++---------------- cns/client/client_test.go | 11 +---------- 2 files changed, 12 insertions(+), 26 deletions(-) diff --git a/cns/client/client.go b/cns/client/client.go index 0db01c25f3..1fabe5f92d 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -414,17 +414,11 @@ func (c *Client) GetHTTPServiceData(ctx context.Context) (*restserver.GetHTTPSer // DeleteNetworkContainer destroys the requested network container matching the // provided ID. -func (c *Client) DeleteNetworkContainer(ncID string) (cns.DeleteNetworkContainerResponse, error) { - // define a utility function to avoid overly verbose error returns imposed by - // the return signature - die := func(err error) (cns.DeleteNetworkContainerResponse, error) { - return cns.DeleteNetworkContainerResponse{}, err - } - +func (c *Client) DeleteNetworkContainer(ncID string) error { // the network container ID is required by the API, so ensure that we have // one before we even make the request if ncID == "" { - return die(errors.New("no network container ID provided")) + return errors.New("no network container ID provided") } // build the request @@ -433,18 +427,18 @@ func (c *Client) DeleteNetworkContainer(ncID string) (cns.DeleteNetworkContainer } body, err := json.Marshal(dncr) if err != nil { - return die(errors.Wrap(err, "encoding request body")) + return errors.Wrap(err, "encoding request body") } u := c.routes[cns.DeleteNetworkContainer] req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) if err != nil { - return die(errors.Wrap(err, "building HTTP request")) + return errors.Wrap(err, "building HTTP request") } // submit the request resp, err := c.client.Do(req) if err != nil { - return die(errors.Wrap(err, "sending HTTP request")) + return errors.Wrap(err, "sending HTTP request") } defer resp.Body.Close() @@ -452,16 +446,17 @@ func (c *Client) DeleteNetworkContainer(ncID string) (cns.DeleteNetworkContainer var out cns.DeleteNetworkContainerResponse err = json.NewDecoder(resp.Body).Decode(&out) if err != nil { - return die(errors.Wrap(err, "decoding response as JSON")) + return errors.Wrap(err, "decoding response as JSON") } // if a non-zero response code was received from CNS, it means something went // wrong and it should be surfaced to the caller as an error if out.Response.ReturnCode != 0 { - return die(errors.New(out.Response.Message)) + return errors.New(out.Response.Message) } - // otherwise return the response, which doesn't have much useful information - // other than the successful response struct. - return out, nil + // otherwise the response isn't terribly useful in a successful case, so it + // doesn't make sense to provide it to callers. The absence of an error is + // sufficient to communicate success. + return nil } diff --git a/cns/client/client_test.go b/cns/client/client_test.go index eaeca70abc..c7719997b6 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -787,7 +787,6 @@ func TestDeleteNetworkContainer(t *testing.T) { ncID string response *RequestCapture expReq *cns.DeleteNetworkContainerRequest - exp cns.DeleteNetworkContainerResponse shouldErr bool }{ { @@ -797,7 +796,6 @@ func TestDeleteNetworkContainer(t *testing.T) { Next: &mockdo{}, }, nil, - cns.DeleteNetworkContainerResponse{}, true, }, { @@ -811,7 +809,6 @@ func TestDeleteNetworkContainer(t *testing.T) { &cns.DeleteNetworkContainerRequest{ NetworkContainerid: "foo", }, - cns.DeleteNetworkContainerResponse{}, false, }, { @@ -829,7 +826,6 @@ func TestDeleteNetworkContainer(t *testing.T) { }, }, &cns.DeleteNetworkContainerRequest{"foo"}, - cns.DeleteNetworkContainerResponse{}, true, }, } @@ -846,7 +842,7 @@ func TestDeleteNetworkContainer(t *testing.T) { } // execute the method under test - got, err := client.DeleteNetworkContainer(test.ncID) + err := client.DeleteNetworkContainer(test.ncID) if err != nil && !test.shouldErr { t.Fatal("unexpected error: err:", err) } @@ -879,11 +875,6 @@ func TestDeleteNetworkContainer(t *testing.T) { t.Error("received request differs from expectation: diff", cmp.Diff(gotReq, expReq)) } } - - // assert that the response is as was expected - if !cmp.Equal(got, test.exp) { - t.Error("received response differs from expectation: diff:", cmp.Diff(got, test.exp)) - } }) } } From d89be0c3b479625417d61271c6e02a377784b09f Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 1 Jul 2022 11:53:07 -0400 Subject: [PATCH 03/10] Add SetOrchestratorType endpoint to cns client DNC uses SetOrchestratorType currently by making straight HTTP requests to the CNS backend. Since we should have one client only, this moves these endpoints into the CNS client so that it can be consumed in DNC. --- cns/client/client.go | 53 +++++++++++++++ cns/client/client_test.go | 135 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+) diff --git a/cns/client/client.go b/cns/client/client.go index 1fabe5f92d..a1ad0dd3c8 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -460,3 +460,56 @@ func (c *Client) DeleteNetworkContainer(ncID string) error { // sufficient to communicate success. return nil } + +// SetOrchestratorType sets the orchestrator type for a given node +func (c *Client) SetOrchestratorType(ctx context.Context, sotr cns.SetOrchestratorTypeRequest) error { + // validate that the request has all of the required fields before we waste a + // round trip + if sotr.OrchestratorType == "" { + return errors.New("request missing field OrchestratorType") + } + + if sotr.DncPartitionKey == "" { + return errors.New("request missing field DncPartitionKey") + } + + if sotr.NodeID == "" { + return errors.New("request missing field NodeID") + } + + // build the HTTP request using the supplied request body + // submit the request + body, err := json.Marshal(sotr) + if err != nil { + return errors.Wrap(err, "encoding request body") + } + u := c.routes[cns.SetOrchestratorType] + req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + if err != nil { + return errors.Wrap(err, "building HTTP request") + } + + // send the request + resp, err := c.client.Do(req) + if err != nil { + return errors.Wrap(err, "sending HTTP request") + } + defer resp.Body.Close() + + // decode the response + var out cns.Response + err = json.NewDecoder(resp.Body).Decode(&out) + if err != nil { + return errors.Wrap(err, "decoding JSON response") + } + + // if there was a non-zero response code, this is an error that + // should be communicated back to the caller... + if out.ReturnCode != 0 { + return errors.New(out.Message) + } + + // ...otherwise it's a success and returning nil is sufficient to + // communicate that + return nil +} diff --git a/cns/client/client_test.go b/cns/client/client_test.go index c7719997b6..dcffe17654 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -879,6 +879,141 @@ func TestDeleteNetworkContainer(t *testing.T) { } } +func TestSetOrchestrator(t *testing.T) { + // define the required routes for the CNS client + emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) + + // define test cases + setOrchestratorTests := []struct { + name string + req cns.SetOrchestratorTypeRequest + response *RequestCapture + expReq *cns.SetOrchestratorTypeRequest + shouldErr bool + }{ + { + "empty", + cns.SetOrchestratorTypeRequest{}, + &RequestCapture{ + Next: &mockdo{}, + }, + nil, + true, + }, + { + "missing dnc partition key", + cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + NodeID: "12345", + }, + &RequestCapture{ + Next: &mockdo{}, + }, + nil, + true, + }, + { + "missing node id key", + cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "foo", + }, + &RequestCapture{ + Next: &mockdo{}, + }, + nil, + true, + }, + { + "full request", + cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "foo", + NodeID: "12345", + }, + &RequestCapture{ + Next: &mockdo{}, + }, + &cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "foo", + NodeID: "12345", + }, + false, + }, + { + "unspecified error", + cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "foo", + NodeID: "12345", + }, + &RequestCapture{ + Next: &mockdo{ + errToReturn: nil, + objToReturn: cns.Response{ + ReturnCode: types.MalformedSubnet, + }, + httpStatusCodeToReturn: http.StatusBadRequest, + }, + }, + &cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "foo", + NodeID: "12345", + }, + true, + }, + } + + for _, test := range setOrchestratorTests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // set up a client with the mocked routes + client := Client{ + client: test.response, + routes: emptyRoutes, + } + + // execute + err := client.SetOrchestratorType(context.TODO(), test.req) + if err != nil && !test.shouldErr { + t.Fatal("request produced an error where none was expected: err:", err) + } + + if err == nil && test.shouldErr { + t.Fatal("expected an error from the request, but none received") + } + + // check to see if we expected a request to be sent. If so, + // compare it to the request we actually received + if test.expReq != nil { + // first make sure any request at all was received + if test.response.Request == nil { + t.Fatal("expected a request to be sent, but none was") + } + + var gotReq cns.SetOrchestratorTypeRequest + err := json.NewDecoder(test.response.Request.Body).Decode(&gotReq) + if err != nil { + t.Fatal("decoding received request body") + } + + // because a nil pointer in the expected request means "no + // request expected", we have to dereference it here to make + // sure that the type aligns with the gotReq + expReq := *test.expReq + + if !cmp.Equal(gotReq, expReq) { + t.Error("received request differs from expectation: diff:", cmp.Diff(gotReq, expReq)) + } + } + }) + } +} + func TestDeleteHostNCApipaEndpoint(t *testing.T) { emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) tests := []struct { From ebdfd6b4348162fae81d8dc314e5b45311e961fc Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 1 Jul 2022 15:01:43 -0400 Subject: [PATCH 04/10] Fix two linter issues In one instance the linter had a false positive on an error that doesn't really need to be wrapped. The other was a good suggestion since it helps readers of the test understand what is going on. --- cns/client/client_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cns/client/client_test.go b/cns/client/client_test.go index dcffe17654..788b49fbc1 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -773,6 +773,7 @@ func (r *RequestCapture) Do(req *http.Request) (*http.Response, error) { r.Request = req.Clone(context.Background()) // invoke the next handler in the chain and transparently return its results + //nolint:wrapcheck // we don't need error wrapping for tests return r.Next.Do(req) } @@ -825,7 +826,9 @@ func TestDeleteNetworkContainer(t *testing.T) { httpStatusCodeToReturn: http.StatusBadRequest, }, }, - &cns.DeleteNetworkContainerRequest{"foo"}, + &cns.DeleteNetworkContainerRequest{ + NetworkContainerid: "foo", + }, true, }, } From 52b4dcf42f4ed62c00bc2f625a2a4b5ff06d1052 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Thu, 7 Jul 2022 18:42:17 -0400 Subject: [PATCH 05/10] Add CreateNetworkContainer endpoint to client Turns out DNC uses a few more endpoints than it would seem. This adds a CreateNetworkContainer method to encapsulate the /network/createorupdatenetworkcontainer endpoint. --- cns/client/client.go | 48 +++++++++++++++ cns/client/client_test.go | 119 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+) diff --git a/cns/client/client.go b/cns/client/client.go index a1ad0dd3c8..797fb834d7 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -513,3 +513,51 @@ func (c *Client) SetOrchestratorType(ctx context.Context, sotr cns.SetOrchestrat // communicate that return nil } + +// CreateNetworkContainer will create the provided network container, or update +// an existing one if one already exists. +func (c *Client) CreateNetworkContainer(ctx context.Context, cncr cns.CreateNetworkContainerRequest) error { + // CreateNetworkContainerRequest is a deep and complicated struct, so + // validating fields before we send it off is difficult and likely redundant + // since the backend will have similar checks. However, we can be pretty + // certain that if the NetworkContainerid is missing, it's likely an invalid + // request (since that parameter is mandatory). + if cncr.NetworkContainerid == "" { + return errors.New("empty request provided") + } + + // build the request using the supplied struct and the client's internal + // routes + body, err := json.Marshal(cncr) + if err != nil { + return errors.Wrap(err, "encoding request as JSON") + } + u := c.routes[cns.CreateOrUpdateNetworkContainer] + req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + if err != nil { + return errors.Wrap(err, "building HTTP request") + } + + // send the request + resp, err := c.client.Do(req) + if err != nil { + return errors.Wrap(err, "sending HTTP request") + } + defer resp.Body.Close() + + // decode the response + var out cns.Response + err = json.NewDecoder(resp.Body).Decode(&out) + if err != nil { + return errors.Wrap(err, "decoding JSON response") + } + + // if there was a non-zero response code, this is an error that + // should be communicated back to the caller... + if out.ReturnCode != 0 { + return errors.New(out.Message) + } + + // ...otherwise the request was successful so + return nil +} diff --git a/cns/client/client_test.go b/cns/client/client_test.go index 788b49fbc1..d3592fb5e9 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -882,6 +882,125 @@ func TestDeleteNetworkContainer(t *testing.T) { } } +func TestCreateOrUpdateNetworkContainer(t *testing.T) { + // create the routes necessary for a test client + emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) + + // define test cases + createNCTests := []struct { + name string + client *RequestCapture + req cns.CreateNetworkContainerRequest + expReq *cns.CreateNetworkContainerRequest + shouldErr bool + }{ + { + "empty request", + &RequestCapture{ + Next: &mockdo{}, + }, + cns.CreateNetworkContainerRequest{}, + nil, + true, + }, + { + "valid", + &RequestCapture{ + Next: &mockdo{}, + }, + cns.CreateNetworkContainerRequest{ + Version: "12345", + NetworkContainerType: "blah", + NetworkContainerid: "4815162342", + // to get a proper zero value for this informational field, we have to + // do this json.RawMessage trick: + OrchestratorContext: json.RawMessage("null"), + }, + &cns.CreateNetworkContainerRequest{ + Version: "12345", + NetworkContainerType: "blah", + NetworkContainerid: "4815162342", + OrchestratorContext: json.RawMessage("null"), + }, + false, + }, + { + "unspecified error", + &RequestCapture{ + Next: &mockdo{ + errToReturn: nil, + objToReturn: cns.Response{ + ReturnCode: types.MalformedSubnet, + }, + httpStatusCodeToReturn: http.StatusBadRequest, + }, + }, + cns.CreateNetworkContainerRequest{ + Version: "12345", + NetworkContainerType: "blah", + NetworkContainerid: "4815162342", + // to get a proper zero value for this informational field, we have to + // do this json.RawMessage trick: + OrchestratorContext: json.RawMessage("null"), + }, + &cns.CreateNetworkContainerRequest{ + Version: "12345", + NetworkContainerType: "blah", + NetworkContainerid: "4815162342", + OrchestratorContext: json.RawMessage("null"), + }, + true, + }, + } + + for _, test := range createNCTests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // create a new client + client := &Client{ + client: test.client, + routes: emptyRoutes, + } + + // execute + err := client.CreateNetworkContainer(context.TODO(), test.req) + if err != nil && !test.shouldErr { + t.Fatal("unexpected error: err:", err) + } + + if err == nil && test.shouldErr { + t.Fatal("expected an error but received none") + } + + // make sure that if we expected a request, that the correct one was + // received + if test.expReq != nil { + // first make sure a request was actually received + if test.client.Request == nil { + t.Fatal("expected to receive a request, but none received") + } + + // decode the received request for later comparison + var gotReq cns.CreateNetworkContainerRequest + err = json.NewDecoder(test.client.Request.Body).Decode(&gotReq) + if err != nil { + t.Fatal("error decoding received request: err:", err) + } + + // we know a non-nil request is present (i.e. we expect a request), so + // we dereference it so that cmp can properly compare the types + expReq := *test.expReq + + if !cmp.Equal(gotReq, expReq) { + t.Error("received request differs from expectation: diff:", cmp.Diff(gotReq, expReq)) + } + } + }) + } +} + func TestSetOrchestrator(t *testing.T) { // define the required routes for the CNS client emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) From 49cb2eb4b6f088ddee339ecf390888e29a41c9a1 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 8 Jul 2022 11:40:57 -0400 Subject: [PATCH 06/10] Add a PublishNetworkContainer to CNS client Publishing network containers via CNS is something that DNC does directly through an HTTP client. Given how common this is, it makes sense to adopt this into the CNS client so that DNC can use the client instead. --- cns/client/client.go | 48 ++++++++++++++++++ cns/client/client_test.go | 104 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+) diff --git a/cns/client/client.go b/cns/client/client.go index 797fb834d7..2e32964f7a 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -561,3 +561,51 @@ func (c *Client) CreateNetworkContainer(ctx context.Context, cncr cns.CreateNetw // ...otherwise the request was successful so return nil } + +// PublishNetworkContainer publishes the provided network container via the +// NMAgent resident on the node where CNS is running. This effectively proxies +// the publication through CNS which can be useful for avoiding throttling +// issues from Wireserver. +func (c *Client) PublishNetworkContainer(ctx context.Context, pncr cns.PublishNetworkContainerRequest) error { + // Given that the PublishNetworkContainer endpoint is intended to publish + // network containers, it's reasonable to assume that the request is invalid + // if it's missing a NetworkContainerID. Check for its presence and + // pre-emptively fail if that ID is missing: + if pncr.NetworkContainerID == "" { + return errors.New("boom") + } + + // Now that the request is valid it can be packaged as an HTTP request: + body, err := json.Marshal(pncr) + if err != nil { + return errors.Wrap(err, "encoding request body as json") + } + u := c.routes[cns.PublishNetworkContainer] + req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + if err != nil { + return errors.Wrap(err, "building HTTP request") + } + + // send the HTTP request + resp, err := c.client.Do(req) + if err != nil { + return errors.Wrap(err, "sending HTTP request") + } + defer resp.Body.Close() + + // decode the response to see if it was successful + var out cns.Response + err = json.NewDecoder(resp.Body).Decode(&out) + if err != nil { + return errors.Wrap(err, "decoding JSON response") + } + + // if there was a non-zero response code, this is an error that + // should be communicated back to the caller... + if out.ReturnCode != 0 { + return errors.New(out.Message) + } + + // ...otherwise the request was successful so + return nil +} diff --git a/cns/client/client_test.go b/cns/client/client_test.go index d3592fb5e9..b8f46d73ba 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -1001,6 +1001,110 @@ func TestCreateOrUpdateNetworkContainer(t *testing.T) { } } +func TestPublishNC(t *testing.T) { + // create the routes necessary for a test client + emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) + + // define the test cases + publishNCTests := []struct { + name string + client *RequestCapture + req cns.PublishNetworkContainerRequest + expReq *cns.PublishNetworkContainerRequest + shouldErr bool + }{ + { + "empty", + &RequestCapture{ + Next: &mockdo{}, + }, + cns.PublishNetworkContainerRequest{}, + nil, + true, + }, + { + "complete", + &RequestCapture{ + Next: &mockdo{}, + }, + cns.PublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "frob", + JoinNetworkURL: "http://example.com", + CreateNetworkContainerURL: "http://example.com", + CreateNetworkContainerRequestBody: []byte{}, + }, + &cns.PublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "frob", + JoinNetworkURL: "http://example.com", + CreateNetworkContainerURL: "http://example.com", + CreateNetworkContainerRequestBody: []byte{}, + }, + false, + }, + { + "unspecified error", + &RequestCapture{ + Next: &mockdo{ + errToReturn: nil, + objToReturn: cns.Response{ + ReturnCode: types.MalformedSubnet, + }, + httpStatusCodeToReturn: http.StatusBadRequest, + }, + }, + cns.PublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "frob", + JoinNetworkURL: "http://example.com", + CreateNetworkContainerURL: "http://example.com", + CreateNetworkContainerRequestBody: []byte{}, + }, + &cns.PublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "frob", + JoinNetworkURL: "http://example.com", + CreateNetworkContainerURL: "http://example.com", + CreateNetworkContainerRequestBody: []byte{}, + }, + true, + }, + } + + for _, test := range publishNCTests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // create a client + client := &Client{ + client: test.client, + routes: emptyRoutes, + } + + // invoke the endpoint + err := client.PublishNetworkContainer(context.TODO(), test.req) + if err != nil && !test.shouldErr { + t.Fatal("unexpected error: err:", err) + } + + if err == nil && test.shouldErr { + t.Fatal("expected an error but received none") + } + + // if we expected to receive a request, make sure it's identical to the + // one we received + if test.expReq != nil { + // first ensure that we actually got a request + if test.client.Request == nil { + t.Fatal("expected to receive a request, but received none") + } + } + }) + } +} + func TestSetOrchestrator(t *testing.T) { // define the required routes for the CNS client emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) From 3492f5428bab0895b374c30d4039be8adf4aed76 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 8 Jul 2022 13:51:58 -0400 Subject: [PATCH 07/10] Fix placeholder error message This was just used in testing and was forgotten. --- cns/client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cns/client/client.go b/cns/client/client.go index 2e32964f7a..d251d1a099 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -572,7 +572,7 @@ func (c *Client) PublishNetworkContainer(ctx context.Context, pncr cns.PublishNe // if it's missing a NetworkContainerID. Check for its presence and // pre-emptively fail if that ID is missing: if pncr.NetworkContainerID == "" { - return errors.New("boom") + return errors.New("network container id missing from request") } // Now that the request is valid it can be packaged as an HTTP request: From 9c68785891228d11cc92424a66628dcadd071fcb Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 8 Jul 2022 13:52:52 -0400 Subject: [PATCH 08/10] Add context to DeleteNetworkContainer Everything involving the network should take a context parameter. --- cns/client/client.go | 2 +- cns/client/client_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cns/client/client.go b/cns/client/client.go index d251d1a099..510fa2c7e0 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -414,7 +414,7 @@ func (c *Client) GetHTTPServiceData(ctx context.Context) (*restserver.GetHTTPSer // DeleteNetworkContainer destroys the requested network container matching the // provided ID. -func (c *Client) DeleteNetworkContainer(ncID string) error { +func (c *Client) DeleteNetworkContainer(ctx context.Context, ncID string) error { // the network container ID is required by the API, so ensure that we have // one before we even make the request if ncID == "" { diff --git a/cns/client/client_test.go b/cns/client/client_test.go index b8f46d73ba..adc13507c7 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -845,7 +845,7 @@ func TestDeleteNetworkContainer(t *testing.T) { } // execute the method under test - err := client.DeleteNetworkContainer(test.ncID) + err := client.DeleteNetworkContainer(context.TODO(), test.ncID) if err != nil && !test.shouldErr { t.Fatal("unexpected error: err:", err) } From 48c4646db7a20638d5a9937f719c66360381c1ec Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 8 Jul 2022 13:54:44 -0400 Subject: [PATCH 09/10] Fix PublishNetworkContainer return type The PublishNetworkContainer endpoint returns a wrapping type around the cns.Response. This updates the tests and the endpoint to reflect that. --- cns/client/client.go | 6 +++--- cns/client/client_test.go | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cns/client/client.go b/cns/client/client.go index 510fa2c7e0..ca5a79b4ac 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -594,7 +594,7 @@ func (c *Client) PublishNetworkContainer(ctx context.Context, pncr cns.PublishNe defer resp.Body.Close() // decode the response to see if it was successful - var out cns.Response + var out cns.PublishNetworkContainerResponse err = json.NewDecoder(resp.Body).Decode(&out) if err != nil { return errors.Wrap(err, "decoding JSON response") @@ -602,8 +602,8 @@ func (c *Client) PublishNetworkContainer(ctx context.Context, pncr cns.PublishNe // if there was a non-zero response code, this is an error that // should be communicated back to the caller... - if out.ReturnCode != 0 { - return errors.New(out.Message) + if out.Response.ReturnCode != 0 { + return errors.New(out.Response.Message) } // ...otherwise the request was successful so diff --git a/cns/client/client_test.go b/cns/client/client_test.go index adc13507c7..b1d153fee3 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -1048,8 +1048,10 @@ func TestPublishNC(t *testing.T) { &RequestCapture{ Next: &mockdo{ errToReturn: nil, - objToReturn: cns.Response{ - ReturnCode: types.MalformedSubnet, + objToReturn: cns.PublishNetworkContainerResponse{ + Response: cns.Response{ + ReturnCode: types.MalformedSubnet, + }, }, httpStatusCodeToReturn: http.StatusBadRequest, }, From ff4a03a833eb8377af6e99868bd9a79d42276bba Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 8 Jul 2022 13:55:36 -0400 Subject: [PATCH 10/10] Add UnpublishNC to CNS Client Unpublishing NCs is accomplished by DNC currently by directly making HTTP calls. This adds that functionality to the client so the client can be used instead. --- cns/client/client.go | 46 +++++++++++++++++ cns/client/client_test.go | 102 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) diff --git a/cns/client/client.go b/cns/client/client.go index ca5a79b4ac..488e3ea0de 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -609,3 +609,49 @@ func (c *Client) PublishNetworkContainer(ctx context.Context, pncr cns.PublishNe // ...otherwise the request was successful so return nil } + +// UnpublishNC unpublishes the network container via the NMAgent running +// alongside the CNS service. This is useful to avoid throttling issues imposed +// by Wireserver. +func (c *Client) UnpublishNC(ctx context.Context, uncr cns.UnpublishNetworkContainerRequest) error { + // In order to unpublish a Network Container, we need its ID. If the ID is + // missing, we can assume that the request is invalid and immediately return + // an error + if uncr.NetworkContainerID == "" { + return errors.New("request missing network container id") + } + + // Now that the request is valid it can be packaged as an HTTP request: + body, err := json.Marshal(uncr) + if err != nil { + return errors.Wrap(err, "encoding request body as json") + } + u := c.routes[cns.UnpublishNetworkContainer] + req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + if err != nil { + return errors.Wrap(err, "building HTTP request") + } + + // send the HTTP request + resp, err := c.client.Do(req) + if err != nil { + return errors.Wrap(err, "sending HTTP request") + } + defer resp.Body.Close() + + // decode the response to see if it was successful + var out cns.UnpublishNetworkContainerResponse + err = json.NewDecoder(resp.Body).Decode(&out) + if err != nil { + return errors.Wrap(err, "decoding JSON response") + } + + // if there was a non-zero response code, this is an error that + // should be communicated back to the caller... + if out.Response.ReturnCode != 0 { + return errors.New(out.Response.Message) + } + + // ...otherwise the request was successful so + return nil +} diff --git a/cns/client/client_test.go b/cns/client/client_test.go index b1d153fee3..ae5deb929e 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -1107,6 +1107,108 @@ func TestPublishNC(t *testing.T) { } } +func TestUnpublishNC(t *testing.T) { + // create the routes necessary for a test client + emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) + + // define test cases + unpublishTests := []struct { + name string + client *RequestCapture + req cns.UnpublishNetworkContainerRequest + expReq *cns.UnpublishNetworkContainerRequest + shouldErr bool + }{ + { + "empty", + &RequestCapture{ + Next: &mockdo{}, + }, + cns.UnpublishNetworkContainerRequest{}, + nil, + true, + }, + { + "complete", + &RequestCapture{ + Next: &mockdo{}, + }, + cns.UnpublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "bar", + JoinNetworkURL: "http://example.com", + DeleteNetworkContainerURL: "http://example.com", + }, + &cns.UnpublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "bar", + JoinNetworkURL: "http://example.com", + DeleteNetworkContainerURL: "http://example.com", + }, + false, + }, + { + "unexpected error", + &RequestCapture{ + Next: &mockdo{ + errToReturn: nil, + objToReturn: cns.UnpublishNetworkContainerResponse{ + Response: cns.Response{ + ReturnCode: types.MalformedSubnet, + }, + }, + httpStatusCodeToReturn: http.StatusBadRequest, + }, + }, + cns.UnpublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "bar", + JoinNetworkURL: "http://example.com", + DeleteNetworkContainerURL: "http://example.com", + }, + &cns.UnpublishNetworkContainerRequest{ + NetworkID: "foo", + NetworkContainerID: "bar", + JoinNetworkURL: "http://example.com", + DeleteNetworkContainerURL: "http://example.com", + }, + true, + }, + } + + for _, test := range unpublishTests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // create a client + client := &Client{ + client: test.client, + routes: emptyRoutes, + } + + // invoke the endpoint + err := client.UnpublishNC(context.TODO(), test.req) + if err != nil && !test.shouldErr { + t.Fatal("unexpected error: err:", err) + } + + if err == nil && test.shouldErr { + t.Fatal("expected an error but received none") + } + + // ensure the received request matches expectations if a request was + // expected to be received + if test.expReq != nil { + // make sure that a request was sent + if test.client.Request == nil { + t.Fatal("expected a request to be sent but none was received") + } + } + }) + } +} + func TestSetOrchestrator(t *testing.T) { // define the required routes for the CNS client emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths)