From 01caa03312d1600e86dd56a91e8f1cf7f5528228 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Thu, 28 Sep 2023 12:51:12 -0400 Subject: [PATCH 1/4] Add coverage around GetInterfaces in WS client There's a bug with the wireserver client, in that the Wireserver IP from the CNS config isn't respected. This adds coverage so that it becomes safer to make changes here. --- cns/service/main.go | 7 +++- cns/wireserver/client.go | 8 +++-- cns/wireserver/client_test.go | 63 +++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 cns/wireserver/client_test.go diff --git a/cns/service/main.go b/cns/service/main.go index 0314092eb7..d1f1e47af6 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -677,7 +677,12 @@ func main() { HTTPClient: &http.Client{}, } - httpRestService, err := restserver.NewHTTPRestService(&config, &wireserver.Client{HTTPClient: &http.Client{}}, &wsProxy, nmaClient, + wsclient := &wireserver.Client{ + HTTPClient: &http.Client{}, + Logger: logger.Log, + } + + httpRestService, err := restserver.NewHTTPRestService(&config, wsclient, &wsProxy, nmaClient, endpointStateStore, conflistGenerator, homeAzMonitor) if err != nil { logger.Errorf("Failed to create CNS object, err:%v.\n", err) diff --git a/cns/wireserver/client.go b/cns/wireserver/client.go index 43e3c566aa..9d70f63cb8 100644 --- a/cns/wireserver/client.go +++ b/cns/wireserver/client.go @@ -7,7 +7,6 @@ import ( "io" "net/http" - "github.com/Azure/azure-container-networking/cns/logger" "github.com/pkg/errors" ) @@ -26,11 +25,14 @@ type do interface { type Client struct { HTTPClient do + Logger interface { + Printf(string, ...any) + } } // GetInterfaces queries interfaces from the wireserver. func (c *Client) GetInterfaces(ctx context.Context) (*GetInterfacesResult, error) { - logger.Printf("[Azure CNS] GetPrimaryInterfaceInfoFromHost") + c.Logger.Printf("[Azure CNS] GetPrimaryInterfaceInfoFromHost") req, err := http.NewRequestWithContext(ctx, http.MethodGet, hostQueryURL, nil) if err != nil { @@ -46,7 +48,7 @@ func (c *Client) GetInterfaces(ctx context.Context) (*GetInterfacesResult, error return nil, errors.Wrap(err, "failed to read response body") } - logger.Printf("[Azure CNS] Response received from NMAgent for get interface details: %s", string(b)) + c.Logger.Printf("[Azure CNS] Response received from NMAgent for get interface details: %s", string(b)) var res GetInterfacesResult if err := xml.NewDecoder(bytes.NewReader(b)).Decode(&res); err != nil { diff --git a/cns/wireserver/client_test.go b/cns/wireserver/client_test.go new file mode 100644 index 0000000000..b21041511b --- /dev/null +++ b/cns/wireserver/client_test.go @@ -0,0 +1,63 @@ +package wireserver_test + +import ( + "context" + "encoding/xml" + "net/http" + "net/http/httptest" + "testing" + + "github.com/Azure/azure-container-networking/cns/wireserver" +) + +var _ http.RoundTripper = &TestTripper{} + +// TestTripper is a mock implementation of a round tripper that allows clients +// to substitute their own implementation, so that HTTP requests can be +// asserted against and stub responses can be generated. +type TestTripper struct { + RoundTripF func(*http.Request) (*http.Response, error) +} + +func (t *TestTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return t.RoundTripF(req) +} + +type NOPLogger struct{} + +func (m *NOPLogger) Printf(_ string, _ ...any) {} + +func TestGetInterfaces(t *testing.T) { + // create a wireserver client using a test tripper so that it can be asserted + // that the correct requests are sent. + expURL := "http://168.63.129.16/machine/plugins?comp=nmagent&type=getinterfaceinfov1" + var reqURL string + client := &wireserver.Client{ + Logger: &NOPLogger{}, + HTTPClient: &http.Client{ + Transport: &TestTripper{ + RoundTripF: func(req *http.Request) (*http.Response, error) { + reqURL = req.URL.String() + rr := httptest.NewRecorder() + resp := wireserver.GetInterfacesResult{} + err := xml.NewEncoder(rr).Encode(&resp) + if err != nil { + t.Fatal("unexpected error encoding mock wireserver response: err:", err) + } + + return rr.Result(), nil + }, + }, + }, + } + + // invoke the endpoint on Wireserver + _, err := client.GetInterfaces(context.TODO()) + if err != nil { + t.Fatal("unexpected error invoking GetInterfaces: err:", err) + } + + if expURL != reqURL { + t.Error("received request URL to wireserve does not match expectation:\n\texp:", expURL, "\n\tgot:", reqURL) + } +} From 48f2b05819890fbc060cc21e3c1b6eecc8e04aa9 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Thu, 28 Sep 2023 13:40:19 -0400 Subject: [PATCH 2/4] Allow Wireserver client to use a configurable Host This allows the Wireserver client to use a configurable host so that this could, theoretically be mocked. As it currently stands, it's an impediment to running CNS locally, since it uses the real Wireserver IP. --- cns/wireserver/client.go | 31 +++++++++++++- cns/wireserver/client_test.go | 81 +++++++++++++++++++++++------------ 2 files changed, 83 insertions(+), 29 deletions(-) diff --git a/cns/wireserver/client.go b/cns/wireserver/client.go index 9d70f63cb8..58ded4c21b 100644 --- a/cns/wireserver/client.go +++ b/cns/wireserver/client.go @@ -5,12 +5,17 @@ import ( "context" "encoding/xml" "io" + "net" "net/http" + "net/url" + "strconv" "github.com/pkg/errors" ) -const hostQueryURL = "http://168.63.129.16/machine/plugins?comp=nmagent&type=getinterfaceinfov1" +const ( + WireserverIP = "168.63.129.16" +) type GetNetworkContainerOpts struct { NetworkContainerID string @@ -24,17 +29,39 @@ type do interface { } type Client struct { + Host string + Port uint16 + HTTPClient do Logger interface { Printf(string, ...any) } } +func (c *Client) hostport() string { + if c.Port != 0 { + port := strconv.FormatUint(uint64(c.Port), 10) + return net.JoinHostPort(c.Host, port) + } + return c.Host +} + // GetInterfaces queries interfaces from the wireserver. func (c *Client) GetInterfaces(ctx context.Context) (*GetInterfacesResult, error) { c.Logger.Printf("[Azure CNS] GetPrimaryInterfaceInfoFromHost") - req, err := http.NewRequestWithContext(ctx, http.MethodGet, hostQueryURL, nil) + q := &url.Values{} + q.Add("comp", "nmagent") + q.Add("type", "getinterfaceinfov1") + + reqURL := &url.URL{ + Scheme: "http", + Host: c.hostport(), + Path: "/machine/plugins", + RawQuery: q.Encode(), + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL.String(), nil) if err != nil { return nil, errors.Wrap(err, "failed to construct request") } diff --git a/cns/wireserver/client_test.go b/cns/wireserver/client_test.go index b21041511b..0e5bb10284 100644 --- a/cns/wireserver/client_test.go +++ b/cns/wireserver/client_test.go @@ -28,36 +28,63 @@ type NOPLogger struct{} func (m *NOPLogger) Printf(_ string, _ ...any) {} func TestGetInterfaces(t *testing.T) { - // create a wireserver client using a test tripper so that it can be asserted - // that the correct requests are sent. - expURL := "http://168.63.129.16/machine/plugins?comp=nmagent&type=getinterfaceinfov1" - var reqURL string - client := &wireserver.Client{ - Logger: &NOPLogger{}, - HTTPClient: &http.Client{ - Transport: &TestTripper{ - RoundTripF: func(req *http.Request) (*http.Response, error) { - reqURL = req.URL.String() - rr := httptest.NewRecorder() - resp := wireserver.GetInterfacesResult{} - err := xml.NewEncoder(rr).Encode(&resp) - if err != nil { - t.Fatal("unexpected error encoding mock wireserver response: err:", err) - } - - return rr.Result(), nil - }, - }, + tests := []struct { + name string + host string + port uint16 + expURL string + }{ + { + "real ws url", + "168.63.129.16", + 0, // a.k.a. "no port" + "http://168.63.129.16/machine/plugins?comp=nmagent&type=getinterfaceinfov1", + }, + { + "local ws url", + "127.0.0.1", + 9001, // a.k.a. "no port" + "http://127.0.0.1:9001/machine/plugins?comp=nmagent&type=getinterfaceinfov1", }, } - // invoke the endpoint on Wireserver - _, err := client.GetInterfaces(context.TODO()) - if err != nil { - t.Fatal("unexpected error invoking GetInterfaces: err:", err) - } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + // create a wireserver client using a test tripper so that it can be asserted + // that the correct requests are sent. + var gotURL string + client := &wireserver.Client{ + Host: test.host, + Port: test.port, + Logger: &NOPLogger{}, + HTTPClient: &http.Client{ + Transport: &TestTripper{ + RoundTripF: func(req *http.Request) (*http.Response, error) { + gotURL = req.URL.String() + rr := httptest.NewRecorder() + resp := wireserver.GetInterfacesResult{} + err := xml.NewEncoder(rr).Encode(&resp) + if err != nil { + t.Fatal("unexpected error encoding mock wireserver response: err:", err) + } + + return rr.Result(), nil + }, + }, + }, + } + + // invoke the endpoint on Wireserver + _, err := client.GetInterfaces(context.TODO()) + if err != nil { + t.Fatal("unexpected error invoking GetInterfaces: err:", err) + } - if expURL != reqURL { - t.Error("received request URL to wireserve does not match expectation:\n\texp:", expURL, "\n\tgot:", reqURL) + if test.expURL != gotURL { + t.Error("received request URL to wireserve does not match expectation:\n\texp:", test.expURL, "\n\tgot:", gotURL) + } + }) } } From 59e65b76d7f09bcf84f75f5cf632b7be0710934b Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 18 Aug 2023 11:57:29 -0400 Subject: [PATCH 3/4] Add WireserverIP config into Wireserver Client The Wireserver client now accepts a hostport configuration option so that its requests can be redirected. This plumbs the config for the WireserverIP to it so that it can be redirected during test. --- cns/service/main.go | 1 + cns/wireserver/client.go | 11 ++--------- cns/wireserver/client_test.go | 16 ++++++---------- 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index d1f1e47af6..3304541540 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -678,6 +678,7 @@ func main() { } wsclient := &wireserver.Client{ + HostPort: cnsconfig.WireserverIP, HTTPClient: &http.Client{}, Logger: logger.Log, } diff --git a/cns/wireserver/client.go b/cns/wireserver/client.go index 58ded4c21b..12385d2d2b 100644 --- a/cns/wireserver/client.go +++ b/cns/wireserver/client.go @@ -5,10 +5,8 @@ import ( "context" "encoding/xml" "io" - "net" "net/http" "net/url" - "strconv" "github.com/pkg/errors" ) @@ -29,8 +27,7 @@ type do interface { } type Client struct { - Host string - Port uint16 + HostPort string HTTPClient do Logger interface { @@ -39,11 +36,7 @@ type Client struct { } func (c *Client) hostport() string { - if c.Port != 0 { - port := strconv.FormatUint(uint64(c.Port), 10) - return net.JoinHostPort(c.Host, port) - } - return c.Host + return c.HostPort } // GetInterfaces queries interfaces from the wireserver. diff --git a/cns/wireserver/client_test.go b/cns/wireserver/client_test.go index 0e5bb10284..55ed954eff 100644 --- a/cns/wireserver/client_test.go +++ b/cns/wireserver/client_test.go @@ -29,21 +29,18 @@ func (m *NOPLogger) Printf(_ string, _ ...any) {} func TestGetInterfaces(t *testing.T) { tests := []struct { - name string - host string - port uint16 - expURL string + name string + hostport string + expURL string }{ { "real ws url", "168.63.129.16", - 0, // a.k.a. "no port" "http://168.63.129.16/machine/plugins?comp=nmagent&type=getinterfaceinfov1", }, { "local ws url", - "127.0.0.1", - 9001, // a.k.a. "no port" + "127.0.0.1:9001", "http://127.0.0.1:9001/machine/plugins?comp=nmagent&type=getinterfaceinfov1", }, } @@ -56,9 +53,8 @@ func TestGetInterfaces(t *testing.T) { // that the correct requests are sent. var gotURL string client := &wireserver.Client{ - Host: test.host, - Port: test.port, - Logger: &NOPLogger{}, + HostPort: test.hostport, + Logger: &NOPLogger{}, HTTPClient: &http.Client{ Transport: &TestTripper{ RoundTripF: func(req *http.Request) (*http.Response, error) { From b5e1f3bf6411a398f9e7a97722ed66786dd5db27 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Mon, 6 Nov 2023 15:45:11 +0000 Subject: [PATCH 4/4] fix: replace nil with http.NoBody Signed-off-by: Evan Baker --- cns/wireserver/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cns/wireserver/client.go b/cns/wireserver/client.go index 12385d2d2b..417e60ef6f 100644 --- a/cns/wireserver/client.go +++ b/cns/wireserver/client.go @@ -54,7 +54,7 @@ func (c *Client) GetInterfaces(ctx context.Context) (*GetInterfacesResult, error RawQuery: q.Encode(), } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL.String(), nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL.String(), http.NoBody) if err != nil { return nil, errors.Wrap(err, "failed to construct request") }