gateway icon indicating copy to clipboard operation
gateway copied to clipboard

proposal: ablility to add clusters in `PostHTTPListenerModify` hook

Open zirain opened this issue 1 year ago • 6 comments

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

zirain avatar Sep 30 '24 06:09 zirain

cc @liorokman

guydc avatar Oct 01 '24 18:10 guydc

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, ... )

guydc avatar Oct 01 '24 18:10 guydc

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.

zirain avatar Oct 08 '24 03:10 zirain

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?

liorokman avatar Oct 20 '24 12:10 liorokman

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

zirain avatar Oct 20 '24 13:10 zirain

@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.

liorokman avatar Oct 21 '24 04:10 liorokman

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

github-actions[bot] avatar Nov 20 '24 08:11 github-actions[bot]

we didn't need this any more.

zirain avatar Dec 27 '25 03:12 zirain