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

feat: use separate bind for http ingress server #1615

Merged
merged 1 commit into from
Jun 1, 2024

Conversation

wesbillman
Copy link
Collaborator

Fixes #1609

@alecthomas I took a stab at this one. Lemme know if there's a better approach here. :)

@wesbillman wesbillman requested a review from alecthomas as a code owner May 31, 2024 15:56
@wesbillman wesbillman requested review from a team and matt2e and removed request for a team May 31, 2024 15:56
@ftl-robot ftl-robot mentioned this pull request May 31, 2024
go func() {
err := rpc.ServeHTTP(ctx, config.IngressBind, ingressHandler)
if err != nil {
logger.Errorf(err, "Ingress server failed")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we want to return an error here instead of logging? I wasn't 100% sure since I wanted to keep the current setup of returning the rpc.Serve result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We definitely do want to return an error. Just put both into an errgroup an return eg.Wait()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't think we should be using an rpc.Server for this, it has a whole lot of machinery that we just don't want for normal HTTP serving. Better to create a new package internal/http (or similar).

@wesbillman wesbillman force-pushed the separate-http-server branch from 00305f8 to 0ec9cea Compare May 31, 2024 16:08
go func() {
err := rpc.ServeHTTP(ctx, config.IngressBind, ingressHandler)
if err != nil {
logger.Errorf(err, "Ingress server failed")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We definitely do want to return an error. Just put both into an errgroup an return eg.Wait()

BaseContext: func(net.Listener) context.Context { return ctx },
}

return &Server{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you just return a http.Server here?

go func() {
err := rpc.ServeHTTP(ctx, config.IngressBind, ingressHandler)
if err != nil {
logger.Errorf(err, "Ingress server failed")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't think we should be using an rpc.Server for this, it has a whole lot of machinery that we just don't want for normal HTTP serving. Better to create a new package internal/http (or similar).

@wesbillman wesbillman force-pushed the separate-http-server branch 2 times, most recently from 0353016 to c00648b Compare May 31, 2024 22:58

type Server struct {
listen *url.URL
Server *http.Server
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably embed this directly.

Copy link
Collaborator Author

@wesbillman wesbillman May 31, 2024

Choose a reason for hiding this comment

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

DUUUUUUDE!!! Thank you. This is so much cleaner. Sorry it took me a bit to get there 😂

@alecthomas
Copy link
Collaborator

Noice

@wesbillman wesbillman force-pushed the separate-http-server branch from c00648b to c7a4c6a Compare May 31, 2024 23:56
@wesbillman wesbillman force-pushed the separate-http-server branch from c7a4c6a to de6f0d7 Compare June 1, 2024 00:01
@wesbillman wesbillman merged commit f445e18 into main Jun 1, 2024
27 checks passed
@wesbillman wesbillman deleted the separate-http-server branch June 1, 2024 00:05
@matt2e matt2e added the approved Marks an already closed PR as approved label Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Marks an already closed PR as approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we remove the /ingress from the path from http ingress request
3 participants