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.

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

Add WebSocket handler for WebUI database sessions #49749

Merged
merged 20 commits into from
Dec 14, 2024

Conversation

gabrielcorado
Copy link
Contributor

Part of #44956 (RFD 0181)

This PR adds a new WebSocket that the WebUI will use to start database sessions. Those sessions will rely on protocol-specific REPLs (we're initially adding PostgreSQL REPL), which will act as a database client for the WebUI terminal client. The handle is responsible for issuing session certificates and calling the REPL implementation, which will connect to the database proxy server using the given configuration. This flow is similar to what is done for Kube Exec from WebUI, where we establish a "localhost" connection to the proxy using the database client.

Note

This implementation differs slightly from my original PoC and the short RFD description. Instead of directly dialing the database servers (duplicating the flow implemented by the Proxy server), we're dialing the proxy server to perform its job, so the handler + REPL act like regular database clients (such as psql). The main reason for this change was to try and keep the closest flow as tsh, and it also proved easier to implement (as the proxy server already has test coverage). This implementation detail should keep the UX of databases WebUI Access as described on the RFD.

sequenceDiagram
  participant UI as WebUI
  participant P as Proxy
  participant A as Auth Server
  participant S as Database Server
  
  UI->>+P: Establish WebSocket connection
  UI->>+P: Sends connect request
  P->>+A: Request certificates (interact with WebSocket if necessary for MFA)
  A->>-P: Session certificates
  P->>-UI: Session metadata
  P->>+P: Establish connection to Database Proxy server
  P->>+S: Connect to the database using its client (same flow as tsh)
  S->>-P: Database Connection
  
  loop REPL
    P->>-UI: Read
    UI->>P: Write
  end
Loading

The PostgreSQL REPL implementation is decoupled (at #49598), but this PR defines the function signature and who/how will be responsible for connecting to the database. So the changes here will directly affect the other PR.

@gabrielcorado gabrielcorado changed the title Add WebSocket handler WebUI database sessions Add WebSocket handler for WebUI database sessions Dec 4, 2024
lib/web/databases.go Outdated Show resolved Hide resolved
@gabrielcorado gabrielcorado added the no-changelog Indicates that a PR does not require a changelog entry label Dec 6, 2024
@gabrielcorado gabrielcorado marked this pull request as ready for review December 6, 2024 00:35
@github-actions github-actions bot requested review from creack and timothyb89 December 6, 2024 00:35
Copy link
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass.

@@ -265,6 +266,10 @@ type Config struct {
// AccessGraph represents AccessGraph server config
AccessGraph AccessGraphConfig

// DatabaseREPLGetter is used to retrieve datatabase REPL given the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// DatabaseREPLGetter is used to retrieve datatabase REPL given the
// DatabaseREPLGetter is used to retrieve datatabase REPL given the

lib/web/databases.go Show resolved Hide resolved
Comment on lines 50 to 55
// REPLGetter is an interface for retrieving REPL constructor functions given
// the database protocol.
type REPLGetter interface {
// GetREPL returns a start function for the specified protocol.
GetREPL(ctx context.Context, dbProtocol string) (REPLNewFunc, error)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some interface nits:

is REPLNewFunc necessary? can GetREPL return the REPLInstance?

would it be easier if REPLGetter is a func instead of an interface? Do we expect other functions for REPLGetter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't initialize the REPL right away, so we can reuse this function to fill the SupportsInteractive field on the API. The WebUI uses this field to show the connect button on the database connect dialog.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. if the only goal is the check SupportsInteractive at an earlier stage, could we have a dedicated call? something like:

type REPLGetter interface {
	IsDatabaseProtocolSupported(dbProtocol string) bool
	NewREPLInstance(context.Context, *NewREPLConfig) (REPLInstance, error)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the interface, can you take a look?

lib/client/db/repl/repl.go Outdated Show resolved Hide resolved
lib/web/databases.go Outdated Show resolved Hide resolved
lib/web/databases.go Outdated Show resolved Hide resolved
lib/web/databases.go Show resolved Hide resolved
lib/web/databases_test.go Outdated Show resolved Hide resolved
lib/web/terminal.go Outdated Show resolved Hide resolved
lib/web/databases_test.go Outdated Show resolved Hide resolved
@@ -355,10 +371,10 @@ func MakeDatabase(database types.Database, dbUsers, dbNames []string, requiresRe
}

// MakeDatabases creates database objects.
func MakeDatabases(databases []*types.DatabaseV3, dbUsers, dbNames []string) []Database {
func MakeDatabases(databases []*types.DatabaseV3, dbUsers, dbNames []string, interactiveChecker DatabaseInteractiveChecker) []Database {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this function doesn't look right to me. Each database should have different dbUsers and dbNames.

Could you double-check if this function is still needed? My understanding is that the current WebUI will only use the UnifiedResourceAPI so the caller "databases" API is no longer necessary. Even if we want to keep it, the caller should run the loop and use access checker for every database.

If we can fix that, I would prefer MakeDatabase to take in a bool for SupportsInteractive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it. Now, the MakeDatabase only receives the values, and the MakeDatabases performs the database users/names listing (using the Enumerate functions). Note that I didn't update other places that directly call the MakeDatabase to use the same flow (as this seems out of this PR scope). That will be done in a separate PR.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from creack December 13, 2024 16:16
@gabrielcorado gabrielcorado added this pull request to the merge queue Dec 14, 2024
Merged via the queue into master with commit 903b5f5 Dec 14, 2024
41 checks passed
@gabrielcorado gabrielcorado deleted the gabrielcorado/pg-webui-handler branch December 14, 2024 02:36
gabrielcorado added a commit that referenced this pull request Dec 16, 2024
* feat(web): add websocket handler for database webui sessions

* refactor: move common structs into a separate package

* refactor(web): use ALPN local proxy to dial databases

* feat(repl): add default registry

* refactor(web): code review suggestions

* refactor: update repl config parameters

* refactor: move default getter implementation

* feat(web): add supports_interactive field on dbs

* refactor: code review suggestions

* refactor: update database REPL interfaces

* chore(web): remove debug print

* feat: register postgres repl

* refactor(web): update MakeDatabase to receive access checker and interactive

* chore(web): remove unused function
github-merge-queue bot pushed a commit that referenced this pull request Dec 16, 2024
* Add PostgreSQL REPL implementation (#49598)

* feat(repl): add postgres

* refactor(repl): change repl to use a single Run function

* test(repl): reduce usage of require.Eventually blocks

* refactor(repl): code review suggestions

* refactor(repl): code review suggestions

* test(repl): increase timeout values

* fix(repl): commands formatting

* refactor(repl): send close pgconn using a different context

* fix(repl): add proper spacing between multi queries

* test(repl): add fuzz test for processing commands

* Add WebSocket handler for WebUI database sessions (#49749)

* feat(web): add websocket handler for database webui sessions

* refactor: move common structs into a separate package

* refactor(web): use ALPN local proxy to dial databases

* feat(repl): add default registry

* refactor(web): code review suggestions

* refactor: update repl config parameters

* refactor: move default getter implementation

* feat(web): add supports_interactive field on dbs

* refactor: code review suggestions

* refactor: update database REPL interfaces

* chore(web): remove debug print

* feat: register postgres repl

* refactor(web): update MakeDatabase to receive access checker and interactive

* chore(web): remove unused function

* Database access through WebUI (#49979)

* feat(web): add database terminal access

* chore(web): make explict type cast

* refactor(web): code review suggestions

* chore(web): fix lint errors

* refactor(web): lint errors

* refactor: code review suggestions

* refactor(web): filter wildcard options from connect dialog

* chore(web): lint

* refactor(web): code review suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/lg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants