Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: ablility to add clusters in PostHTTPListenerModify hook #4373

Open
zirain opened this issue Sep 30, 2024 · 7 comments
Open

proposal: ablility to add clusters in PostHTTPListenerModify hook #4373

zirain opened this issue Sep 30, 2024 · 7 comments
Labels
area/extensions kind/decision A record of a decision made by the community. stale

Comments

@zirain
Copy link
Member

zirain commented Sep 30, 2024

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 zirain added area/extensions kind/decision A record of a decision made by the community. labels Sep 30, 2024
@guydc
Copy link
Contributor

guydc commented Oct 1, 2024

cc @liorokman

@guydc
Copy link
Contributor

guydc commented Oct 1, 2024

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
Copy link
Member Author

zirain commented Oct 8, 2024

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
Copy link
Contributor

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
Copy link
Member Author

zirain commented Oct 20, 2024

@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
Copy link
Contributor

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

Copy link

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

@github-actions github-actions bot added the stale label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extensions kind/decision A record of a decision made by the community. stale
Projects
None yet
Development

No branches or pull requests

3 participants