From 934322c9f9ede79b265cd0e03bb2f2861df417be Mon Sep 17 00:00:00 2001 From: Scott Leggett Date: Fri, 16 Aug 2024 11:07:04 +0800 Subject: [PATCH 1/2] fix: move NATS message definitions into separate package This change fixes a bug where ssh-portal binaries were unnecessarily exposing ssh-portal-api related metrics (these metrics are always zero in ssh-portal). Previously ssh-portal's sshserver package was importing sshportalapi (and thus getting the unnecessary metrics) just for the bus message struct definitions. By moving the message struct definitions out into their own package, both sshserver and sshportalapi can import the message definitions as required without any side effects (such as metrics registrations). --- internal/bus/ssh.go | 29 +++++++++++++++++++++++++++++ internal/sshportalapi/server.go | 14 +++++++++++--- internal/sshportalapi/sshportal.go | 30 +++--------------------------- internal/sshserver/authhandler.go | 6 +++--- 4 files changed, 46 insertions(+), 33 deletions(-) create mode 100644 internal/bus/ssh.go diff --git a/internal/bus/ssh.go b/internal/bus/ssh.go new file mode 100644 index 00000000..72b1c290 --- /dev/null +++ b/internal/bus/ssh.go @@ -0,0 +1,29 @@ +// Package bus contains the definitions of the messages passed across NATS. +package bus + +import "log/slog" + +const ( + // SubjectSSHAccessQuery defines the NATS subject for SSH access queries. + SubjectSSHAccessQuery = "lagoon.sshportal.api" +) + +// SSHAccessQuery defines the structure of an SSH access query. +type SSHAccessQuery struct { + SSHFingerprint string + NamespaceName string + ProjectID int + EnvironmentID int + SessionID string +} + +// LogValue implements the slog.LogValuer interface. +func (q SSHAccessQuery) LogValue() slog.Value { + return slog.GroupValue( + slog.String("sshFingerprint", q.SSHFingerprint), + slog.String("namespaceName", q.NamespaceName), + slog.Int("projectID", q.ProjectID), + slog.Int("environmentID", q.EnvironmentID), + slog.String("sessionID", q.SessionID), + ) +} diff --git a/internal/sshportalapi/server.go b/internal/sshportalapi/server.go index bdbd78cd..826a7aec 100644 --- a/internal/sshportalapi/server.go +++ b/internal/sshportalapi/server.go @@ -11,6 +11,7 @@ import ( "github.com/google/uuid" "github.com/nats-io/nats.go" + "github.com/uselagoon/ssh-portal/internal/bus" "github.com/uselagoon/ssh-portal/internal/lagoon" "github.com/uselagoon/ssh-portal/internal/lagoondb" "github.com/uselagoon/ssh-portal/internal/rbac" @@ -36,8 +37,15 @@ type KeycloakService interface { } // ServeNATS sshportalapi NATS requests. -func ServeNATS(ctx context.Context, stop context.CancelFunc, log *slog.Logger, - p *rbac.Permission, l LagoonDBService, k KeycloakService, natsURL string) error { +func ServeNATS( + ctx context.Context, + stop context.CancelFunc, + log *slog.Logger, + p *rbac.Permission, + l LagoonDBService, + k KeycloakService, + natsURL string, +) error { // setup synchronisation wg := sync.WaitGroup{} wg.Add(1) @@ -65,7 +73,7 @@ func ServeNATS(ctx context.Context, stop context.CancelFunc, log *slog.Logger, } defer nc.Close() // set up request/response callback for sshportal - _, err = nc.QueueSubscribe(SubjectSSHAccessQuery, queue, + _, err = nc.QueueSubscribe(bus.SubjectSSHAccessQuery, queue, sshportal(ctx, log, nc, p, l, k)) if err != nil { return fmt.Errorf("couldn't subscribe to queue: %v", err) diff --git a/internal/sshportalapi/sshportal.go b/internal/sshportalapi/sshportal.go index aaf7fc11..ef1be73a 100644 --- a/internal/sshportalapi/sshportal.go +++ b/internal/sshportalapi/sshportal.go @@ -9,37 +9,13 @@ import ( "github.com/nats-io/nats.go" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" + "github.com/uselagoon/ssh-portal/internal/bus" "github.com/uselagoon/ssh-portal/internal/lagoon" "github.com/uselagoon/ssh-portal/internal/lagoondb" "github.com/uselagoon/ssh-portal/internal/rbac" "go.opentelemetry.io/otel" ) -const ( - // SubjectSSHAccessQuery defines the NATS subject for SSH access queries. - SubjectSSHAccessQuery = "lagoon.sshportal.api" -) - -// SSHAccessQuery defines the structure of an SSH access query. -type SSHAccessQuery struct { - SSHFingerprint string - NamespaceName string - ProjectID int - EnvironmentID int - SessionID string -} - -// LogValue implements the slog.LogValuer interface. -func (q SSHAccessQuery) LogValue() slog.Value { - return slog.GroupValue( - slog.String("sshFingerprint", q.SSHFingerprint), - slog.String("namespaceName", q.NamespaceName), - slog.Int("projectID", q.ProjectID), - slog.Int("environmentID", q.EnvironmentID), - slog.String("sessionID", q.SessionID), - ) -} - var ( requestsCounter = promauto.NewCounter(prometheus.CounterOpts{ Name: "sshportalapi_requests_total", @@ -55,10 +31,10 @@ func sshportal( l LagoonDBService, k KeycloakService, ) nats.Handler { - return func(_, replySubject string, query *SSHAccessQuery) { + return func(_, replySubject string, query *bus.SSHAccessQuery) { var realmRoles, userGroups []string // set up tracing and update metrics - ctx, span := otel.Tracer(pkgName).Start(ctx, SubjectSSHAccessQuery) + ctx, span := otel.Tracer(pkgName).Start(ctx, bus.SubjectSSHAccessQuery) defer span.End() requestsCounter.Inc() log := log.With(slog.Any("query", query)) diff --git a/internal/sshserver/authhandler.go b/internal/sshserver/authhandler.go index 99b0632f..f1e35dcc 100644 --- a/internal/sshserver/authhandler.go +++ b/internal/sshserver/authhandler.go @@ -8,8 +8,8 @@ import ( "github.com/nats-io/nats.go" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" + "github.com/uselagoon/ssh-portal/internal/bus" "github.com/uselagoon/ssh-portal/internal/k8s" - "github.com/uselagoon/ssh-portal/internal/sshportalapi" gossh "golang.org/x/crypto/ssh" ) @@ -60,7 +60,7 @@ func pubKeyAuth(log *slog.Logger, nc *nats.EncodedConn, } // construct ssh access query fingerprint := gossh.FingerprintSHA256(pubKey) - q := sshportalapi.SSHAccessQuery{ + q := bus.SSHAccessQuery{ SSHFingerprint: fingerprint, NamespaceName: ctx.User(), ProjectID: pid, @@ -69,7 +69,7 @@ func pubKeyAuth(log *slog.Logger, nc *nats.EncodedConn, } // send query var ok bool - err = nc.Request(sshportalapi.SubjectSSHAccessQuery, q, &ok, natsTimeout) + err = nc.Request(bus.SubjectSSHAccessQuery, q, &ok, natsTimeout) if err != nil { log.Warn("couldn't make NATS request", slog.Any("error", err)) return false From 39bed8258e999af85f5c944f5b75e6aafe472151 Mon Sep 17 00:00:00 2001 From: Scott Leggett Date: Fri, 16 Aug 2024 11:21:03 +0800 Subject: [PATCH 2/2] fix: use correct argument order to errors.Is --- internal/sshserver/serve.go | 2 +- internal/sshtoken/serve.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/sshserver/serve.go b/internal/sshserver/serve.go index 3b8c6514..3a868a20 100644 --- a/internal/sshserver/serve.go +++ b/internal/sshserver/serve.go @@ -70,7 +70,7 @@ func Serve( log.Warn("couldn't shutdown cleanly", slog.Any("error", err)) } }() - if err := srv.Serve(l); !errors.Is(ssh.ErrServerClosed, err) { + if err := srv.Serve(l); !errors.Is(err, ssh.ErrServerClosed) { return err } return nil diff --git a/internal/sshtoken/serve.go b/internal/sshtoken/serve.go index 06c52dce..482417f4 100644 --- a/internal/sshtoken/serve.go +++ b/internal/sshtoken/serve.go @@ -58,7 +58,7 @@ func Serve( log.Warn("couldn't shutdown cleanly", slog.Any("error", err)) } }() - if err := srv.Serve(l); !errors.Is(ssh.ErrServerClosed, err) { + if err := srv.Serve(l); !errors.Is(err, ssh.ErrServerClosed) { return err } return nil