From c7b41f1635e8fad25b1382b5aec0744b15430d18 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Mon, 20 Mar 2023 11:43:19 +0100 Subject: [PATCH 01/11] chore: refactor middleware setup Signed-off-by: Florian Bacher --- core/pkg/middleware/cors/cors.go | 45 ++++ core/pkg/middleware/h2c/h2c.go | 17 ++ core/pkg/middleware/interface.go | 19 ++ core/pkg/middleware/metrics/recorder.go | 279 ++++++++++++++++++++++++ 4 files changed, 360 insertions(+) create mode 100644 core/pkg/middleware/cors/cors.go create mode 100644 core/pkg/middleware/h2c/h2c.go create mode 100644 core/pkg/middleware/interface.go create mode 100644 core/pkg/middleware/metrics/recorder.go diff --git a/core/pkg/middleware/cors/cors.go b/core/pkg/middleware/cors/cors.go new file mode 100644 index 000000000..195f1ea30 --- /dev/null +++ b/core/pkg/middleware/cors/cors.go @@ -0,0 +1,45 @@ +package cors + +import ( + "github.com/rs/cors" + "net/http" +) + +type Middleware struct { + cors *cors.Cors +} + +func New(allowedOrigins []string) *Middleware { + return &Middleware{ + cors: cors.New(cors.Options{ + AllowedMethods: []string{ + http.MethodHead, + http.MethodGet, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + }, + AllowedOrigins: allowedOrigins, + AllowedHeaders: []string{"*"}, + ExposedHeaders: []string{ + // Content-Type is in the default safelist. + "Accept", + "Accept-Encoding", + "Accept-Post", + "Connect-Accept-Encoding", + "Connect-Content-Encoding", + "Content-Encoding", + "Grpc-Accept-Encoding", + "Grpc-Encoding", + "Grpc-Message", + "Grpc-Status", + "Grpc-Status-Details-Bin", + }, + }), + } +} + +func (c Middleware) Handle(handler http.Handler) http.Handler { + return c.cors.Handler(handler) +} diff --git a/core/pkg/middleware/h2c/h2c.go b/core/pkg/middleware/h2c/h2c.go new file mode 100644 index 000000000..ce463147e --- /dev/null +++ b/core/pkg/middleware/h2c/h2c.go @@ -0,0 +1,17 @@ +package h2c + +import ( + "golang.org/x/net/http2" + "golang.org/x/net/http2/h2c" + "net/http" +) + +type Middleware struct{} + +func New() *Middleware { + return &Middleware{} +} + +func (m Middleware) Handle(handler http.Handler) http.Handler { + return h2c.NewHandler(handler, &http2.Server{}) +} diff --git a/core/pkg/middleware/interface.go b/core/pkg/middleware/interface.go new file mode 100644 index 000000000..6f66ac05b --- /dev/null +++ b/core/pkg/middleware/interface.go @@ -0,0 +1,19 @@ +package middleware + +import ( + "fmt" + "net/http" +) + +type Middleware interface { + Handle(handler http.Handler) http.Handler +} + +type Logger struct{} + +func (Logger) Handle(handler http.Handler) http.Handler { + return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + fmt.Println("log") + handler.ServeHTTP(writer, request) + }) +} diff --git a/core/pkg/middleware/metrics/recorder.go b/core/pkg/middleware/metrics/recorder.go new file mode 100644 index 000000000..7ad730607 --- /dev/null +++ b/core/pkg/middleware/metrics/recorder.go @@ -0,0 +1,279 @@ +package service + +import ( + "bufio" + "context" + "errors" + "fmt" + "log" + "net" + "net/http" + "strconv" + "time" + + "github.com/open-feature/flagd/core/pkg/logger" + "github.com/prometheus/client_golang/prometheus" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/metric/unit" + "go.opentelemetry.io/otel/sdk/instrumentation" + "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/aggregation" + + semconv "go.opentelemetry.io/otel/semconv/v1.13.0" +) + +var ( + _ http.ResponseWriter = &responseWriterInterceptor{} + _ http.Hijacker = &responseWriterInterceptor{} + _ http.Flusher = &responseWriterInterceptor{} +) + +type HTTPReqProperties struct { + Service string + ID string + Method string + Code string +} + +type Recorder interface { + // OTelObserveHTTPRequestDuration measures the duration of an HTTP request. + OTelObserveHTTPRequestDuration(props HTTPReqProperties, duration time.Duration) + // OTelObserveHTTPResponseSize measures the size of an HTTP response in bytes. + OTelObserveHTTPResponseSize(props HTTPReqProperties, sizeBytes int64) + + // OTelInFlightRequestStart count the active requests. + OTelInFlightRequestStart(props HTTPReqProperties) + // OTelInFlightRequestEnd count the finished requests. + OTelInFlightRequestEnd(props HTTPReqProperties) +} + +type Reporter interface { + Method() string + URLPath() string + StatusCode() int + BytesWritten() int64 +} + +type HTTPProperties struct { + Service string + ID string +} + +type OTelMetricsRecorder struct { + httpRequestDurHistogram instrument.Float64Histogram + httpResponseSizeHistogram instrument.Float64Histogram + httpRequestsInflight instrument.Int64UpDownCounter +} + +func (r OTelMetricsRecorder) setAttributes(p HTTPReqProperties) []attribute.KeyValue { + return []attribute.KeyValue{ + semconv.ServiceNameKey.String(p.Service), + semconv.HTTPURLKey.String(p.ID), + semconv.HTTPMethodKey.String(p.Method), + semconv.HTTPStatusCodeKey.String(p.Code), + } +} + +func (r OTelMetricsRecorder) OTelObserveHTTPRequestDuration(p HTTPReqProperties, duration time.Duration) { + r.httpRequestDurHistogram.Record(context.TODO(), duration.Seconds(), r.setAttributes(p)...) +} + +func (r OTelMetricsRecorder) OTelObserveHTTPResponseSize(p HTTPReqProperties, sizeBytes int64) { + r.httpResponseSizeHistogram.Record(context.TODO(), float64(sizeBytes), r.setAttributes(p)...) +} + +func (r OTelMetricsRecorder) OTelInFlightRequestStart(p HTTPReqProperties) { + r.httpRequestsInflight.Add(context.TODO(), 1, r.setAttributes(p)...) +} + +func (r OTelMetricsRecorder) OTelInFlightRequestEnd(p HTTPReqProperties) { + r.httpRequestsInflight.Add(context.TODO(), -1, r.setAttributes(p)...) +} + +type MiddlewareConfig struct { + Recorder Recorder + MetricReader metric.Reader + Logger *logger.Logger + Service string + GroupedStatus bool + DisableMeasureSize bool +} + +type Middleware struct { + cfg MiddlewareConfig + HandlerID string +} + +func New(cfg MiddlewareConfig) Middleware { + cfg.defaults() + m := Middleware{cfg: cfg} + return m +} + +func (cfg *MiddlewareConfig) defaults() { + if cfg.Logger == nil { + log.Fatal("missing logger") + } + if cfg.MetricReader == nil { + log.Fatal("missing MetricReader/Exporter") + } + cfg.Recorder = cfg.newOTelRecorder(cfg.MetricReader) +} + +func (cfg *MiddlewareConfig) getDurationView(name string, bucket []float64) metric.View { + return metric.NewView( + metric.Instrument{ + // we change aggregation only for instruments with this name and scope + Name: name, + Scope: instrumentation.Scope{ + Name: cfg.Service, + }, + }, + metric.Stream{Aggregation: aggregation.ExplicitBucketHistogram{ + Boundaries: bucket, + }}, + ) +} + +func (cfg *MiddlewareConfig) newOTelRecorder(exporter metric.Reader) *OTelMetricsRecorder { + const requestDurationName = "http_request_duration_seconds" + const responseSizeName = "http_response_size_bytes" + + // create a metric provider with custom bucket size for histograms + provider := metric.NewMeterProvider( + metric.WithReader(exporter), + metric.WithView(cfg.getDurationView(requestDurationName, prometheus.DefBuckets)), + metric.WithView(cfg.getDurationView(responseSizeName, prometheus.ExponentialBuckets(100, 10, 8))), + ) + meter := provider.Meter(cfg.Service) + // we can ignore errors from OpenTelemetry since they could occur if we select the wrong aggregator + hduration, _ := meter.Float64Histogram( + requestDurationName, + instrument.WithDescription("The latency of the HTTP requests"), + ) + hsize, _ := meter.Float64Histogram( + responseSizeName, + instrument.WithDescription("The size of the HTTP responses"), + instrument.WithUnit(unit.Bytes), + ) + reqCounter, _ := meter.Int64UpDownCounter( + "http_requests_inflight", + instrument.WithDescription("The number of inflight requests being handled at the same time"), + ) + return &OTelMetricsRecorder{ + httpRequestDurHistogram: hduration, + httpResponseSizeHistogram: hsize, + httpRequestsInflight: reqCounter, + } +} + +func (m Middleware) Measure(handlerID string, reporter Reporter, next func()) { + // If there isn't predefined handler ID we + // set that ID as the URL path. + hid := handlerID + if handlerID == "" { + hid = reporter.URLPath() + } + + // If we need to group the status code, it uses the + // first number of the status code because is the least + // required identification way. + var code string + if m.cfg.GroupedStatus { + code = fmt.Sprintf("%dxx", reporter.StatusCode()/100) + } else { + code = strconv.Itoa(reporter.StatusCode()) + } + props := HTTPReqProperties{ + Service: m.cfg.Service, + ID: hid, + Method: reporter.Method(), + Code: code, + } + + m.cfg.Recorder.OTelInFlightRequestStart(props) + defer m.cfg.Recorder.OTelInFlightRequestEnd(props) + + // Start the timer and when finishing measure the duration. + start := time.Now() + defer func() { + duration := time.Since(start) + + m.cfg.Recorder.OTelObserveHTTPRequestDuration(props, duration) + + // Measure size of response if required. + if !m.cfg.DisableMeasureSize { + m.cfg.Recorder.OTelObserveHTTPResponseSize(props, reporter.BytesWritten()) + } + }() + + // Call the wrapped logic. + next() +} + +// Handle returns an measuring standard http.Handler. +func (m Middleware) Handle(h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + wi := &responseWriterInterceptor{ + statusCode: http.StatusOK, + ResponseWriter: w, + } + reporter := &stdReporter{ + w: wi, + r: r, + } + + m.Measure(m.HandlerID, reporter, func() { + h.ServeHTTP(wi, r) + }) + }) +} + +type stdReporter struct { + w *responseWriterInterceptor + r *http.Request +} + +func (s *stdReporter) Method() string { return s.r.Method } + +func (s *stdReporter) URLPath() string { return s.r.URL.Path } + +func (s *stdReporter) StatusCode() int { return s.w.statusCode } + +func (s *stdReporter) BytesWritten() int64 { return int64(s.w.bytesWritten) } + +// responseWriterInterceptor is a simple wrapper to intercept set data on a +// ResponseWriter. +type responseWriterInterceptor struct { + http.ResponseWriter + statusCode int + bytesWritten int +} + +func (w *responseWriterInterceptor) WriteHeader(statusCode int) { + w.statusCode = statusCode + w.ResponseWriter.WriteHeader(statusCode) +} + +func (w *responseWriterInterceptor) Write(p []byte) (int, error) { + w.bytesWritten += len(p) + return w.ResponseWriter.Write(p) +} + +func (w *responseWriterInterceptor) Hijack() (net.Conn, *bufio.ReadWriter, error) { + h, ok := w.ResponseWriter.(http.Hijacker) + if !ok { + return nil, nil, errors.New("type assertion failed http.ResponseWriter not a http.Hijacker") + } + return h.Hijack() +} + +func (w *responseWriterInterceptor) Flush() { + f, ok := w.ResponseWriter.(http.Flusher) + if !ok { + return + } + + f.Flush() +} From 5e996bee13c8452ada97820075436e8bfb81bc46 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Mon, 20 Mar 2023 14:03:30 +0100 Subject: [PATCH 02/11] added unit tests for injecting middleware Signed-off-by: Florian Bacher --- Makefile | 1 + core/pkg/middleware/mock/interface.go | 49 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 core/pkg/middleware/mock/interface.go diff --git a/Makefile b/Makefile index e53e8aef1..2a2f8b060 100644 --- a/Makefile +++ b/Makefile @@ -60,6 +60,7 @@ mockgen: install-mockgen cd core; mockgen -source=pkg/sync/grpc/grpc_sync.go -destination=pkg/sync/grpc/mock/grpc.go -package=grpcmock cd core; mockgen -source=pkg/sync/grpc/credentials/builder.go -destination=pkg/sync/grpc/credentials/mock/builder.go -package=credendialsmock cd core; mockgen -source=pkg/eval/ievaluator.go -destination=pkg/eval/mock/ievaluator.go -package=evalmock + cd core; mockgen -source=pkg/middleware/interface.go -destination=pkg/middleware/mock/interface.go -package=middlewaremock generate-docs: cd flagd; go run ./cmd/doc/main.go diff --git a/core/pkg/middleware/mock/interface.go b/core/pkg/middleware/mock/interface.go new file mode 100644 index 000000000..c27fc0055 --- /dev/null +++ b/core/pkg/middleware/mock/interface.go @@ -0,0 +1,49 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: pkg/middleware/interface.go + +// Package middlewaremock is a generated GoMock package. +package middlewaremock + +import ( + http "net/http" + reflect "reflect" + + gomock "github.com/golang/mock/gomock" +) + +// MockMiddleware is a mock of Middleware interface. +type MockMiddleware struct { + ctrl *gomock.Controller + recorder *MockMiddlewareMockRecorder +} + +// MockMiddlewareMockRecorder is the mock recorder for MockMiddleware. +type MockMiddlewareMockRecorder struct { + mock *MockMiddleware +} + +// NewMockMiddleware creates a new mock instance. +func NewMockMiddleware(ctrl *gomock.Controller) *MockMiddleware { + mock := &MockMiddleware{ctrl: ctrl} + mock.recorder = &MockMiddlewareMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockMiddleware) EXPECT() *MockMiddlewareMockRecorder { + return m.recorder +} + +// Handle mocks base method. +func (m *MockMiddleware) Handle(handler http.Handler) http.Handler { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Handle", handler) + ret0, _ := ret[0].(http.Handler) + return ret0 +} + +// Handle indicates an expected call of Handle. +func (mr *MockMiddlewareMockRecorder) Handle(handler interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Handle", reflect.TypeOf((*MockMiddleware)(nil).Handle), handler) +} From 4ac22ae2becbf61eebdc13e357698463c9094e57 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Mon, 20 Mar 2023 15:41:28 +0100 Subject: [PATCH 03/11] remove obsolete code Signed-off-by: Florian Bacher --- core/pkg/middleware/interface.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/core/pkg/middleware/interface.go b/core/pkg/middleware/interface.go index 6f66ac05b..d4acafec3 100644 --- a/core/pkg/middleware/interface.go +++ b/core/pkg/middleware/interface.go @@ -1,19 +1,9 @@ package middleware import ( - "fmt" "net/http" ) type Middleware interface { Handle(handler http.Handler) http.Handler } - -type Logger struct{} - -func (Logger) Handle(handler http.Handler) http.Handler { - return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { - fmt.Println("log") - handler.ServeHTTP(writer, request) - }) -} From 5ca7f7a69ff00d9fd674d9ff5eb64e28813a8cfc Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Tue, 21 Mar 2023 13:02:14 +0100 Subject: [PATCH 04/11] added simple unit test to verify middleware Signed-off-by: Florian Bacher --- core/pkg/middleware/cors/cors_test.go | 43 +++++++++++++++++++++++++++ core/pkg/middleware/h2c/h2c_test.go | 38 +++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 core/pkg/middleware/cors/cors_test.go create mode 100644 core/pkg/middleware/h2c/h2c_test.go diff --git a/core/pkg/middleware/cors/cors_test.go b/core/pkg/middleware/cors/cors_test.go new file mode 100644 index 000000000..8e9e72021 --- /dev/null +++ b/core/pkg/middleware/cors/cors_test.go @@ -0,0 +1,43 @@ +package cors + +import ( + "github.com/golang/mock/gomock" + middlewaremock "github.com/open-feature/flagd/core/pkg/middleware/mock" + "github.com/stretchr/testify/require" + "net/http" + "net/http/httptest" + "testing" +) + +func TestMiddleware(t *testing.T) { + ctrl := gomock.NewController(t) + mockMw := middlewaremock.NewMockMiddleware(ctrl) + + handlerFunc := http.HandlerFunc( + func(writer http.ResponseWriter, request *http.Request) { + writer.WriteHeader(http.StatusOK) + }, + ) + + mockMw.EXPECT().Handle(gomock.Any()).Return(handlerFunc) + + ts := httptest.NewServer(handlerFunc) + + defer ts.Close() + + mw := New([]string{"*"}) + require.NotNil(t, mw) + + // wrap the cors middleware around the mock to make sure the wrapped handler is called by the cors middleware + ts.Config.Handler = mw.Handle(mockMw.Handle(handlerFunc)) + + req, err := http.NewRequest(http.MethodGet, ts.URL, nil) + + require.Nil(t, err) + + client := http.DefaultClient + resp, err := client.Do(req) + + require.Nil(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) +} diff --git a/core/pkg/middleware/h2c/h2c_test.go b/core/pkg/middleware/h2c/h2c_test.go new file mode 100644 index 000000000..e190e3a08 --- /dev/null +++ b/core/pkg/middleware/h2c/h2c_test.go @@ -0,0 +1,38 @@ +package h2c + +import ( + "github.com/golang/mock/gomock" + middlewaremock "github.com/open-feature/flagd/core/pkg/middleware/mock" + "github.com/stretchr/testify/require" + "net/http" + "net/http/httptest" + "testing" +) + +func TestMiddleware(t *testing.T) { + ctrl := gomock.NewController(t) + mockMw := middlewaremock.NewMockMiddleware(ctrl) + + handlerFunc := http.HandlerFunc( + func(writer http.ResponseWriter, request *http.Request) { + writer.WriteHeader(http.StatusOK) + }, + ) + + mockMw.EXPECT().Handle(gomock.Any()).Return(handlerFunc) + + ts := httptest.NewServer(handlerFunc) + + defer ts.Close() + + mw := New() + require.NotNil(t, mw) + + // wrap the h2c middleware around the mock to make sure the wrapped handler is called by the h2c middleware + ts.Config.Handler = mw.Handle(mockMw.Handle(handlerFunc)) + + resp, err := http.Get(ts.URL) + + require.Nil(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) +} From a97c144cfa1c75e809b974f99c6b04ee88dc7cfd Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Mar 2023 09:13:25 +0100 Subject: [PATCH 05/11] adapted to recent refactoring Signed-off-by: Florian Bacher --- Makefile | 2 +- core/pkg/middleware/interface.go | 9 - core/pkg/middleware/metrics/recorder.go | 279 ------------------ core/pkg/middleware/mock/interface.go | 49 --- .../flag-evaluation/connect_service.go | 44 +-- .../flag-evaluation/connect_service_test.go | 51 ++++ .../pkg/{ => service}/middleware/cors/cors.go | 2 +- .../middleware/cors/cors_test.go | 8 +- core/pkg/{ => service}/middleware/h2c/h2c.go | 2 +- .../{ => service}/middleware/h2c/h2c_test.go | 8 +- core/pkg/service/middleware/interface.go | 9 + .../middleware/{ => metrics}/http_metrics.go | 7 +- .../{ => metrics}/http_metrics_test.go | 5 +- core/pkg/service/middleware/mock/interface.go | 49 +++ .../pkg/sync/grpc/credentials/mock/builder.go | 2 +- 15 files changed, 155 insertions(+), 371 deletions(-) delete mode 100644 core/pkg/middleware/interface.go delete mode 100644 core/pkg/middleware/metrics/recorder.go delete mode 100644 core/pkg/middleware/mock/interface.go rename core/pkg/{ => service}/middleware/cors/cors.go (92%) rename core/pkg/{ => service}/middleware/cors/cors_test.go (75%) rename core/pkg/{ => service}/middleware/h2c/h2c.go (77%) rename core/pkg/{ => service}/middleware/h2c/h2c_test.go (72%) create mode 100644 core/pkg/service/middleware/interface.go rename core/pkg/service/middleware/{ => metrics}/http_metrics.go (95%) rename core/pkg/service/middleware/{ => metrics}/http_metrics_test.go (98%) create mode 100644 core/pkg/service/middleware/mock/interface.go diff --git a/Makefile b/Makefile index 2a2f8b060..77ddbf7d3 100644 --- a/Makefile +++ b/Makefile @@ -60,7 +60,7 @@ mockgen: install-mockgen cd core; mockgen -source=pkg/sync/grpc/grpc_sync.go -destination=pkg/sync/grpc/mock/grpc.go -package=grpcmock cd core; mockgen -source=pkg/sync/grpc/credentials/builder.go -destination=pkg/sync/grpc/credentials/mock/builder.go -package=credendialsmock cd core; mockgen -source=pkg/eval/ievaluator.go -destination=pkg/eval/mock/ievaluator.go -package=evalmock - cd core; mockgen -source=pkg/middleware/interface.go -destination=pkg/middleware/mock/interface.go -package=middlewaremock + cd core; mockgen -source=pkg/service/middleware/interface.go -destination=pkg/service/middleware/mock/interface.go -package=middlewaremock generate-docs: cd flagd; go run ./cmd/doc/main.go diff --git a/core/pkg/middleware/interface.go b/core/pkg/middleware/interface.go deleted file mode 100644 index d4acafec3..000000000 --- a/core/pkg/middleware/interface.go +++ /dev/null @@ -1,9 +0,0 @@ -package middleware - -import ( - "net/http" -) - -type Middleware interface { - Handle(handler http.Handler) http.Handler -} diff --git a/core/pkg/middleware/metrics/recorder.go b/core/pkg/middleware/metrics/recorder.go deleted file mode 100644 index 7ad730607..000000000 --- a/core/pkg/middleware/metrics/recorder.go +++ /dev/null @@ -1,279 +0,0 @@ -package service - -import ( - "bufio" - "context" - "errors" - "fmt" - "log" - "net" - "net/http" - "strconv" - "time" - - "github.com/open-feature/flagd/core/pkg/logger" - "github.com/prometheus/client_golang/prometheus" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/metric/instrument" - "go.opentelemetry.io/otel/metric/unit" - "go.opentelemetry.io/otel/sdk/instrumentation" - "go.opentelemetry.io/otel/sdk/metric" - "go.opentelemetry.io/otel/sdk/metric/aggregation" - - semconv "go.opentelemetry.io/otel/semconv/v1.13.0" -) - -var ( - _ http.ResponseWriter = &responseWriterInterceptor{} - _ http.Hijacker = &responseWriterInterceptor{} - _ http.Flusher = &responseWriterInterceptor{} -) - -type HTTPReqProperties struct { - Service string - ID string - Method string - Code string -} - -type Recorder interface { - // OTelObserveHTTPRequestDuration measures the duration of an HTTP request. - OTelObserveHTTPRequestDuration(props HTTPReqProperties, duration time.Duration) - // OTelObserveHTTPResponseSize measures the size of an HTTP response in bytes. - OTelObserveHTTPResponseSize(props HTTPReqProperties, sizeBytes int64) - - // OTelInFlightRequestStart count the active requests. - OTelInFlightRequestStart(props HTTPReqProperties) - // OTelInFlightRequestEnd count the finished requests. - OTelInFlightRequestEnd(props HTTPReqProperties) -} - -type Reporter interface { - Method() string - URLPath() string - StatusCode() int - BytesWritten() int64 -} - -type HTTPProperties struct { - Service string - ID string -} - -type OTelMetricsRecorder struct { - httpRequestDurHistogram instrument.Float64Histogram - httpResponseSizeHistogram instrument.Float64Histogram - httpRequestsInflight instrument.Int64UpDownCounter -} - -func (r OTelMetricsRecorder) setAttributes(p HTTPReqProperties) []attribute.KeyValue { - return []attribute.KeyValue{ - semconv.ServiceNameKey.String(p.Service), - semconv.HTTPURLKey.String(p.ID), - semconv.HTTPMethodKey.String(p.Method), - semconv.HTTPStatusCodeKey.String(p.Code), - } -} - -func (r OTelMetricsRecorder) OTelObserveHTTPRequestDuration(p HTTPReqProperties, duration time.Duration) { - r.httpRequestDurHistogram.Record(context.TODO(), duration.Seconds(), r.setAttributes(p)...) -} - -func (r OTelMetricsRecorder) OTelObserveHTTPResponseSize(p HTTPReqProperties, sizeBytes int64) { - r.httpResponseSizeHistogram.Record(context.TODO(), float64(sizeBytes), r.setAttributes(p)...) -} - -func (r OTelMetricsRecorder) OTelInFlightRequestStart(p HTTPReqProperties) { - r.httpRequestsInflight.Add(context.TODO(), 1, r.setAttributes(p)...) -} - -func (r OTelMetricsRecorder) OTelInFlightRequestEnd(p HTTPReqProperties) { - r.httpRequestsInflight.Add(context.TODO(), -1, r.setAttributes(p)...) -} - -type MiddlewareConfig struct { - Recorder Recorder - MetricReader metric.Reader - Logger *logger.Logger - Service string - GroupedStatus bool - DisableMeasureSize bool -} - -type Middleware struct { - cfg MiddlewareConfig - HandlerID string -} - -func New(cfg MiddlewareConfig) Middleware { - cfg.defaults() - m := Middleware{cfg: cfg} - return m -} - -func (cfg *MiddlewareConfig) defaults() { - if cfg.Logger == nil { - log.Fatal("missing logger") - } - if cfg.MetricReader == nil { - log.Fatal("missing MetricReader/Exporter") - } - cfg.Recorder = cfg.newOTelRecorder(cfg.MetricReader) -} - -func (cfg *MiddlewareConfig) getDurationView(name string, bucket []float64) metric.View { - return metric.NewView( - metric.Instrument{ - // we change aggregation only for instruments with this name and scope - Name: name, - Scope: instrumentation.Scope{ - Name: cfg.Service, - }, - }, - metric.Stream{Aggregation: aggregation.ExplicitBucketHistogram{ - Boundaries: bucket, - }}, - ) -} - -func (cfg *MiddlewareConfig) newOTelRecorder(exporter metric.Reader) *OTelMetricsRecorder { - const requestDurationName = "http_request_duration_seconds" - const responseSizeName = "http_response_size_bytes" - - // create a metric provider with custom bucket size for histograms - provider := metric.NewMeterProvider( - metric.WithReader(exporter), - metric.WithView(cfg.getDurationView(requestDurationName, prometheus.DefBuckets)), - metric.WithView(cfg.getDurationView(responseSizeName, prometheus.ExponentialBuckets(100, 10, 8))), - ) - meter := provider.Meter(cfg.Service) - // we can ignore errors from OpenTelemetry since they could occur if we select the wrong aggregator - hduration, _ := meter.Float64Histogram( - requestDurationName, - instrument.WithDescription("The latency of the HTTP requests"), - ) - hsize, _ := meter.Float64Histogram( - responseSizeName, - instrument.WithDescription("The size of the HTTP responses"), - instrument.WithUnit(unit.Bytes), - ) - reqCounter, _ := meter.Int64UpDownCounter( - "http_requests_inflight", - instrument.WithDescription("The number of inflight requests being handled at the same time"), - ) - return &OTelMetricsRecorder{ - httpRequestDurHistogram: hduration, - httpResponseSizeHistogram: hsize, - httpRequestsInflight: reqCounter, - } -} - -func (m Middleware) Measure(handlerID string, reporter Reporter, next func()) { - // If there isn't predefined handler ID we - // set that ID as the URL path. - hid := handlerID - if handlerID == "" { - hid = reporter.URLPath() - } - - // If we need to group the status code, it uses the - // first number of the status code because is the least - // required identification way. - var code string - if m.cfg.GroupedStatus { - code = fmt.Sprintf("%dxx", reporter.StatusCode()/100) - } else { - code = strconv.Itoa(reporter.StatusCode()) - } - props := HTTPReqProperties{ - Service: m.cfg.Service, - ID: hid, - Method: reporter.Method(), - Code: code, - } - - m.cfg.Recorder.OTelInFlightRequestStart(props) - defer m.cfg.Recorder.OTelInFlightRequestEnd(props) - - // Start the timer and when finishing measure the duration. - start := time.Now() - defer func() { - duration := time.Since(start) - - m.cfg.Recorder.OTelObserveHTTPRequestDuration(props, duration) - - // Measure size of response if required. - if !m.cfg.DisableMeasureSize { - m.cfg.Recorder.OTelObserveHTTPResponseSize(props, reporter.BytesWritten()) - } - }() - - // Call the wrapped logic. - next() -} - -// Handle returns an measuring standard http.Handler. -func (m Middleware) Handle(h http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - wi := &responseWriterInterceptor{ - statusCode: http.StatusOK, - ResponseWriter: w, - } - reporter := &stdReporter{ - w: wi, - r: r, - } - - m.Measure(m.HandlerID, reporter, func() { - h.ServeHTTP(wi, r) - }) - }) -} - -type stdReporter struct { - w *responseWriterInterceptor - r *http.Request -} - -func (s *stdReporter) Method() string { return s.r.Method } - -func (s *stdReporter) URLPath() string { return s.r.URL.Path } - -func (s *stdReporter) StatusCode() int { return s.w.statusCode } - -func (s *stdReporter) BytesWritten() int64 { return int64(s.w.bytesWritten) } - -// responseWriterInterceptor is a simple wrapper to intercept set data on a -// ResponseWriter. -type responseWriterInterceptor struct { - http.ResponseWriter - statusCode int - bytesWritten int -} - -func (w *responseWriterInterceptor) WriteHeader(statusCode int) { - w.statusCode = statusCode - w.ResponseWriter.WriteHeader(statusCode) -} - -func (w *responseWriterInterceptor) Write(p []byte) (int, error) { - w.bytesWritten += len(p) - return w.ResponseWriter.Write(p) -} - -func (w *responseWriterInterceptor) Hijack() (net.Conn, *bufio.ReadWriter, error) { - h, ok := w.ResponseWriter.(http.Hijacker) - if !ok { - return nil, nil, errors.New("type assertion failed http.ResponseWriter not a http.Hijacker") - } - return h.Hijack() -} - -func (w *responseWriterInterceptor) Flush() { - f, ok := w.ResponseWriter.(http.Flusher) - if !ok { - return - } - - f.Flush() -} diff --git a/core/pkg/middleware/mock/interface.go b/core/pkg/middleware/mock/interface.go deleted file mode 100644 index c27fc0055..000000000 --- a/core/pkg/middleware/mock/interface.go +++ /dev/null @@ -1,49 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: pkg/middleware/interface.go - -// Package middlewaremock is a generated GoMock package. -package middlewaremock - -import ( - http "net/http" - reflect "reflect" - - gomock "github.com/golang/mock/gomock" -) - -// MockMiddleware is a mock of Middleware interface. -type MockMiddleware struct { - ctrl *gomock.Controller - recorder *MockMiddlewareMockRecorder -} - -// MockMiddlewareMockRecorder is the mock recorder for MockMiddleware. -type MockMiddlewareMockRecorder struct { - mock *MockMiddleware -} - -// NewMockMiddleware creates a new mock instance. -func NewMockMiddleware(ctrl *gomock.Controller) *MockMiddleware { - mock := &MockMiddleware{ctrl: ctrl} - mock.recorder = &MockMiddlewareMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockMiddleware) EXPECT() *MockMiddlewareMockRecorder { - return m.recorder -} - -// Handle mocks base method. -func (m *MockMiddleware) Handle(handler http.Handler) http.Handler { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Handle", handler) - ret0, _ := ret[0].(http.Handler) - return ret0 -} - -// Handle indicates an expected call of Handle. -func (mr *MockMiddlewareMockRecorder) Handle(handler interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Handle", reflect.TypeOf((*MockMiddleware)(nil).Handle), handler) -} diff --git a/core/pkg/service/flag-evaluation/connect_service.go b/core/pkg/service/flag-evaluation/connect_service.go index 79b0bc82b..8b3376938 100644 --- a/core/pkg/service/flag-evaluation/connect_service.go +++ b/core/pkg/service/flag-evaluation/connect_service.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "github.com/open-feature/flagd/core/pkg/service/middleware" "net" "net/http" "sync" @@ -15,12 +16,12 @@ import ( "github.com/open-feature/flagd/core/pkg/logger" "github.com/open-feature/flagd/core/pkg/otel" "github.com/open-feature/flagd/core/pkg/service" - "github.com/open-feature/flagd/core/pkg/service/middleware" + corsmw "github.com/open-feature/flagd/core/pkg/service/middleware/cors" + h2cmw "github.com/open-feature/flagd/core/pkg/service/middleware/h2c" + metricsmw "github.com/open-feature/flagd/core/pkg/service/middleware/metrics" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/rs/cors" "go.uber.org/zap" - "golang.org/x/net/http2" - "golang.org/x/net/http2/h2c" ) const ErrorPrefix = "FlagdError:" @@ -101,30 +102,39 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene path, handler := schemaConnectV1.NewServiceHandler(fes) mux.Handle(path, handler) - mdlw := middleware.NewHttpMetric(middleware.Config{ + s.server = http.Server{ + ReadHeaderTimeout: time.Second, + Handler: handler, + } + + go bindMetrics(s, svcConf) + + // Add middlewares + + metricsMiddleware := metricsmw.NewHttpMetric(metricsmw.Config{ Service: "openfeature/flagd", MetricRecorder: s.Metrics, Logger: s.Logger, + HandlerID: "", }) - h := middleware.Handler("", mdlw, mux) - go bindMetrics(s, svcConf) + s.AddMiddleware(metricsMiddleware) - if s.ConnectServiceConfiguration.ServerCertPath != "" && s.ConnectServiceConfiguration.ServerKeyPath != "" { - handler = s.newCORS().Handler(h) - } else { - handler = h2c.NewHandler( - s.newCORS().Handler(h), - &http2.Server{}, - ) - } - s.server = http.Server{ - ReadHeaderTimeout: time.Second, - Handler: handler, + corsMiddleware := corsmw.New(s.ConnectServiceConfiguration.CORS) + s.AddMiddleware(corsMiddleware) + + if s.ConnectServiceConfiguration.ServerCertPath == "" || s.ConnectServiceConfiguration.ServerKeyPath == "" { + h2cMiddleware := h2cmw.New() + s.AddMiddleware(h2cMiddleware) } + return lis, nil } +func (s *ConnectService) AddMiddleware(mw middleware.IMiddleware) { + s.server.Handler = mw.Handler(s.server.Handler) +} + func (s *ConnectService) Notify(n service.Notification) { s.eventingConfiguration.mu.RLock() defer s.eventingConfiguration.mu.RUnlock() diff --git a/core/pkg/service/flag-evaluation/connect_service_test.go b/core/pkg/service/flag-evaluation/connect_service_test.go index e879946fd..ed493f552 100644 --- a/core/pkg/service/flag-evaluation/connect_service_test.go +++ b/core/pkg/service/flag-evaluation/connect_service_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" + "net/http" "os" "testing" "time" @@ -119,3 +121,52 @@ func TestConnectService_UnixConnection(t *testing.T) { }) } } + +func TestAddMiddleware(t *testing.T) { + const port = 12345 + ctrl := gomock.NewController(t) + + mwMock := middlewaremock.NewMockIMiddleware(ctrl) + + mwMock.EXPECT().Handler(gomock.Any()).Return( + http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + writer.WriteHeader(http.StatusTeapot) + })) + + exp := metric.NewManualReader() + metricRecorder := otel.NewOTelRecorder(exp, "my-exporter") + + svc := ConnectService{ + ConnectServiceConfiguration: &ConnectServiceConfiguration{}, + Logger: logger.NewLogger(nil, false), + Metrics: metricRecorder, + } + + serveConf := iservice.Configuration{ + ReadinessProbe: func() bool { + return true + }, + Port: port, + } + ctx := context.Background() + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + go func() { + err := svc.Serve(ctx, nil, serveConf) + fmt.Println(err) + }() + + require.Eventually(t, func() bool { + return svc.server.Handler != nil + }, 3*time.Second, 100*time.Millisecond) + + svc.AddMiddleware(mwMock) + + // call an endpoint provided by the server + resp, err := http.Get(fmt.Sprintf("http://localhost:%d/schema.v1.Service/ResolveAll", port)) + + require.Nil(t, err) + // verify that the status we return in the mocked middleware + require.Equal(t, http.StatusTeapot, resp.StatusCode) +} diff --git a/core/pkg/middleware/cors/cors.go b/core/pkg/service/middleware/cors/cors.go similarity index 92% rename from core/pkg/middleware/cors/cors.go rename to core/pkg/service/middleware/cors/cors.go index 195f1ea30..43597d4b5 100644 --- a/core/pkg/middleware/cors/cors.go +++ b/core/pkg/service/middleware/cors/cors.go @@ -40,6 +40,6 @@ func New(allowedOrigins []string) *Middleware { } } -func (c Middleware) Handle(handler http.Handler) http.Handler { +func (c Middleware) Handler(handler http.Handler) http.Handler { return c.cors.Handler(handler) } diff --git a/core/pkg/middleware/cors/cors_test.go b/core/pkg/service/middleware/cors/cors_test.go similarity index 75% rename from core/pkg/middleware/cors/cors_test.go rename to core/pkg/service/middleware/cors/cors_test.go index 8e9e72021..b12e84c73 100644 --- a/core/pkg/middleware/cors/cors_test.go +++ b/core/pkg/service/middleware/cors/cors_test.go @@ -2,7 +2,7 @@ package cors import ( "github.com/golang/mock/gomock" - middlewaremock "github.com/open-feature/flagd/core/pkg/middleware/mock" + middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" "github.com/stretchr/testify/require" "net/http" "net/http/httptest" @@ -11,7 +11,7 @@ import ( func TestMiddleware(t *testing.T) { ctrl := gomock.NewController(t) - mockMw := middlewaremock.NewMockMiddleware(ctrl) + mockMw := middlewaremock.NewMockIMiddleware(ctrl) handlerFunc := http.HandlerFunc( func(writer http.ResponseWriter, request *http.Request) { @@ -19,7 +19,7 @@ func TestMiddleware(t *testing.T) { }, ) - mockMw.EXPECT().Handle(gomock.Any()).Return(handlerFunc) + mockMw.EXPECT().Handler(gomock.Any()).Return(handlerFunc) ts := httptest.NewServer(handlerFunc) @@ -29,7 +29,7 @@ func TestMiddleware(t *testing.T) { require.NotNil(t, mw) // wrap the cors middleware around the mock to make sure the wrapped handler is called by the cors middleware - ts.Config.Handler = mw.Handle(mockMw.Handle(handlerFunc)) + ts.Config.Handler = mw.Handler(mockMw.Handler(handlerFunc)) req, err := http.NewRequest(http.MethodGet, ts.URL, nil) diff --git a/core/pkg/middleware/h2c/h2c.go b/core/pkg/service/middleware/h2c/h2c.go similarity index 77% rename from core/pkg/middleware/h2c/h2c.go rename to core/pkg/service/middleware/h2c/h2c.go index ce463147e..2b163cf43 100644 --- a/core/pkg/middleware/h2c/h2c.go +++ b/core/pkg/service/middleware/h2c/h2c.go @@ -12,6 +12,6 @@ func New() *Middleware { return &Middleware{} } -func (m Middleware) Handle(handler http.Handler) http.Handler { +func (m Middleware) Handler(handler http.Handler) http.Handler { return h2c.NewHandler(handler, &http2.Server{}) } diff --git a/core/pkg/middleware/h2c/h2c_test.go b/core/pkg/service/middleware/h2c/h2c_test.go similarity index 72% rename from core/pkg/middleware/h2c/h2c_test.go rename to core/pkg/service/middleware/h2c/h2c_test.go index e190e3a08..014162beb 100644 --- a/core/pkg/middleware/h2c/h2c_test.go +++ b/core/pkg/service/middleware/h2c/h2c_test.go @@ -2,7 +2,7 @@ package h2c import ( "github.com/golang/mock/gomock" - middlewaremock "github.com/open-feature/flagd/core/pkg/middleware/mock" + middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" "github.com/stretchr/testify/require" "net/http" "net/http/httptest" @@ -11,7 +11,7 @@ import ( func TestMiddleware(t *testing.T) { ctrl := gomock.NewController(t) - mockMw := middlewaremock.NewMockMiddleware(ctrl) + mockMw := middlewaremock.NewMockIMiddleware(ctrl) handlerFunc := http.HandlerFunc( func(writer http.ResponseWriter, request *http.Request) { @@ -19,7 +19,7 @@ func TestMiddleware(t *testing.T) { }, ) - mockMw.EXPECT().Handle(gomock.Any()).Return(handlerFunc) + mockMw.EXPECT().Handler(gomock.Any()).Return(handlerFunc) ts := httptest.NewServer(handlerFunc) @@ -29,7 +29,7 @@ func TestMiddleware(t *testing.T) { require.NotNil(t, mw) // wrap the h2c middleware around the mock to make sure the wrapped handler is called by the h2c middleware - ts.Config.Handler = mw.Handle(mockMw.Handle(handlerFunc)) + ts.Config.Handler = mw.Handler(mockMw.Handler(handlerFunc)) resp, err := http.Get(ts.URL) diff --git a/core/pkg/service/middleware/interface.go b/core/pkg/service/middleware/interface.go new file mode 100644 index 000000000..47aaf307a --- /dev/null +++ b/core/pkg/service/middleware/interface.go @@ -0,0 +1,9 @@ +package middleware + +import ( + "net/http" +) + +type IMiddleware interface { + Handler(handler http.Handler) http.Handler +} diff --git a/core/pkg/service/middleware/http_metrics.go b/core/pkg/service/middleware/metrics/http_metrics.go similarity index 95% rename from core/pkg/service/middleware/http_metrics.go rename to core/pkg/service/middleware/metrics/http_metrics.go index 563009ae2..1a7c6e740 100644 --- a/core/pkg/service/middleware/http_metrics.go +++ b/core/pkg/service/middleware/metrics/http_metrics.go @@ -1,4 +1,4 @@ -package middleware +package metrics import ( "bufio" @@ -21,6 +21,7 @@ type Config struct { Service string GroupedStatus bool DisableMeasureSize bool + HandlerID string } type Middleware struct { @@ -90,7 +91,7 @@ func (m Middleware) Measure(ctx context.Context, handlerID string, reporter Repo } // Handler returns an measuring standard http.Handler. -func Handler(handlerID string, m Middleware, h http.Handler) http.Handler { +func (m Middleware) Handler(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { wi := &responseWriterInterceptor{ statusCode: http.StatusOK, @@ -100,7 +101,7 @@ func Handler(handlerID string, m Middleware, h http.Handler) http.Handler { w: wi, r: r, } - m.Measure(r.Context(), handlerID, reporter, func() { + m.Measure(r.Context(), m.cfg.HandlerID, reporter, func() { h.ServeHTTP(wi, r) }) }) diff --git a/core/pkg/service/middleware/http_metrics_test.go b/core/pkg/service/middleware/metrics/http_metrics_test.go similarity index 98% rename from core/pkg/service/middleware/http_metrics_test.go rename to core/pkg/service/middleware/metrics/http_metrics_test.go index 550b2b230..9ed7df783 100644 --- a/core/pkg/service/middleware/http_metrics_test.go +++ b/core/pkg/service/middleware/metrics/http_metrics_test.go @@ -1,4 +1,4 @@ -package middleware +package metrics import ( "context" @@ -22,11 +22,12 @@ func TestMiddlewareExposesMetrics(t *testing.T) { MetricRecorder: otel.NewOTelRecorder(exp, svcName), Service: svcName, Logger: logger.NewLogger(l, true), + HandlerID: "id", }) handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("answer")) }) - svr := httptest.NewServer(Handler("id", m, handler)) + svr := httptest.NewServer(m.Handler(handler)) defer svr.Close() resp, err := http.Get(svr.URL) if err != nil { diff --git a/core/pkg/service/middleware/mock/interface.go b/core/pkg/service/middleware/mock/interface.go new file mode 100644 index 000000000..f4b098472 --- /dev/null +++ b/core/pkg/service/middleware/mock/interface.go @@ -0,0 +1,49 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: pkg/service/middleware/interface.go + +// Package middlewaremock is a generated GoMock package. +package middlewaremock + +import ( + http "net/http" + reflect "reflect" + + gomock "github.com/golang/mock/gomock" +) + +// MockIMiddleware is a mock of IMiddleware interface. +type MockIMiddleware struct { + ctrl *gomock.Controller + recorder *MockIMiddlewareMockRecorder +} + +// MockIMiddlewareMockRecorder is the mock recorder for MockIMiddleware. +type MockIMiddlewareMockRecorder struct { + mock *MockIMiddleware +} + +// NewMockIMiddleware creates a new mock instance. +func NewMockIMiddleware(ctrl *gomock.Controller) *MockIMiddleware { + mock := &MockIMiddleware{ctrl: ctrl} + mock.recorder = &MockIMiddlewareMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockIMiddleware) EXPECT() *MockIMiddlewareMockRecorder { + return m.recorder +} + +// Handler mocks base method. +func (m *MockIMiddleware) Handler(handler http.Handler) http.Handler { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Handler", handler) + ret0, _ := ret[0].(http.Handler) + return ret0 +} + +// Handler indicates an expected call of Handler. +func (mr *MockIMiddlewareMockRecorder) Handler(handler interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Handler", reflect.TypeOf((*MockIMiddleware)(nil).Handler), handler) +} diff --git a/core/pkg/sync/grpc/credentials/mock/builder.go b/core/pkg/sync/grpc/credentials/mock/builder.go index 933a926a6..c47dd64a9 100644 --- a/core/pkg/sync/grpc/credentials/mock/builder.go +++ b/core/pkg/sync/grpc/credentials/mock/builder.go @@ -1,5 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: core/pkg/sync/grpc/credentials/builder.go +// Source: pkg/sync/grpc/credentials/builder.go // Package credendialsmock is a generated GoMock package. package credendialsmock From 9b9e02397fa3426504df5dcff43d1cfc0f6ee20d Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Mar 2023 09:44:05 +0100 Subject: [PATCH 06/11] adopted the test to avoid race conditions Signed-off-by: Florian Bacher --- .../service/flag-evaluation/connect_service_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/pkg/service/flag-evaluation/connect_service_test.go b/core/pkg/service/flag-evaluation/connect_service_test.go index ed493f552..b6acf35ea 100644 --- a/core/pkg/service/flag-evaluation/connect_service_test.go +++ b/core/pkg/service/flag-evaluation/connect_service_test.go @@ -130,7 +130,7 @@ func TestAddMiddleware(t *testing.T) { mwMock.EXPECT().Handler(gomock.Any()).Return( http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { - writer.WriteHeader(http.StatusTeapot) + writer.WriteHeader(http.StatusOK) })) exp := metric.NewManualReader() @@ -158,15 +158,17 @@ func TestAddMiddleware(t *testing.T) { }() require.Eventually(t, func() bool { - return svc.server.Handler != nil + resp, err := http.Get(fmt.Sprintf("http://localhost:%d/schema.v1.Service/ResolveAll", port)) + // with the default http handler we should get a method not allowed (405) when attempting a GET request + return err == nil && resp.StatusCode == http.StatusMethodNotAllowed }, 3*time.Second, 100*time.Millisecond) svc.AddMiddleware(mwMock) - // call an endpoint provided by the server + // with the injected middleware, the GET method should work resp, err := http.Get(fmt.Sprintf("http://localhost:%d/schema.v1.Service/ResolveAll", port)) require.Nil(t, err) // verify that the status we return in the mocked middleware - require.Equal(t, http.StatusTeapot, resp.StatusCode) + require.Equal(t, http.StatusOK, resp.StatusCode) } From cee0db3499a692c37de3a5c29139f30509bf2883 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Mar 2023 09:59:53 +0100 Subject: [PATCH 07/11] remove obsolete code Signed-off-by: Florian Bacher --- .../flag-evaluation/connect_service.go | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/core/pkg/service/flag-evaluation/connect_service.go b/core/pkg/service/flag-evaluation/connect_service.go index 8b3376938..ca74c8184 100644 --- a/core/pkg/service/flag-evaluation/connect_service.go +++ b/core/pkg/service/flag-evaluation/connect_service.go @@ -20,7 +20,6 @@ import ( h2cmw "github.com/open-feature/flagd/core/pkg/service/middleware/h2c" metricsmw "github.com/open-feature/flagd/core/pkg/service/middleware/metrics" "github.com/prometheus/client_golang/prometheus/promhttp" - "github.com/rs/cors" "go.uber.org/zap" ) @@ -143,35 +142,6 @@ func (s *ConnectService) Notify(n service.Notification) { } } -func (s *ConnectService) newCORS() *cors.Cors { - return cors.New(cors.Options{ - AllowedMethods: []string{ - http.MethodHead, - http.MethodGet, - http.MethodPost, - http.MethodPut, - http.MethodPatch, - http.MethodDelete, - }, - AllowedOrigins: s.ConnectServiceConfiguration.CORS, - AllowedHeaders: []string{"*"}, - ExposedHeaders: []string{ - // Content-Type is in the default safelist. - "Accept", - "Accept-Encoding", - "Accept-Post", - "Connect-Accept-Encoding", - "Connect-Content-Encoding", - "Content-Encoding", - "Grpc-Accept-Encoding", - "Grpc-Encoding", - "Grpc-Message", - "Grpc-Status", - "Grpc-Status-Details-Bin", - }, - }) -} - func bindMetrics(s *ConnectService, svcConf service.Configuration) { s.Logger.Info(fmt.Sprintf("metrics and probes listening at %d", svcConf.MetricsPort)) server := &http.Server{ From 09d28eef85c503c40a7c5c1badcb375b3a907374 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Mar 2023 10:10:20 +0100 Subject: [PATCH 08/11] revert out of scope change Signed-off-by: Florian Bacher --- core/pkg/sync/grpc/credentials/mock/builder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/pkg/sync/grpc/credentials/mock/builder.go b/core/pkg/sync/grpc/credentials/mock/builder.go index c47dd64a9..933a926a6 100644 --- a/core/pkg/sync/grpc/credentials/mock/builder.go +++ b/core/pkg/sync/grpc/credentials/mock/builder.go @@ -1,5 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: pkg/sync/grpc/credentials/builder.go +// Source: core/pkg/sync/grpc/credentials/builder.go // Package credendialsmock is a generated GoMock package. package credendialsmock From ffda4705959b5f8a566175fd96f1bd85c6b6bcf9 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Mar 2023 10:56:32 +0100 Subject: [PATCH 09/11] gofumpt'ed changed files Signed-off-by: Florian Bacher --- core/pkg/service/flag-evaluation/connect_service.go | 3 ++- core/pkg/service/flag-evaluation/connect_service_test.go | 3 ++- core/pkg/service/middleware/cors/cors.go | 3 ++- core/pkg/service/middleware/cors/cors_test.go | 7 ++++--- core/pkg/service/middleware/h2c/h2c.go | 3 ++- core/pkg/service/middleware/h2c/h2c_test.go | 7 ++++--- 6 files changed, 16 insertions(+), 10 deletions(-) diff --git a/core/pkg/service/flag-evaluation/connect_service.go b/core/pkg/service/flag-evaluation/connect_service.go index ca74c8184..8b2653acf 100644 --- a/core/pkg/service/flag-evaluation/connect_service.go +++ b/core/pkg/service/flag-evaluation/connect_service.go @@ -5,12 +5,13 @@ import ( "context" "errors" "fmt" - "github.com/open-feature/flagd/core/pkg/service/middleware" "net" "net/http" "sync" "time" + "github.com/open-feature/flagd/core/pkg/service/middleware" + schemaConnectV1 "buf.build/gen/go/open-feature/flagd/bufbuild/connect-go/schema/v1/schemav1connect" "github.com/open-feature/flagd/core/pkg/eval" "github.com/open-feature/flagd/core/pkg/logger" diff --git a/core/pkg/service/flag-evaluation/connect_service_test.go b/core/pkg/service/flag-evaluation/connect_service_test.go index b6acf35ea..c4fb08503 100644 --- a/core/pkg/service/flag-evaluation/connect_service_test.go +++ b/core/pkg/service/flag-evaluation/connect_service_test.go @@ -4,12 +4,13 @@ import ( "context" "errors" "fmt" - middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" "net/http" "os" "testing" "time" + middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" + schemaGrpcV1 "buf.build/gen/go/open-feature/flagd/grpc/go/schema/v1/schemav1grpc" schemaV1 "buf.build/gen/go/open-feature/flagd/protocolbuffers/go/schema/v1" "github.com/golang/mock/gomock" diff --git a/core/pkg/service/middleware/cors/cors.go b/core/pkg/service/middleware/cors/cors.go index 43597d4b5..303be268d 100644 --- a/core/pkg/service/middleware/cors/cors.go +++ b/core/pkg/service/middleware/cors/cors.go @@ -1,8 +1,9 @@ package cors import ( - "github.com/rs/cors" "net/http" + + "github.com/rs/cors" ) type Middleware struct { diff --git a/core/pkg/service/middleware/cors/cors_test.go b/core/pkg/service/middleware/cors/cors_test.go index b12e84c73..f5f72cb52 100644 --- a/core/pkg/service/middleware/cors/cors_test.go +++ b/core/pkg/service/middleware/cors/cors_test.go @@ -1,12 +1,13 @@ package cors import ( - "github.com/golang/mock/gomock" - middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" - "github.com/stretchr/testify/require" "net/http" "net/http/httptest" "testing" + + "github.com/golang/mock/gomock" + middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" + "github.com/stretchr/testify/require" ) func TestMiddleware(t *testing.T) { diff --git a/core/pkg/service/middleware/h2c/h2c.go b/core/pkg/service/middleware/h2c/h2c.go index 2b163cf43..73e573ad9 100644 --- a/core/pkg/service/middleware/h2c/h2c.go +++ b/core/pkg/service/middleware/h2c/h2c.go @@ -1,9 +1,10 @@ package h2c import ( + "net/http" + "golang.org/x/net/http2" "golang.org/x/net/http2/h2c" - "net/http" ) type Middleware struct{} diff --git a/core/pkg/service/middleware/h2c/h2c_test.go b/core/pkg/service/middleware/h2c/h2c_test.go index 014162beb..c292dfa76 100644 --- a/core/pkg/service/middleware/h2c/h2c_test.go +++ b/core/pkg/service/middleware/h2c/h2c_test.go @@ -1,12 +1,13 @@ package h2c import ( - "github.com/golang/mock/gomock" - middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" - "github.com/stretchr/testify/require" "net/http" "net/http/httptest" "testing" + + "github.com/golang/mock/gomock" + middlewaremock "github.com/open-feature/flagd/core/pkg/service/middleware/mock" + "github.com/stretchr/testify/require" ) func TestMiddleware(t *testing.T) { From b54358b8b0d65945a0aaa9bbd0d9913f6f74f763 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Mar 2023 16:27:04 +0100 Subject: [PATCH 10/11] fixed merge conflicts Signed-off-by: Florian Bacher --- core/pkg/service/flag-evaluation/connect_service.go | 4 ++-- core/pkg/service/flag-evaluation/connect_service_test.go | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/pkg/service/flag-evaluation/connect_service.go b/core/pkg/service/flag-evaluation/connect_service.go index fab9cb96b..3d797ebc8 100644 --- a/core/pkg/service/flag-evaluation/connect_service.go +++ b/core/pkg/service/flag-evaluation/connect_service.go @@ -104,7 +104,7 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene // Add middlewares - metricsMiddleware := metricsmw.NewHttpMetric(metricsmw.Config{ + metricsMiddleware := metricsmw.NewHTTPMetric(metricsmw.Config{ Service: "openfeature/flagd", MetricRecorder: s.Metrics, Logger: s.Logger, @@ -116,7 +116,7 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene corsMiddleware := corsmw.New(svcConf.CORS) s.AddMiddleware(corsMiddleware) - if svcConf.CertPath == "" || svcConf.ServerKeyPath == "" { + if svcConf.CertPath == "" || svcConf.KeyPath == "" { h2cMiddleware := h2cmw.New() s.AddMiddleware(h2cMiddleware) } diff --git a/core/pkg/service/flag-evaluation/connect_service_test.go b/core/pkg/service/flag-evaluation/connect_service_test.go index 04322eb6d..098673e08 100644 --- a/core/pkg/service/flag-evaluation/connect_service_test.go +++ b/core/pkg/service/flag-evaluation/connect_service_test.go @@ -136,9 +136,8 @@ func TestAddMiddleware(t *testing.T) { metricRecorder := otel.NewOTelRecorder(exp, "my-exporter") svc := ConnectService{ - ConnectServiceConfiguration: &ConnectServiceConfiguration{}, - Logger: logger.NewLogger(nil, false), - Metrics: metricRecorder, + Logger: logger.NewLogger(nil, false), + Metrics: metricRecorder, } serveConf := iservice.Configuration{ From 09bc299ee12c21b9b7fefa8958af944c16013b50 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Fri, 24 Mar 2023 07:42:00 +0100 Subject: [PATCH 11/11] incorporated PR review Signed-off-by: Florian Bacher --- core/pkg/service/flag-evaluation/connect_service.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/pkg/service/flag-evaluation/connect_service.go b/core/pkg/service/flag-evaluation/connect_service.go index 3d797ebc8..3bdd642bb 100644 --- a/core/pkg/service/flag-evaluation/connect_service.go +++ b/core/pkg/service/flag-evaluation/connect_service.go @@ -66,6 +66,8 @@ func (s *ConnectService) Serve(ctx context.Context, eval eval.IEvaluator, svcCon close(errChan) }() + go s.startMetricsServer(svcConf) + select { case err := <-errChan: return err @@ -100,12 +102,10 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene Handler: handler, } - go bindMetrics(s, svcConf) - // Add middlewares metricsMiddleware := metricsmw.NewHTTPMetric(metricsmw.Config{ - Service: "openfeature/flagd", + Service: svcConf.ServiceName, MetricRecorder: s.Metrics, Logger: s.Logger, HandlerID: "", @@ -136,7 +136,7 @@ func (s *ConnectService) Notify(n service.Notification) { } } -func bindMetrics(s *ConnectService, svcConf service.Configuration) { +func (s *ConnectService) startMetricsServer(svcConf service.Configuration) { s.Logger.Info(fmt.Sprintf("metrics and probes listening at %d", svcConf.MetricsPort)) server := &http.Server{ Addr: fmt.Sprintf(":%d", svcConf.MetricsPort),