BuildableClient use transport http.RoundTripper instead of transport *http.Transport

[KafuuEriri]

Describe the feature

github.com/aws/[email protected]/aws/transport/http

type BuildableClient struct {
	transport *http.Transport
	dialer    *net.Dialer

	initOnce sync.Once

	clientTimeout time.Duration
	client        *http.Client
}

use transport http.RoundTripper instead of transport *http.Transport can better support custom configuration on the client

Use Case

I would like to us tracking
eg.

type tracerTransport struct {
	*http.Transport

	debug       bool
	tracer      trace.Tracer
	propagation propagation.TextMapPropagator
}

func (r *tracerTransport) RoundTrip(req *http.Request) (resp *http.Response, err error) {
	attrs := make([]attribute.KeyValue, 0)
	ctx, span := r.tracer.Start(req.Context(),
		fmt.Sprintf("%s %s%s", req.Method, req.Host, req.URL.Path),
		trace.WithSpanKind(trace.SpanKindClient))
	r.propagation.Inject(ctx, propagation.HeaderCarrier(req.Header))
	defer span.End()

	attrs = append(attrs, peerAttr(req.RemoteAddr)...)
	attrs = append(attrs, semconv.HTTPClientAttributesFromHTTPRequest(req)...)
	attrs = append(attrs, semconv.HTTPTargetKey.String(req.URL.Path))

	data, _ := io.ReadAll(req.Body)
	if len(data) > 0 {
		attrs = append(attrs, attribute.String("http.request.body", string(data)))
	}

	req = req.WithContext(ctx)
	req.ContentLength = int64(len(data))
	r.propagation.Inject(ctx, propagation.HeaderCarrier(req.Header))
	resp, err = r.Transport.RoundTrip(req)
	if err != nil {
		span.RecordError(err, trace.WithTimestamp(time.Now()))
		return
	}

	if resp.Body != nil {
		respDump, _ := httputil.DumpResponse(resp, r.debug)
		if len(respDump) > 0 {
			attrs = append(attrs, attribute.String("http.response.data", string(respDump)))
		}
	}

	attrs = append(attrs, semconv.HTTPStatusCodeKey.Int(resp.StatusCode))
	span.SetAttributes(attrs...)

	return resp, nil
}

httpClient = awshttp.NewBuildableClient().WithTransportOptions(func(transport *http. RoundTripper) {
			transport = createTransport(o.timeout)
		})

Proposed Solution

I tried using a custom client, but it may cause errors 'net/http: http: ContentLength=222 with Body length 0' when rewriting httpclient due to some middleware.But the official client doesn't have this problem

Other Information

No response

Acknowledgements

• I may be able to implement this feature request
• This feature might incur a breaking change

AWS Go SDK V2 Module Versions Used

github.com/aws/aws-sdk-go-v2 v1.22.2

Go version used

1.21

[lucix-aws]

There are a few issues with your RoundTrip implementation:

1. You're consuming the body without resetting it. This combined with my next point is almost certainly why you're seeing that failure of "content length x with body length 0" - you're using up the entire body, so when the underlying transport goes to actually send the request, it tries to read the body and gets nothing. As an aside I wouldn't recommend discarding the error on ReadAll but that's up to your discretion.
2. You're mutating the request which directly violates the semantic restrictions on http.RoundTripper - see the API doc there, but more specifically, implementations SHOULD NOT modify the request (which you are doing when you set ContentLength).

• specifically in the SDK you will run into various issues (i.e. with signing and content hash calculations) if you do this. The correct way to modify the behavior of SDK requests is via middleware.

Beyond that I'm not sure how implementing the requested change would solve the problem here. Let's address the above issues in advance of any discussion.

[KafuuEriri]

There are a few issues with your RoundTrip implementation:

1. You're consuming the body without resetting it. This combined with my next point is almost certainly why you're seeing that failure of "content length x with body length 0" - you're using up the entire body, so when the underlying transport goes to actually send the request, it tries to read the body and gets nothing. As an aside I wouldn't recommend discarding the error on ReadAll but that's up to your discretion.
2. You're mutating the request which directly violates the semantic restrictions on http.RoundTripper - see the API doc there, but more specifically, implementations SHOULD NOT modify the request (which you are doing when you set ContentLength).

• specifically in the SDK you will run into various issues (i.e. with signing and content hash calculations) if you do this. The correct way to modify the behavior of SDK requests is via middleware.

Beyond that I'm not sure how implementing the requested change would solve the problem here. Let's address the above issues in advance of any discussion.

Thank you for your help.
It is indeed caused by the "io. ReadAll" method, and I have fixed this error

data, _ := io.ReadAll(req.Body)
if len(data) > 0 {
attrs = append(attrs, attribute.String("http.request.body", string(data)))
}
req.Body = io.NopCloser(bytes.NewBuffer(data))

Although creating a custom client in this way can enable recording of request data during requests, I would like to use the default client with options http.RoundTripper. *http. Transport is an implementation of http. RoundTripper

[lucix-aws]

I agree in principle that BuildableClient should have its transport field by of type RoundTripper (that's how the net/http client does it) but unfortunately we're not able to make this change - the transport of BuildableClient is exposed as the transport type in an exported API.

Using an instance of the net/http client itself will let you accomplish the desired behavior though, and its Do API is directly compatible with the SDK client config HTTPClient interface.

e.g.

type roundTripper struct { /* ... */ } // your implementation

func (*roundTripper) RoundTrip(...) (...) { /* ... */ }

svc := s3.NewFromConfig(cfg, func (o *s3.Options) {
    o.HTTPClient = &http.Client{
        Transport: &roundTripper{ /* ... */ },
    }
})

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.