search

envoyproxy / gateway

Manages Envoy Proxy as a Standalone or Kubernetes-based Application Gateway
https://gateway.envoyproxy.io
Apache License 2.0
1.63k stars 352 forks source link

proposal: ablility to add clusters in `PostHTTPListenerModify` hook #4373

Open zirain opened 1 month ago

zirain commented 1 month ago

I have a CRD managed by an ExtensionManager, which will create an insert HttpFilter in the HCM (think about RateLimit filter as an example). The question is how do I manage the cluster referenced by the new filter? Now, I can only add a cluster in the "PostTranslateModify" hook, but I can't filter only the CRDS referenced by the gateway. I recommend enabling 'PostHTTPListenerModify' to add clusters.

WDYT? @Alice-Lilith @LiorLieberman @guydc @arkodg @zhaohuabing

guydc commented 1 month ago

cc @liorokman

guydc commented 1 month ago

I recommend enabling 'PostHTTPListenerModify' to add clusters

There's some risk here for a conflict with EG, if the cluster created by the extension server has the same "id" as a cluster that EG needs to create, if we allow extension server to create clusters before EG.

What about having a notion of user-defined backend extensions? Essentially, a CRD for defining clusters. If you control creation of listeners and clusters, you can make sure to use a consistent id.

This can even be supported in backendrefs, allowing vendors and users to build custom service discovery extensions that integrate with non-k8s service registries (consul, ... )

zirain commented 1 month ago

What about having a notion of user-defined backend extensions? Essentially, a CRD for defining clusters. If you control creation of listeners and clusters, you can make sure to use a consistent id.

  1. another kind of Backend?
  2. we may won't share clusters across listner?

This can even be supported in backendrefs, allowing vendors and users to build custom service discovery extensions that integrate with non-k8s service registries (consul, ... )

this will bring more complex than expected.

liorokman commented 3 weeks ago

I recommend enabling 'PostHTTPListenerModify' to add clusters

There's some risk here for a conflict with EG, if the cluster created by the extension server has the same "id" as a cluster that EG needs to create, if we allow extension server to create clusters before EG. 

The thing is that once there's an extension server modifying configuration, the potential for conflict with EG is there anyways. To the point that an extension server written for a specific EG version might create invalid configuration with a newer/older EG version, due to changes in the way that EG creates XDS.

Just the risk for conflict with EG on it's own is not a good enough reason not to allow this. The underlying reality underpinning the use of an extension server is that if your extension server breaks the configuration, you get to keep both pieces.

My concern is that adding clusters in a hook dedicated to listeners is a little strange. Wouldn't it be cleaner to have an extension server that needs to add a cluster due simply keep its state internally and add the cluster when the PostTranslateModify hook is called? The order of the hook calls is that the PostTranslateModify hook is called last, after all of the other hooks were called.

What about having a notion of user-defined backend extensions? Essentially, a CRD for defining clusters. If you control creation of listeners and clusters, you can make sure to use a consistent id.

Why do we need a user-defined backend extension type? Why not add functionality in EG to allow defining a backend with the current existing Backend CRD that is also added to the translated XDS as a cluster even if it isn't being referenced in any route?

This can even be supported in backendrefs, allowing vendors and users to build custom service discovery extensions that integrate with non-k8s service registries (consul, ... )

This is an interesting use-case. Since custom discovery would probably require additional code anyways, maybe what we need is to create a Backend type that is basically a placeholder - creates XDS that doesn't have any endpoints of any type in it - and then allow an extension server to fill in the blanks using whatever programmatic logic it wants.

Maybe a feature flag that disables validating that the standard Backend type has defined something in the Endpoints array?

zirain commented 3 weeks ago

@liorokman do you mean make PostTranslateModify add context with resources referenced by the Gateway? the risk it's that the number may too much.

liorokman commented 3 weeks ago

@liorokman do you mean make PostTranslateModify add context with resources referenced by the Gateway? the risk it's that the number may too much.

That's one way to do this. I agree that since the PostTranslateModify hook is a catch-all type of hook, the amount of information passed on to it would be really large. I wouldn't want to go down this path.

Another way that might work a little better would be for the extension server to keep track (as internal state) of what it wants to add to the configuration during the per-listener or per-route hook invocations, and then when the PostTranslateModify hook is called to add it. No additional context needs to be added to the hook itself. I think this sort of manipulation can be done already.