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

HTTPSO-based Routing Table #669

Merged
merged 26 commits into from
Jun 14, 2023
Merged

Conversation

t0rr3sp3dr0
Copy link
Contributor

Reimplements routing table of the HTTP Add-On to make it using HTTPSO objects as the source of truth instead of relying on a ConfigMap that is updated periodicaly by the operator.

Highlights:

  • Remove all ConfigMap-related routines and permissions.
  • Use a SharedInformer to interface with the API Server. HTTPSO objects are cached on memory and events are emitted when an object change. It's possible to pull objects from this cached manually instead of relying on events.
  • Routing table indexes routes using a Radix Tree, similar to what one of the fastest Golang HTTP Routers do: https://github.com/julienschmidt/httprouter#how-does-it-work. This also allows path-based routing to be added in the near future.
  • No user-facing interface was broken, the change is transparent for the end-user.

Checklist

Fixes #605

Sorry, something went wrong.

@t0rr3sp3dr0 t0rr3sp3dr0 requested a review from a team as a code owner May 9, 2023 04:30
@t0rr3sp3dr0
Copy link
Contributor Author

@JorTurFer @tomkerkhove, sorry for the delay! Here we have the new Routing Table implementation.

I'll be adding new tests and fixing existing ones in the following days. I would appreciate reviews in the meantime.

I left the support for path-based routing out of this PR because it alredy changes a lot of stuff, but I architected the new RT with that feature in mind. I'll be working on that next.

@t0rr3sp3dr0 t0rr3sp3dr0 force-pushed the pedrotorres/issue-605 branch 4 times, most recently from df71c09 to dcee452 Compare May 9, 2023 07:33
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Looking fantastic!
Some comments inline
I'll review it deeper during next days

@similark
Copy link
Contributor

similark commented May 11, 2023

Hi @t0rr3sp3dr0
Nice work!
I actually started working on integrating that very same httpRouter lib in order to add support for path-based routing.
Now, assuming this pr will be merged (hopefully 🙏 ) - I'll be very happy to help in whatever way to achieve it using your implementation instead.
Please let me know what's the status on that one?

Thanks and well done!

@t0rr3sp3dr0
Copy link
Contributor Author

@similark, it is WIP and I should have a PR by Monday. On this very first version we will only allow one path per HTTPScaledObject, every URL that matches such prefix will be routed to the target specified on the manifest. Is that enough for your use case?

The 2nd version will allow multiple paths to be directed to a single target. But this would be better implemented by having a new API Version of HTTPScaledObject and that needs to be discussed an may take longer to be implemented.

@t0rr3sp3dr0 t0rr3sp3dr0 force-pushed the pedrotorres/issue-605 branch 20 times, most recently from b9e465b to 1487432 Compare May 12, 2023 07:34
@t0rr3sp3dr0 t0rr3sp3dr0 force-pushed the pedrotorres/issue-605 branch 2 times, most recently from 2bb2d49 to d402fba Compare June 1, 2023 21:37
@t0rr3sp3dr0 t0rr3sp3dr0 force-pushed the pedrotorres/issue-605 branch from d402fba to 4c81994 Compare June 1, 2023 21:40
Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
@tomkerkhove tomkerkhove mentioned this pull request Jun 8, 2023
@andrey-bondar
Copy link

Hi! I've just started using this addon and this feature is really crucial as well as #677.
@t0rr3sp3dr0 can you predict when it could be completed?

@t0rr3sp3dr0
Copy link
Contributor Author

@andrey-bondar, this patch is currently feature-complete and I’m adding more tests while I wait for an in-depth review. The review may take some time, given the size of the PR and that we currently have a small number of maintainers, but it is being worked on. In the meantime, feel free to leave a review as well.

@JorTurFer
Copy link
Member

JorTurFer commented Jun 11, 2023

I have checked the PR and looks good from coding pov, but I'd like to review it in depth with calm to understand correctly what is happening

Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
Signed-off-by: Pedro Tôrres <[email protected]>
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

You have done an awesome job @t0rr3sp3dr0, I have left some small comments inline

Signed-off-by: Pedro Tôrres <[email protected]>
@JorTurFer
Copy link
Member

LGTM!
Thanks a lot for this awesome improvement 😄

@JorTurFer JorTurFer merged commit fb17e77 into kedacore:main Jun 14, 2023
t0rr3sp3dr0 added a commit to t0rr3sp3dr0/http-add-on that referenced this pull request Jun 23, 2023
Signed-off-by: Pedro Tôrres <[email protected]>
JorTurFer pushed a commit that referenced this pull request Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changes on HTTPScaledObjects don't update routing table
5 participants