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

[hold/DNM] allow extensions to provide custom Upstreams #9901

Closed
wants to merge 72 commits into from

Conversation

stevenctl
Copy link
Contributor

Description

We have an extension point for upstreams and endpoints with DiscoveryPlugin, but the EDS emitting logic is based on HybridUpstreamClient. This adds the ability to provide extension there as well.

Code changes

  • NewHybridUpstreamClient now takes a list of []plugin.Plugin and adds to the clientMap
  • Util function to filter plugins by type using generics
  • Replace existing type-filter for DiscoveryPlugin to use helper

Testing steps

WIP enterprise PoC

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@github-actions github-actions bot added keep pr updated signals bulldozer to keep pr up to date with base branch work in progress signals bulldozer to keep pr open (don't auto-merge) labels Aug 15, 2024
Copy link

github-actions bot commented Aug 15, 2024

Visit the preview URL for this PR (updated for commit 2523cb8):

https://gloo-edge--pr9901-stevenctl-hybrid-us-br6z6h1p.web.app

(expires Fri, 11 Oct 2024 21:03:30 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

soloio-bulldozer bot and others added 25 commits August 15, 2024 21:06
@sam-heilbron
Copy link
Contributor

for _, plugin := range clientPlugins {
name, client := plugin.SourceName(), plugin.Client()
if _, f := clientMap[name]; f {
return nil, eris.Errorf("HybridUpstreamClient already has a source %q", name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this error occurs, is that a developer error that we could/should error more loudly about, or is this a case that can occur for some customers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a dev error. This is called from RunGlooWithExtensions which also bubbles up the error, so I think it's crashing, right?

@stevenctl
Copy link
Contributor Author

/kick

@stevenctl stevenctl changed the title allow extensions to provide custom Upstreams [hold/DNM] allow extensions to provide custom Upstreams Sep 18, 2024
@stevenctl
Copy link
Contributor Author

I might not want to merge this after all, some work @lgadban is doing may make it obsolete.

@timflannagan timflannagan removed the keep pr updated signals bulldozer to keep pr up to date with base branch label Sep 18, 2024
@timflannagan
Copy link
Contributor

@stevenctl is this still relevant based on your last comment or should we close this out?

@stevenctl
Copy link
Contributor Author

closed in favor of KRT

@stevenctl stevenctl closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress signals bulldozer to keep pr open (don't auto-merge)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants