Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow configuring DialContext for RoundTripper. #2963

Closed
wants to merge 1 commit into from
Closed

Allow configuring DialContext for RoundTripper. #2963

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 18 additions & 12 deletions conformance/utils/suite/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"io/fs"
"net"
"slices"
"sort"
"strings"
Expand Down Expand Up @@ -118,17 +119,18 @@ type ConformanceTestSuite struct {

// Options can be used to initialize a ConformanceTestSuite.
type ConformanceOptions struct {
Client client.Client
Clientset clientset.Interface
RestConfig *rest.Config
GatewayClassName string
Debug bool
RoundTripper roundtripper.RoundTripper
BaseManifests string
MeshManifests string
NamespaceLabels map[string]string
NamespaceAnnotations map[string]string
ReportOutputPath string
Client client.Client
Clientset clientset.Interface
RestConfig *rest.Config
GatewayClassName string
Debug bool
RoundTripperDialContext func(context.Context, string, string) (net.Conn, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you'll also want to add this to the ConformanceTestSuite. See 60d3e58 for an example in reverse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving this value as nil should preserve the existing behavior. Please refer to the other comment regarding default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Options can be used to intialize a ConformanceTestSuite, so you need to add RoundTripperDialContext to ConformanceTestSuite and make sure it gets populated from the ConformanceOptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @candita, RoundTripperDialContext is used for initializing a roundtripper.DefaultRoundTripper on line 190 which is then added to ConformanceTestSuite if the user has not provided a custom RoundTripper implementation. Given that this is only specific to the RoundTripper, it's not essential to incorporate this as a member of ConformanceTestSuite since it will be present in ConformanceTestSuite.RoundTripper. Please let me know if ConformanceOptions.RoundTripperDialContext should have a docstring describing this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand. You want to add this option but not allow it to be set to anything other than nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HI @candita glad to explain the life of a RoundTripperDialContext option:

  1. User configures a ConformanceOptions with a non-nil RoundTripperDialContext.
  2. User calls NewConformanceTestSuite with the configured (non-nil) RoundTripperDialContext
  3. sutie.go:190: a roundtripper.DefaultRoundTripper, roundTripper, is constructed using the custom dial context if the user has not specified a round tripper implementation.
  4. sutie.go:236: roundTripper is assigned to ConformanceTestSuite.RoundTripper which will be used for the conformance tests.
  5. Conformance tests pass ConformanceTestSuite.RoundTripper to http.MakeRequestAndExpectEventuallyConsistentResponse, eg http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, tc)

Copy link
Contributor

Choose a reason for hiding this comment

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

The user can just set ConformanceOption.RoundTripper with whatever dial context they want, correct? The option RoundTripperDialContext is not used by anything other than RoundTripper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @candita, the default round tripper sets the TLS client config which other RoundTripper implementations would need to duplicate. Ideally this wouldn't be necessary if it's feasible to inject a custom dialer for this purpose.

Copy link
Member

Choose a reason for hiding this comment

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

I personally think this could be a very useful option. In general I'd like to see as many people as possible be able to use our default/built-in roundtripper, and I think these kinds of options should help.

RoundTripper roundtripper.RoundTripper
BaseManifests string
MeshManifests string
NamespaceLabels map[string]string
NamespaceAnnotations map[string]string
ReportOutputPath string

// CleanupBaseResources indicates whether or not the base test
// resources such as Gateways should be cleaned up after the run.
Expand Down Expand Up @@ -185,7 +187,11 @@ func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite,

roundTripper := options.RoundTripper
if roundTripper == nil {
roundTripper = &roundtripper.DefaultRoundTripper{Debug: options.Debug, TimeoutConfig: options.TimeoutConfig}
roundTripper = &roundtripper.DefaultRoundTripper{
Debug: options.Debug,
TimeoutConfig: options.TimeoutConfig,
CustomDialContext: options.RoundTripperDialContext,
Copy link
Contributor

@candita candita Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure CustomDialContext belongs in the default without some kind of default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @candita, thanks for the comment. Please correct me if I'm wrong, but according to the http.Transport docs, a nil DialContext will result in the Dial method from the net package being used.

https://pkg.go.dev/net/http#Transport

	// DialContext specifies the dial function for creating unencrypted TCP connections.
	// If DialContext is nil (and the deprecated Dial below is also nil),
	// then the transport dials using package net.
	//
	// DialContext runs concurrently with calls to RoundTrip.
	// A RoundTrip call that initiates a dial may end up using
	// a connection dialed previously when the earlier connection
	// becomes idle before the later DialContext completes.

}
}

installedCRDs := &apiextensionsv1.CustomResourceDefinitionList{}
Expand Down