-
Notifications
You must be signed in to change notification settings - Fork 138
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
Add in --duplex flag to run HTTP and GRPC servers on the same port #931
Changes from 2 commits
e85f31c
659028a
a35f37b
96315d3
bfdf175
5390b14
d0f8235
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,13 +24,22 @@ import ( | |
"net/http" | ||
"os" | ||
"path/filepath" | ||
"strconv" | ||
"strings" | ||
"time" | ||
|
||
"chainguard.dev/go-grpc-kit/pkg/duplex" | ||
"github.com/goadesign/goa/grpc/middleware" | ||
ctclient "github.com/google/certificate-transparency-go/client" | ||
"github.com/google/certificate-transparency-go/jsonclient" | ||
grpcmw "github.com/grpc-ecosystem/go-grpc-middleware" | ||
grpc_zap "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap" | ||
grpc_recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery" | ||
grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus" | ||
"github.com/grpc-ecosystem/grpc-gateway/v2/runtime" | ||
"github.com/prometheus/client_golang/prometheus" | ||
"github.com/prometheus/client_golang/prometheus/promhttp" | ||
"github.com/sigstore/fulcio/pkg/ca" | ||
certauth "github.com/sigstore/fulcio/pkg/ca" | ||
"github.com/sigstore/fulcio/pkg/ca/ephemeralca" | ||
"github.com/sigstore/fulcio/pkg/ca/fileca" | ||
|
@@ -39,12 +48,18 @@ import ( | |
"github.com/sigstore/fulcio/pkg/ca/pkcs11ca" | ||
"github.com/sigstore/fulcio/pkg/ca/tinkca" | ||
"github.com/sigstore/fulcio/pkg/config" | ||
"github.com/sigstore/fulcio/pkg/generated/protobuf" | ||
"github.com/sigstore/fulcio/pkg/generated/protobuf/legacy" | ||
"github.com/sigstore/fulcio/pkg/log" | ||
"github.com/sigstore/fulcio/pkg/server" | ||
"github.com/sigstore/sigstore/pkg/cryptoutils" | ||
"github.com/spf13/cobra" | ||
"github.com/spf13/pflag" | ||
"github.com/spf13/viper" | ||
"go.uber.org/zap" | ||
"google.golang.org/grpc" | ||
"google.golang.org/grpc/credentials/insecure" | ||
"google.golang.org/protobuf/proto" | ||
) | ||
|
||
const serveCmdEnvPrefix = "FULCIO_SERVE" | ||
|
@@ -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") | ||
cmd.Flags().String("grpc-port", "8081", "[DEPRECATED] The port on which to serve requests for GRPC") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One other issue is by deprecating these, we remove the ability to expose the service only on gRPC, which is removing functionality. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might affect those who run their own infrastructure. |
||
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") | ||
priyawadhwa marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm it would work, but as a user I don't find it very intuitive. As a user I'd expect that setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
// convert "http-host" flag to "host" and "http-port" flag to be "port" | ||
cmd.Flags().SetNormalizeFunc(func(f *pflag.FlagSet, name string) pflag.NormalizedName { | ||
|
@@ -116,6 +132,7 @@ func (la logAdaptor) Printf(s string, args ...interface{}) { | |
} | ||
|
||
func runServeCmd(cmd *cobra.Command, args []string) { | ||
ctx := cmd.Context() | ||
// If a config file is provided, modify the viper config to locate and read it | ||
if err := checkServeCmdConfigFile(); err != nil { | ||
log.Logger.Fatal(err) | ||
|
@@ -254,6 +271,25 @@ func runServeCmd(cmd *cobra.Command, args []string) { | |
} | ||
} | ||
|
||
if viper.GetBool("duplex") { | ||
p := viper.GetString("port") | ||
priyawadhwa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
port, err := strconv.Atoi(p) | ||
if err != nil { | ||
log.Logger.Fatal("%s is not a valid port", p) | ||
} | ||
mp := viper.GetString("metrics-port") | ||
priyawadhwa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
metricsPort, err := strconv.Atoi(mp) | ||
if err != nil { | ||
log.Logger.Fatal("%s is not a valid port", mp) | ||
} | ||
if err := StartDuplexServer(ctx, cfg, ctClient, baseca, port, metricsPort); err != nil { | ||
priyawadhwa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
log.Logger.Fatal(err) | ||
} | ||
priyawadhwa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
log.Logger.Warnln("Hosting HTTP and GRPC servers on different ports will soon be deprecated, please set the --duplex flag to serve both on a single port." + | ||
"The single port can be set with the --port flag.") | ||
} | ||
|
||
httpServerEndpoint := fmt.Sprintf("%v:%v", viper.GetString("http-host"), viper.GetString("http-port")) | ||
|
||
reg := prometheus.NewRegistry() | ||
|
@@ -271,7 +307,7 @@ func runServeCmd(cmd *cobra.Command, args []string) { | |
} | ||
legacyGRPCServer.startUnixListener() | ||
|
||
httpServer := createHTTPServer(context.Background(), httpServerEndpoint, grpcServer, legacyGRPCServer) | ||
httpServer := createHTTPServer(ctx, httpServerEndpoint, grpcServer, legacyGRPCServer) | ||
httpServer.startListener() | ||
|
||
readHeaderTimeout := viper.GetDuration("read-header-timeout") | ||
|
@@ -313,3 +349,78 @@ func checkServeCmdConfigFile() error { | |
} | ||
return nil | ||
} | ||
|
||
func StartDuplexServer(ctx context.Context, cfg *config.FulcioConfig, ctClient *ctclient.LogClient, baseca ca.CertificateAuthority, port, metricsPort int) error { | ||
haydentherapper marked this conversation as resolved.
Show resolved
Hide resolved
|
||
logger, opts := log.SetupGRPCLogging() | ||
|
||
d := duplex.New( | ||
port, | ||
grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
runtime.WithMetadata(extractOIDCTokenFromAuthHeader), | ||
grpc.UnaryInterceptor(grpcmw.ChainUnaryServer( | ||
grpc_recovery.UnaryServerInterceptor(grpc_recovery.WithRecoveryHandlerContext(panicRecoveryHandler)), // recovers from per-transaction panics elegantly, so put it first | ||
middleware.UnaryRequestID(middleware.UseXRequestIDMetadataOption(true), middleware.XRequestMetadataLimitOption(128)), | ||
grpc_zap.UnaryServerInterceptor(logger, opts...), | ||
PassFulcioConfigThruContext(cfg), | ||
grpc_prometheus.UnaryServerInterceptor, | ||
)), | ||
grpc.MaxRecvMsgSize(int(maxMsgSize)), | ||
runtime.WithForwardResponseOption(HTTPResponseModifier), | ||
) | ||
|
||
// GRPC server | ||
grpcCAServer := server.NewGRPCCAServer(ctClient, baseca) | ||
protobuf.RegisterCAServer(d.Server, grpcCAServer) | ||
if err := d.RegisterHandler(ctx, protobuf.RegisterCAHandlerFromEndpoint); err != nil { | ||
return fmt.Errorf("registering grpc ca handler: %w", err) | ||
} | ||
|
||
// Legacy server | ||
legacyGRPCCAServer := server.NewLegacyGRPCCAServer(grpcCAServer) | ||
legacy.RegisterCAServer(d.Server, legacyGRPCCAServer) | ||
if err := d.RegisterHandler(ctx, legacy.RegisterCAHandlerFromEndpoint); err != nil { | ||
return fmt.Errorf("registering legacy grpc ca handler: %w", err) | ||
} | ||
|
||
// Prometheus | ||
reg := prometheus.NewRegistry() | ||
grpcMetrics := grpc_prometheus.DefaultServerMetrics | ||
grpcMetrics.EnableHandlingTimeHistogram() | ||
reg.MustRegister(grpcMetrics, server.MetricLatency, server.RequestsCount) | ||
grpc_prometheus.Register(d.Server) | ||
|
||
// Register prometheus handle. | ||
d.RegisterListenAndServeMetrics(metricsPort, false) | ||
|
||
if err := d.ListenAndServe(ctx); err != nil { | ||
return fmt.Errorf("duplex server: %w", err) | ||
} | ||
priyawadhwa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return nil | ||
} | ||
|
||
// The fulcio HTTP legacy client requires a 201 response status code to pass | ||
// However, even though the legacy client sets the 201 code, GRPC will automatically | ||
// change it to 200, causing the cert request to fail | ||
// GRPC recommends controlling HTTP status codes with this response modifier | ||
// https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_your_gateway/#controlling-http-response-status-codes | ||
// This is required to use fulcio with duplex. | ||
func HTTPResponseModifier(ctx context.Context, w http.ResponseWriter, p proto.Message) error { | ||
priyawadhwa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
md, ok := runtime.ServerMetadataFromContext(ctx) | ||
if !ok { | ||
return nil | ||
} | ||
|
||
// set http status code | ||
if vals := md.HeaderMD.Get(server.HTTPResponseCodeMetadataKey); len(vals) > 0 { | ||
code, err := strconv.Atoi(vals[0]) | ||
if err != nil { | ||
return err | ||
} | ||
// delete the headers to not expose any grpc-metadata in http response | ||
delete(md.HeaderMD, server.HTTPResponseCodeMetadataKey) | ||
delete(w.Header(), "Grpc-Metadata-X-Http-Code") | ||
w.WriteHeader(code) | ||
} | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
// Copyright 2021 The Sigstore Authors. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// | ||
|
||
package app | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"io" | ||
"log" | ||
"net/http" | ||
"net/url" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
"github.com/sigstore/fulcio/pkg/api" | ||
"github.com/sigstore/fulcio/pkg/ca/ephemeralca" | ||
"github.com/sigstore/fulcio/pkg/config" | ||
"github.com/sigstore/fulcio/pkg/generated/protobuf" | ||
"google.golang.org/grpc" | ||
"google.golang.org/grpc/credentials/insecure" | ||
) | ||
|
||
func TestDuplex(t *testing.T) { | ||
// Start a server with duplex on port 8089 | ||
ctx := context.Background() | ||
ca, err := ephemeralca.NewEphemeralCA() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
port := 8089 | ||
serverURL, err := url.Parse(fmt.Sprintf("http://localhost:%d", port)) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
metricsPort := 2114 | ||
|
||
go func() { | ||
if err := StartDuplexServer(ctx, config.DefaultConfig, nil, ca, port, metricsPort); err != nil { | ||
log.Fatalf("error starting duplex server: %v", err) | ||
} | ||
}() | ||
|
||
var rootCert string | ||
t.Run("http", func(t *testing.T) { | ||
// Make sure we can grab the rootcert with the v1 endpoint | ||
legacyClient := api.NewClient(serverURL) | ||
resp, err := legacyClient.RootCert() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
rootCert = string(resp.ChainPEM) | ||
}) | ||
|
||
var grpcRootCert string | ||
t.Run("grpc", func(t *testing.T) { | ||
// Grab the rootcert with the v2 endpoint | ||
conn, err := grpc.Dial(fmt.Sprintf("127.0.0.1:%d", port), grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
grpcClient := protobuf.NewCAClient(conn) | ||
tb, err := grpcClient.GetTrustBundle(ctx, &protobuf.GetTrustBundleRequest{}) | ||
if err != nil { | ||
t.Fatalf("error getting trust bundle: %v", err) | ||
} | ||
if len(tb.Chains) != 1 { | ||
t.Fatalf("didn't get expected length certificate chain: %v", tb.Chains) | ||
} | ||
if len(tb.Chains[0].Certificates) != 1 { | ||
t.Fatalf("didn't get expected length certs: %v", tb.Chains) | ||
} | ||
grpcRootCert = strings.TrimSuffix(tb.Chains[0].Certificates[0], "\n") | ||
}) | ||
|
||
t.Run("compare root certs", func(t *testing.T) { | ||
if d := cmp.Diff(rootCert, grpcRootCert); d != "" { | ||
t.Fatal(d) | ||
} | ||
}) | ||
|
||
t.Run("prometheus", func(t *testing.T) { | ||
// make sure there are metrics on the metrics port | ||
url := fmt.Sprintf("http://localhost:%d/metrics", metricsPort) | ||
resp, err := http.Get(url) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
contents, err := io.ReadAll(resp.Body) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
// make sure there's something about hitting the GetTrustBundle in there | ||
// this just confirms some metrics are being printed | ||
if !strings.Contains(string(contents), "GetTrustBundle") { | ||
t.Fatalf("didn't get expected metrics output: %s", string(contents)) | ||
} | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.