Skip to content
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

Fix audit log reporting for source events and commands #1022

Merged
merged 5 commits into from
Mar 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions cmd/botkube/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"context"
"encoding/json"
"errors"
"fmt"
"log"
"net/http"
Expand Down Expand Up @@ -402,9 +403,13 @@ func run(ctx context.Context) error {

errGroup.Go(func() error {
defer func() {
err := reportFatalError("while starting k8s collector", err)
if err != nil {
logger.Errorf("while reporting fatal error %w", err)
if err == nil {
return
}

reportErr := reportFatalError("while starting k8s collector", err)
if reportErr != nil {
logger.Errorf("while reporting fatal error: %s", reportErr.Error())
}
}()
heartbeatReporter := heartbeat.GetReporter(remoteCfgEnabled, logger, gqlClient)
Expand Down Expand Up @@ -504,6 +509,11 @@ func getK8sClients(cfg *rest.Config) (discovery.DiscoveryInterface, error) {

func reportFatalErrFn(logger logrus.FieldLogger, reporter analytics.Reporter, status status.StatusReporter) func(ctx string, err error) error {
return func(ctx string, err error) error {
if errors.Is(err, context.Canceled) {
logger.Debugf("Context was cancelled. Skipping reporting error...")
return nil
}

// use separate ctx as parent ctx might be cancelled already
ctxTimeout, cancel := context.WithTimeout(context.Background(), time.Second*10)
defer cancel()
Expand Down
8 changes: 8 additions & 0 deletions internal/analytics/segment_reporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ func (r *SegmentReporter) ReportHandledEventSuccess(event ReportEvent) error {
// ReportHandledEventError reports a failure while handling event using a given communication platform.
// The RegisterCurrentIdentity needs to be called first.
func (r *SegmentReporter) ReportHandledEventError(event ReportEvent, err error) error {
if err == nil {
return nil
}

return r.reportEvent("Event handled", map[string]interface{}{
"platform": event.Platform,
"type": event.IntegrationType,
Expand All @@ -123,6 +127,10 @@ func (r *SegmentReporter) ReportHandledEventError(event ReportEvent, err error)
// ReportFatalError reports a fatal app error.
// It doesn't need a registered identity.
func (r *SegmentReporter) ReportFatalError(err error) error {
if err == nil {
return nil
}

properties := map[string]interface{}{
"error": err.Error(),
}
Expand Down
7 changes: 5 additions & 2 deletions internal/audit/gql_reporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,11 @@ func (r *GraphQLAuditReporter) ReportSourceAuditEvent(ctx context.Context, e Sou
DeploymentID: r.gql.DeploymentID(),
Type: remoteapi.AuditEventTypeSourceEventEmitted,
SourceEventEmitted: &remoteapi.AuditEventSourceCreateInput{
Event: e.Event,
Bindings: e.Bindings,
Event: e.Event,
Source: remoteapi.AuditEventSourceDetailsInput{
Name: e.Source.Name,
DisplayName: e.Source.DisplayName,
},
},
},
}
Expand Down
9 changes: 7 additions & 2 deletions internal/audit/reporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type ExecutorAuditEvent struct {
CreatedAt string
PluginName string
PlatformUser string
BotPlatform remoteapi.BotPlatform
BotPlatform *remoteapi.BotPlatform
Command string
Channel string
}
Expand All @@ -29,7 +29,12 @@ type SourceAuditEvent struct {
CreatedAt string
PluginName string
Event string
Bindings []string
Source SourceDetails
}

type SourceDetails struct {
Name string
DisplayName string
}

// GetReporter creates new AuditReporter
Expand Down
5 changes: 0 additions & 5 deletions internal/config/remote/gql_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,6 @@ func NewGqlClient(options ...Option) *Gql {
opt(c)
}

// skip client creation when not requested
if c.endpoint == "" {
return nil
}

httpCli := &http.Client{
Transport: newAPIKeySecuredTransport(c.apiKey),
Timeout: defaultTimeout,
Expand Down
2 changes: 1 addition & 1 deletion internal/heartbeat/gql_reporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func newGraphQLHeartbeatReporter(logger logrus.FieldLogger, client GraphQLClient

func (r *GraphQLHeartbeatReporter) ReportHeartbeat(ctx context.Context, heartbeat DeploymentHeartbeatInput) error {
logger := r.log.WithFields(logrus.Fields{
"deploymentID": r.gql.DeploymentID,
"deploymentID": r.gql.DeploymentID(),
"heartbeat": heartbeat,
})
logger.Debug("Sending heartbeat...")
Expand Down
33 changes: 20 additions & 13 deletions internal/remote/api.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package remote

import (
"fmt"
"strings"
)

Expand All @@ -17,16 +16,21 @@ type AuditEventCreateInput struct {

// AuditEventCommandCreateInput contains create input specific to executor events
type AuditEventCommandCreateInput struct {
PlatformUser string `json:"platformUser"`
Channel string `json:"channel"`
BotPlatform BotPlatform `json:"botPlatform"`
Command string `json:"command"`
PlatformUser string `json:"platformUser"`
Channel string `json:"channel"`
BotPlatform *BotPlatform `json:"botPlatform"`
Command string `json:"command"`
}

// AuditEventSourceCreateInput contains create input specific to source events
type AuditEventSourceCreateInput struct {
Event string `json:"event"`
Bindings []string `json:"bindings"`
Event string `json:"event"`
Source AuditEventSourceDetailsInput `json:"source"`
}

type AuditEventSourceDetailsInput struct {
Name string `json:"name"`
DisplayName string `json:"displayName"`
}

// BotPlatform are the supported bot platforms
Expand All @@ -44,23 +48,26 @@ const (
)

// NewBotPlatform creates new BotPlatform from string
func NewBotPlatform(s string) (BotPlatform, error) {
func NewBotPlatform(s string) *BotPlatform {
var platform BotPlatform
switch strings.ToUpper(s) {
case "SLACK":
fallthrough
case "SOCKETSLACK":
return BotPlatformSlack, nil
platform = BotPlatformSlack
case "DISCORD":
return BotPlatformDiscord, nil
platform = BotPlatformDiscord
case "MATTERMOST":
return BotPlatformMattermost, nil
platform = BotPlatformMattermost
case "TEAMS":
fallthrough
case "MS_TEAMS":
return BotPlatformMsTeams, nil
platform = BotPlatformMsTeams
default:
return "", fmt.Errorf("given BotPlatform %s is not supported", s)
return nil
}

return &platform
}

// AuditEventType is the type of audit events
Expand Down
39 changes: 22 additions & 17 deletions internal/source/dispatcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package source

import (
"context"
"encoding/json"
"fmt"
"time"

Expand Down Expand Up @@ -155,7 +156,7 @@ func (d *Dispatcher) dispatchMsg(ctx context.Context, event source.Event, dispat
}
err := n.SendMessage(ctx, msg, sources)
if err != nil {
reportErr := d.reportError(ctx, err, n, pluginName, event, dispatch.sourceName)
reportErr := d.reportError(err, n, pluginName, event)
if reportErr != nil {
err = multierror.Append(err, fmt.Errorf("while reporting error: %w", reportErr))
}
Expand All @@ -164,7 +165,7 @@ func (d *Dispatcher) dispatchMsg(ctx context.Context, event source.Event, dispat
return
}

reportErr := d.reportSuccess(ctx, n, pluginName, event, dispatch.sourceName)
reportErr := d.reportSuccess(n, pluginName, event)
if reportErr != nil {
d.log.Error(err)
}
Expand All @@ -176,7 +177,7 @@ func (d *Dispatcher) dispatchMsg(ctx context.Context, event source.Event, dispat
defer analytics.ReportPanicIfOccurs(d.log, d.reporter)
err := n.SendEvent(ctx, event.RawObject, sources)
if err != nil {
reportErr := d.reportError(ctx, err, n, pluginName, event, dispatch.sourceName)
reportErr := d.reportError(err, n, pluginName, event)
if reportErr != nil {
err = multierror.Append(err, fmt.Errorf("while reporting error: %w", reportErr))
}
Expand All @@ -185,13 +186,17 @@ func (d *Dispatcher) dispatchMsg(ctx context.Context, event source.Event, dispat
return
}

reportErr := d.reportSuccess(ctx, n, pluginName, event, dispatch.sourceName)
reportErr := d.reportSuccess(n, pluginName, event)
if reportErr != nil {
d.log.Error(err)
}
}(n)
}

if err := d.reportAuditEvent(ctx, pluginName, event.RawObject, dispatch.sourceName, dispatch.sourceDisplayName); err != nil {
d.log.Errorf("while reporting audit event for source %q: %s", dispatch.sourceName, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about d.log.Errorf("while reporting audit event for source %q: %w", dispatch.sourceName, err)? To not lose error details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is logging, so actually we'll still print just error message. When using %w, we still have just an error message, but wrapped in a bit odd format: https://go.dev/play/p/nDVTk9L-VPl

So %s for logging is more readable, and it is used in all over the places 🤔 To get all the stacktrace etc. we'd need to use %v, but that would make our logs hard to read 😞

}

// execute actions
actions, err := d.actionProvider.RenderedActions(event.RawObject, sources)
if err != nil {
Expand Down Expand Up @@ -227,12 +232,20 @@ func (d *Dispatcher) dispatch(ctx context.Context, event []byte, dispatch Plugin
}, dispatch)
}

func (d *Dispatcher) reportAudit(ctx context.Context, pluginName, event, source string) error {
func (d *Dispatcher) reportAuditEvent(ctx context.Context, pluginName string, event any, sourceName, sourceDisplayName string) error {
eventBytes, err := json.Marshal(event)
if err != nil {
return fmt.Errorf("while marshaling audit event: %w", err)
}

e := audit.SourceAuditEvent{
CreatedAt: time.Now().Format(time.RFC3339),
PluginName: pluginName,
Event: event,
Bindings: []string{source},
Event: string(eventBytes),
Source: audit.SourceDetails{
Name: sourceName,
DisplayName: sourceDisplayName,
},
}
return d.auditReporter.ReportSourceAuditEvent(ctx, e)
}
Expand All @@ -242,7 +255,7 @@ type genericNotifier interface {
Type() config.IntegrationType
}

func (d *Dispatcher) reportSuccess(ctx context.Context, n genericNotifier, pluginName string, event source.Event, sourceName string) error {
func (d *Dispatcher) reportSuccess(n genericNotifier, pluginName string, event source.Event) error {
errs := multierror.New()
reportErr := d.reporter.ReportHandledEventSuccess(analytics.ReportEvent{
IntegrationType: n.Type(),
Expand All @@ -253,14 +266,10 @@ func (d *Dispatcher) reportSuccess(ctx context.Context, n genericNotifier, plugi
if reportErr != nil {
errs = multierror.Append(errs, fmt.Errorf("while reporting %s analytics: %w", n.Type(), reportErr))
}
if err := d.reportAudit(ctx, pluginName, fmt.Sprintf("%v", event.RawObject), sourceName); err != nil {
errs = multierror.Append(errs, fmt.Errorf("while reporting %s audit event: %w", n.Type(), reportErr))
}

return errs.ErrorOrNil()
}

func (d *Dispatcher) reportError(ctx context.Context, err error, n genericNotifier, pluginName string, event source.Event, sourceName string) error {
func (d *Dispatcher) reportError(err error, n genericNotifier, pluginName string, event source.Event) error {
errs := multierror.New()
reportErr := d.reporter.ReportHandledEventError(analytics.ReportEvent{
IntegrationType: n.Type(),
Expand All @@ -271,10 +280,6 @@ func (d *Dispatcher) reportError(ctx context.Context, err error, n genericNotifi
if reportErr != nil {
errs = multierror.Append(errs, fmt.Errorf("while reporting %s analytics: %w", n.Type(), reportErr))
}
// TODO: add additional metadata about event failed to send
if err := d.reportAudit(ctx, pluginName, fmt.Sprintf("%v", event.RawObject), sourceName); err != nil {
errs = multierror.Append(errs, fmt.Errorf("while reporting %s audit event: %w", n.Type(), reportErr))
}

return errs.ErrorOrNil()
}
8 changes: 7 additions & 1 deletion internal/source/scheduler.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type PluginDispatch struct {
pluginName string
pluginConfigs []*source.Config
sourceName string
sourceDisplayName string
isInteractivitySupported bool
cfg *config.Config
pluginContext config.PluginContext
Expand Down Expand Up @@ -145,7 +146,11 @@ func (d *Scheduler) schedulePlugin(ctx context.Context, isInteractivitySupported
d.startProcesses[key] = struct{}{}

sourcePluginConfigs := map[string][]*source.Config{}
plugins := d.cfg.Sources[sourceName].Plugins
srcConfig, exists := d.cfg.Sources[sourceName]
if !exists {
return fmt.Errorf("source %q not found", sourceName)
}
plugins := srcConfig.Plugins
var pluginContext config.PluginContext
for pluginName, pluginCfg := range plugins {
if !pluginCfg.Enabled {
Expand All @@ -170,6 +175,7 @@ func (d *Scheduler) schedulePlugin(ctx context.Context, isInteractivitySupported
pluginConfigs: configs,
isInteractivitySupported: isInteractivitySupported,
sourceName: sourceName,
sourceDisplayName: srcConfig.DisplayName,
cfg: d.cfg,
pluginContext: pluginContext,
})
Expand Down
6 changes: 3 additions & 3 deletions internal/status/gql_reporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func newGraphQLStatusReporter(logger logrus.FieldLogger, client GraphQLClient, r
// ReportDeploymentStartup reports deployment startup to GraphQL server.
func (r *GraphQLStatusReporter) ReportDeploymentStartup(ctx context.Context) error {
logger := r.log.WithFields(logrus.Fields{
"deploymentID": r.gql.DeploymentID,
"deploymentID": r.gql.DeploymentID(),
"resourceVersion": r.getResourceVersion(),
"type": "startup",
})
Expand Down Expand Up @@ -73,7 +73,7 @@ func (r *GraphQLStatusReporter) ReportDeploymentStartup(ctx context.Context) err
// ReportDeploymentShutdown reports deployment shutdown to GraphQL server.
func (r *GraphQLStatusReporter) ReportDeploymentShutdown(ctx context.Context) error {
logger := r.log.WithFields(logrus.Fields{
"deploymentID": r.gql.DeploymentID,
"deploymentID": r.gql.DeploymentID(),
"resourceVersion": r.getResourceVersion(),
"type": "shutdown",
})
Expand All @@ -100,7 +100,7 @@ func (r *GraphQLStatusReporter) ReportDeploymentShutdown(ctx context.Context) er
// ReportDeploymentFailed reports deployment failure to GraphQL server.
func (r *GraphQLStatusReporter) ReportDeploymentFailed(ctx context.Context) error {
logger := r.log.WithFields(logrus.Fields{
"deploymentID": r.gql.DeploymentID,
"deploymentID": r.gql.DeploymentID(),
"resourceVersion": r.getResourceVersion(),
"type": "failure",
})
Expand Down
8 changes: 6 additions & 2 deletions pkg/action/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (

const (
// unknownValue defines an unknown string value.
unknownValue = "unknown"
unknownValue = "n/a"
)

// ExecutorFactory facilitates creation of execute.Executor instances.
Expand Down Expand Up @@ -80,6 +80,7 @@ func (p *Provider) RenderedActions(e any, sourceBindings []string) ([]event.Acti

// ExecuteAction executes action for given event.
func (p *Provider) ExecuteAction(ctx context.Context, action event.Action) interactive.CoreMessage {
userName := fmt.Sprintf("Automation %q", action.DisplayName)
e := p.executorFactory.NewDefault(execute.NewDefaultInput{
Conversation: execute.Conversation{
IsAuthenticated: true,
Expand All @@ -92,7 +93,10 @@ func (p *Provider) ExecuteAction(ctx context.Context, action event.Action) inter
Platform: unknownValue,
NotifierHandler: &universalNotifierHandler{},
Message: strings.TrimSpace(strings.TrimPrefix(action.Command, api.MessageBotNamePlaceholder)),
User: fmt.Sprintf("Automation %q", action.DisplayName),
User: execute.UserInput{
Mention: userName,
DisplayName: userName,
},
})
response := e.Execute(ctx)

Expand Down
Loading