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.

Sign up for GitHub

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

Jump to bottom

TLS Routing native WebSocket connection upgrade support #36343

Merged
merged 11 commits into from
Feb 12, 2024
Merged

TLS Routing native WebSocket connection upgrade support #36343

merged 11 commits into from
Feb 12, 2024

Conversation

greedy52
Copy link
Contributor

@greedy52 greedy52 commented Jan 5, 2024 •
edited
Loading

Implements #30493

changelog: added TLS routing native WebSocket connection upgrade support

Summary

TODO:

  • Integration tests passed (no change)
  • Do we need env var toggles to pick individual upgrade type instead of sending both?
  • Manual Test
    • IP is forwarded
    • Agent joining
    • AWS ALB
    • GCP ALB
    • cloudflared tunnel (through UI)
    • cloudflared tunnel (through CLI. e.g cloudflared tunnel --loglevel debug run --url https://localhost:9090 --no-tls-verify teleport-test-2)
    • old client vs new server
    • new client vs old server
    • CloudFlare (regular proxied domain, with TLS option Full )
  • Will do a separate doc update

@greedy52 greedy52 added the tls-routing Issues related to TLS routing label Jan 5, 2024
@greedy52 greedy52 self-assigned this Jan 5, 2024
@greedy52 greedy52 force-pushed the STeve/30493/tls_conn_upgrade_web_socket branch from 6f5f035 to 91ce8b9 Compare January 5, 2024 21:23
@greedy52 greedy52 mentioned this pull request Jan 8, 2024
Merged
@strideynet strideynet self-requested a review January 8, 2024 20:19
@marcoandredinis marcoandredinis self-requested a review January 22, 2024 11:47
@greedy52 greedy52 requested a review from smallinsky January 22, 2024 18:23
@greedy52 greedy52 marked this pull request as ready for review January 22, 2024 18:26
@github-actions github-actions bot requested a review from Tener January 22, 2024 18:26
@marcoandredinis marcoandredinis removed their request for review January 26, 2024 17:11
strideynet
strideynet reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@strideynet strideynet left a comment

Choose a reason for hiding this comment

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

Generally looks good and compliant with the RFD - just a few queries here.

lib/web/conn_upgrade_test.go Show resolved Hide resolved
lib/web/conn_upgrade.go Outdated Show resolved Hide resolved
api/constants/constants.go Show resolved Hide resolved
api/client/alpn_conn_upgrade.go Outdated Show resolved Hide resolved
lib/web/conn_upgrade.go Outdated Show resolved Hide resolved
lib/web/conn_upgrade.go Outdated Show resolved Hide resolved
@greedy52 greedy52 requested a review from strideynet February 1, 2024 16:45
smallinsky
smallinsky reviewed Feb 7, 2024
View reviewed changes
api/client/alpn_websocket.go Outdated Show resolved Hide resolved
api/client/alpn_conn_upgrade.go Outdated Show resolved Hide resolved
api/client/alpn_conn_upgrade.go Outdated Show resolved Hide resolved
api/client/alpn_websocket.go Show resolved Hide resolved
api/client/alpn_websocket.go Outdated Show resolved Hide resolved
lib/web/conn_upgrade.go Show resolved Hide resolved
lib/web/conn_upgrade.go Show resolved Hide resolved
@greedy52 greedy52 force-pushed the STeve/30493/tls_conn_upgrade_web_socket branch from c99372b to 4f7e38b Compare February 8, 2024 17:54
@greedy52 greedy52 requested a review from smallinsky February 8, 2024 19:46
smallinsky
smallinsky approved these changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

LGTM, Great job with adding native websocket connection upgrade support.

api/client/alpn_conn_upgrade.go Outdated Show resolved Hide resolved
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from Tener February 9, 2024 13:07
@greedy52 greedy52 enabled auto-merge February 12, 2024 16:35
@greedy52 greedy52 disabled auto-merge February 12, 2024 17:47
@greedy52 greedy52 enabled auto-merge February 12, 2024 17:47
@greedy52 greedy52 added this pull request to the merge queue Feb 12, 2024
Merged via the queue into master with commit 5cabb53 Feb 12, 2024
38 checks passed
@greedy52 greedy52 deleted the STeve/30493/tls_conn_upgrade_web_socket branch February 12, 2024 18:21
@public-teleport-github-review-bot
Copy link

@greedy52 See the table below for backport results.

Branch Result
branch/v15 Failed

greedy52 added a commit that referenced this pull request Feb 12, 2024
@greedy52
TLS Routing native WebSocket connection upgrade support (#36343)
fcb3aff 
* TLS routing connection upgrade using native websocket

* update ut in api

* lib/web/UT update

* fix typo, lint and race

* deal with subprotocol negotiation

* review comments round 1

* fix lint?

* add UT and address some other comments

* add env var to toggle mode

* fix lint
This was referenced Feb 12, 2024
Merged
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 26, 2024
@greedy52
TLS Routing native WebSocket connection upgrade support (#36343) (#38108
18079da 
)

* TLS routing connection upgrade using native websocket

* update ut in api

* lib/web/UT update

* fix typo, lint and race

* deal with subprotocol negotiation

* review comments round 1

* fix lint?

* add UT and address some other comments

* add env var to toggle mode

* fix lint
This was referenced Mar 11, 2024
Closed
Closed
This was referenced Mar 11, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@strideynet strideynet strideynet approved these changes

@smallinsky smallinsky smallinsky approved these changes

Assignees

@greedy52 greedy52

Labels
backport/branch/v15 size/md tls-routing Issues related to TLS routing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@greedy52 @strideynet @smallinsky