API client connection overhaul #5625

Joerger · 2021-02-18T21:29:27Z

This PR adds support for connecting client through proxy tunnel address and web proxy address, and fixes client initialization performance issues with concurrent connection attempts. A lot of code had to be moved around to enable proxy dialing while maintaining a very small go module in client/api.

Changes:

Added support for connecting to server via proxy tunnel address.a
- Added NewTunnelDialer function to create an ssh tunnel dialer for the client.
- Added SSHConfig method to Credentials, only IdentityCreds implements it fully.
- Added barebones http client for retrieving PublicTunnelAddr from web proxy in findTunnelAddr().
- Moved logic needed for ssh tunnel dialer to contextdialer.go, ssh.go, and chconn.go in the api/client package, and refactor moved code to reduce imports.
  - Moved SSH logic in Key methods to api/utils/ssh.go to be reused by IdentityFile.
  - Moved LoadIdentity from tsh/common to lib/client/interfaces.go as KeyFromIdentityFile. Moved logic specific to actually loading the identityFile to ReadIdentityFile.
Added synchronous connection logic to client constructor to concurrently try all connection options, either using the first connection to succeed, or returning all errors.
- Completely refactored connect function to enable synchronous logic and improve separation of concerns.

To expand on the client initialization performance issue, the client takes a list of Credentials and a list of Addresses to connect the client to a server. The addresses can be web proxy, tunnel proxy, or auth server addresses. So each Credential+Address combination has to be dialed as each type of address, leading to 3nm performance where n is # of addrs and m is # of creds. In practice, both n and m should be low, but with a default dial timeout of 30 seconds, it quickly become unusable. Given the way the grpc dialer behaves, there isn't a good way (that I could come up with) to handle these combinations without dialing each one and testing the connection one my one. This led me to the solution of concurrently trying all combinations, which quickly hones in on a connection. I'm very open to other solution ideas as this feels a bit overly complicated for a normal API client.

awly

Please also manually test that dialing auth directly and via a proxy works

api/client/client.go

api/client/credentials_test.go

api/client/identityfile.go

api/client/tunneldialer.go

api/utils/ssh.go

lib/client/interfaces.go

api/client/tunneldialer.go

api/client/client.go

api/client/contextdialer.go

api/client/credentials.go

api/client/ssh.go

russjones · 2021-03-05T02:19:39Z

@awly @r0mant Can you review?

api/client/client.go

api/client/contextdialer.go

api/client/identityfile.go

api/sshutils/conn.go

… comment; Add nonblocking NewTLSAuthClient function for use in teleport libraries.

…urrent connection logic; Refactor lib/auth/clt.Client implementation to decouple the http client into its own struct.

…eleport into joerger/api-proxy-dialer

api/client/client.go

api/client/webclient.go

lib/client/weblogin.go

andrejtokarcik

A couple of minor remarks.

constants.go

andrejtokarcik · 2021-03-19T18:36:48Z

api/client/credentials.go

 	if err != nil {
 		return nil, trace.Wrap(err)
 	}

 	return configure(tlsConfig), nil
 }

+// SSHClientConfig returns SSH configuration used to connect to Proxy.
+func (c *IdentityCreds) SSHClientConfig() (*ssh.ClientConfig, error) {
+	identityFile, err := ReadIdentityFile(c.path)


Is there a disadvantage to doing

type IdentityCreds struct { identityFile *IdentityFile }

(or similar) right away?

api/client/credentials_test.go

lib/client/interfaces.go

Joerger · 2021-03-22T15:53:37Z

@russjones ready for bot.

russjones

Bot.

* Added support for connecting API client through tunnel proxy and web proxy addresses (with identity file). * Added concurrent dialing logic to dial several possible dialing combinations and seamlessly return the first client to connect.

* API client connection overhaul (#5625) * Add Credential loader support for tsh profiles. (#5993) * Profile credentials dialer fix (#6122)

awly reviewed Feb 19, 2021

View reviewed changes

Joerger force-pushed the joerger/api-proxy-dialer branch from a0419a8 to 1bcaa55 Compare February 19, 2021 01:37

Joerger force-pushed the joerger/api-credential-providers branch from ccad89f to 556f49c Compare February 19, 2021 01:40

Joerger force-pushed the joerger/api-proxy-dialer branch from 8c868d1 to 521d83f Compare February 19, 2021 18:24

a-palchikov reviewed Feb 19, 2021

View reviewed changes

awly self-requested a review February 19, 2021 22:08

russjones added this to the 6.0.1 "Ides of March" milestone Feb 20, 2021

Base automatically changed from joerger/api-credential-providers to master February 23, 2021 00:43

Joerger changed the title ~~API proxy dialer~~ API client connection refactor Mar 4, 2021

Joerger marked this pull request as ready for review March 4, 2021 02:56

Joerger requested review from klizhentas, r0mant and russjones as code owners March 4, 2021 02:56

Joerger changed the title ~~API client connection refactor~~ API client connection overhaul Mar 4, 2021

Joerger requested a review from alex-kovoy as a code owner March 5, 2021 02:49

Joerger force-pushed the joerger/api-proxy-dialer branch 2 times, most recently from 22f6534 to d8ca29a Compare March 6, 2021 01:03

Joerger requested a review from andrejtokarcik March 9, 2021 18:21

API client connection overhaul

e4bf696

Joerger force-pushed the joerger/api-proxy-dialer branch 2 times, most recently from 390ac2c to 9b36d55 Compare March 9, 2021 21:05

awly reviewed Mar 9, 2021

View reviewed changes

Small tweaks.

4dd796c

russjones modified the milestones: 6.0.1 "Ides of March", 6.1 Mar 10, 2021

Joerger added 2 commits March 10, 2021 12:04

Apply changes from code review; fix cltChan logic; Improve client.New…

07f07e4

… comment; Add nonblocking NewTLSAuthClient function for use in teleport libraries.

Add DialInBackground client.Config option which uses its own non-conc…

1259daf

…urrent connection logic; Refactor lib/auth/clt.Client implementation to decouple the http client into its own struct.

Joerger force-pushed the joerger/api-proxy-dialer branch from 9b36d55 to 1259daf Compare March 10, 2021 21:40

Add web client with Ping endpoint for tunnel addr discovery.

ab0b2bc

Joerger added 2 commits March 17, 2021 11:54

Merge branch 'joerger/api-proxy-dialer' of github.com:gravitational/t…

3f26775

…eleport into joerger/api-proxy-dialer

Merge branch 'master' into joerger/api-proxy-dialer

077418a

Joerger requested a review from awly March 17, 2021 19:46

Joerger mentioned this pull request Mar 17, 2021

API client Profile credential loader #5993

Merged

awly approved these changes Mar 19, 2021

View reviewed changes

api/client/client.go Outdated Show resolved Hide resolved

api/client/webclient.go Outdated Show resolved Hide resolved

lib/client/weblogin.go Outdated Show resolved Hide resolved

andrejtokarcik approved these changes Mar 19, 2021

View reviewed changes

Joerger and others added 2 commits March 19, 2021 13:04

Resolve comments; Move Ping to /api; Add load method to IdentityCreds.

ea5ccfc

Merge branch 'master' into joerger/api-proxy-dialer

d403ca4

Joerger enabled auto-merge (squash) March 19, 2021 20:15

Joerger added 2 commits March 22, 2021 12:10

Move sshutils under utils directory.

0b091ce

Merge branch 'master' into joerger/api-proxy-dialer

87546dd

russjones approved these changes Mar 23, 2021

View reviewed changes

Merge branch 'master' into joerger/api-proxy-dialer

3ca7ad3

Joerger merged commit 2beb991 into master Mar 23, 2021

Joerger deleted the joerger/api-proxy-dialer branch March 23, 2021 21:39

Joerger mentioned this pull request Mar 26, 2021

Backport - API Client Credential/Connection changes #6131

Merged

Joerger added a commit that referenced this pull request Mar 30, 2021

Backport - API Client Credential/Connection changes (#6131)

26db273

* API client connection overhaul (#5625) * Add Credential loader support for tsh profiles. (#5993) * Profile credentials dialer fix (#6122)

dboslee mentioned this pull request Apr 11, 2022

Prevent blocking forever when transport channel fails to open #11875

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API client connection overhaul #5625

API client connection overhaul #5625

Joerger commented Feb 18, 2021 •

edited

Loading

awly left a comment

russjones commented Mar 5, 2021

andrejtokarcik left a comment

andrejtokarcik Mar 19, 2021

Joerger commented Mar 22, 2021

russjones left a comment

API client connection overhaul #5625

API client connection overhaul #5625

Conversation

Joerger commented Feb 18, 2021 • edited Loading

awly left a comment

Choose a reason for hiding this comment

russjones commented Mar 5, 2021

andrejtokarcik left a comment

Choose a reason for hiding this comment

andrejtokarcik Mar 19, 2021

Choose a reason for hiding this comment

Joerger commented Mar 22, 2021

russjones left a comment

Choose a reason for hiding this comment

Joerger commented Feb 18, 2021 •

edited

Loading