Add in --duplex flag to run HTTP and GRPC servers on the same port #931

priyawadhwa · 2022-12-19T20:21:06Z

also adds in a unit test for this. we can optionally start trying this out in staging, and if it works well we can consider deprecating the previous two-port solution.

Signed-off-by: Priya Wadhwa priya@chainguard.dev

Release Note

Add in --duplex flag to run HTTP and GRPC servers on the same port

cc @vaikas

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

haydentherapper · 2022-12-19T20:24:46Z

Thanks! Let's hold this until the new year, and we can cut a new release at the same time.

haydentherapper · 2022-12-19T22:21:36Z

@priyawadhwa Is there any change in behavior between serving gRPC on its own port and duplex? Any differences for metrics?

priyawadhwa · 2022-12-19T22:46:12Z

There shouldn't be!

vaikas

Awesomesauce!

cmd/app/serve.go

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

haydentherapper · 2022-12-20T19:34:26Z

Thanks y’all, I’ll test out these changes after the holidays!

haydentherapper · 2023-01-03T21:52:12Z

cmd/app/serve.go

@@ -81,10 +96,11 @@ func newServeCmd() *cobra.Command {
 	cmd.Flags().String("tink-keyset-path", "", "Path to KMS-encrypted keyset for Tink-backed CA")
 	cmd.Flags().String("host", "0.0.0.0", "The host on which to serve requests for HTTP; --http-host is alias")
 	cmd.Flags().String("port", "8080", "The port on which to serve requests for HTTP; --http-port is alias")
-	cmd.Flags().String("grpc-host", "0.0.0.0", "The host on which to serve requests for GRPC")
-	cmd.Flags().String("grpc-port", "8081", "The port on which to serve requests for GRPC")
+	cmd.Flags().String("grpc-host", "0.0.0.0", "[DEPRECATED] The host on which to serve requests for GRPC")


I would prefer we don't deprecate these flags, because there are no plans to cut a new major release of Fulcio anytime soon, and ideally we don't mark something as deprecated right after we cut 1.0.

If we eventually plan to remove them (which I think makes the most sense) then it's best to deprecate them sooner. IIUC we'll still maintain support for at least a few releases or a few months (whatever the policy comes up with suggests).

I don't think it's a great look to deprecate functionality right after we've introduced it as stable. I know it's a tiny part of the service, but I think we shouldn't begin deprecation of features right after a 1.0 release until we start thinking about a 2.0 release, and right now, I don't see any features in the pipeline to need a 2.0 release.

haydentherapper · 2023-01-03T21:52:49Z

cmd/app/serve.go

-	cmd.Flags().String("grpc-host", "0.0.0.0", "The host on which to serve requests for GRPC")
-	cmd.Flags().String("grpc-port", "8081", "The port on which to serve requests for GRPC")
+	cmd.Flags().String("grpc-host", "0.0.0.0", "[DEPRECATED] The host on which to serve requests for GRPC")
+	cmd.Flags().String("grpc-port", "8081", "[DEPRECATED] The port on which to serve requests for GRPC")


One other issue is by deprecating these, we remove the ability to expose the service only on gRPC, which is removing functionality.

What's the use case for GRPC only? My guess is there's a pretty small subset of people running their own Fulcio to begin with, and I'm not sure how likely it is they'd specifically want to run only GRPC. It's just a hunch, but I'm guessing removing this functionality probably wouldn't have much of an effect on users 🤔

It might affect those who run their own infrastructure.

haydentherapper · 2023-01-03T21:54:50Z

cmd/app/serve.go

 	cmd.Flags().String("metrics-port", "2112", "The port on which to serve prometheus metrics endpoint")
 	cmd.Flags().Duration("read-header-timeout", 10*time.Second, "The time allowed to read the headers of the requests in seconds")
+	cmd.Flags().Bool("duplex", false, "experimental: serve HTTP and GRPC on the same port instead of on two separate ports")


What do you think about not adding any flags, but changing the behavior such that when the HTTP and gRPC ports match, we use duplex?

This has a few benefits:

No deprecations needed

No breaking changes, since it would not have been possible to set the port values to be the same previously

No additional flags, which I always see as a win

Hmm it would work, but as a user I don't find it very intuitive. As a user I'd expect that setting --port and --grpc-port to the same value would cause an error. I'd still prefer to deprecate the grpc flags and have one --port flag to expose everything on, mostly because I think longterm it'll be cleaner, easier for users to understand, and because I don't see a use case for GRPC-only functionality.

I see the addition of this change as fixing the error of setting the ports to be the same, rather than adding new functionality. I also would prefer to not use duplex as a flag name since that's exposing implementation details.

cmd/app/serve.go

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

priyawadhwa · 2023-01-10T22:18:55Z

Discussed with @haydentherapper and made the following changes:

Removed the --duplex flag in favor of using duplex if --grpc-port and --port are set to the same value.
Removed the deprecation warnings for now, we can add those back in later once we feel duplex is running as expected.

@haydentherapper this is RFAL.

cmd/app/serve.go

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

priyawadhwa · 2023-01-19T19:19:52Z

This should be RFAL!

codecov-commenter · 2023-01-19T19:21:28Z

Codecov Report

Merging #931 (d0f8235) into main (d8d8124) will increase coverage by 0.11%.
The diff coverage is 57.62%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
+ Coverage   53.97%   54.08%   +0.11%     
==========================================
  Files          38       38              
  Lines        2366     2424      +58     
==========================================
+ Hits         1277     1311      +34     
- Misses        996     1016      +20     
- Partials       93       97       +4

Impacted Files	Coverage Δ
cmd/app/serve.go	`26.92% <57.62%> (+8.06%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

haydentherapper · 2023-01-24T17:16:07Z

cc @bobcallaway for another look

haydentherapper

LGTM, just needs to be rebased for go.mod

cmd/app/serve.go

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

priyawadhwa · 2023-01-26T17:41:42Z

Rebased! could i please get another lgtm?

Add in --duplex flag to run HTTP and GRPC servers on the same port

e85f31c

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

priyawadhwa requested a review from haydentherapper December 19, 2022 20:21

haydentherapper requested a review from bobcallaway December 19, 2022 20:24

vaikas reviewed Dec 20, 2022

View reviewed changes

cmd/app/serve.go Outdated Show resolved Hide resolved

Mark grpc flags as deprecated and add warning

659028a

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

vaikas previously approved these changes Dec 20, 2022

View reviewed changes

haydentherapper reviewed Jan 3, 2023

View reviewed changes

haydentherapper reviewed Jan 5, 2023

View reviewed changes

cmd/app/serve.go Outdated Show resolved Hide resolved

priyawadhwa added 2 commits January 10, 2023 14:15

Fix merge conflict

a35f37b

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

Remove duplex flag and deprecation warnings

96315d3

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

priyawadhwa dismissed vaikas’s stale review via 96315d3 January 10, 2023 22:18

bobcallaway reviewed Jan 12, 2023

View reviewed changes

Code review comments

bfdf175

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

priyawadhwa force-pushed the duplex branch from 28e7618 to bfdf175 Compare January 19, 2023 19:13

Fixed merge conflict

5390b14

priyawadhwa force-pushed the duplex branch from 390240f to 5390b14 Compare January 19, 2023 19:17

bobcallaway previously approved these changes Jan 25, 2023

View reviewed changes

haydentherapper reviewed Jan 25, 2023

View reviewed changes

cmd/app/serve.go Outdated Show resolved Hide resolved

haydentherapper mentioned this pull request Jan 26, 2023

update builder release to use go 1.19.5 #962

Closed

fix merge conflict

d0f8235

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

priyawadhwa dismissed bobcallaway’s stale review via d0f8235 January 26, 2023 17:41

bobcallaway approved these changes Jan 26, 2023

View reviewed changes

priyawadhwa merged commit d47a2c0 into sigstore:main Jan 26, 2023

priyawadhwa deleted the duplex branch January 26, 2023 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in --duplex flag to run HTTP and GRPC servers on the same port #931

Add in --duplex flag to run HTTP and GRPC servers on the same port #931

priyawadhwa commented Dec 19, 2022

haydentherapper commented Dec 19, 2022

haydentherapper commented Dec 19, 2022

priyawadhwa commented Dec 19, 2022

vaikas left a comment

haydentherapper commented Dec 20, 2022

haydentherapper Jan 3, 2023

priyawadhwa Jan 4, 2023

haydentherapper Jan 4, 2023

haydentherapper Jan 3, 2023

priyawadhwa Jan 4, 2023

haydentherapper Jan 4, 2023

haydentherapper Jan 3, 2023

priyawadhwa Jan 4, 2023

haydentherapper Jan 4, 2023

priyawadhwa commented Jan 10, 2023

priyawadhwa commented Jan 19, 2023

codecov-commenter commented Jan 19, 2023 •

edited

Loading

haydentherapper commented Jan 24, 2023

haydentherapper left a comment

priyawadhwa commented Jan 26, 2023

Add in --duplex flag to run HTTP and GRPC servers on the same port #931

Add in --duplex flag to run HTTP and GRPC servers on the same port #931

Conversation

priyawadhwa commented Dec 19, 2022

Release Note

haydentherapper commented Dec 19, 2022

haydentherapper commented Dec 19, 2022

priyawadhwa commented Dec 19, 2022

vaikas left a comment

Choose a reason for hiding this comment

haydentherapper commented Dec 20, 2022

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

Choose a reason for hiding this comment

priyawadhwa commented Jan 10, 2023

priyawadhwa commented Jan 19, 2023

codecov-commenter commented Jan 19, 2023 • edited Loading

Codecov Report

haydentherapper commented Jan 24, 2023

haydentherapper left a comment

Choose a reason for hiding this comment

priyawadhwa commented Jan 26, 2023

codecov-commenter commented Jan 19, 2023 •

edited

Loading