Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
3714fbc
Add implementation for GetNetworkConfiguration
timraymond Mar 16, 2022
e87cbe6
Refactor retry loops to use shared function
timraymond Mar 16, 2022
641388c
Slight re-org
timraymond Mar 16, 2022
b9ef256
Add retries for Unauthorized responses
timraymond Mar 17, 2022
491171d
Add a WireserverTransport
timraymond Mar 17, 2022
b9dcee3
Reorganize the nmagent internal package
timraymond Mar 17, 2022
0a3903a
Use WireserverTransport
timraymond Mar 19, 2022
e8a28c2
Add PutNetworkContainer method to NMAgent client
timraymond Mar 21, 2022
3278686
Switch NMAgent client port to uint16
timraymond Mar 22, 2022
748680f
Add missing body close and context propagation
timraymond Mar 22, 2022
bf2f713
Add DeleteNetworkContainer endpoint
timraymond Mar 22, 2022
0119867
Move internal imports to another section
timraymond Mar 22, 2022
22f0fce
Additional Validation / Retry improvements
timraymond Mar 22, 2022
3a2993b
Move GetNetworkConfig request params to a struct
timraymond Mar 23, 2022
f2080ef
Add Validation and Path to put request
timraymond Mar 23, 2022
6bab759
Introduce Request and Option
timraymond Mar 23, 2022
e398a6e
Add additional error documentation
timraymond Mar 23, 2022
ff5e8e9
Fix issue with cooldown remembering state
timraymond Mar 23, 2022
be0e60c
Move Validation to reflection-based helper
timraymond Mar 24, 2022
7eb129d
Housekeeping: file renaming
timraymond Mar 24, 2022
a0b0fb0
Remove Vim fold markers
timraymond Mar 29, 2022
50a007a
Set default scheme to http for nmagent client
timraymond Mar 29, 2022
f073993
Change retry functions to return durations
timraymond Mar 29, 2022
aeedeb9
Rename imports
timraymond Mar 30, 2022
f942f3b
Duplicate the request in wireserver transport
timraymond Mar 31, 2022
005ca05
Drain and close http ResponseBodies
timraymond Mar 31, 2022
244fa4d
Capture unexpected content from Wireserver
timraymond Apr 6, 2022
20fb63a
Don't return a response with an error in RoundTrip
timraymond Apr 6, 2022
9c7191c
Remove extra vim folding marks
timraymond Apr 6, 2022
3910ae0
Replace fmt.Errorf with errors.Wrap
timraymond Apr 11, 2022
6f79a16
Use Config struct instead of functional Options
timraymond Apr 11, 2022
5cb39d6
Remove validation struct tags
timraymond Apr 11, 2022
d4f4d62
Address Linter Feedback
timraymond Apr 12, 2022
2bd7f8e
Remove the UnauthorizedGracePeriod
timraymond Apr 12, 2022
196e5c5
Add error source to NMA error
timraymond Apr 14, 2022
1fdf924
Remove leftover unauthorizedGracePeriod
timraymond Apr 15, 2022
f5ac638
Remove extra validation tag
timraymond Apr 15, 2022
11ec25a
Add the body to the nmagent.Error
timraymond Apr 15, 2022
8774ca9
Remove VirtualNetworkID
timraymond Apr 29, 2022
cecc8ba
Add StatusCode to error
timraymond Apr 29, 2022
d04614a
Add GreKey field to PutNetworkContainerRequest
timraymond Apr 29, 2022
a9e1d24
Add periods at the end of all docstrings
timraymond Apr 29, 2022
dc9522a
Remove unused Option type
timraymond May 2, 2022
ba69161
Change `error` to a function
timraymond May 2, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 201 additions & 0 deletions nmagent/client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
package nmagent

import (
"context"
"encoding/json"
"io"
"net"
"net/http"
"net/url"
"strconv"
"time"

"github.com/Azure/azure-container-networking/nmagent/internal"
"github.com/pkg/errors"
)

// NewClient returns an initialized Client using the provided configuration.
func NewClient(c Config) (*Client, error) {
if err := c.Validate(); err != nil {
return nil, errors.Wrap(err, "validating config")
}

client := &Client{
httpClient: &http.Client{
Transport: &internal.WireserverTransport{
Transport: http.DefaultTransport,
},
},
host: c.Host,
port: c.Port,
enableTLS: c.UseTLS,
retrier: internal.Retrier{
// nolint:gomnd // the base parameter is explained in the function
Cooldown: internal.Exponential(1*time.Second, 2),
},
}

return client, nil
}

// Client is an agent for exchanging information with NMAgent.
type Client struct {
httpClient *http.Client

// config
host string
port uint16

enableTLS bool

retrier interface {
Do(context.Context, func() error) error
}
}

// JoinNetwork joins a node to a customer's virtual network.
func (c *Client) JoinNetwork(ctx context.Context, jnr JoinNetworkRequest) error {
req, err := c.buildRequest(ctx, jnr)
if err != nil {
return errors.Wrap(err, "building request")
}

err = c.retrier.Do(ctx, func() error {
resp, err := c.httpClient.Do(req) // nolint:govet // the shadow is intentional
if err != nil {
return errors.Wrap(err, "executing request")
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like we read the body in some operations (maybe that's existing DNC behavior anyways). Would be nice to do something like io.Copy(io.Discard, resp.Body) to ensure any payload is consumed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard library will recycle connections if you close the body. I suspect that requiring package authors to drain the bodies was too obtuse, so they made Close() affirmatively mean "I'm done with this connection" here:
https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/net/http/transport.go;l=2135-2140

The check to see whether an EOF was seen is still present, but it's short-circuited by alive in this conditional:
https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/net/http/transport.go;l=2150-2153

alive is basically a latch that makes the connection eligible for recycling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connection re-use (or lack of) can be shown with this example:

package main

import (
	"context"
	"io"
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptrace"
)

func main() {
	clientTrace := &httptrace.ClientTrace{
		GotConn: func(info httptrace.GotConnInfo) {
			log.Printf("conn: %+v", info)
		},
	}
	ctx := httptrace.WithClientTrace(context.Background(), clientTrace)

	req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://example.com", nil)
	if err != nil {
		log.Fatal(err)
	}
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		log.Fatal(err)
	}

	if _, err := io.Copy(ioutil.Discard, res.Body); err != nil {
		log.Fatal(err)
	}
	res.Body.Close()

	req, err = http.NewRequestWithContext(ctx, http.MethodGet, "http://example.com", nil)
	if err != nil {
		log.Fatal(err)
	}
	_, err = http.DefaultClient.Do(req)
	if err != nil {
		log.Fatal(err)
	}
}

You can comment out the code that consumes the body, and the trace should show no connection reuse. This is still the behavior in 1.18. It's possible that there are some scenarios where this won't be the case but this has bit me enough in the past to make me want to be extra careful :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind if I reuse this example as a repro for a Go issue? (I'll mention you on it) IMO, this violates least surprise, and it appears there's been some effort to make Close() do the right thing... it's apparently not complete though

Copy link
Contributor

@ramiro-gamarra ramiro-gamarra May 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free. Code slightly modified from this article btw. Would be nice to see some clarification around the code you pointed to, though it seems to me that this behavior is still consistent with the docs. Maybe Gopher slack or golang-nuts would be a good place to ask first?

return die(resp.StatusCode, resp.Header, resp.Body)
}
return nil
})

return err // nolint:wrapcheck // wrapping this just introduces noise
}

// GetNetworkConfiguration retrieves the configuration of a customer's virtual
// network. Only subnets which have been delegated will be returned.
func (c *Client) GetNetworkConfiguration(ctx context.Context, gncr GetNetworkConfigRequest) (VirtualNetwork, error) {
var out VirtualNetwork

req, err := c.buildRequest(ctx, gncr)
if err != nil {
return out, errors.Wrap(err, "building request")
}

err = c.retrier.Do(ctx, func() error {
resp, err := c.httpClient.Do(req) // nolint:govet // the shadow is intentional
if err != nil {
return errors.Wrap(err, "executing http request to")
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return die(resp.StatusCode, resp.Header, resp.Body)
}

ct := resp.Header.Get(internal.HeaderContentType)
if ct != internal.MimeJSON {
return NewContentError(ct, resp.Body, resp.ContentLength)
}

err = json.NewDecoder(resp.Body).Decode(&out)
if err != nil {
return errors.Wrap(err, "decoding json response")
}

return nil
})

return out, err // nolint:wrapcheck // wrapping just introduces noise here
}

// PutNetworkContainer applies a Network Container goal state and publishes it
// to PubSub.
func (c *Client) PutNetworkContainer(ctx context.Context, pncr *PutNetworkContainerRequest) error {
req, err := c.buildRequest(ctx, pncr)
if err != nil {
return errors.Wrap(err, "building request")
}

resp, err := c.httpClient.Do(req)
if err != nil {
return errors.Wrap(err, "submitting request")
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return die(resp.StatusCode, resp.Header, resp.Body)
}
return nil
}

// DeleteNetworkContainer removes a Network Container, its associated IP
// addresses, and network policies from an interface.
func (c *Client) DeleteNetworkContainer(ctx context.Context, dcr DeleteContainerRequest) error {
req, err := c.buildRequest(ctx, dcr)
if err != nil {
return errors.Wrap(err, "building request")
}

resp, err := c.httpClient.Do(req)
if err != nil {
return errors.Wrap(err, "submitting request")
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return die(resp.StatusCode, resp.Header, resp.Body)
}

return nil
}

func die(code int, headers http.Header, body io.ReadCloser) error {
// nolint:errcheck // make a best effort to return whatever information we can
// returning an error here without the code and source would
// be less helpful
bodyContent, _ := io.ReadAll(body)
return Error{
Code: code,
// this is a little strange, but the conversion below is to avoid forcing
// consumers to depend on an internal type (which they can't anyway)
Source: internal.GetErrorSource(headers).String(),
Body: bodyContent,
}
}

func (c *Client) hostPort() string {
port := strconv.Itoa(int(c.port))
return net.JoinHostPort(c.host, port)
}

func (c *Client) buildRequest(ctx context.Context, req Request) (*http.Request, error) {
if err := req.Validate(); err != nil {
return nil, errors.Wrap(err, "validating request")
}

fullURL := &url.URL{
Scheme: c.scheme(),
Host: c.hostPort(),
Path: req.Path(),
}

body, err := req.Body()
if err != nil {
return nil, errors.Wrap(err, "retrieving request body")
}

// nolint:wrapcheck // wrapping doesn't provide useful information
return http.NewRequestWithContext(ctx, req.Method(), fullURL.String(), body)
}

func (c *Client) scheme() string {
if c.enableTLS {
return "https"
}
return "http"
}
24 changes: 24 additions & 0 deletions nmagent/client_helpers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package nmagent

import (
"net/http"

"github.com/Azure/azure-container-networking/nmagent/internal"
)

// NewTestClient is a factory function available in tests only for creating
// NMAgent clients with a mock transport
func NewTestClient(transport http.RoundTripper) *Client {
return &Client{
httpClient: &http.Client{
Transport: &internal.WireserverTransport{
Transport: transport,
},
},
host: "localhost",
port: 12345,
retrier: internal.Retrier{
Cooldown: internal.AsFastAsPossible(),
},
}
}
Loading