palantir / conjure-go-runtime Goto Github PK

httpclient, by default, will use http2.ConfigureTransport to enable h2 on the client's underlying http.Transport which may contain the provided *tls.Config specified in WithTLSConfig. This will mutate the *tls.Config, and because it is a pointer, if that *tls.Config is used in other transports that are not configured to use h2, the NextProtos of that config will indicate to the server that it should use h2 but the Transport will not be able to handle it, resulting in errors like this:

Server error:

http2: server: error reading preface from client 127.0.0.1:57496: bogus greeting "GET /ping HTTP/1.1\r\nHost"

Client error:

Get https://127.0.0.1:57494/ping: net/http: HTTP/1.x transport connection broken: malformed HTTP response "\x00\x00\x18\x04\x00\x00\x00\x00\x00\x00\x05\x00\x10\x00\x00\x00\x03\x00\x00\x00\xfa\x00\x06\x00\x10\x01@\x00\x04\x00\x10\x00\x00"

Should simply use *tls.Config.Clone() to make a defensive copy rather than use the provided config pointer directly.

What happened?

Currently, when a client recieves a 429, it just continues to the next URI, which can result in more requests ultimately being made when a server is trying to indicate that it would like you to slow down. The code around handling this response code indicates that a service mesh should handle this throttling... but it would be nice to have some basic backoff in the client here.

What did you want to happen?

Upon receiving a 429, the client backs off before retrying the same node internally. The same max retries behavior can be preserved, but this would give the server a little bit of time to recover/catch up before continuing to hit it with requests.

httpclient.WithConfig fails on httpclient.MetricsMiddleware due to an empty serviceName tag if the service name is not set in the provided config or explicitly set by httpclient.WithServiceName before httpclient.WithConfig is called.

// error
httpclient.NewClient(
    httpclient.WithConfig(httpclient.ClientConfig{}), 
    httpclient.WithServiceName("foo"))

// no error
httpclient.NewClient(
    httpclient.WithServiceName("foo"), 
    httpclient.WithConfig(httpclient.ClientConfig{}))

Currently, the Do function of httpclient.clientImpl uses rand.Intn(len(uris)) to determine the offset that should be used for the URI.

This code uses the global source for randomness, so unless the client application explicitly seeds the global random source, the sequence of offsets that this code chooses will always be the same.

This is not necessarily an issue in and of itself, but does seem somewhat counter-intuitive. Possible approaches:

• Document that sequence of offsets chosen will be deterministic unless global source of randomness is seeded
• Store *rand.Rand as part of the client and seed it on construction (and optionally expose a way to disable seeding it/specifying a seed manually if there is a desire to control the sequence)

What happened?

When creating a new request with httpclient.WithRawRequestBodyProvider, calling request.GetBody() from middleware resulted in a panic due to request.GetBody being nil.

Test repro:

func TestMiddlewareCanReadBody(t *testing.T) {
	unencodedBody := "body"
	encodedBody, err := codecs.Plain.Marshal(unencodedBody)
	require.NoError(t, err)
	bodyIsCorrect := func(body io.ReadCloser) {
		content, err := ioutil.ReadAll(body)
		require.NoError(t, err)
		require.Equal(t, encodedBody, content)
	}

	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
		bodyIsCorrect(req.Body)
		rw.WriteHeader(http.StatusOK)
	}))
	defer server.Close()

	client, err := httpclient.NewClient(
		httpclient.WithMiddleware(httpclient.MiddlewareFunc(func(req *http.Request, next http.RoundTripper) (*http.Response, error) {
			body, err := req.GetBody()
			require.NoError(t, err)
			bodyIsCorrect(body)
			return next.RoundTrip(req)
		})),
		httpclient.WithBaseURLs([]string{server.URL}),
	)
	require.NoError(t, err)

	requestBodyProvider := func() io.ReadCloser {
		return ioutil.NopCloser(bytes.NewReader(encodedBody))
	}
	resp, err := client.Do(context.Background(), httpclient.WithRawRequestBodyProvider(requestBodyProvider), httpclient.WithRequestMethod("GET"))
	require.NoError(t, err)
	require.NotNil(t, resp)
}

The issue has been identified to this line: https://github.com/palantir/conjure-go-runtime/blob/develop/conjure-go-client/httpclient/body_handler.go#L65. The call to http.NewRequest with a body of type io.ReadCloser does not set GetBody as expected.

This can be seen in this function: https://github.com/golang/go/blob/master/src/net/http/request.go#L875.

What did you want to happen?

We should be setting GetBody on this request.

What happened?

When a URI(s) specified in an HTTP client is unavailable, there is a 1/N (where N is the total number of URIs) possibility that any given request will first be sent to the unavailable URI and fail. This is because retry behavior is reinitialized on each request and unavailable URIs are not tracked across requests made using the same HTTP client.

