move route config provider manager to singleton manager registry#1449
move route config provider manager to singleton manager registry#1449mattklein123 merged 14 commits intomasterfrom
Conversation
| static constexpr char route_config_provider_manager_singleton_name[] = | ||
| "route_config_provider_manager_singleton_name"; | ||
| static Registry::RegisterFactory< | ||
| Singleton::RegistrationImpl<route_config_provider_manager_singleton_name>, |
There was a problem hiding this comment.
I wonder if it makes sense to have a macro to avoid the boilerplate when we declare singletons. E.g. make this something more like DEFINE_SINGLETON(route_config_provider_manager);, which then synthesizes the rest. Could clean this up for the date provider too.
There was a problem hiding this comment.
that sounds like a good idea.
| }); | ||
|
|
||
| std::shared_ptr<HttpConnectionManagerConfig> http_config(new HttpConnectionManagerConfig( | ||
| config, context, *date_provider, *route_config_provider_manager)); |
There was a problem hiding this comment.
Are we losing the reference count here? The singleton manager only has a weak pointer, when route_config_provider_manager goes out of scope above, it will delete the RouteConfigProviderManager.
There was a problem hiding this comment.
It's captured in the lambda below. I meant to put a comment on this and I admit it's a little hard to see. I would just add a comment to make it clear.
There was a problem hiding this comment.
Also, I haven't looked at your admin change yet, but this change will have implications on that one (not sure exactly what your plan is there) so I would land this one first.
| /** | ||
| * @return RouteConfigProviderManager& singleton for use by the entire server. | ||
| */ | ||
| virtual Router::RouteConfigProviderManager& routeConfigProviderManager() PURE; |
There was a problem hiding this comment.
I don't see any mock cleanups in this change. Please get those also.
| static constexpr char route_config_provider_manager_singleton_name[] = | ||
| "route_config_provider_manager_singleton_name"; | ||
| static Registry::RegisterFactory< | ||
| Singleton::RegistrationImpl<route_config_provider_manager_singleton_name>, |
There was a problem hiding this comment.
that sounds like a good idea.
| }); | ||
|
|
||
| std::shared_ptr<HttpConnectionManagerConfig> http_config(new HttpConnectionManagerConfig( | ||
| config, context, *date_provider, *route_config_provider_manager)); |
There was a problem hiding this comment.
It's captured in the lambda below. I meant to put a comment on this and I admit it's a little hard to see. I would just add a comment to make it clear.
| }); | ||
|
|
||
| std::shared_ptr<HttpConnectionManagerConfig> http_config(new HttpConnectionManagerConfig( | ||
| config, context, *date_provider, *route_config_provider_manager)); |
There was a problem hiding this comment.
Also, I haven't looked at your admin change yet, but this change will have implications on that one (not sure exactly what your plan is there) so I would land this one first.
junr03
force-pushed
the
singleton-manager-route-config
branch
2 times, most recently
from
a0f7dfb to
c590189
Compare
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM other than switching to a macro for defining a singleton per @htuch. Thanks.
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM as well modulo singleton macro.
|
@htuch updated with the macro. I am not sure if it is preferable to have it here on in the manager interface, open to your suggestion. |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Looks good. I'd probably but it in singleton.h, but arguably it's related to both header files but in neither case does it have the set of dependencies actually fully included.
|
|
||
| std::shared_ptr<Router::RouteConfigProviderManager> route_config_provider_manager = | ||
| context.singletonManager().getTyped<Router::RouteConfigProviderManager>( | ||
| route_config_provider_manager_singleton_name, [&context] { |
There was a problem hiding this comment.
We should have a macro to get this name, rather than assuming folks know to suffix _singleton_name.
There was a problem hiding this comment.
+1 please put in singleton.h and +1 to macro for getting name.
|
|
||
| typedef std::shared_ptr<Instance> InstanceSharedPtr; | ||
|
|
||
| /** |
There was a problem hiding this comment.
nit: IMO this should go in manager.h, but I don't feel that strongly about it if you prefer it here.
There was a problem hiding this comment.
@mattklein123 Makes sense with the RegistrationImpl being here.
**Description**
This allows configuring the header where api credentials will be
injected when talking to the upstream MCP servers. Previously, we could
only inject Bearer token credentials, but some MCP servers require them
to be on a different header. With this change users can configure the
header like in the following example:
```yaml
apiVersion: aigateway.envoyproxy.io/v1alpha1
kind: MCPRoute
metadata:
name: mcp-route
namespace: default
spec:
parentRefs:
- name: aigw-run
kind: Gateway
group: gateway.networking.k8s.io
path: "/mcp"
backendRefs:
- name: mcp-backend
kind: Backend
group: gateway.envoyproxy.io
securityPolicy:
apiKey:
header: "X-Api-Key"
secretRef:
name: apikey-secret
```
**Related Issues/PRs (if applicable)**
N/A
**Special notes for reviewers (if applicable)**
N/A
---------
Signed-off-by: Ignasi Barrera <ignasi@tetrate.io>
3 participants
Move the RouteConfigProviderManager singleton from server ownership to shared ownership by HttpConnectionManagers. Registered with the Singleton Manager introduced in #1410.
WIP need to look at asan and tsan failures