From 158fb8db034cdab2ead60b96c4008f8f06a5bd7a Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 12 Aug 2022 09:55:22 -0400 Subject: [PATCH 1/9] Add NumOfCPUCores method to CNS client This is the last part of CNS's API surface that is used by DNC, but not covered by a method from the CNS client. --- cns/NetworkContainerContract.go | 1 + cns/client/client.go | 33 ++++++++++++++++++++++ cns/client/client_test.go | 50 +++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+) diff --git a/cns/NetworkContainerContract.go b/cns/NetworkContainerContract.go index d253eb0b19..d6e9f78718 100644 --- a/cns/NetworkContainerContract.go +++ b/cns/NetworkContainerContract.go @@ -29,6 +29,7 @@ const ( PathDebugIPAddresses = "/debug/ipaddresses" PathDebugPodContext = "/debug/podcontext" PathDebugRestData = "/debug/restdata" + NumberOfCPUCores = "/hostcpucores" ) // NetworkContainer Prefixes diff --git a/cns/client/client.go b/cns/client/client.go index 32cbbbaa86..c90c697cd4 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -37,6 +37,7 @@ var clientPaths = []string{ cns.PublishNetworkContainer, cns.CreateOrUpdateNetworkContainer, cns.SetOrchestratorType, + cns.NumberOfCPUCores, } type do interface { @@ -416,6 +417,38 @@ func (c *Client) GetHTTPServiceData(ctx context.Context) (*restserver.GetHTTPSer return &resp, nil } +// NumOfCPUCores returns the number of CPU cores available on the host that +// CNS is running on. +func (c *Client) NumOfCPUCores(ctx context.Context) (cns.NumOfCPUCoresResponse, error) { + // define a wrapper function to avoid repeatedly dealing with the empty + // response type + die := func(err error, msg string) (cns.NumOfCPUCoresResponse, error) { + return cns.NumOfCPUCoresResponse{}, errors.Wrap(err, msg) + } + + // build the request + u := c.routes[cns.NumberOfCPUCores] + req, err := http.NewRequest(http.MethodGet, u.String(), nil) + if err != nil { + return die(err, "building http request") + } + + // submit the request + resp, err := c.client.Do(req) + if err != nil { + return die(err, "sending HTTP request") + } + defer resp.Body.Close() + + // decode the response + var out cns.NumOfCPUCoresResponse + err = json.NewDecoder(resp.Body).Decode(&out) + if err != nil { + return die(err, "decoding response as JSON") + } + return out, nil +} + // DeleteNetworkContainer destroys the requested network container matching the // provided ID. func (c *Client) DeleteNetworkContainer(ctx context.Context, ncID string) error { diff --git a/cns/client/client_test.go b/cns/client/client_test.go index e1e03f7d55..79d654c99d 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -2009,3 +2009,53 @@ func TestGetHTTPServiceData(t *testing.T) { }) } } + +func TestNumberOfCPUCores(t *testing.T) { + emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) + tests := []struct { + name string + shouldErr bool + exp *cns.NumOfCPUCoresResponse + }{ + { + "happy path", + false, + &cns.NumOfCPUCoresResponse{ + Response: cns.Response{ + ReturnCode: 0, + Message: "success", + }, + NumOfCPUCores: 42, + }, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + client := &Client{ + client: &mockdo{ + errToReturn: nil, + objToReturn: test.exp, + httpStatusCodeToReturn: http.StatusOK, + }, + routes: emptyRoutes, + } + + got, err := client.NumOfCPUCores(context.Background()) + 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 !cmp.Equal(got, *test.exp) { + t.Error("received response differs from expectation: diff:", cmp.Diff(got, *test.exp)) + } + }) + } +} From 572ff917b228e86757ef38c93a43ec900e73eea5 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Thu, 18 Aug 2022 14:18:16 -0400 Subject: [PATCH 2/9] Remove unnecessary error wrapping lambda The error handling here is simple enough that it doesn't really warrant wrapping it with a lambda. --- cns/client/client.go | 26 +++++++++++++++----------- cns/client/client_test.go | 15 +++++++++++++-- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/cns/client/client.go b/cns/client/client.go index c90c697cd4..74e9680c6f 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -419,24 +419,18 @@ func (c *Client) GetHTTPServiceData(ctx context.Context) (*restserver.GetHTTPSer // NumOfCPUCores returns the number of CPU cores available on the host that // CNS is running on. -func (c *Client) NumOfCPUCores(ctx context.Context) (cns.NumOfCPUCoresResponse, error) { - // define a wrapper function to avoid repeatedly dealing with the empty - // response type - die := func(err error, msg string) (cns.NumOfCPUCoresResponse, error) { - return cns.NumOfCPUCoresResponse{}, errors.Wrap(err, msg) - } - +func (c *Client) NumOfCPUCores(ctx context.Context) (*cns.NumOfCPUCoresResponse, error) { // build the request u := c.routes[cns.NumberOfCPUCores] req, err := http.NewRequest(http.MethodGet, u.String(), nil) if err != nil { - return die(err, "building http request") + return nil, errors.Wrap(err, "building http request") } // submit the request resp, err := c.client.Do(req) if err != nil { - return die(err, "sending HTTP request") + return nil, errors.Wrap(err, "sending HTTP request") } defer resp.Body.Close() @@ -444,9 +438,19 @@ func (c *Client) NumOfCPUCores(ctx context.Context) (cns.NumOfCPUCoresResponse, var out cns.NumOfCPUCoresResponse err = json.NewDecoder(resp.Body).Decode(&out) if err != nil { - return die(err, "decoding response as JSON") + return nil, errors.Wrap(err, "decoding response as JSON") + } + + // if the return code is non-zero, something went wrong and it should be + // surfaced to the caller + if out.Response.ReturnCode != 0 { + return nil, &CNSClientError{ + Code: out.Response.ReturnCode, + Err: errors.New(out.Response.Message), + } } - return out, nil + + return &out, nil } // DeleteNetworkContainer destroys the requested network container matching the diff --git a/cns/client/client_test.go b/cns/client/client_test.go index 79d654c99d..3dd1d1030c 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -2028,6 +2028,17 @@ func TestNumberOfCPUCores(t *testing.T) { NumOfCPUCores: 42, }, }, + { + "unspecified error", + true, + &cns.NumOfCPUCoresResponse{ + Response: cns.Response{ + ReturnCode: types.MalformedSubnet, + Message: "malformed subnet", + }, + NumOfCPUCores: 0, + }, + }, } for _, test := range tests { @@ -2053,8 +2064,8 @@ func TestNumberOfCPUCores(t *testing.T) { t.Fatal("expected an error but received none") } - if !cmp.Equal(got, *test.exp) { - t.Error("received response differs from expectation: diff:", cmp.Diff(got, *test.exp)) + if !test.shouldErr && !cmp.Equal(got, test.exp) { + t.Error("received response differs from expectation: diff:", cmp.Diff(got, test.exp)) } }) } From 13676ec0e1e1a9ed1a86613152376d2779151fef Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 19 Aug 2022 11:01:27 -0400 Subject: [PATCH 3/9] Add NmAgentSupportedAPIs to the CNS Client DNC uses this API to detect GRE key capabilities. Since it's not supported by the client, it does this with direct HTTP requests currently. In order to ensure that there's only one way of accessing CNS, this implements the method so that the client can be used in DNC instead. --- cns/NetworkContainerContract.go | 1 + cns/client/client.go | 47 +++++++++++++++++++++++ cns/client/client_test.go | 66 +++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+) diff --git a/cns/NetworkContainerContract.go b/cns/NetworkContainerContract.go index d6e9f78718..753cf3f4b6 100644 --- a/cns/NetworkContainerContract.go +++ b/cns/NetworkContainerContract.go @@ -30,6 +30,7 @@ const ( PathDebugPodContext = "/debug/podcontext" PathDebugRestData = "/debug/restdata" NumberOfCPUCores = "/hostcpucores" + NMAgentSupportedAPIs = "/network/nmagentsupportedapis" ) // NetworkContainer Prefixes diff --git a/cns/client/client.go b/cns/client/client.go index 74e9680c6f..a02b922800 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -696,3 +696,50 @@ func (c *Client) UnpublishNC(ctx context.Context, uncr cns.UnpublishNetworkConta // ...otherwise the request was successful so return nil } + +// NMAgentSupportedAPIs returns the supported API names from NMAgent. This can +// be used, for example, to detect whether the node is capable for GRE +// allocations. +func (c *Client) NMAgentSupportedAPIs(ctx context.Context) (*cns.NmAgentSupportedApisResponse, error) { + // build the request + reqBody := &cns.NmAgentSupportedApisRequest{ + // the IP used below is that of the Wireserver + GetNmAgentSupportedApisURL: "http://168.63.129.16/machine/plugins/?comp=nmagent&type=GetSupportedApis", + } + + body, err := json.Marshal(reqBody) + if err != nil { + return nil, errors.Wrap(err, "encoding request body") + } + + u := c.routes[cns.NMAgentSupportedAPIs] + req, err := http.NewRequest(http.MethodGet, u.String(), bytes.NewReader(body)) + if err != nil { + return nil, errors.Wrap(err, "building http request") + } + + // submit the request + resp, err := c.client.Do(req) + if err != nil { + return nil, errors.Wrap(err, "sending http request") + } + defer resp.Body.Close() + + // decode response + var out cns.NmAgentSupportedApisResponse + err = json.NewDecoder(resp.Body).Decode(&out) + if err != nil { + return nil, errors.Wrap(err, "decoding response body") + } + + // if there was a non-zero status code, that indicates an error and should be + // communicated as such + if out.Response.ReturnCode != 0 { + return nil, &CNSClientError{ + Code: out.Response.ReturnCode, + Err: errors.New(out.Response.Message), + } + } + + return &out, nil +} diff --git a/cns/client/client_test.go b/cns/client/client_test.go index 3dd1d1030c..3558bbe8a3 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -2070,3 +2070,69 @@ func TestNumberOfCPUCores(t *testing.T) { }) } } + +func TestNMASupportedAPIs(t *testing.T) { + emptyRoutes, _ := buildRoutes(defaultBaseURL, clientPaths) + tests := []struct { + name string + shouldErr bool + exp *cns.NmAgentSupportedApisResponse + }{ + { + "happy", + false, + &cns.NmAgentSupportedApisResponse{ + Response: cns.Response{ + ReturnCode: 0, + Message: "success", + }, + SupportedApis: []string{}, + }, + }, + { + "unspecified error", + true, + &cns.NmAgentSupportedApisResponse{ + Response: cns.Response{ + ReturnCode: types.MalformedSubnet, + Message: "malformed subnet", + }, + SupportedApis: []string{}, + }, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + client := &Client{ + client: &mockdo{ + errToReturn: nil, + objToReturn: test.exp, + httpStatusCodeToReturn: http.StatusOK, + }, + routes: emptyRoutes, + } + + got, err := client.NMAgentSupportedAPIs(context.Background()) + if err != nil && !test.shouldErr { + t.Fatal("unexpected error: err:", err) + } + + if err == nil && test.shouldErr { + t.Fatal("expected an error but received none") + } + + // there should always be a response when there's no error + if err == nil && got == nil { + t.Fatal("expected a response but received none") + } + + if !test.shouldErr && !cmp.Equal(got, test.exp) { + t.Error("received response differs from expectation: diff:", cmp.Diff(got, test.exp)) + } + }) + } +} From a2801f7a205eef6cdff966ce41b8242de2f2000f Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 23 Aug 2022 08:56:22 -0400 Subject: [PATCH 4/9] Add NMA supported APIs to client URL list This was forgotten in a previous iteration. --- cns/client/client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cns/client/client.go b/cns/client/client.go index a02b922800..c84c9d8460 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -38,6 +38,7 @@ var clientPaths = []string{ cns.CreateOrUpdateNetworkContainer, cns.SetOrchestratorType, cns.NumberOfCPUCores, + cns.NMAgentSupportedAPIs, } type do interface { From 7985fabd213d9e90534be004f2a197713d59ffe9 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 23 Aug 2022 08:57:13 -0400 Subject: [PATCH 5/9] Handle non-200 status code by returning an error The NMAgentSupportedAPIs method did not error when a non-2XX status code was encountered. This leads to surprising behavior on the part of the consumer. --- cns/client/client.go | 4 ++++ cns/client/client_test.go | 11 ++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cns/client/client.go b/cns/client/client.go index c84c9d8460..9b44aa5431 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -726,6 +726,10 @@ func (c *Client) NMAgentSupportedAPIs(ctx context.Context) (*cns.NmAgentSupporte } defer resp.Body.Close() + if code := resp.StatusCode; code != http.StatusOK { + return nil, fmt.Errorf("http request failed: %s (%d)", http.StatusText(code), code) + } + // decode response var out cns.NmAgentSupportedApisResponse err = json.NewDecoder(resp.Body).Decode(&out) diff --git a/cns/client/client_test.go b/cns/client/client_test.go index 3558bbe8a3..e986848239 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -2076,11 +2076,13 @@ func TestNMASupportedAPIs(t *testing.T) { tests := []struct { name string shouldErr bool + respCode int exp *cns.NmAgentSupportedApisResponse }{ { "happy", false, + http.StatusOK, &cns.NmAgentSupportedApisResponse{ Response: cns.Response{ ReturnCode: 0, @@ -2092,6 +2094,7 @@ func TestNMASupportedAPIs(t *testing.T) { { "unspecified error", true, + http.StatusOK, &cns.NmAgentSupportedApisResponse{ Response: cns.Response{ ReturnCode: types.MalformedSubnet, @@ -2100,6 +2103,12 @@ func TestNMASupportedAPIs(t *testing.T) { SupportedApis: []string{}, }, }, + { + "not found", + true, + http.StatusNotFound, + nil, + }, } for _, test := range tests { @@ -2111,7 +2120,7 @@ func TestNMASupportedAPIs(t *testing.T) { client: &mockdo{ errToReturn: nil, objToReturn: test.exp, - httpStatusCodeToReturn: http.StatusOK, + httpStatusCodeToReturn: test.respCode, }, routes: emptyRoutes, } From f8f4186ebab0023a1147a3ebc026970ad1c8de2a Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 23 Aug 2022 09:35:51 -0400 Subject: [PATCH 6/9] Use http.NoBody instead of nil This was a linter suggestion, and a good one. http.NoBody is more semantically meaningful than passing a nil. --- 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 9b44aa5431..9a08706c74 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -423,7 +423,7 @@ func (c *Client) GetHTTPServiceData(ctx context.Context) (*restserver.GetHTTPSer func (c *Client) NumOfCPUCores(ctx context.Context) (*cns.NumOfCPUCoresResponse, error) { // build the request u := c.routes[cns.NumberOfCPUCores] - req, err := http.NewRequest(http.MethodGet, u.String(), nil) + req, err := http.NewRequest(http.MethodGet, u.String(), http.NoBody) if err != nil { return nil, errors.Wrap(err, "building http request") } From d6a4bfdd9ceb6638032cb0690ad5649a3653cf1f Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 23 Aug 2022 09:36:22 -0400 Subject: [PATCH 7/9] Create a FailedHTTPRequest error type This is in response to a linter complaint that dynamic errors were being used instead of a static error. Declaring an error type with var wouldn't carry along necessary debugging information (namely the HTTP status code), so it wasn't really appropriate. Rather than disable the check entirely with a linter directive, this defines a reusable error type so that HTTP errors can be communicated upward consistently. --- cns/client/client.go | 4 +++- cns/client/error.go | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cns/client/client.go b/cns/client/client.go index 9a08706c74..f14645c0a7 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -727,7 +727,9 @@ func (c *Client) NMAgentSupportedAPIs(ctx context.Context) (*cns.NmAgentSupporte defer resp.Body.Close() if code := resp.StatusCode; code != http.StatusOK { - return nil, fmt.Errorf("http request failed: %s (%d)", http.StatusText(code), code) + return nil, &FailedHTTPRequest{ + Code: code, + } } // decode response diff --git a/cns/client/error.go b/cns/client/error.go index 66ac7afc31..36881a707e 100644 --- a/cns/client/error.go +++ b/cns/client/error.go @@ -3,10 +3,21 @@ package client import ( "errors" "fmt" + "net/http" "github.com/Azure/azure-container-networking/cns/types" ) +// FailedHTTPRequest describes an HTTP request to CNS that has returned a +// non-200 status code. +type FailedHTTPRequest struct { + Code int +} + +func (f *FailedHTTPRequest) Error() string { + return fmt.Sprintf("http request failed: %s (%d)", http.StatusText(f.Code), f.Code) +} + // CNSClientError records an error and relevant code type CNSClientError struct { Code types.ResponseCode From fdf6011e4bd50b083201426f1195405d1e370261 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Thu, 25 Aug 2022 15:02:17 -0400 Subject: [PATCH 8/9] Use an alias for CNS client paths These really should be a single set of paths, but there's this existing block of constants to define a "contract with DNC." Whether or not that's complete, the spirit of it is somewhat clear. However, it's not great to be duplicating constants all over the place. This is somewhat of a compromise by defining the newer constants in terms of the older ones. --- cns/NetworkContainerContract.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cns/NetworkContainerContract.go b/cns/NetworkContainerContract.go index 753cf3f4b6..8b47eb14df 100644 --- a/cns/NetworkContainerContract.go +++ b/cns/NetworkContainerContract.go @@ -29,8 +29,8 @@ const ( PathDebugIPAddresses = "/debug/ipaddresses" PathDebugPodContext = "/debug/podcontext" PathDebugRestData = "/debug/restdata" - NumberOfCPUCores = "/hostcpucores" - NMAgentSupportedAPIs = "/network/nmagentsupportedapis" + NumberOfCPUCores = NumberOfCPUCoresPath + NMAgentSupportedAPIs = NmAgentSupportedApisPath ) // NetworkContainer Prefixes From 04777c002dbf5a11713255732d2baa7f0cf3d3a2 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Wed, 7 Sep 2022 12:02:48 -0400 Subject: [PATCH 9/9] Add missing context to outbound HTTP requests This was a good suggestion from the linter. --- cns/client/client.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cns/client/client.go b/cns/client/client.go index f14645c0a7..9843709657 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -423,7 +423,7 @@ func (c *Client) GetHTTPServiceData(ctx context.Context) (*restserver.GetHTTPSer func (c *Client) NumOfCPUCores(ctx context.Context) (*cns.NumOfCPUCoresResponse, error) { // build the request u := c.routes[cns.NumberOfCPUCores] - req, err := http.NewRequest(http.MethodGet, u.String(), http.NoBody) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), http.NoBody) if err != nil { return nil, errors.Wrap(err, "building http request") } @@ -472,7 +472,7 @@ func (c *Client) DeleteNetworkContainer(ctx context.Context, ncID string) error return errors.Wrap(err, "encoding request body") } u := c.routes[cns.DeleteNetworkContainer] - req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewReader(body)) if err != nil { return errors.Wrap(err, "building HTTP request") } @@ -526,7 +526,7 @@ func (c *Client) SetOrchestratorType(ctx context.Context, sotr cns.SetOrchestrat return errors.Wrap(err, "encoding request body") } u := c.routes[cns.SetOrchestratorType] - req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewReader(body)) if err != nil { return errors.Wrap(err, "building HTTP request") } @@ -575,7 +575,7 @@ func (c *Client) CreateNetworkContainer(ctx context.Context, cncr cns.CreateNetw return errors.Wrap(err, "encoding request as JSON") } u := c.routes[cns.CreateOrUpdateNetworkContainer] - req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewReader(body)) if err != nil { return errors.Wrap(err, "building HTTP request") } @@ -623,7 +623,7 @@ func (c *Client) PublishNetworkContainer(ctx context.Context, pncr cns.PublishNe 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)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewReader(body)) if err != nil { return errors.Wrap(err, "building HTTP request") } @@ -669,7 +669,7 @@ func (c *Client) UnpublishNC(ctx context.Context, uncr cns.UnpublishNetworkConta 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)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewReader(body)) if err != nil { return errors.Wrap(err, "building HTTP request") } @@ -714,7 +714,7 @@ func (c *Client) NMAgentSupportedAPIs(ctx context.Context) (*cns.NmAgentSupporte } u := c.routes[cns.NMAgentSupportedAPIs] - req, err := http.NewRequest(http.MethodGet, u.String(), bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), bytes.NewReader(body)) if err != nil { return nil, errors.Wrap(err, "building http request") }