Add a timeout in when the proxy agent dials to the remote

[ScheererJ]

This pull request fixes the issue if the net.Dial call based on a dial request blocks the receive loop and with that all communication from the proxy server. We have seen instances where all traffic was blocked for roughly 10 minutes.
The problem is solved by first making the dial request processing asynchronous. Secondly, we added a timeout to the net.Dial call to the target.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

[k8s-ci-robot]

Welcome @ScheererJ!

It looks like this is your first PR to kubernetes-sigs/apiserver-network-proxy 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/apiserver-network-proxy has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

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

[ScheererJ]

Signed CLA.

[Jefftree]

/ok-to-test

[ydp]

Hi @ScheererJ I am learning the code base, does your change just move all the action in PacketType_DIAL_REQ into a go routine, which solved your problem?

[ScheererJ]

Hi @ScheererJ I am learning the code base, does your change just move all the action in PacketType_DIAL_REQ into a go routine, which solved your problem?

As mentioned in the pull request description above, the change essentially consists of only two changes, but the resulting indentation make the change look bigger than it is. We make the processing of PacketType_DIAL_REQ asynchronous by using a go routine. In addition to that, we added a timeout to the net.Dial call.

[caesarxuchao]

@ydp if you add "?w=1" in to the url, you can see the diff with all the whitespace differences ignored, that would make this change easier to review.

@ScheererJ the dial timeout looks good. Moving the dial to a separate goroutine is reasonable as well. One concern I have is that if a goroutine is spawned to dial, and before the dial completes, the proxy server forwards a PACKET_CLOSE_REQ, then because the connManager doesn't have the connection yet, then the code will go to this else block without calling ctx.cleanup(). That would lead to leaking goroutines of removeToProxy and proxyToRemote, and leak the network connection if there is no timeout mechanism built there.

Perhaps we should bookkeep the pending dials, and cleanup pending dials upon receiving PACKET_CLOSE_REQ.

[ScheererJ]

@caesarxuchao do you suggest to have separate book keeping of the pending dials or simply move the a.connManager.Add(connID, ctx) call before the dial call and ensure that the cleanFunc handles the situation properly by synchronizing with the dial (ensuring it finished)?

[caesarxuchao]

I think we will have to add a separate book keeping, because before the dial returns, ctx.conn is not available.

[caesarxuchao]

something like this:

type connContext struct {
  // add dialDone
  dialDone chan struct{}
  conn      net.Conn
  cleanFunc func()
  ...
  
}

// in client.Serve()
...
case PACKET_DIAL_REQUEST:
  dialDone := new(chan struct{})
  ctx := connContext {
    dialDone: dialDone,
  }
  connManager.Add(ctx, connID)
  go func {
    conn, err := net.DialTimeout(...)
    ctx.conn = conn
    close(dialDone)
  }
  ctx.cleanFunc := func() {
    <- dialDone
    if ctx.conn != nil {
       ctx.conn.Close()
    }
    ...
  }

[ScheererJ]

I added the proposed change in a new commit, but I did not add the if ctx.conn != nil check as it cannot be nil after a successful dial.
Please advise in case I missed something.

[ScheererJ]

and now it should work again...

[ScheererJ]

Thanks @ScheererJ.

We will need unit tests to make sure we don't introduce goroutine leaks. Let me know if you have time for adding tests. If not, I can add tests in followups.

@caesarxuchao I would prefer if you could add the tests as a follow-up.

[rtheis]

Posted test results with this fix to the associated issue: #180 (comment)

[cheftako]

/assign @caesarxuchao

[cheftako]

/test pull-apiserver-network-proxy-make-lint

[relyt0925]

cc @cheftako is this still possible to get in as well since it's an important fix?

[caesarxuchao]

Sorry for dropping the ball. I took another pass and left another comment about the race.

Given that @jkh52 and his team are working hard stabilizing ANP right now, we need to be extra cautious merging this PR.

How about we split this PR into two parts? In the first part we only add a 10s timeout on the net.Dial, so that we can avoid a 10min block. And in the second part we make this more risky goroutine change.

Hopefully in the future the ANP repo can have a multi-branch structure like k/k so that we can stabilize the code in one branch and merge the more risky PRs in another branch.

[caesarxuchao]

If we move the entire handling of DIAL_REQ into a goroutine like this, we can still enter a race with the handling of CLOSE_REQ that I mentioned in #179 (comment).

How about we only move the net.Dial into a goroutine? That way a.connManager.Add is called sequentially and that would prevent the race. This is also what I suggested in #179 (comment).

[ScheererJ]

@caesarxuchao So what you are suggesting is that the go routine should start after the a.connManager.Add call. Right?
In the beginning the go routine simply started shortly before the net.Dial call, but after your suggested refactoring we can indeed move the start of the go routine to a later point in time, i.e. directly before the net.Dial call.

[caesarxuchao]

Yeah, the goroutine should start after the a.connManager.Add call. Or in other words, "move the start of the go routine to a later point in time, i.e. directly before the net.Dial call".

[ScheererJ]

Sorry for dropping the ball. I took another pass and left another comment about the race.

Given that @jkh52 and his team are working hard stabilizing ANP right now, we need to be extra cautious merging this PR.

How about we split this PR into two parts? In the first part we only add a 10s timeout on the net.Dial, so that we can avoid a 10min block. And in the second part we make this more risky goroutine change.

Hopefully in the future the ANP repo can have a multi-branch structure like k/k so that we can stabilize the code in one branch and merge the more risky PRs in another branch.

@caesarxuchao Should I only include the 10s timeout, i.e. the minimal change, into the first pull request or also include the reordering and change with regards to a.connManager.Add?

[relyt0925]

This is just a note that without this it appears the Kubernetes e2e tests will not pass when Konnectivity is in use. cc @caesarxuchao

I think this is critical toward stabilization of the system. See @rtheis's comment #179 (comment) for more details.

I know we (IBM + RedHat) have had to carry this patch in order to utilize Konnectivity in our systems.

[caesarxuchao]

Sorry for not being responsive. I wan't monitoring this PR closely. Feel free to ping me on Slack.

[caesarxuchao]

Yeah, the goroutine should start after the a.connManager.Add call. Or in other words, "move the start of the go routine to a later point in time, i.e. directly before the net.Dial call".

[caesarxuchao]

Should I only include the 10s timeout, i.e. the minimal change, into the first pull request or also include the reordering and change with regards to a.connManager.Add?

In the first PR, let's only include the 10s timeout.

In the second PR, let's include the goroutine change, including the reordering and change with regards to a.connManager.Add.

[relyt0925]

@ScheererJ do you still have time to make the changes? If not I can see if we can find someone to continue your great work here.

[ScheererJ]

Added #252 as the second part of this improvement with the go routine change.

@relyt0925 I will adapt this pull request now to only include the timeout change.

[ScheererJ]

Now, this pull request only includes the timeout change and the extended asynchronous processing via go routine is done in #252.

[caesarxuchao]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, ScheererJ

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 [caesarxuchao]

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