From 1c3ee2a97dd7ecaa48d00b9dcafaa0357e5a5702 Mon Sep 17 00:00:00 2001 From: Julien Perrochet Date: Wed, 20 Mar 2024 17:19:19 +0100 Subject: [PATCH] PR comments --- cmds/core-service/main.go | 2 +- pkg/logging/logging.go | 3 --- pkg/scd/dss_report_handler.go | 49 ++++++++++++++++++++++++++++++++--- pkg/scd/repos/repos.go | 8 ------ pkg/scd/server.go | 26 +------------------ 5 files changed, 47 insertions(+), 41 deletions(-) diff --git a/cmds/core-service/main.go b/cmds/core-service/main.go index 3efd4808e..bf6fbca58 100644 --- a/cmds/core-service/main.go +++ b/cmds/core-service/main.go @@ -198,7 +198,7 @@ func createSCDServer(ctx context.Context, logger *zap.Logger) (*scd.Server, erro return &scd.Server{ Store: scdStore, - DssReportHandler: scd.JSONLoggingDssReportHandler{ReportLogger: logger}, + DSSReportHandler: &scd.JSONLoggingDssReportHandler{ReportLogger: logger}, Timeout: *timeout, EnableHTTP: *enableHTTP, }, nil diff --git a/pkg/logging/logging.go b/pkg/logging/logging.go index ca5eda00e..6b894c69a 100644 --- a/pkg/logging/logging.go +++ b/pkg/logging/logging.go @@ -77,9 +77,6 @@ func Configure(level string, format string) error { // WithValuesFromContext augments logger with relevant fields from ctx and returns // the resulting logger. func WithValuesFromContext(ctx context.Context, logger *zap.Logger) *zap.Logger { - // TODO: WithValuesFromContext is used in multiple places that might assume this - // method does more than it actually does: - // given it was added in 2019 we may want to check? // Naive implementation for now, meant to evolve over time. return logger } diff --git a/pkg/scd/dss_report_handler.go b/pkg/scd/dss_report_handler.go index 638722fdf..9b9a3ff7f 100644 --- a/pkg/scd/dss_report_handler.go +++ b/pkg/scd/dss_report_handler.go @@ -4,20 +4,28 @@ import ( "context" "encoding/json" "github.com/google/uuid" + "github.com/interuss/dss/pkg/api" restapi "github.com/interuss/dss/pkg/api/scdv1" + dsserr "github.com/interuss/dss/pkg/errors" "github.com/interuss/dss/pkg/logging" "github.com/interuss/stacktrace" "go.uber.org/zap" ) -// JSONLoggingDssReportHandler a DssReportHandler that simply logs the received report as JSON. +// DSSReporting takes care of handling a DSS report. +type DSSReporting interface { + // HandleDssReport handles a DSS report request. Returns the error report passed in 'req' after having set its identifier. + HandleDssReport(ctx context.Context, req *restapi.MakeDssReportRequest) (*restapi.ErrorReport, error) +} + +// JSONLoggingDssReportHandler a DSSReportHandler that simply logs the received report as JSON. type JSONLoggingDssReportHandler struct { // ReportLogger is the logger to which the received reports will be logged. ReportLogger *zap.Logger } // HandleDssReport logs the received report as a JSON string to a logger. -func (h JSONLoggingDssReportHandler) HandleDssReport(ctx context.Context, req *restapi.MakeDssReportRequest) (*restapi.ErrorReport, error) { +func (h *JSONLoggingDssReportHandler) HandleDssReport(ctx context.Context, req *restapi.MakeDssReportRequest) (*restapi.ErrorReport, error) { reportID, err := uuid.NewRandom() if err != nil { return nil, stacktrace.Propagate(err, "Failed to generate report ID") @@ -29,8 +37,41 @@ func (h JSONLoggingDssReportHandler) HandleDssReport(ctx context.Context, req *r jsonReport, err := json.Marshal(req.Body) if err != nil { logging.WithValuesFromContext(ctx, logging.Logger).Error("Failed to serialize DSS Report", zap.Error(err)) - return nil, stacktrace.Propagate(err, "Failed to serialize DSS Report") + return nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to serialize DSS Report") } - h.ReportLogger.Info("Received DSS Report", zap.String("report", string(jsonReport))) + h.ReportLogger.Info("Received DSS Report", zap.String("reportID", reportIDStr), zap.String("report", string(jsonReport))) return rVal, nil } + +// MakeDssReport creates an error report about a DSS. +func (a *Server) MakeDssReport(ctx context.Context, req *restapi.MakeDssReportRequest, +) restapi.MakeDssReportResponseSet { + if req.Auth.Error != nil { + resp := restapi.MakeDssReportResponseSet{} + setAuthError(ctx, stacktrace.Propagate(req.Auth.Error, "Auth failed"), &resp.Response401, &resp.Response403, &resp.Response500) + return resp + } + + if req.BodyParseError != nil { + return restapi.MakeDssReportResponseSet{Response400: &restapi.ErrorResponse{ + Message: dsserr.Handle(ctx, stacktrace.PropagateWithCode(req.BodyParseError, dsserr.BadRequest, "Malformed params"))}} + } + + report, err := a.DSSReportHandler.HandleDssReport(ctx, req) + + if err != nil { + err = stacktrace.Propagate(err, "Could not delete operational intent") + errResp := &restapi.ErrorResponse{Message: dsserr.Handle(ctx, err)} + switch stacktrace.GetCode(err) { + case dsserr.BadRequest: + return restapi.MakeDssReportResponseSet{Response400: errResp} + case dsserr.PermissionDenied: + return restapi.MakeDssReportResponseSet{Response403: errResp} + default: + return restapi.MakeDssReportResponseSet{Response500: &api.InternalServerErrorBody{ + ErrorMessage: *dsserr.Handle(ctx, stacktrace.Propagate(err, "Got an unexpected error"))}} + } + } + + return restapi.MakeDssReportResponseSet{Response201: report} +} diff --git a/pkg/scd/repos/repos.go b/pkg/scd/repos/repos.go index 89b3ed3f7..ab1cc4a9f 100644 --- a/pkg/scd/repos/repos.go +++ b/pkg/scd/repos/repos.go @@ -3,7 +3,6 @@ package repos import ( "context" "github.com/golang/geo/s2" - restapi "github.com/interuss/dss/pkg/api/scdv1" dssmodels "github.com/interuss/dss/pkg/models" scdmodels "github.com/interuss/dss/pkg/scd/models" ) @@ -81,14 +80,7 @@ type Constraint interface { DeleteConstraint(ctx context.Context, id dssmodels.ID) error } -// DssReport takes care of handling a DSS report. -type DssReport interface { - // HandleDssReport handles a DSS report request. Returns the error report passed in 'req' after having set its identifier. - HandleDssReport(ctx context.Context, req *restapi.MakeDssReportRequest) (*restapi.ErrorReport, error) -} - // Repository aggregates all SCD-specific repo interfaces. -// Note that 'DssReport' is not yet part of it, while we figure exactly how we want to handle DSS reports type Repository interface { OperationalIntent Subscription diff --git a/pkg/scd/server.go b/pkg/scd/server.go index 95e54bff3..1a5b54ae9 100644 --- a/pkg/scd/server.go +++ b/pkg/scd/server.go @@ -2,7 +2,6 @@ package scd import ( "context" - "github.com/interuss/dss/pkg/scd/repos" "time" "github.com/interuss/dss/pkg/api" @@ -37,34 +36,11 @@ func makeSubscribersToNotify(subscriptions []*scdmodels.Subscription) []restapi. // Server implements scdv1.Implementation. type Server struct { Store scdstore.Store - DssReportHandler repos.DssReport + DSSReportHandler DSSReporting Timeout time.Duration EnableHTTP bool } -// MakeDssReport creates an error report about a DSS. -func (a *Server) MakeDssReport(ctx context.Context, req *restapi.MakeDssReportRequest, -) restapi.MakeDssReportResponseSet { - if req.Auth.Error != nil { - resp := restapi.MakeDssReportResponseSet{} - setAuthError(ctx, stacktrace.Propagate(req.Auth.Error, "Auth failed"), &resp.Response401, &resp.Response403, &resp.Response500) - return resp - } - - if req.BodyParseError != nil { - return restapi.MakeDssReportResponseSet{Response400: &restapi.ErrorResponse{ - Message: dsserr.Handle(ctx, stacktrace.PropagateWithCode(req.BodyParseError, dsserr.BadRequest, "Malformed params"))}} - } - - resp, err := a.DssReportHandler.HandleDssReport(ctx, req) - if err != nil { - return restapi.MakeDssReportResponseSet{Response400: &restapi.ErrorResponse{ - Message: dsserr.Handle(ctx, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to handle DSS Report"))}} - } - - return restapi.MakeDssReportResponseSet{Response201: resp} -} - func setAuthError(ctx context.Context, authErr error, resp401, resp403 **restapi.ErrorResponse, resp500 **api.InternalServerErrorBody) { switch stacktrace.GetCode(authErr) { case dsserr.Unauthenticated: