Add scheme option to bind to both HTTP and HTTPS #1215

raz-amir · 2023-06-15T10:09:32Z

We have a use case where we need the same server to run in both HTTP and HTTPS.
This PR starts two servers when needed (the default single HTTPS server is kept).
It is controlled by adding/changing the following command line arguments:

Existing scheme argument can now get the value both
New port-http argument, defaults to 8080, is used to bind HTTP port only when scheme is set to both. To clarify: to maintain backwards compatibility, when the scheme is set to http, this argument isn't used. The previous port will be used to bind HTTPS.

Added/updated tests
Updated README

fsouza · 2023-06-23T17:52:33Z

We have a use case where we need the same server to run in both HTTP and HTTPS.

Interesting. Can you share more details on that use case? Like, why you need the server to bind on both?

raz-amir · 2023-06-23T19:13:41Z

@fsouza, we have an internal testing setup that is running with http because it was easier to setup. We want to use gsutil in that setup, and it supports https only.

fsouza · 2023-06-24T01:19:51Z

main.go

+	if cfg.Scheme != "both" {
+		startServer(logger, &cfg, cfg.Scheme)
+	} else {
+		startServer(logger, &cfg, "http")
+		startServer(logger, &cfg, "https")
+	}


Can we setup two listeners that point to the same server instance? I feel like that would make the most sense.

@fsouza , I can do that, although I am not sure there is a big penalty when having two servers.
Started implementing but ran into lister code inside NewServerWithOptions which complicates a bit the transition to 2 listeners with a single server:
https://github.com/ramir-savvy/fake-gcs-server/blob/ea0ba662c5381c1317c13151f231e361cb7e51e9/fakestorage/server.go#L163

options.NoListener is set to true by default and only set to false in the tests.
Can you clarify its use?
If the NewServerWithOptions function just returned in that line instead of starting listeners for tests it would be easier.
Or maybe keep it as it is and ignore that in the server run because it is relevant just for tests?

although I am not sure there is a big penalty when having two servers

I was under the assumption that you wanted both servers to serve the same underlying buckets and objects? Without that, the memory backend will not have the same underlying objects, and I'd prefer that we don't diverge the capabilities of the backends here.

We can also decouple the backend from the server (i.e. support taking a backend instance in the server constructor), and in that case having two servers won't matter (similar to how the grpc server takes the backend instance).

@fsouza , you are right of course, I didn't take that into account.

I have pushed a commit that changes the code to have 2 listeners and a single server.
I looked at decoupling the backend from the server but it didn't seem to be so trivial.
Let me know what you think. Thanks

fsouza · 2023-06-24T01:20:02Z

@ramir-savvy gotcha, thanks for clarifying!

fsouza

Can you take a look at the test failure? Thanks!

raz-amir · 2023-07-30T07:41:31Z

Apologies @fsouza !
First: I was focused on "both" and neglected testing without the scheme param - fixed now.
Second: I didn't notice that you ran the tests, so I wasn't aware of the failure till you pinged me - Can you ping me when you start rerunning the tests?
Thanks

fsouza

Thank you! It looks like some examples are failing due to the comment I left about existing usages of -scheme http -port <something>.

Let me know what you think!

main.go

fsouza · 2023-07-30T13:04:16Z

internal/config/config.go

+	port := c.Port
+	if scheme == "http" {
+		port = c.PortHTTP
+	}


Won't this be a breaking change? Like, if I'm currently using fake-gcs-server like this:

fake-gcs-server -scheme http -port 8080

The server will now start on port 8000 instead of 8080.

I know this is fairly complicated, but could we make it so:

port and port-http no longer have a default at the flag definition. We'll handle it ourselves

if the user specifies -scheme http and -port, we use that port (keep current behavior)

if the user specifies just -scheme http, we use the default -port (keep current behavior)

if the user specifies just -scheme both, we'll use the default -port and -port-http

if the user doesn't provide any flags, we'll use the default -port and -scheme https

Perhaps we can turn each of the 5 examples in test cases for the function/core logic that handles this.

We may be able to implement this custom logic with a custom implementation of flag.Value, but it sounds tricky as it'd need to be aware of the value of another flag. The biggest advantage of using a custom flag.Value is differentiating between the user not providing the flag and setting it to 0 (I'm not super concerned about that though - right now if the user sets it to 0, we start the server in a random port, but never print that port to the user, so who cares? heh)

You are correct. I overlooked the http port in my last fix and broke the behavior, that I previously maintained. Thanks for catching that!

I implemented the port and port-http without defaults in the flag API but set it myself in the code if their value is 0. As you suggest, I also think that no one uses it today with port 0, so I don't think it is a breaking change, so I didn't implement the custom flag.Value.
I have an idea of how to implement it, maybe using flag.Visit, and I will do it if you think it is needed.
Please let me know.
Implemented after all using flag.Visit - so the port 0 can be set by a user as before.

Will work on adding the test cases soon.

@fsouza
Added unit tests to cover the examples you listed and few more to be on the safe side. The tests found an issue with both scheme in the cfg.externalURL config - fixed.
Added 2 more runs to the github action to cover both scheme with default and non-default ports.

Let me know what you think. Thanks

Co-authored-by: fsouza <108725+fsouza@users.noreply.github.com>

fsouza

Thank you!

raz-amir · 2023-08-01T07:10:37Z

@fsouza, I just learned that after all there is a breaking change with this PR.
Before this PR, when the server was started with -scheme http without specifying -port, the default port was 4443 (even though it was http). This PR changed it to 8000.
Sorry about that.
I will submit a PR soon to revert this behavior.

In 1.47 the default port changed to 8000 if http is used. This also sets it explicitly should it ever change again. See fsouza/fake-gcs-server#1215 .

13738: fix(ci): configure fake gcs server port r=oleschoenburg a=megglos ## Description In 1.47 the default port changed to 8000 if http is used. This also sets it explicitly should it ever change again. See fsouza/fake-gcs-server#1215 . Co-authored-by: Meggle (Sebastian Bathke) <sebastian.bathke@camunda.com>

In 1.47 the default port changed to 8000 if http is used. This also sets it explicitly should it ever change again. See fsouza/fake-gcs-server#1215 . (cherry picked from commit eebcda3)

…1272)

raz-amir added 2 commits June 15, 2023 13:02

Add scheme option to bind to both HTTP and HTTPS

0e467a4

changes

ea0ba66

raz-amir marked this pull request as ready for review June 15, 2023 10:15

raz-amir mentioned this pull request Jun 15, 2023

Support XML API (#331) #1164

Open

fsouza reviewed Jun 24, 2023

View reviewed changes

Change to 2 listeners and a single server

b053e10

raz-amir requested a review from fsouza June 28, 2023 14:09

fsouza reviewed Jul 29, 2023

View reviewed changes

fix

d708fee

raz-amir force-pushed the ramir_both_schemes branch from b8b3dbd to d708fee Compare July 30, 2023 07:35

fsouza reviewed Jul 30, 2023

View reviewed changes

raz-amir and others added 3 commits July 31, 2023 09:26

Apply suggestions from code review

46a6906

Co-authored-by: fsouza <108725+fsouza@users.noreply.github.com>

fix for default ports handling

9cffba9

check if flags default value using visit

8614c04

raz-amir force-pushed the ramir_both_schemes branch from b6836e3 to 8614c04 Compare July 31, 2023 09:53

add example test runs and unit tests

da920f7

fsouza approved these changes Aug 1, 2023

View reviewed changes

fsouza merged commit a33604b into fsouza:main Aug 1, 2023

megglos added a commit to camunda/camunda that referenced this pull request Aug 1, 2023

fix(ci): configure fake gcs server port

8940b1d

In 1.47 the default port changed to 8000 if http is used. This also sets it explicitly should it ever change again. See fsouza/fake-gcs-server#1215 .

megglos added a commit to camunda/camunda that referenced this pull request Aug 1, 2023

fix(ci): configure fake gcs server port

e9030e7

In 1.47 the default port changed to 8000 if http is used. This also sets it explicitly should it ever change again. See fsouza/fake-gcs-server#1215 .

megglos mentioned this pull request Aug 1, 2023

fix(ci): configure fake gcs server port camunda/camunda#13738

Merged

raz-amir added a commit to raz-amir/fake-gcs-server that referenced this pull request Aug 1, 2023

Restore 4443 as default port for '-scheme http' as before PR fsouza#1215

e9b49b6

This was referenced Aug 1, 2023

Restore 4443 as default port for '-scheme http' as before PR #1215 #1272

Merged

Issue upgrading 1.45.2 -> 1.46.0 #1271

Closed

megglos added a commit to camunda/camunda that referenced this pull request Aug 1, 2023

fix(ci): configure fake gcs server port

eebcda3

In 1.47 the default port changed to 8000 if http is used. This also sets it explicitly should it ever change again. See fsouza/fake-gcs-server#1215 .

fsouza pushed a commit that referenced this pull request Aug 1, 2023

Restore 4443 as default port for '-scheme http' as before PR #1215 (#…

d717a60

…1272)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scheme option to bind to both HTTP and HTTPS #1215

Add scheme option to bind to both HTTP and HTTPS #1215

raz-amir commented Jun 15, 2023

fsouza commented Jun 23, 2023

raz-amir commented Jun 23, 2023

fsouza Jun 24, 2023

raz-amir Jun 24, 2023

fsouza Jun 24, 2023

raz-amir Jun 26, 2023

fsouza commented Jun 24, 2023

fsouza left a comment

raz-amir commented Jul 30, 2023

fsouza left a comment

fsouza Jul 30, 2023

raz-amir Jul 31, 2023 •

edited

Loading

raz-amir Jul 31, 2023

fsouza left a comment

raz-amir commented Aug 1, 2023 •

edited

Loading

Add scheme option to bind to both HTTP and HTTPS #1215

Add scheme option to bind to both HTTP and HTTPS #1215

Conversation

raz-amir commented Jun 15, 2023

fsouza commented Jun 23, 2023

raz-amir commented Jun 23, 2023

fsouza Jun 24, 2023

Choose a reason for hiding this comment

raz-amir Jun 24, 2023

Choose a reason for hiding this comment

fsouza Jun 24, 2023

Choose a reason for hiding this comment

raz-amir Jun 26, 2023

Choose a reason for hiding this comment

fsouza commented Jun 24, 2023

fsouza left a comment

Choose a reason for hiding this comment

raz-amir commented Jul 30, 2023

fsouza left a comment

Choose a reason for hiding this comment

fsouza Jul 30, 2023

Choose a reason for hiding this comment

raz-amir Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

raz-amir Jul 31, 2023

Choose a reason for hiding this comment

fsouza left a comment

Choose a reason for hiding this comment

raz-amir commented Aug 1, 2023 • edited Loading

raz-amir Jul 31, 2023 •

edited

Loading

raz-amir commented Aug 1, 2023 •

edited

Loading