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

wants to merge 1 commit into from

Conversation

briantkennedy
Copy link
Contributor

What type of PR is this?

/kind test
/area conformance

What this PR does / why we need it:

Allows configuring DialContext for default round tripper.

Which issue(s) this PR fixes:

Fixes #2962

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 11, 2024
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 11, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @briantkennedy. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@robscott
Copy link
Member

Thanks @briantkennedy!

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 11, 2024
@robscott
Copy link
Member

This seems reasonable to me, will defer to someone else for LGTM.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: briantkennedy, robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 11, 2024
candita
candita reviewed Apr 15, 2024
View reviewed changes
conformance/utils/suite/suite.go
roundTripper = &roundtripper.DefaultRoundTripper{
Debug: options.Debug,
TimeoutConfig: options.TimeoutConfig,
CustomDialContext: options.RoundTripperDialContext,
Copy link
Contributor

@candita candita Apr 15, 2024 •
edited

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.

candita
candita reviewed Apr 15, 2024
View reviewed changes
conformance/utils/suite/suite.go
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.

@robscott
Copy link
Member

Synced offline on this one and decided to move forward with a different approach that would pass this option to the RoundTripper instead, something like this:

opts.Roundtripper = roundtripper.DefaultRoundTripper{
            Debug:             opts.Debug,
            TimeoutConfig:     opts.TimeoutConfig,
            CustomDialContext: MY_CUSTOM_DIAL_CONTEXT_HERE,
}

Closing this one out.

@robscott robscott closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robscott robscott robscott left review comments

@candita candita candita left review comments

@kflynn kflynn Awaiting requested review from kflynn

@LiorLieberman LiorLieberman Awaiting requested review from LiorLieberman

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow configuring Dialer in conformance tests
4 participants
@briantkennedy @k8s-ci-robot @robscott @candita