Support HTTP tunneling on all Upstreams#10712
Conversation
|
Issues linked to changelog: |
|
Visit the preview URL for this PR (updated for commit 63a5227): https://gloo-edge--pr10712-rolds-remote-jwks-up-43q77f4s.web.app (expires Wed, 09 Apr 2025 22:24:34 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
…om/solo-io/gloo into rolds/remote_jwks_upstream_tunneling
…om/solo-io/gloo into rolds/remote_jwks_upstream_tunneling
sam-heilbron
left a comment
There was a problem hiding this comment.
We have an in-memory e2e test: https://github.com/solo-io/gloo/blob/main/test/e2e/dynamic_forward_proxy_test.go. Now that we support a new use case, is it worth it to add a test to prove that case?
Will do. |
I think those tests are unrelated. There are in-memory E2E tests for HTTP tunneling. I understand your ask to be that you would like a non-route bound Upstream to be confirmed to work. |
|
@sam-heilbron, I looked at non-EE cases to test and it's pretty much tracing. As noted in the description, I'm working on adding a test for the customer's case. Does this work or would you really like two tests to confirm that routes and non-routed Upstreams work in Gloo even though both use the same code path? |
sam-heilbron
left a comment
There was a problem hiding this comment.
LGTM! I like the testing plan you proposed
| func (p *plugin) GeneratedResources(params plugins.Params, | ||
| func (p *plugin) GeneratedResources( | ||
| params plugins.Params, | ||
| proxy *v1.Proxy, |
There was a problem hiding this comment.
I really like this change! I feel like it makes the plugin more self-contained, and a set of resources are associated with the Proxy resource anyway, so I think this makes a lot of sense
| cfg, err := utils.NewSslConfigTranslator().ResolveUpstreamSslConfig(params.Snapshot.Secrets, | ||
| us.GetHttpConnectSslConfig()) | ||
| if err != nil { | ||
| // if we are configured to warn on missing tls secret and we match that error, add a |
There was a problem hiding this comment.
I like the idea here, and we may be required to have this logic built into this plugin. What I wish existed, was that we could just compile errors and warnings in reports (and basically not make plugins aware of how they are consumed), and then when we process them, we would inspect the validation settings and processes/consume those reports differently.
https://github.com/solo-io/gloo/blob/main/projects/gloo/pkg/translator/reporting.go#L173 is that in practice, https://github.com/solo-io/gloo/blob/main/projects/gloo/pkg/plugins/configuration_error.go.
Do you know if there is a way to ensure that plugins can be unaware of high-level settings like this?
There was a problem hiding this comment.
The plugin does need to know about setting as the behavior (not standing-up the tunnel) depends on the setting and error.
It may be possible to push the check down into the Resolve<X>SslConfig methods, but the need to check for the error and branch is still needed. I'm not sure if we want to tightly couple the error for those methods to the ResourceReports.
|
/kick-ci |
|
I've merged a refactor that should support gateway2. I will be confirming it works with my solo-projects PR once CI finishes. |
| newInClusterName := clusterName + OriginalClusterSuffix | ||
|
|
||
| // use an in-memory pipe to ourselves (only works on linux) | ||
| forwardingPipe := "@/" + clusterName |
There was a problem hiding this comment.
Could consider
https://www.envoyproxy.io/docs/envoy/latest/configuration/other_features/internal_listener
at some point, not truly proposing this now
There was a problem hiding this comment.
I will add a comment nudging future engineers to consider switching to an internal listener.
| endpoints = append(endpoints, generatedEndpoints...) | ||
| routeConfigs = append(routeConfigs, generatedRouteConfigs...) | ||
| listeners = append(listeners, generatedListeners...) | ||
| newClusters, newEndpoints, newRouteConfigs, newListeners := plugin.GeneratedResources(params, proxy, |
There was a problem hiding this comment.
GeneratedResources has a comment about being deprecated, this (the only non-test caller) doesn't have anything like a if (is ggv2) continue? Should we have that? Otherwise maybe we should clarify the deprecation comment.
There was a problem hiding this comment.
There are a few other plugins that use GeneratedResources in the gateway2 translation path, so we can't skip it. The check here prevents the HTTP tunnel plugin from double adding the clusters.
When the other plugins are updated, we can pull what you're highlghting out and only use UpstreamGeneratedResources for both the gloo and gateway2 translation paths.
I will add a comment clarifying the intended approach to removing this.
There was a problem hiding this comment.
I'm adding:
// GeneratedResources is deprecated and being replaced by UpstreamGeneratedResources
// The plan is to move the few remaining plugins in Gloo/EE to use the new
// interface. Once that is done and the new interface is called in the Gloo translation
// path, we can remove this code.
// During this transition period, gateway2 will call both interfaces and it's translator
// has (must have) checks that prevent duplicate resources from being added.
stevenctl
left a comment
There was a problem hiding this comment.
It would also be good to have some of the YAML-only goldenfile style tests for this: see ggv2setup_test.go and assoc testdata.
Great callout. I will make sure that happens. |
…om/solo-io/gloo into rolds/remote_jwks_upstream_tunneling
Description
Refactors the HTTP tunneling plugin to support all Upstreams. I have a PR that confirms a tunnel can be used with a remote JWKS (customer use case). This is Solution 3 in the design document.
While working on tests for the customer use case I discovered that the lifecycle differences with krt require introducing a new plugin interface that was krt-safe. After talking with Yuval, I implemented my understanding of his recommendation. This work should help with other uses of
GeneratedResourcesin gateway2/kgateway as well.The plugin has two paths that use common logic to transforming and creating the additional clusters/listeners:
GeneratedResourcesUpstreamGeneratedResourcesTests for the customer case have been added to https://github.com/solo-io/solo-projects/pull/8080.
Changes:
ClusterResult(holds the additional clusters/listeners) and plumbed through the gateway2 translator and syncerGeneratedResourcesso that the plugin could report issues with specific UpstreamsChecklist: