Updated to use konnectivity client v0.0.21, and implemented placehold… #103126

jdnurme · 2021-06-23T19:31:24Z

…er context

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR implements konnectivity client fixes that were recently merged

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

NONE

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

k8s-ci-robot · 2021-06-23T19:31:32Z

Hi @jdnurme. 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.

jkh52 · 2021-06-23T19:37:12Z

staging/src/k8s.io/apiserver/pkg/server/egressselector/egress_selector.go

@@ -203,7 +203,8 @@ func (u *udsGRPCConnector) connect() (proxier, error) {
 		return c, err
 	})

-	tunnel, err := client.CreateSingleUseGrpcTunnel(udsName, dialOption, grpc.WithInsecure())
+        ctx := context.TODO()


couldn't the delta have been limited to this, and defer the proxier change until used?

This is not possible due to KNP v0.0.21. Due to the breaking change merged in v0.0.21 here: https://github.com/jkh52/apiserver-network-proxy/blob/89eb6ca8c58f2085f1de5e7e699cc0a75d9cd4dc/konnectivity-client/pkg/client/client.go#L178

JD points out that g.tunnel.Dial("tcp", addr) changes to return g.tunnel.DialContext(ctx, "tcp", addr) so the proxy signature has to change.

agreed, thanks.

Jefftree · 2021-06-23T19:38:35Z

/ok-to-test

caesarxuchao · 2021-06-24T20:05:41Z

/triage accepted

BenTheElder

vendor changes LGTM

cheftako · 2021-06-25T04:42:05Z

/lgtm
/approve

BenTheElder · 2021-06-25T06:05:10Z

/assign @liggitt

liggitt · 2021-06-25T12:21:19Z

staging/src/k8s.io/apiserver/pkg/server/egressselector/egress_selector.go

@@ -234,7 +235,7 @@ func (d *dialerCreator) createDialer() utilnet.DialFunc {
 			egressmetrics.Metrics.ObserveDialFailure(d.options.protocol, d.options.transport, egressmetrics.StageConnect)
 			return nil, err
 		}
-		conn, err := proxier.proxy(addr)
+		conn, err := proxier.proxy(ctx, addr)


we should also plumb ctx to the proxier, err := d.connector.connect() call on line 233 so the request lifetime informs how long we wait for a dial

correct, tho proxier is only updated out of necessity. The goal was just to update the singleUseGrpcTunnel in this change

liggitt · 2021-06-25T12:21:58Z

staging/src/k8s.io/apiserver/pkg/server/egressselector/egress_selector.go

+	ctx := context.TODO()
+	tunnel, err := client.CreateSingleUseGrpcTunnel(ctx, udsName, dialOption, grpc.WithInsecure())


context.TODO() here is not good... it means the timeout of the dial is not connected to the timeout of the triggering request... plumbing the request context into connect() would let us connect those

This is intended to be a placeholder while we determine what actual context should be plumbed through

@cheftako can elaborate

ok. @cheftako will follow-up to wire this up once he confirms the lifetime of the request context matches

as is, this PR gets us context timeouts on the dial and connection to the backend, which is the most critical

liggitt · 2021-06-25T12:23:41Z

staging/src/k8s.io/apiserver/pkg/server/egressselector/egress_selector.go

@@ -144,7 +144,7 @@ type httpConnectProxier struct {
 	proxyAddress string
 }

-func (t *httpConnectProxier) proxy(addr string) (net.Conn, error) {
+func (t *httpConnectProxier) proxy(ctx context.Context, addr string) (net.Conn, error) {


are we not using the context for http connect at all?

Not currently, just had to add it to all implementations of Proxier because of a breaking change in 0.0.21. This PR is intended to only update gRPC, but this change is unavoidable

liggitt · 2021-06-25T12:29:21Z

dependency bump looks good, but the placeholder context means that it's not actually accomplishing anything in kube-apiserver yet...

liggitt · 2021-06-25T18:41:19Z

/lgtm
/approve

k8s-ci-robot · 2021-06-25T18:41:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, cheftako, jdnurme, liggitt

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:

~~OWNERS~~ [liggitt]
~~staging/src/k8s.io/legacy-cloud-providers/OWNERS~~ [cheftako,liggitt]

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

cheftako · 2021-06-25T21:37:45Z

/lgtm

jkh52 · 2021-06-25T22:16:31Z

/retest

…er context

liggitt · 2021-06-26T20:11:15Z

/lgtm

fejta-bot · 2021-06-26T23:15:47Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

…3126-upstream-release-1.21 Automated cherry pick of #103126 upstream release 1.21: Updated to use konnectivity client v0.0.21, and implemented placehold…

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 23, 2021

k8s-ci-robot requested review from andrewsykim, brendandburns and a team June 23, 2021 19:32

jkh52 reviewed Jun 23, 2021

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 24, 2021

BenTheElder approved these changes Jun 24, 2021

View reviewed changes

k8s-ci-robot assigned cheftako Jun 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2021

k8s-ci-robot assigned liggitt Jun 25, 2021

liggitt reviewed Jun 25, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2021

jdnurme force-pushed the konn-21-update branch from dbb4f0c to 3671cd8 Compare June 25, 2021 21:35

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2021

Updated to use konnectivity client v0.0.21, and implemented placehold…

21fd224

…er context

jdnurme force-pushed the konn-21-update branch from 3671cd8 to 21fd224 Compare June 25, 2021 22:28

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2021

k8s-ci-robot merged commit 2d1026c into kubernetes:master Jun 27, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated to use konnectivity client v0.0.21, and implemented placehold… #103126

Updated to use konnectivity client v0.0.21, and implemented placehold… #103126

jdnurme commented Jun 23, 2021 •

edited

k8s-ci-robot commented Jun 23, 2021

jkh52 Jun 23, 2021

cheftako Jun 23, 2021

jdnurme Jun 23, 2021

cheftako Jun 23, 2021

jkh52 Jun 24, 2021

Jefftree commented Jun 23, 2021

caesarxuchao commented Jun 24, 2021

BenTheElder left a comment

cheftako commented Jun 25, 2021

BenTheElder commented Jun 25, 2021

liggitt Jun 25, 2021

jdnurme Jun 25, 2021

liggitt Jun 25, 2021

jdnurme Jun 25, 2021

jdnurme Jun 25, 2021

liggitt Jun 25, 2021

liggitt Jun 25, 2021

jdnurme Jun 25, 2021

liggitt commented Jun 25, 2021

liggitt commented Jun 25, 2021

k8s-ci-robot commented Jun 25, 2021

cheftako commented Jun 25, 2021

jkh52 commented Jun 25, 2021

liggitt commented Jun 26, 2021

fejta-bot commented Jun 26, 2021

		ctx := context.TODO()
		tunnel, err := client.CreateSingleUseGrpcTunnel(ctx, udsName, dialOption, grpc.WithInsecure())

Updated to use konnectivity client v0.0.21, and implemented placehold… #103126

Updated to use konnectivity client v0.0.21, and implemented placehold… #103126

Conversation

jdnurme commented Jun 23, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefftree commented Jun 23, 2021

caesarxuchao commented Jun 24, 2021

BenTheElder left a comment

Choose a reason for hiding this comment

cheftako commented Jun 25, 2021

BenTheElder commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jun 25, 2021

liggitt commented Jun 25, 2021

k8s-ci-robot commented Jun 25, 2021

cheftako commented Jun 25, 2021

jkh52 commented Jun 25, 2021

liggitt commented Jun 26, 2021

fejta-bot commented Jun 26, 2021

jdnurme commented Jun 23, 2021 •

edited