feat: Use the address in the HTTP connect message as the upstream address#285
Conversation
…Simplify upstream config.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #285 +/- ##
==========================================
+ Coverage 85.30% 85.32% +0.02%
==========================================
Files 36 36
Lines 2701 2657 -44
==========================================
- Hits 2304 2267 -37
+ Misses 269 263 -6
+ Partials 128 127 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR changes upstream routing so that both Kubernetes (HTTP CONNECT) and SSH proxying use the destination address provided in the authenticated CONNECT message (GAT), rather than a statically configured upstream address. It also removes SSH upstream configuration and simplifies the Helm chart/config layout accordingly.
Changes:
- Route upstream connections using
conn.GetAddress()from the CONNECT message for Kubernetes reverse proxying and SSH upstream dialing. - Remove SSH upstream configuration (
ssh.upstreams) and update tests/config fixtures to match. - Treat
kubernetes.upstreamsas credentials-only (testing) and ignore any configured upstream address in favor of CONNECT-provided routing; update Helm values/schema and snapshots.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/ssh_test.go | Drops SSH upstream config usage in integration setup. |
| test/data/config.yaml | Removes hardcoded Kubernetes upstream address from test config fixture. |
| internal/sshhandler/proxy.go | Uses CONNECT-provided address for SSH upstream dialing; removes unknown-upstream logic. |
| internal/sshhandler/proxy_test.go | Updates SSH proxy tests to no longer rely on configured upstreams; removes unknown-upstream test. |
| internal/sshhandler/config.go | Removes SSH upstream mapping; stores only gateway username for upstream connections. |
| internal/sshhandler/config_test.go | Updates SSH config tests to reflect removal of upstreams. |
| internal/proxy/proxy_test.go | Removes SSH upstreams from test config. |
| internal/httphandler/http_proxy.go | Uses CONNECT-provided address as reverse proxy target; switches to credential fields on config. |
| internal/httphandler/http_proxy_test.go | Updates HTTP proxy test config and GAT claims to match new config/API expectations. |
| internal/httphandler/config.go | Refactors Kubernetes handler config to load credentials (in-cluster or first upstream entry) and ignore upstream address for routing. |
| internal/httphandler/config_test.go | Updates tests for new Kubernetes handler config behavior (external creds vs in-cluster default). |
| internal/config/config.go | Removes SSH upstream config types/validation; relaxes Kubernetes upstream validation rules. |
| internal/config/config_test.go | Updates config parsing/validation tests for new Kubernetes/SSH configuration behavior. |
| deploy/gateway/values.yaml | Removes SSH upstream values and clarifies Kubernetes upstreams as external creds-only (testing). |
| deploy/gateway/values.schema.json | Updates Helm schema to remove inCluster/address for k8s upstreams and remove SSH upstreams entirely. |
| deploy/gateway/tests/snapshot_test.yaml | Updates Helm snapshot tests to remove upstream settings. |
| deploy/gateway/tests/gateway-configmap_test.yaml | Updates configmap rendering tests for new kubernetes/ssh blocks and schema expectations. |
| deploy/gateway/tests/snapshot/snapshot_test.yaml.snap | Refreshes rendered snapshot output to match new template behavior. |
| deploy/gateway/templates/gateway-configmap.yaml | Adjusts templating to render kubernetes block without inCluster upstreams and removes ssh.upstreams rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
clement0010
left a comment
There was a problem hiding this comment.
I think we should also include a GitHub Issue in the PR description for tracking purposes right?
| } | ||
|
|
||
| inClusterConfig, err := rest.InClusterConfig() | ||
| upstream := k8sConfig.Upstreams[0] |
There was a problem hiding this comment.
nit: the comment from previous code // Multiple upstreams support will be added soon! was removed (accidental?) - maybe you intended to keep it
There was a problem hiding this comment.
That comment is meant to explain why we only use the first upstream with k8sConfig.Upstreams[0]. Now that there is no upstreams in the config, I think it's not needed. Plus, we'll support multiple upstreams for k8s at some point but probably not "soon" because there is no use case/user request yet 😬
Issue: #286
Changes
upstreamsin Gateway config. Leave k8s upstream for testing for now