Customizable net.Listener for advanced use cases, such as tunneling #245

Ignaciojeria · 2024-11-30T15:50:58Z

ccoVeille · 2024-11-30T17:46:39Z

No test added?

dylanhitt · 2024-11-30T20:24:06Z

Would you mind following the With* pattern as well such as here

dylanhitt · 2024-11-30T20:42:31Z

Also we have two entrypoints Run and RunTLS. Updating both would be best.

Also maybe a refactor is in order within the s.setup method. We should probably create the default listener in the Server constructor, and instead of calling ListenAndServe* we just call Serve*.

EwenQuim

Thanks a lot, good idea!
If you can make the requested changes, it would be nice.

dylanhitt · 2024-12-01T01:22:33Z

Pardon me. I was on a phone so I was short.

I will say this is a great contribution. I hope you’re enjoying fuego. 😄

Ignaciojeria · 2024-12-01T07:04:35Z

I've addressed some of the suggestions and pushed a new change. I haven't had the chance to fully test it yet, but I'll be running some tests soon. Any feedback is greatly appreciated!

ccoVeille · 2024-12-01T09:43:37Z

serve.go

+	if err := s.setupDefaultListener(); err != nil {
+		return fmt.Errorf("failed to start server: %w", err)
+	}
 	s.setup()


The setup listener is now called before setup, at all place setup is called if I'm not wrong

What about calling it in setup, and update setup signature to return an error?

Thank you! Two heads are better than one. I made some adjustments and have been running manual tests (locally only).

ccoVeille · 2024-12-01T17:23:58Z

serve.go

+	if err := s.setup(certFile, keyFile); err != nil {
+		return err
+	}


Can't you call WithTLSListener here?

It would allow to pass certFile and keyFile to setup, and the check on isTLS

The code was significantly simplified based on the observation. Can you help check if there’s any room for improvement? Thank you very much!

Great.

I'll open a PR to your fork

I opened Ignaciojeria#1

ccoVeille · 2024-12-01T19:57:57Z

serve.go

@@ -100,7 +100,7 @@ func (s *Server) setupDefaultListener() error {
 	}
 	addr := s.Server.Addr
 	if addr == "" {
-		addr = "127.0.0.1:9999"
+		addr = ":9999"


please check TestPetstoreOpenAPIGeneration

it seems broken now

i will check now

I don't see any problem, but I can add host:port by default to avoid breaking tests. I will run the tests in the next iteration. Let me handle the issues. I'll make sure to run the tests and update you on how it goes.

I did it in Ignaciojeria#1

But let me ask you the question here.

Why did you remove the local host? To listen on all interfaces?

Was this changes needed in this exact PR?

Now I maintained localhost as the default to preserve the original implementation

ccoVeille · 2024-12-01T21:14:44Z

I'm not a fuego maintainer

I opened
Ignaciojeria#1

Please consider it.

server.go

ccoVeille · 2024-12-01T21:55:37Z

server.go

+func getServerAddress(s *Server) string {
+	if s.Listener != nil {
+		return s.Listener.Addr().String()
+	}
+	if s.Server.Addr != "" {
+		return s.Server.Addr
+	}
+	// Default address if none is set
+	return ":9999"
+}


There is no reason for this method.

setListener now sets s.Server.Addr

function removed

ccoVeille · 2024-12-01T22:01:31Z

server.go

@@ -68,6 +70,8 @@ type Server struct {

 	OpenApiSpec openapi3.T // OpenAPI spec generated by the server

+	Listener net.Listener


You are adding a listener

So it should be closed when server stops

So please add this

func (s *Server) Close () { if s.Listener != nil } s.Listener.Close() } s.Server.Close() return }

method added

ccoVeille · 2024-12-01T22:02:42Z

server.go

@@ -68,6 +70,8 @@ type Server struct {

 	OpenApiSpec openapi3.T // OpenAPI spec generated by the server

+	Listener net.Listener


I don't see a need for the listener to be public, you added WithListener

you are right now is private

ccoVeille · 2024-12-01T22:03:30Z

server.go

@@ -125,7 +129,6 @@ func NewServer(options ...func(*Server)) *Server {
 	}

 	defaultOptions := [...]func(*Server){
-		WithAddr("localhost:9999"),


There is a problem with this.

Please take a look at the PR I opened in your fork

PR merged. I removed WithTLSListener because it added unnecessary complexity. I believe RunTLS and WithListener already provide the user with the necessary customization options. Please correct me if I’m wrong.

I thought the same. 👍

Also, semantically, I was annoyed that WithTLSListener was not allowing to pass a listener

I'm a bit confused on why this is a problem?

ccoVeille · 2024-12-02T03:44:32Z

server.go

+// WithListener configures the server to use a custom listener.
+// If no listener is provided, it creates a default listener using the server's address.


Suggested change

// WithListener configures the server to use a custom listener.

// If no listener is provided, it creates a default listener using the server's address.

// WithListener configures the server to use a custom listener.

The sentence is no longer valid, as it raises a panic

sentence removed

ccoVeille · 2024-12-02T03:48:02Z

server.go

+func WithListener(listener net.Listener) func(*Server) {
+	return func(s *Server) {
+		if s.listener != nil {
+			panic("a listener is already configured; cannot overwrite it")
+		}
+		s.listener = listener
+		s.Server.Addr = listener.Addr().String()
+	}
+}


Please check, but I think there is a need for something like this

Suggested change

func WithListener(listener net.Listener) func(*Server) {

return func(s *Server) {

if s.listener != nil {

panic("a listener is already configured; cannot overwrite it")

}

s.listener = listener

s.Server.Addr = listener.Addr().String()

}

}

func WithListener(listener net.Listener) func(*Server) {

return func(s *Server) {

if s.listener != nil {

panic("a listener is already configured; cannot overwrite it")

}

s.listener = listener

if _, ok := listener.(tls.Listener) {

t.isTLS = true

}

s.Server.Addr = listener.Addr().String()

}

}

This is pseudo code made on my phone, you may need to adapt/fix it

I used reflection to check if the instance is of type tls.listener

// WithListener configures the server to use a custom listener. func WithListener(listener net.Listener) func(*Server) { return func(s *Server) { if s.listener != nil { panic("a listener is already configured; cannot overwrite it") } s.isTLS = isTLSListener(listener) WithAddr(listener.Addr().String())(s) s.listener = listener } } func isTLSListener(listener net.Listener) bool { listenerType := reflect.TypeOf(listener) if listenerType != nil && listenerType.String() == "*tls.listener" { return true } return false }

The code is way better, but using assertion seems overkill, also it's very slow.

ccoVeille · 2024-12-02T03:50:37Z

server.go

 	s.OpenApiSpec.Servers = append(s.OpenApiSpec.Servers, &openapi3.Server{
-		URL:         "http://" + s.Addr,
+		URL:         "http://" + s.Server.Addr,


Shouldn't it be https:// when isTLS is true?

right, i get protocol from proto() method

ccoVeille · 2024-12-02T08:22:26Z

server.go

+func isTLSListener(listener net.Listener) bool {
+	listenerType := reflect.TypeOf(listener)
+	if listenerType != nil && listenerType.String() == "*tls.listener" {
+		return true
+	}
+	return false
+}


Reflexion is slow, can't you use a type assertion?

ccoVeille · 2024-12-02T08:23:31Z

server.go

+func WithListener(listener net.Listener) func(*Server) {
+	return func(s *Server) {
+		if s.listener != nil {
+			panic("a listener is already configured; cannot overwrite it")
+		}
+		s.listener = listener
+		s.Server.Addr = listener.Addr().String()
+	}
+}


The code is way better, but using assertion seems overkill, also it's very slow.

Ignaciojeria · 2024-12-02T14:55:21Z

To handle the overwriting of the listener or address, I have three options in mind:

If the listener or address is overwritten, print a warning.
Continue throwing a panic, stopping the process with a message.
Create an array of errors as a private attribute in the server instance and handle them during setup.

What do you think is the best option, or do you have any suggestions?

ccoVeille · 2024-12-02T19:09:11Z

To handle the overwriting of the listener or address, I have three options in mind:

If the listener or address is overwritten, print a warning.

Continue throwing a panic, stopping the process with a message.

Create an array of errors as a private attribute in the server instance and handle them during setup.

What do you think is the best option, or do you have any suggestions?

I would say no with the last option, it hides things. People may never calls it.

Throwing a panic is not my favorite either, but why not.

But first, why is there a problem to overload the listener if the server is not started?

Why not checking if the server is started to raise panic instead of checking s.listener ?

I mean for me there is the exact same issue with WithPort or WithAddress, you can overload until you started the server.

Another option would have been to use Option patrern that supports errors handling, but it's a bit late maybe. But maybe it was what you were suggesting with option 2, add an errServe to the struct, set in on error, check it after the for opt loop.

dylanhitt · 2024-12-02T22:09:31Z

I also don't understand the checking around Addr. The default listener should should be created with whatever value Server.Server.Addr is. We default when one isn't provided so it should always be set.

serve.go

server.go

dylanhitt · 2024-12-02T23:38:32Z

server.go

 	s.OpenApiSpec.Servers = append(s.OpenApiSpec.Servers, &openapi3.Server{
-		URL:         "http://" + s.Addr,
+		URL:         fmt.Sprintf("%s://%s", s.proto(), s.Server.Addr),


I believe this would always be just http at this point except in the case where a Listener is passed. The current behavior is that we don’t know until the user calls RunTLS which is fine.

I think we’d need even an even larger refactor to address this well. This falls out of scope of this PR

Hmmm. Actually probably the proper places is at the end of setup().

I could be wrong it’s late 😅

I agree, I thought it could be a problem too.

So does it have to be merged like that? With somehow a bug?

Or the refactoring should gf done before? Either here or in another PR?

Doing it in setup now is fine.

server.go

serve.go

server.go

dylanhitt

The last bit would be nice as it would setup the server stuff well. Also not sure why we need to know if the listener is TLS or not care to explain?

All in all thank you for the work. This PR was challenging and it didn't fit quite well into the patterns we've been sticking too but this looks good. 😄

It would be nice to have bit of organized commit history as I'm fan of commits being a body of work. We don't really have anything around here for commit guidelines yet so feel free to do as you wish unless someone expresses their opinions.

Thanks again

Ignaciojeria · 2024-12-04T03:00:07Z

I removed the method that checks if the listener is TLS. Sorry if my commits are chaotic; I’m still learning. What do you think about canceling this pull request? I could create a new fork and consolidate all the changes into a single commit.

ccoVeille · 2024-12-04T10:16:12Z

I removed the method that checks if the listener is TLS. Sorry if my commits are chaotic; I’m still learning.

It's OK, really.

What do you think about canceling this pull request? I could create a new fork and consolidate all the changes into a single commit.

I would say keep the PR, it can be squashed in a single commit once merged.

If you still want to do it by your own, you can use git rebase -i and merge your commit into one.

But, from my perspective. It's better to have the history of changes in a PR than having it left behind, then another "clean but useless" one

dylanhitt

Yes, git rebase -i HEAD~<number> is a great tool. I use it pretty much everyday.

Anywho, great work. It's awesome that you're learning by contributing to open source. I found that helped me more than anything.

Cheers!

serve.go

dylanhitt · 2024-12-04T17:08:56Z

Thank you for the contribution. Seems you need to make lint and we should be good.

Ignaciojeria · 2024-12-05T05:50:32Z

I still have pending unit tests as adding a couple of lines caused the coverage to drop. I'll review it more thoroughly this week after work.

EwenQuim · 2024-12-06T10:59:25Z

server.go

+	addr := s.Addr
+	if s.listener != nil {
+		addr = s.listener.Addr().String()
+	}


Useless, isn't it? Can't we directly use s.Addr as it has been set in WithListener?

It is possible for the user to do something poor here say the user adds their own listener and then their own withaddr. The value could be off since listener trumps everything. I plan on getting rid of this possibility with #248. Basically pushing the openapi server setup as close to startup as we can.

Good idea. Good thing mux and server are decorrelated in the std lib

serve.go

…lidate if listenner is tls using reflection

…ener when listener != nil

dylanhitt · 2024-12-20T17:52:18Z

Hey @Ignaciojeria,

I added the tests and did come cleanup. I am gonna need you to rebase or you main branch though. (The rebase isn't the most fun either I just went through 😅). Here is the PR: Ignaciojeria#2

the two commits i added are:

I also could just open the PR myself. Idk whatever you want to do.

dylanhitt · 2024-12-20T22:43:24Z

hmm. It seems all commits are duplicated. Did you rebase locally?

Ignaciojeria · 2024-12-20T23:28:31Z

hmm. It seems all commits are duplicated. Did you rebase locally?

Let me clean this up and push the updated branch

dylanhitt · 2024-12-23T03:39:29Z

Hey any update on this? 😄

examples/with-listener/go.mod

dylanhitt · 2024-12-27T12:59:43Z

@EwenQuim when we go on this flurry of merges I think we should pull this over the line first.

…lid address in withTls gosum included Update all .mod files instead of only with-listener

dylanhitt · 2025-01-04T16:22:50Z

@EwenQuim bump on this. I’ve added commits so I don’t like to approve my own work.

EwenQuim approved these changes Nov 30, 2024

View reviewed changes

ccoVeille reviewed Dec 1, 2024

View reviewed changes

ccoVeille reviewed Dec 2, 2024

View reviewed changes

dylanhitt requested changes Dec 2, 2024

View reviewed changes

dylanhitt reviewed Dec 3, 2024

View reviewed changes

server.go Outdated Show resolved Hide resolved

dylanhitt reviewed Dec 3, 2024

View reviewed changes

server.go Outdated Show resolved Hide resolved

server.go Outdated Show resolved Hide resolved

dylanhitt reviewed Dec 4, 2024

View reviewed changes

dylanhitt force-pushed the main branch from 63a1ae2 to 26650b6 Compare December 4, 2024 12:46

dylanhitt approved these changes Dec 4, 2024

View reviewed changes

dylanhitt reviewed Dec 4, 2024

View reviewed changes

serve.go Outdated Show resolved Hide resolved

dylanhitt mentioned this pull request Dec 4, 2024

refactor setting of - Server.OpenApiSpec.Servers #248

Closed

dylanhitt approved these changes Dec 4, 2024

View reviewed changes

EwenQuim requested changes Dec 6, 2024

View reviewed changes

Ignaciojeria and others added 12 commits December 20, 2024 12:09

set server addres if is empty added again to avoid broke tests and va…

65f5018

…lidate if listenner is tls using reflection

codebase improvements based on observations

d319ece

WithAddr doc improved

e7d8152

Checking if the server is TLS is not needed.

aef48a3

OpenAPI local server moved to setup.

094dadd

set openapi spec server from listener(if present) or address

0230de7

make lint

8dca718

Remove unnecessary comments and WithAddr handling in defaultSetupList…

84f071f

…ener when listener != nil

setupDefaultListenner fixed

a778cf8

Set attributes directly instead of using the with function.

b30b63e

chore: remove unneeded set of address in WithListener

625a60b

chore: add tests for use of WithListener

401b5e4

Merge branch 'go-fuego:main' into chore/withlistenter_tests

180d78d

Ignaciojeria force-pushed the main branch from 92e88b6 to 713d34d Compare December 21, 2024 02:09

Merge branch 'go-fuego:main' into chore/withlistenter_tests

379c559

Ignaciojeria force-pushed the main branch from 5fa3e51 to 713d34d Compare December 26, 2024 17:15

chore(tests): add withListener integration tests and cleanup

15b2939

Ignaciojeria force-pushed the main branch from 713d34d to 15b2939 Compare December 26, 2024 23:28

Merge branch 'main' into main

47b484b

dylanhitt reviewed Dec 27, 2024

View reviewed changes

examples/with-listener/go.mod Outdated Show resolved Hide resolved

Increase with-listener version, execute gowork, and add test for inva…

fe4d541

…lid address in withTls gosum included Update all .mod files instead of only with-listener

Ignaciojeria force-pushed the main branch from 6339fc3 to fe4d541 Compare December 27, 2024 14:44

Ignaciojeria and others added 2 commits January 2, 2025 11:08

Merge branch 'main' into main

940178f

Merge branch 'main' into main

7fd494a

		@@ -68,6 +70,8 @@ type Server struct {

		OpenApiSpec openapi3.T // OpenAPI spec generated by the server

		Listener net.Listener

		// WithListener configures the server to use a custom listener.
		// If no listener is provided, it creates a default listener using the server's address.

Customizable net.Listener for advanced use cases, such as tunneling #245

Are you sure you want to change the base?

Customizable net.Listener for advanced use cases, such as tunneling #245

Conversation

Ignaciojeria commented Nov 30, 2024

ccoVeille commented Nov 30, 2024

dylanhitt commented Nov 30, 2024

dylanhitt commented Nov 30, 2024 • edited Loading

EwenQuim left a comment • edited Loading

Choose a reason for hiding this comment

dylanhitt commented Dec 1, 2024

Ignaciojeria commented Dec 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ignaciojeria Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoVeille commented Dec 1, 2024

Choose a reason for hiding this comment

Ignaciojeria Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ignaciojeria Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ignaciojeria Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ignaciojeria Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ignaciojeria commented Dec 2, 2024 • edited Loading

ccoVeille commented Dec 2, 2024

dylanhitt commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanhitt left a comment

Choose a reason for hiding this comment

Ignaciojeria commented Dec 4, 2024

ccoVeille commented Dec 4, 2024

dylanhitt left a comment

Choose a reason for hiding this comment

dylanhitt commented Dec 4, 2024

Ignaciojeria commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanhitt commented Dec 20, 2024

dylanhitt commented Dec 20, 2024

Ignaciojeria commented Dec 20, 2024 • edited Loading

dylanhitt commented Dec 23, 2024

dylanhitt commented Dec 27, 2024

dylanhitt commented Jan 4, 2025

dylanhitt commented Nov 30, 2024 •

edited

Loading

EwenQuim left a comment •

edited

Loading

Ignaciojeria Dec 1, 2024 •

edited

Loading

Ignaciojeria Dec 2, 2024 •

edited

Loading

Ignaciojeria Dec 2, 2024 •

edited

Loading

Ignaciojeria Dec 2, 2024 •

edited

Loading

Ignaciojeria Dec 2, 2024 •

edited

Loading

Ignaciojeria commented Dec 2, 2024 •

edited

Loading

dylanhitt commented Dec 2, 2024 •

edited

Loading

Ignaciojeria commented Dec 20, 2024 •

edited

Loading