What did you want to happen?

Ideally, bad URIs would be tracked and request backoff implemented across requests made using the same HTTP client to avoid repeatedly making requests that are very likely to fail.

One possible implementation would be to follow the example in dialogue and scale permits on a limiter up and down based on successful and failed requests respectively.

A simpler approach that would also decrease the number of failed requests would be to use a similar backoff strategy currently used on a single request across requests within the client, reseting the backoff state for the URI on a successful request.

seems to currently use nanoseconds which conflicts with other clients that write this metric which use microseconds

https://github.com/palantir/conjure-go-runtime/blob/0.2.7/vendor/github.com/palantir/go-metrics/timer.go#L243

In #171 we introduced refreshable configuration which reloads state based on a provided supplier. As a caveat, we we not able to implement refreshability for TLS (Security) parameters. Instead, a warning is logged if the values are updated.

A *tls.Config is more complex than the net and http structs because many of its struct fields are functional types which are not compatible with reflect.DeepEqual, used internally by the refreshables. Equality checking is important because we do not want unnecessary updates to downstream listeners.

There may be a solution involving an intermediate struct of all primitive types, but we need to continue to support things like certificate providers that poll on their own schedule. Maybe if they are interface types implemented by comparable structs we will get away with it, but this requires more thought and work.

What happened?

Custom error decoders are able to arbitrarily modify request and response bodies. This produces a huge API surface and a huge potential spread of client behavior. We should remove this ability and become opinionated about error handling.

This blocks moving retry behavior into middleware before error decoding, as retry behavior depends on whether or not an error was returned.

Ideally, we would also remove the WithDisableRestErrors functionality, although that will require more work.

What did you want to happen?

Users should not be able to define custom decoders.

What happened?

Currently retry logic happens in the client at the layer above the middleware. Because error decoding a response body to be nil (see [0]), the retry logic cannot inspect headers and therefore cannot do any advanced handling of http codes >= 400.

What did you want to happen?

If the retry logic were to be moved into the middleware and run before the error decoding middleware, we would be able to inspect these headers.

[0] #78 (comment)

WithRawResponseBody sets the Accept header to application/octet-stream. This can lead to 406 Not Acceptable responses if the endpoint doesn't support the advertised type, requiring clients to explicitly add additional types to the request header, which could be potentially confusing to consumers of this library.

https://github.com/palantir/conjure-go-runtime/blob/develop/conjure-go-client/httpclient/request_params.go#L124

What happened?

if you specify a maxRetries value in your client of 1, it will actually retry zero times (and so on)

https://github.com/palantir/conjure-go-runtime/blob/develop/conjure-go-client/httpclient/client.go#L98

What did you want to happen?

retry the correct number of times

What happened?

In conjure-go errors can be registered with https://github.com/palantir/conjure-go-runtime/blob/develop/conjure-go-contract/errors/error_type_registry.go

I have a module G that generates a conjure package from an IR X that registers an error. The module has an exported package E that imports the generated conjure package.

If, in another module H, I generate a new conjure package from IR X and import E, the same error gets registered twice: one from the conjure generated package of X for module H and once in the vendor directory from the conjure generated package of X for module G. This results in a panic.

What did you want to happen?

Vendored packages should be able to use conjure packages that registers errors without conflicts with non-vendored packages. In other words, you should be able to export a package that has an import path to a conjure generated package.

What happened?

The CGR HTTP client will incorrectly double encode URL parameters, which was introduced by the following change: #155

Conjure generated clients already encode URL parameters a single time when building the request, so every param will already be encoded when the client.Do() method is called. However, the client.Do() will construct the URI again and when building it will re-encode one more time, making the original request incorrect.
For example, the following shows the initial client URL parameter encoding when building the request path:
https://github.com/palantir/conjure-go/blob/aaad4d3e22ebf4e48816fb1597fb7153dde97895/integration_test/testgenerated/client/api/services.conjure.go#L91
Then in the client.doOnce function, it will call out to joinURIandPath (link), which builds the string and encodes parameters one more time.
The following test will repro the issue

func TestDoubleURLEncoding(t *testing.T) {
	actual, err := joinURIAndPath("https://localhost", "/api/+t%2FA=")
	require.NoError(t, err)
	assert.Equal(t, "https://localhost/api/+t%2FA=", actual)
}

Where the result is the following:

=== RUN   TestDoubleURLEncoding
    client_path_test.go:27: 
        	Error Trace:	client_path_test.go:27
        	Error:      	Not equal: 
        	            	expected: "https://localhost/api/+t%2FA="
        	            	actual  : "https://localhost/api/+t%252FA="
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-https://localhost/api/+t%2FA=
        	            	+https://localhost/api/+t%252FA=
        	Test:       	TestDoubleURLEncoding
