add a DIalContext function to SpdyRoundTripper

[tzneal]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add a DIalContext function to SpdyRoundTripper and use it for creating underlying connections. Update remotecommand.RoundTripperFor to pass the rest.Config's Dial function to the SpdyRoundTripper.

Prior to this change, remotecommand.RoundTripperFor would ignore the Dial function set on the rest.Config.

Which issue(s) this PR fixes:

Fixes #112847

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @tzneal. Thanks for your PR.

I'm waiting for a kubernetes 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.

[tzneal]

I found #109135 after running into this problem, coding this up and looking for an issue to link the PR to. I had written some additional tests and that PR has merge conflicts, so I submitted this PR.

[fedebongio]

/assign @aojea
Antonio would you be able to take a look?
Thanks
/triage accepted

[aojea]

can you split the changes in 2 different commits?

1. add the DialContext
2. plumb the DialFunc from the constructor

[tzneal]

Sure, two PRs or just two commits in the same PR?

[aojea]

2 commits is fine

/cc @enj

[aojea]

/ok-to-test

[aojea]

/lgtm
/assign @enj @liggitt

[liggitt]

it's really unfortunate we have to duplicate this and manage handshaking ourselves... there's no way to use stdlib tls setup with a custom dialer?

[tzneal]

Agreed, but I don't see a way around it given the current state of things:

package tls

func Dial(network, addr string, config *Config) (*Conn, error) {
	return DialWithDialer(new(net.Dialer), network, addr, config)
}
func DialWithDialer(dialer *net.Dialer, network, addr string, config *Config) (*Conn, error) {
	return dial(context.Background(), dialer, network, addr, config)
}
func dial(ctx context.Context, netDialer *net.Dialer, network, addr string, config *Config) (*Conn, error) {
...

There's an issue golang/go#9360 that suggests using a standard dialer interface throughout the Go stdlib when only dial methods are used. I can't find any specific language that prohibits replacing a concrete type with an interface with respect to Go1 compatibility, but I suspect it's considered a breaking change as the issue is tagged v2.

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tzneal
Once this PR has been reviewed and has the lgtm label, please ask for approval from liggitt by writing /assign @liggitt in a comment. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• staging/src/k8s.io/apimachinery/pkg/OWNERS
• staging/src/k8s.io/client-go/OWNERS

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]

@tzneal: PR needs rebase.

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.

[dims]

If you still need this PR then please rebase, if not, please close the PR