--- FAIL: TestDoubleURLEncoding (0.00s)
FAIL
FAIL	github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient	0.212s
FAIL

As you can see there is a double encoding which results in a %252F for the original %2F which is the encoded form of a /

What did you want to happen?

The encoded URL params should be preserved and not encoded again.
In the concrete example, the URL param is the following:

+t/A=

The / is encoded as %2F when the request is built and thus the full request path should be

https://localhost/api/+t%2FA=

Supporting custom error decoders is desirable, given that some clients can expect servers to return structured errors in some cases.

Currently, it's difficult to add custom error decoding to conjure-go-runtime's client. The current approach for this is something like:

1. Specifying the httpclient.WithDisabledRestErrors() client param when constructing a client.
2. Ensuring custom error middleware is added at the right place in the middleware chain.
3. Hoping the client won't drain the response body before my custom error middleware is called, even when it's the innermost middleware. I'd have to check whether this is even possible with conjure-go-runtime's current implementation.

Thinking of something along these lines:

type ErrorDecoder interface {
    Handles(resp *http.Response) bool
    DecodeError(resp *http.Response) error
}

func WithErrorDecoder(errorDecoder ErrorDecoder) ClientParam {
    // add error middleware in the right place.
}

@bmoylan this is just a first pass at the API; I'm happy to take a stab at implementing this.

In #28, the metrics middleware was removed from the middlewares array in the client builder. The separated middleware is added in the NewClient constructor, but not in the NewHTTPClient constructor.

It's really useful to have p99 metrics.

What happened?

• http.Request.Body and GetBody are set by bodyMiddleware
• custom Middlewares registered using WithMiddleware wrap bodyMiddleware here
• as a result, custom Middlewares' RoundTrip function executes before bodyMiddleware.RoundTrip, i.e. the custom Middlewares "see" the request before the bodyMiddleware, and cannot read the request body before the request is sent

What did you want to happen?

Allow custom Middlewares to read the request body, which is useful for implementing auditing, signed request authentication, etc.

This test reproduces the issue https://github.com/johnhferland/conjure-go-runtime/blob/pl/retry-failure/conjure-go-client/httpclient/client_test.go. We are passing in a ReadCloser for the request body and it is read from during the request, so if we pass in the ReadCloser as the request body a second time for a retry it doesn't return any data because the data has already been read.

What happened?

What did you want to happen?

What happened?

The current usage of "retries" in the client configuration is ambiguous and requires exploring the code to understand how to configure the client to make one attempt (no retries), two attempts (one retry), and unlimited attempts. Furthermore, the current implementation does not allow you to configure the client to make exactly one attempt since the request retrier performs the conditional check retryCount <= maxRetries when determining whether it should retry but the client builder overrides instances where maxRetries == 0 with maxRetries = 2 * len(b.uris).

What did you want to happen?

We should support any number of attempts, from one to unlimited. In order to do so, I propose replacing "retries" with "attempts" to disambiguate the expected behavior. This includes a breaking change as it requires renaming configuration fields and exposed functions. For example, see httpclient.WithMaxRetries. We should also fix the existing implementation to support the full attempt range from [1, inf).

The precedence of how params passed in via config (i.e. httpclient.WithConfig(config)) and params passed manually (i.e. httpclient.WithParam(param)) apply to the returned client is somewhat non-intuitive without diving into the code.

Can we document the precedence of params passed via the config struct versus params passed individually to the builder?

FLUP from #94: In the case of an error and a configured BytesBufferPool, use it to avoid allocating a new byte buffer for unmarshalling the response

1. Create a client without specifying any URIs: client, err := httpclient.NewClient()
2. Make a request using the client: _, err = client.Do(context.Background(), httpclient.WithRequestMethod("GET"))

Expected

Request fails with some kind of error that indicates that no URIs were specified

Actual

Panics (specifically, when trying to generate the random offset for the URIs):

panic: invalid argument to Intn [recovered]
	panic: invalid argument to Intn

goroutine 20 [running]:
testing.tRunner.func1(0xc00015c100)
	/usr/local/go/src/testing/testing.go:830 +0x392
panic(0x1428f80, 0x155b260)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
math/rand.(*Rand).Intn(0xc0000d0180, 0x0, 0x0)
	/usr/local/go/src/math/rand/rand.go:169 +0x9c
math/rand.Intn(...)
	/usr/local/go/src/math/rand/rand.go:329
github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient.(*clientImpl).Do(0xc000160180, 0x156e360, 0xc0000dc000, 0xc0000cf230, 0x1, 0x1, 0x0, 0x35ff4da001660990, 0x5cdf3f31)
	/Volumes/git/go/src/github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient/client.go:87 +0x6e
github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient_test.TestPanic(0xc00015c100)
	/Volumes/git/go/src/github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient/client_builder_test.go:33 +0x13a
testing.tRunner(0xc00015c100, 0x14ee480)
	/usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:916 +0x35a

What happened?

our error decode middleware maps every response status code >400 to a error, which sets the response body to nil. that means that this check https://github.com/palantir/conjure-go-runtime/blob/develop/conjure-go-client/httpclient/client.go#L108 will retry everything >400

What did you want to happen?

retry only a subset of codes >400

What happened?

When errors reference a primitive of type UUID in an API definition, the code generated by Conjure Go doesn't compile.

API definition:

{
    "version": 1,
    "errors": [
        {
            "errorName": {
                "name": "MyError",
                "package": "uuid.api"
            },
            "docs": "Some error.\n",
            "namespace": "Sample",
            "code": "INVALID_ARGUMENT",
            "safeArgs": [
                {
                    "fieldName": "info",
                    "type": {
                        "type": "reference",
                        "reference": {
                            "name": "InfoType",
                            "package": "uuid.api"
                        }
                    },
                    "docs": null
                }
            ],
            "unsafeArgs": []
        }
    ],
    "types": [
        {
            "type": "object",
            "object": {
                "typeName": {
                    "name": "InfoType",
                    "package": "uuid.api"
                },
                "fields": [
                    {
                        "fieldName": "someField",
                        "type": {
                            "type": "primitive",
                            "primitive": "UUID"
                        },
                        "docs": "The UUID.\n"
                    }
                ],
                "docs": "Some UUID.\n"
            }
        }
    ],
    "services": [
        {
            "serviceName": {
                "name": "MyService",
                "package": "uuid.api.service"
            },
            "endpoints": [
                {
                    "endpointName": "getInfo",
                    "httpMethod": "GET",
                    "httpPath": "/info",
                    "auth": {
                        "type": "header",
                        "header": {}
                    },
                    "args": [],
                    "returns": {
                        "type": "reference",
                        "reference": {
                            "name": "InfoType",
                            "package": "uuid.api"
                        }
                    },
                    "docs": "Some information.\n",
                    "deprecated": null,
                    "markers": []
                }
            ],
            "docs": "Returns information.\n"
        }
    ]
}

Compile error:

# sample/uuid/api
../../../../go/src/sample/uuid/api/errors.conjure.go:77:115: cannot use e.errorInstanceID (type "github.com/palantir/pkg/uuid".UUID) as type "github.com/palantir/conjure-go-runtime/vendor/github.com/palantir/pkg/uuid".UUID in field value
../../../../go/src/sample/uuid/api/errors.conjure.go:89:20: cannot use serializableError.ErrorInstanceID (type "github.com/palantir/conjure-go-runtime/vendor/github.com/palantir/pkg/uuid".UUID) as type "github.com/palantir/pkg/uuid".UUID in assignment

Conjure Go issue palantir/conjure-go#78 might be related to this problem.

What did you want to happen?

Package github.com/palantir/pkg/uuid should not be vendored in conjure-go-runtime or a different strategy to avoid a conflict should be implemented in conjure-go.

There are some error cases for requests where the response is nil; these cases fall outside the scope of the existing error decoder middleware implementation. Examples of errors like this are:

• dial timeout
• timeout exceeded while awaiting headers
• DNS lookup no such host
• certificate signed by unknown authority

Ideally, CGR provides some methods that enable us to identify these errors (similar to typing conjure errors / attaching status codes to errors)

What happened?

What did you want to happen?

If you're writing the client package for a service, you want to be able to accept an httpclient.Client in the constructor, however you also want to be able to modify certain parameters of that httpclient.Client, like the service name, which is impossible once you are given a client which was already created.

There's isn't a straightforward extension to allow modifying an existing client given the current design given how the middleware is flattened into a list at client creation time and the ordering is important.

The current solution has been to make parameters request scoped which can always be specified in the service's client package even after receiving a fully formed httpclient.Client, however that can be annoying if the there are many individual httpclient.Client.XXX() calls, because each one must be sure to use these request parameters. Additionally it also bloats the API surface area with parameters that could be for requests or clients. Or you must always wrap the given httpclient.Client with a wrapper that adds the necessary request params to every call then calls the underlying httpclient.Client.

If these approaches are not used or are impossible because a request parameter hasn't been created yet, you end up requiring the service clients to construct their httpclient.Client with special parameters that they should not have to specify. For example, if I'm using a "Foo" service client package, I you may have to specify httpclient.WithServiceName("foo") when constructing the httpclient.Client to pass to fooclient.New(httpc httpclient.Client, when really fooclient.New(...) should just be setting that information internally.

This has also come up for ErrorDecoders as well.

It would be great to not make service client packages have to construct httpclient.Clients with special parameters specific to that package, which means we must enable service client package authors to more easily manipulate the httpclient.Clients they accept as part of their constructors.