From 1420a166c632485418b25feb953c4901b56ed9be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Kosiec?= Date: Wed, 5 Oct 2022 17:33:32 +0200 Subject: [PATCH 1/5] Make event notifications actionable --- cmd/botkube/main.go | 12 +- helm/botkube/README.md | 2 +- helm/botkube/values.yaml | 2 +- pkg/bot/interactive/command.go | 24 ++++ pkg/bot/interactive/message.go | 23 ++++ pkg/bot/slack.go | 2 +- pkg/bot/slack_renderer.go | 150 ++++++++++++++++++++- pkg/bot/socketslack.go | 97 ++++++++++---- pkg/execute/factory.go | 8 ++ pkg/execute/kubectl/commander.go | 86 ++++++++++++ pkg/execute/kubectl/commander_test.go | 7 + pkg/execute/kubectl/guard.go | 185 ++++++++++++++++++++++++++ pkg/execute/kubectl/guard_test.go | 19 +++ pkg/format/msg.go | 31 ++++- pkg/format/short_msg.go | 5 +- test/e2e/bots_test.go | 5 +- 16 files changed, 615 insertions(+), 43 deletions(-) create mode 100644 pkg/bot/interactive/command.go create mode 100644 pkg/execute/kubectl/commander.go create mode 100644 pkg/execute/kubectl/commander_test.go create mode 100644 pkg/execute/kubectl/guard.go create mode 100644 pkg/execute/kubectl/guard_test.go diff --git a/cmd/botkube/main.go b/cmd/botkube/main.go index 7973ed601..d0114f131 100644 --- a/cmd/botkube/main.go +++ b/cmd/botkube/main.go @@ -18,7 +18,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/discovery" - cacheddiscovery "k8s.io/client-go/discovery/cached" + "k8s.io/client-go/discovery/cached/memory" "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -143,7 +143,10 @@ func run() error { resourceNameNormalizerFunc = resourceNameNormalizer.Normalize } - // Create executor factor + cmdGuard := kubectl.NewCommandGuard(logger.WithField(componentLogFieldKey, "Command Guard"), discoveryCli) + commander := kubectl.NewCommander(logger.WithField(componentLogFieldKey, "Commander"), kcMerger, cmdGuard) + + // Create executor factory cfgManager := config.NewManager(logger.WithField(componentLogFieldKey, "Config manager"), conf.Settings.PersistentConfig, k8sCli) executorFactory := execute.NewExecutorFactory( execute.DefaultExecutorFactoryParams{ @@ -156,6 +159,7 @@ func run() error { CfgManager: cfgManager, AnalyticsReporter: reporter, NamespaceLister: k8sCli.CoreV1().Namespaces(), + CommandGuard: cmdGuard, }, ) @@ -194,7 +198,7 @@ func run() error { } if commGroupCfg.SocketSlack.Enabled { - sb, err := bot.NewSocketSlack(commGroupLogger.WithField(botLogFieldKey, "SocketSlack"), commGroupName, commGroupCfg.SocketSlack, executorFactory, reporter) + sb, err := bot.NewSocketSlack(commGroupLogger.WithField(botLogFieldKey, "SocketSlack"), commGroupName, commGroupCfg.SocketSlack, executorFactory, commander, reporter) if err != nil { return reportFatalError("while creating SocketSlack bot", err) } @@ -393,7 +397,7 @@ func getK8sClients(cfg *rest.Config) (dynamic.Interface, discovery.DiscoveryInte return nil, nil, nil, fmt.Errorf("while creating dynamic K8s client: %w", err) } - discoCacheClient := cacheddiscovery.NewMemCacheClient(discoveryClient) + discoCacheClient := memory.NewMemCacheClient(discoveryClient) mapper := restmapper.NewDeferredDiscoveryRESTMapper(discoCacheClient) return dynamicK8sCli, discoveryClient, mapper, nil } diff --git a/helm/botkube/README.md b/helm/botkube/README.md index 5870a3a83..4771fb9ae 100644 --- a/helm/botkube/README.md +++ b/helm/botkube/README.md @@ -55,7 +55,7 @@ Controller for the Botkube Slack app which helps you monitor your Kubernetes clu | [executors.kubectl-read-only.kubectl.namespaces.exclude](./values.yaml#L254) | list | `[]` | List of ignored Kubernetes Namespace. It can also contain a regex expressions: `- "test-.*"` - to specify all Namespaces. | | [executors.kubectl-read-only.kubectl.enabled](./values.yaml#L256) | bool | `false` | If true, enables `kubectl` commands execution. | | [executors.kubectl-read-only.kubectl.commands.verbs](./values.yaml#L260) | list | `["api-resources","api-versions","cluster-info","describe","explain","get","logs","top"]` | Configures which `kubectl` methods are allowed. | -| [executors.kubectl-read-only.kubectl.commands.resources](./values.yaml#L262) | list | `["deployments","pods","namespaces","daemonsets","statefulsets","storageclasses","nodes","configmaps","services"]` | Configures which K8s resource are allowed. | +| [executors.kubectl-read-only.kubectl.commands.resources](./values.yaml#L262) | list | `["deployments","pods","namespaces","daemonsets","statefulsets","storageclasses","nodes","configmaps","services","ingresses"]` | Configures which K8s resource are allowed. | | [executors.kubectl-read-only.kubectl.defaultNamespace](./values.yaml#L264) | string | `"default"` | Configures the default Namespace for executing Botkube `kubectl` commands. If not set, uses the 'default'. | | [executors.kubectl-read-only.kubectl.restrictAccess](./values.yaml#L266) | bool | `false` | If true, enables commands execution from configured channel only. | | [existingCommunicationsSecretName](./values.yaml#L277) | string | `""` | Configures existing Secret with communication settings. It MUST be in the `botkube` Namespace. To reload Botkube once it changes, add label `botkube.io/config-watch: "true"`. | diff --git a/helm/botkube/values.yaml b/helm/botkube/values.yaml index 9d28d64ba..77fd4c0da 100644 --- a/helm/botkube/values.yaml +++ b/helm/botkube/values.yaml @@ -259,7 +259,7 @@ executors: # -- Configures which `kubectl` methods are allowed. verbs: ["api-resources", "api-versions", "cluster-info", "describe", "explain", "get", "logs", "top"] # -- Configures which K8s resource are allowed. - resources: ["deployments", "pods", "namespaces", "daemonsets", "statefulsets", "storageclasses", "nodes", "configmaps", "services"] + resources: ["deployments", "pods", "namespaces", "daemonsets", "statefulsets", "storageclasses", "nodes", "configmaps", "services", "ingresses"] # -- Configures the default Namespace for executing Botkube `kubectl` commands. If not set, uses the 'default'. defaultNamespace: default # -- If true, enables commands execution from configured channel only. diff --git a/pkg/bot/interactive/command.go b/pkg/bot/interactive/command.go new file mode 100644 index 000000000..9eed6069d --- /dev/null +++ b/pkg/bot/interactive/command.go @@ -0,0 +1,24 @@ +package interactive + +// EventCommandsSection defines a structure of commands for a given event. +func EventCommandsSection(cmdPrefix string, optionItems []OptionItem) Section { + section := Section{ + Selects: Selects{ + ID: "", + Items: []Select{ + { + Name: "Select command...", + Command: cmdPrefix, + OptionGroups: []OptionGroup{ + { + Name: "Supported commands", + Options: optionItems, + }, + }, + }, + }, + }, + } + + return section +} diff --git a/pkg/bot/interactive/message.go b/pkg/bot/interactive/message.go index 53998d7b8..6d83fa2d4 100644 --- a/pkg/bot/interactive/message.go +++ b/pkg/bot/interactive/message.go @@ -77,6 +77,29 @@ type Section struct { Buttons Buttons MultiSelect MultiSelect Selects Selects + TextFields TextFields + Context ContextItems +} + +// ContextItems holds context items. +type ContextItems []ContextItem + +// TextFields holds text field items. +type TextFields []TextField + +// TextField holds a text field data. +type TextField struct { + Text string +} + +// IsDefined returns true if there are any context items defined. +func (c ContextItems) IsDefined() bool { + return len(c) > 0 +} + +// ContextItem holds context item. +type ContextItem struct { + Text string } // Selects holds multiple Select objects. diff --git a/pkg/bot/slack.go b/pkg/bot/slack.go index b89bf3a61..f09b513ee 100644 --- a/pkg/bot/slack.go +++ b/pkg/bot/slack.go @@ -287,7 +287,7 @@ func (b *Slack) send(msg slackMessage, req string, resp interactive.Message, onl // SendEvent sends event notification to slack func (b *Slack) SendEvent(ctx context.Context, event events.Event, eventSources []string) error { b.log.Debugf("Sending to Slack: %+v", event) - attachment := b.renderer.RenderEventMessage(event) + attachment := b.renderer.RenderLegacyEventMessage(event) errs := multierror.New() for _, channelName := range b.getChannelsToNotify(event, eventSources) { diff --git a/pkg/bot/slack_renderer.go b/pkg/bot/slack_renderer.go index a54d86ef8..0f21bba89 100644 --- a/pkg/bot/slack_renderer.go +++ b/pkg/bot/slack_renderer.go @@ -5,6 +5,7 @@ import ( "fmt" "strconv" "strings" + "time" "github.com/slack-go/slack" @@ -19,6 +20,14 @@ const ( cmdButtonActionIDPrefix = "cmd:" ) +var emojiForLevel = map[config.Level]string{ + config.Info: ":large_green_circle:", + config.Warn: ":warning:", + config.Debug: ":information_source:", + config.Error: ":x:", + config.Critical: ":x:", +} + // SlackRenderer provides functionality to render Slack specific messages from a generic models. type SlackRenderer struct { notification config.Notification @@ -29,17 +38,17 @@ func NewSlackRenderer(notificationType config.Notification) *SlackRenderer { return &SlackRenderer{notification: notificationType} } -// RenderEventMessage returns Slack message based on a given event. -func (b *SlackRenderer) RenderEventMessage(event events.Event) slack.Attachment { +// RenderLegacyEventMessage returns Slack message based on a given event. +func (b *SlackRenderer) RenderLegacyEventMessage(event events.Event) slack.Attachment { var attachment slack.Attachment switch b.notification.Type { case config.LongNotification: - attachment = b.longNotification(event) + attachment = b.legacyLongNotification(event) case config.ShortNotification: fallthrough default: - attachment = b.shortNotification(event) + attachment = b.legacyShortNotification(event) } // Add timestamp @@ -51,6 +60,26 @@ func (b *SlackRenderer) RenderEventMessage(event events.Event) slack.Attachment return attachment } +// RenderEventMessage returns Slack interactive message based on a given event. +func (b *SlackRenderer) RenderEventMessage(event events.Event, additionalSections ...interactive.Section) interactive.Message { + var sections []interactive.Section + + switch b.notification.Type { + case config.LongNotification: + sections = append(sections, b.longNotificationSection(event)) + case config.ShortNotification: + fallthrough + default: + sections = append(sections, b.shortNotificationSection(event)) + } + + if len(additionalSections) > 0 { + sections = append(sections, additionalSections...) + } + + return interactive.Message{Sections: sections} +} + // RenderModal returns a modal request view based on a given message. func (b *SlackRenderer) RenderModal(msg interactive.Message) slack.ModalViewRequest { title := msg.Header @@ -174,6 +203,10 @@ func (b *SlackRenderer) renderSection(in interactive.Section) []slack.Block { out = append(out, b.mdTextSection(in.Description)) } + if len(in.TextFields) > 0 { + out = append(out, b.renderTextFields(in.TextFields)) + } + if in.Body.Plaintext != "" { out = append(out, b.mdTextSection(in.Body.Plaintext)) } @@ -192,9 +225,49 @@ func (b *SlackRenderer) renderSection(in interactive.Section) []slack.Block { out = append(out, b.renderSelects(in.Selects)) } + if len(in.Context) > 0 { + out = append(out, b.renderContext(in.Context)...) + } + return out } +func (b *SlackRenderer) renderTextFields(in interactive.TextFields) slack.Block { + var textBlockObjs []*slack.TextBlockObject + for _, item := range in { + if item.Text == "" { + // Skip empty sections + continue + } + + textBlockObjs = append(textBlockObjs, slack.NewTextBlockObject(slack.MarkdownType, item.Text, false, false)) + } + + return slack.NewSectionBlock( + nil, + textBlockObjs, + nil, + ) +} + +func (b *SlackRenderer) renderContext(in []interactive.ContextItem) []slack.Block { + var blocks []slack.Block + + for _, item := range in { + if item.Text == "" { + // Skip empty sections + continue + } + + blocks = append(blocks, slack.NewContextBlock( + "", + slack.NewTextBlockObject(slack.MarkdownType, item.Text, false, false), + )) + } + + return blocks +} + // renderButtons renders button section. // // 1. With description, renders one per row. For example: @@ -297,7 +370,72 @@ func (*SlackRenderer) plainTextBlock(msg string) *slack.TextBlockObject { return slack.NewTextBlockObject(slack.PlainTextType, msg, false, false) } -func (b *SlackRenderer) longNotification(event events.Event) slack.Attachment { +func (b *SlackRenderer) longNotificationSection(event events.Event) interactive.Section { + section := b.baseNotificationSection(event) + section.TextFields = interactive.TextFields{ + {Text: fmt.Sprintf("*Kind:* %s", event.Kind)}, + {Text: fmt.Sprintf("*Name:* %s", event.Name)}, + } + section.TextFields = b.appendTextFieldIfNotEmpty(section.TextFields, "Namespace", event.Namespace) + section.TextFields = b.appendTextFieldIfNotEmpty(section.TextFields, "Reason", event.Reason) + section.TextFields = b.appendTextFieldIfNotEmpty(section.TextFields, "Action", event.Action) + section.TextFields = b.appendTextFieldIfNotEmpty(section.TextFields, "Cluster", event.Cluster) + + // Messages, Recommendations and Warnings formatted as bullet point lists. + section.Body.Plaintext = formatx.BulletPointEventAttachments(event) + + return section +} + +func (b *SlackRenderer) appendTextFieldIfNotEmpty(fields []interactive.TextField, title, in string) []interactive.TextField { + if in == "" { + return fields + } + return append(fields, interactive.TextField{ + Text: fmt.Sprintf("*%s:* %s", title, in), + }) +} + +func (b *SlackRenderer) shortNotificationSection(event events.Event) interactive.Section { + section := b.baseNotificationSection(event) + + header := formatx.ShortNotificationHeader(event) + attachments := formatx.BulletPointEventAttachments(event) + prefix := "" + if attachments != "" { + prefix = "\n" + } + + section.Base.Description = fmt.Sprintf( + "%s\n%s%s", + header, + prefix, + attachments, + ) + + return section +} + +func (b *SlackRenderer) baseNotificationSection(event events.Event) interactive.Section { + emoji := emojiForLevel[event.Level] + section := interactive.Section{ + Base: interactive.Base{ + Header: fmt.Sprintf("%s %s", emoji, event.Title), + }, + } + + if !event.TimeStamp.IsZero() { + fallbackTimestampText := event.TimeStamp.Format(time.RFC1123) + timestampText := fmt.Sprintf("", event.TimeStamp.Unix(), fallbackTimestampText) + section.Context = []interactive.ContextItem{{ + Text: timestampText, + }} + } + + return section +} + +func (b *SlackRenderer) legacyLongNotification(event events.Event) slack.Attachment { attachment := slack.Attachment{ Pretext: fmt.Sprintf("*%s*", event.Title), Fields: []slack.AttachmentField{ @@ -338,7 +476,7 @@ func (b *SlackRenderer) appendIfNotEmpty(fields []slack.AttachmentField, in stri }) } -func (b *SlackRenderer) shortNotification(event events.Event) slack.Attachment { +func (b *SlackRenderer) legacyShortNotification(event events.Event) slack.Attachment { return slack.Attachment{ Title: event.Title, Fields: []slack.AttachmentField{ diff --git a/pkg/bot/socketslack.go b/pkg/bot/socketslack.go index 2322804de..dcb59194d 100644 --- a/pkg/bot/socketslack.go +++ b/pkg/bot/socketslack.go @@ -17,6 +17,7 @@ import ( "github.com/kubeshop/botkube/pkg/config" "github.com/kubeshop/botkube/pkg/events" "github.com/kubeshop/botkube/pkg/execute" + "github.com/kubeshop/botkube/pkg/execute/kubectl" "github.com/kubeshop/botkube/pkg/multierror" "github.com/kubeshop/botkube/pkg/sliceutil" "github.com/kubeshop/botkube/pkg/utils" @@ -29,20 +30,26 @@ import ( var _ Bot = &SocketSlack{} +// EventCommandProvider describes a provider for event commands. +type EventCommandProvider interface { + GetCommandsForEvent(event events.Event, executorBindings []string) ([]kubectl.Command, error) +} + // SocketSlack listens for user's message, execute commands and sends back the response. type SocketSlack struct { - log logrus.FieldLogger - executorFactory ExecutorFactory - reporter socketSlackAnalyticsReporter - botID string - client *slack.Client - channelsMutex sync.RWMutex - channels map[string]channelConfigByName - notifyMutex sync.Mutex - botMentionRegex *regexp.Regexp - commGroupName string - renderer *SlackRenderer - mdFormatter interactive.MDFormatter + log logrus.FieldLogger + executorFactory ExecutorFactory + reporter socketSlackAnalyticsReporter + eventCmdProvider EventCommandProvider + botID string + client *slack.Client + channelsMutex sync.RWMutex + channels map[string]channelConfigByName + notifyMutex sync.Mutex + botMentionRegex *regexp.Regexp + commGroupName string + renderer *SlackRenderer + mdFormatter interactive.MDFormatter } type socketSlackMessage struct { @@ -64,7 +71,7 @@ type socketSlackAnalyticsReporter interface { } // NewSocketSlack creates a new SocketSlack instance. -func NewSocketSlack(log logrus.FieldLogger, commGroupName string, cfg config.SocketSlack, executorFactory ExecutorFactory, reporter socketSlackAnalyticsReporter) (*SocketSlack, error) { +func NewSocketSlack(log logrus.FieldLogger, commGroupName string, cfg config.SocketSlack, executorFactory ExecutorFactory, eventCmdProvider EventCommandProvider, reporter socketSlackAnalyticsReporter) (*SocketSlack, error) { client := slack.New(cfg.BotToken, slack.OptionAppLevelToken(cfg.AppToken)) authResp, err := client.AuthTest() @@ -85,16 +92,17 @@ func NewSocketSlack(log logrus.FieldLogger, commGroupName string, cfg config.Soc mdFormatter := interactive.NewMDFormatter(interactive.NewlineFormatter, mdHeaderFormatter) return &SocketSlack{ - log: log, - executorFactory: executorFactory, - reporter: reporter, - botID: botID, - client: client, - channels: channels, - commGroupName: commGroupName, - renderer: NewSlackRenderer(cfg.Notification), - botMentionRegex: botMentionRegex, - mdFormatter: mdFormatter, + log: log, + executorFactory: executorFactory, + reporter: reporter, + botID: botID, + client: client, + channels: channels, + commGroupName: commGroupName, + eventCmdProvider: eventCmdProvider, + renderer: NewSlackRenderer(cfg.Notification), + botMentionRegex: botMentionRegex, + mdFormatter: mdFormatter, }, nil } @@ -374,11 +382,22 @@ func (b *SocketSlack) send(event socketSlackMessage, req string, resp interactiv // SendEvent sends event notification to slack func (b *SocketSlack) SendEvent(ctx context.Context, event events.Event, eventSources []string) error { b.log.Debugf("Sending to Slack: %+v", event) - attachment := b.renderer.RenderEventMessage(event) errs := multierror.New() for _, channelName := range b.getChannelsToNotify(event, eventSources) { - channelID, timestamp, err := b.client.PostMessageContext(ctx, channelName, slack.MsgOptionAttachments(attachment), slack.MsgOptionAsUser(true)) + additionalSection := b.getInteractiveEventSectionIfShould(event, channelName) + + var additionalSections []interactive.Section + if additionalSection != nil { + additionalSections = append(additionalSections, *additionalSection) + } + msg := b.renderer.RenderEventMessage(event, additionalSections...) + + options := []slack.MsgOption{ + b.renderer.RenderInteractiveMessage(msg), + } + + channelID, timestamp, err := b.client.PostMessageContext(ctx, channelName, options...) if err != nil { errs = multierror.Append(errs, fmt.Errorf("while posting message to channel %q: %w", channelName, err)) continue @@ -390,6 +409,34 @@ func (b *SocketSlack) SendEvent(ctx context.Context, event events.Event, eventSo return errs.ErrorOrNil() } +func (b *SocketSlack) getInteractiveEventSectionIfShould(event events.Event, channelName string) *interactive.Section { + channel, isAuthChannel := b.getChannels()[channelName] + if !isAuthChannel { + return nil + } + + commands, err := b.eventCmdProvider.GetCommandsForEvent(event, channel.Bindings.Executors) + if err != nil { + b.log.Errorf("while getting commands for event: %w", err) + return nil + } + + if len(commands) == 0 { + return nil + } + + cmdPrefix := fmt.Sprintf("%s kubectl", b.BotName()) + var optionItems []interactive.OptionItem + for _, cmd := range commands { + optionItems = append(optionItems, interactive.OptionItem{ + Name: cmd.Name, + Value: cmd.Cmd, + }) + } + section := interactive.EventCommandsSection(cmdPrefix, optionItems) + return §ion +} + func (b *SocketSlack) getChannelsToNotify(event events.Event, eventSources []string) []string { // support custom event routing if event.Channel != "" { diff --git a/pkg/execute/factory.go b/pkg/execute/factory.go index b17db86d1..fcaa696ea 100644 --- a/pkg/execute/factory.go +++ b/pkg/execute/factory.go @@ -38,6 +38,7 @@ type DefaultExecutorFactoryParams struct { CfgManager ConfigPersistenceManager AnalyticsReporter AnalyticsReporter NamespaceLister NamespaceLister + CommandGuard CommandGuard } // Executor is an interface for processes to execute commands @@ -58,6 +59,12 @@ type AnalyticsReporter interface { ReportCommand(platform config.CommPlatformIntegration, command string, isButtonClickOrigin bool) error } +// CommandGuard is an interface that allows to check if a given command is allowed to be executed. +type CommandGuard interface { + GetAllowedResourcesForVerb(verb string, allConfiguredResources []string) ([]kubectl.Resource, error) + GetResourceDetails(verb, resourceType string) (kubectl.Resource, error) +} + // NewExecutorFactory creates new DefaultExecutorFactory. func NewExecutorFactory(params DefaultExecutorFactoryParams) *DefaultExecutorFactory { kcExecutor := NewKubectl( @@ -84,6 +91,7 @@ func NewExecutorFactory(params DefaultExecutorFactoryParams) *DefaultExecutorFac params.Merger, kcExecutor, params.NamespaceLister, + // TODO: Pass params.CommandGuard, ), editExecutor: NewEditExecutor( params.Log.WithField("component", "Notifier Executor"), diff --git a/pkg/execute/kubectl/commander.go b/pkg/execute/kubectl/commander.go new file mode 100644 index 000000000..7c56c092d --- /dev/null +++ b/pkg/execute/kubectl/commander.go @@ -0,0 +1,86 @@ +package kubectl + +import ( + "fmt" + "strings" + + "github.com/sirupsen/logrus" + + "github.com/kubeshop/botkube/pkg/config" + "github.com/kubeshop/botkube/pkg/events" +) + +// Command defines a command that is executed by the app. +type Command struct { + Name string + Cmd string +} + +// Commander is responsible for generating kubectl commands for the given event. +type Commander struct { + log logrus.FieldLogger + merger *Merger + guard *CommandGuard +} + +// NewCommander creates a new Commander instance. +func NewCommander(log logrus.FieldLogger, merger *Merger, guard *CommandGuard) *Commander { + return &Commander{log: log, merger: merger, guard: guard} +} + +// GetCommandsForEvent returns a list of commands for the given event based on the executor bindings. +func (c *Commander) GetCommandsForEvent(event events.Event, executorBindings []string) ([]Command, error) { + if event.Type == config.DeleteEvent { + c.log.Debug("Skipping commands for the DELETE type of event for %q...", event.Kind) + return nil, nil + } + + enabledKubectls := c.merger.MergeForNamespace(executorBindings, event.Namespace) + + resourceTypeParts := strings.Split(event.Resource, "/") + resourceName := resourceTypeParts[len(resourceTypeParts)-1] + + if _, exists := enabledKubectls.AllowedKubectlResource[resourceName]; !exists { + // resource not allowed + return nil, nil + } + + allowedVerbs := enabledKubectls.AllowedKubectlVerb + + resMap, err := c.guard.GetServerResourceMap() + if err != nil { + return nil, err + } + + var commands []Command + for verb := range allowedVerbs { + res, err := c.guard.GetResourceDetailsFromMap(verb, resourceName, resMap) + if err != nil { + if err == ErrVerbNotFound { + c.log.Warnf("Not supported verb %q for resource %q. Skipping...", verb, resourceName) + continue + } + + return nil, fmt.Errorf("while getting resource details: %w", err) + } + + var resourceSubstr string + if res.SlashSeparatedInCommand { + resourceSubstr = fmt.Sprintf("%s/%s", resourceName, event.Name) + } else { + resourceSubstr = fmt.Sprintf("%s %s", resourceName, event.Name) + } + + var namespaceSubstr string + if res.Namespaced { + namespaceSubstr = fmt.Sprintf(" --namespace %s", event.Namespace) + } + + commands = append(commands, Command{ + Name: verb, + Cmd: fmt.Sprintf("%s %s%s", verb, resourceSubstr, namespaceSubstr), + }) + } + + return commands, nil +} diff --git a/pkg/execute/kubectl/commander_test.go b/pkg/execute/kubectl/commander_test.go new file mode 100644 index 000000000..53ba4239c --- /dev/null +++ b/pkg/execute/kubectl/commander_test.go @@ -0,0 +1,7 @@ +package kubectl_test + +import "testing" + +func TestCommander_GetCommandsForEvent(t *testing.T) { + +} diff --git a/pkg/execute/kubectl/guard.go b/pkg/execute/kubectl/guard.go new file mode 100644 index 000000000..ba9c2f168 --- /dev/null +++ b/pkg/execute/kubectl/guard.go @@ -0,0 +1,185 @@ +package kubectl + +import ( + "errors" + "fmt" + + "github.com/sirupsen/logrus" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/discovery" + "k8s.io/utils/strings/slices" +) + +// Resource represents a Kubernetes resource. +type Resource struct { + // Name is always plural, e.g. "pods". + Name string + Namespaced bool + + // SlashSeparatedInCommand indicates if the resource name should be separated with a slash in the command. + // So, instead of `kubectl logs pods ` it should be `kubectl logs pods/`. + SlashSeparatedInCommand bool +} + +// CommandGuard is responsible for getting allowed resources for a given command. +type CommandGuard struct { + log logrus.FieldLogger + discoveryCli discovery.DiscoveryInterface +} + +var ( + // ErrVerbNotFound is returned when the verb is not supported for the resource. + ErrVerbNotFound = errors.New("verb not found") + + // ErrResourceNotFound is returned when the resource is not found on the server. + ErrResourceNotFound = errors.New("resource not found") + + // additionalResourceVerbs contains map of per-resource verbs which are not returned by K8s API, but should be supported. + additionalResourceVerbs = map[string][]string{ + "nodes": {"cordon", "uncordon", "drain"}, + } + + // additionalResourcelessVerbs contains map of per-resource verbs which are not returned by K8s API, but should be supported. + // These verbs are resourceless, so they should use different kubectl syntax with slash separator. + additionalVerbsWithSlash = map[string][]string{ + "pods": {"logs"}, + "jobs": {"logs"}, + "deployments": {"logs"}, + "statefulsets": {"logs"}, + } + + // resourcelessVerbs contains verbs which are not resource-specific. + resourcelessVerbs = map[string]struct{}{ + "auth": {}, + "api-versions": {}, + "api-resources": {}, + "cluster-info": {}, + } + + // unsupportedGlobalVerbs contains verbs returned by K8s API which are not supported for event-related operations. + unsupportedGlobalVerbs = []string{"create", "update", "list", "patch", "watch", "deletecollection"} +) + +// NewCommandGuard creates a new CommandGuard instance. +func NewCommandGuard(log logrus.FieldLogger, discoveryCli discovery.DiscoveryInterface) *CommandGuard { + return &CommandGuard{log: log, discoveryCli: discoveryCli} +} + +// GetAllowedResourcesForVerb returns a list of allowed resources for a given verb. +func (g *CommandGuard) GetAllowedResourcesForVerb(verb string, allConfiguredResources []string) ([]Resource, error) { + _, found := resourcelessVerbs[verb] + if found { + return nil, nil + } + + resMap, err := g.GetServerResourceMap() + if err != nil { + return nil, err + } + + var resources []Resource + for _, configuredRes := range allConfiguredResources { + res, err := g.GetResourceDetailsFromMap(verb, configuredRes, resMap) + if err != nil { + if err == ErrVerbNotFound { + continue + } + + return nil, fmt.Errorf("while getting resource details for %q: %w", configuredRes, err) + } + + resources = append(resources, res) + } + + return resources, nil +} + +// GetResourceDetails returns a Resource struct for a given resource type and verb. +func (g *CommandGuard) GetResourceDetails(selectedVerb, resourceType string) (Resource, error) { + resMap, err := g.GetServerResourceMap() + if err != nil { + return Resource{}, err + } + + res, err := g.GetResourceDetailsFromMap(selectedVerb, resourceType, resMap) + if err != nil { + return Resource{}, err + } + + return res, nil +} + +// GetServerResourceMap returns a map of all resources available on the server. +// LIMITATION: This method ignores second occurrences of the same resource name. +func (g *CommandGuard) GetServerResourceMap() (map[string]v1.APIResource, error) { + resList, err := g.discoveryCli.ServerPreferredResources() + if err != nil { + return nil, fmt.Errorf("while getting server resources: %w", err) + } + + resourceMap := make(map[string]v1.APIResource) + for _, item := range resList { + for _, res := range item.APIResources { + // TODO: Resources should be provided with full group version to avoid collisions in names. + // For example, "pods" and "nodes" are both in "v1" and "metrics.k8s.io/v1beta1". + // Ignoring second occurrence for now. + if _, exists := resourceMap[res.Name]; exists { + g.log.Infof("Skipping resource with the same name %q (%q)...", res.Name, item.GroupVersion) + continue + } + + resourceMap[res.Name] = res + } + } + + return resourceMap, nil +} + +// GetResourceDetailsFromMap returns a Resource struct for a given resource type and verb based on the server resource map. +func (g *CommandGuard) GetResourceDetailsFromMap(selectedVerb, resourceType string, resMap map[string]v1.APIResource) (Resource, error) { + res, exists := resMap[resourceType] + if !exists { + return Resource{}, ErrResourceNotFound + } + + verbs := g.getAllSupportedVerbs(resourceType, res.Verbs) + if slices.Contains(verbs, selectedVerb) { + return Resource{ + Name: res.Name, + Namespaced: res.Namespaced, + SlashSeparatedInCommand: false, + }, nil + } + + addVerbsWithSlash, exists := additionalVerbsWithSlash[resourceType] + if exists && slices.Contains(addVerbsWithSlash, selectedVerb) { + return Resource{ + Name: res.Name, + Namespaced: res.Namespaced, + SlashSeparatedInCommand: true, + }, nil + } + + return Resource{}, ErrVerbNotFound +} + +func (g *CommandGuard) getAllSupportedVerbs(resourceType string, inVerbs v1.Verbs) v1.Verbs { + verbs := inVerbs.DeepCopy() + + // filter out not supported verbs + verbs = slices.Filter(nil, verbs, func(s string) bool { + return !slices.Contains(unsupportedGlobalVerbs, s) + }) + + // enrich with additional verbs + addResVerbs, exists := additionalResourceVerbs[resourceType] + if exists { + verbs = append(verbs, addResVerbs...) + } + + if slices.Contains(verbs, "get") { + verbs = append(verbs, "describe") + } + + return verbs +} diff --git a/pkg/execute/kubectl/guard_test.go b/pkg/execute/kubectl/guard_test.go new file mode 100644 index 000000000..2fd88bb7d --- /dev/null +++ b/pkg/execute/kubectl/guard_test.go @@ -0,0 +1,19 @@ +package kubectl_test + +import "testing" + +func TestCommandGuard_GetAllowedResourcesForVerb(t *testing.T) { + +} + +func TestCommandGuard_GetResourceDetails(t *testing.T) { + +} + +func TestCommandGuard_GetServerResourceMap(t *testing.T) { + +} + +func TestCommandGuard_GetResourceDetailsFromMap(t *testing.T) { + +} diff --git a/pkg/format/msg.go b/pkg/format/msg.go index e55df6bba..8a063f873 100644 --- a/pkg/format/msg.go +++ b/pkg/format/msg.go @@ -3,18 +3,47 @@ package format import ( "fmt" "strings" + + "github.com/kubeshop/botkube/pkg/events" ) // JoinMessages joins strings in slice with new line characters. It also appends a trailing newline at the end of message. func JoinMessages(msgs []string) string { + return joinMessages(msgs, "") +} + +// BulletPointListFromMessages creates a Markdown bullet-point list from messages. +// See https://api.slack.com/reference/surfaces/formatting#block-formatting +func BulletPointListFromMessages(msgs []string) string { + return joinMessages(msgs, "• ") +} + +func joinMessages(msgs []string, msgPrefix string) string { if len(msgs) == 0 { return "" } var strBuilder strings.Builder for _, m := range msgs { - strBuilder.WriteString(fmt.Sprintf("%s\n", m)) + strBuilder.WriteString(fmt.Sprintf("%s%s\n", msgPrefix, m)) } return strBuilder.String() } + +// BulletPointEventAttachments returns formatted lists of event messages, recommendations and warnings. +func BulletPointEventAttachments(event events.Event) string { + strBuilder := strings.Builder{} + writeStringIfNotEmpty(&strBuilder, "Messages", BulletPointListFromMessages(event.Messages)) + writeStringIfNotEmpty(&strBuilder, "Recommendations", BulletPointListFromMessages(event.Recommendations)) + writeStringIfNotEmpty(&strBuilder, "Warnings", BulletPointListFromMessages(event.Warnings)) + return strBuilder.String() +} + +func writeStringIfNotEmpty(strBuilder *strings.Builder, title, in string) { + if in == "" { + return + } + + strBuilder.WriteString(fmt.Sprintf("*%s:*\n%s", title, in)) +} diff --git a/pkg/format/short_msg.go b/pkg/format/short_msg.go index 6eda612a8..0cd43b4b3 100644 --- a/pkg/format/short_msg.go +++ b/pkg/format/short_msg.go @@ -12,13 +12,14 @@ const bulletPointFmt = "- %s\n" // ShortMessage prepares message in short event format. func ShortMessage(event events.Event) string { - msg := messageHeader(event) + msg := ShortNotificationHeader(event) msgAttachments := messageAttachments(event) return fmt.Sprintf("%s\n%s", msg, msgAttachments) } -func messageHeader(event events.Event) string { +// ShortNotificationHeader returns short header for event notification. +func ShortNotificationHeader(event events.Event) string { resourceName := event.Name if event.Namespace != "" { resourceName = fmt.Sprintf("%s/%s", event.Namespace, event.Name) diff --git a/test/e2e/bots_test.go b/test/e2e/bots_test.go index 31f193093..700e32fc3 100644 --- a/test/e2e/bots_test.go +++ b/test/e2e/bots_test.go @@ -268,6 +268,7 @@ func runBotTest(t *testing.T, - nodes - configmaps - services + - ingresses defaultNamespace: default restrictAccess: false kubectl-wait-cmd: @@ -352,8 +353,8 @@ func runBotTest(t *testing.T, }) t.Run("Get forbidden resource", func(t *testing.T) { - command := "get ingress" - expectedBody := codeBlock(fmt.Sprintf("Sorry, the kubectl command is not authorized to work with 'ingress' resources in the 'default' Namespace on cluster '%s'. Use 'commands list' to see allowed commands.", appCfg.ClusterName)) + command := "get role" + expectedBody := codeBlock(fmt.Sprintf("Sorry, the kubectl command is not authorized to work with 'role' resources in the 'default' Namespace on cluster '%s'. Use 'commands list' to see allowed commands.", appCfg.ClusterName)) expectedMessage := fmt.Sprintf("%s\n%s", cmdHeader(command), expectedBody) botDriver.PostMessageToBot(t, botDriver.Channel().Identifier(), command) From 1019c3f341a69542bfdd8d5fc67d799b95f770de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Kosiec?= Date: Tue, 11 Oct 2022 11:29:42 +0200 Subject: [PATCH 2/5] Add unit tests --- pkg/bot/interactive/command.go | 2 +- pkg/execute/kubectl/commander.go | 29 ++- pkg/execute/kubectl/commander_test.go | 213 +++++++++++++++++++- pkg/execute/kubectl/guard.go | 28 ++- pkg/execute/kubectl/guard_test.go | 278 +++++++++++++++++++++++++- 5 files changed, 531 insertions(+), 19 deletions(-) diff --git a/pkg/bot/interactive/command.go b/pkg/bot/interactive/command.go index 9eed6069d..f79c02142 100644 --- a/pkg/bot/interactive/command.go +++ b/pkg/bot/interactive/command.go @@ -7,7 +7,7 @@ func EventCommandsSection(cmdPrefix string, optionItems []OptionItem) Section { ID: "", Items: []Select{ { - Name: "Select command...", + Name: "Run command...", Command: cmdPrefix, OptionGroups: []OptionGroup{ { diff --git a/pkg/execute/kubectl/commander.go b/pkg/execute/kubectl/commander.go index 7c56c092d..2956ec6d1 100644 --- a/pkg/execute/kubectl/commander.go +++ b/pkg/execute/kubectl/commander.go @@ -2,9 +2,11 @@ package kubectl import ( "fmt" + "sort" "strings" "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kubeshop/botkube/pkg/config" "github.com/kubeshop/botkube/pkg/events" @@ -19,12 +21,23 @@ type Command struct { // Commander is responsible for generating kubectl commands for the given event. type Commander struct { log logrus.FieldLogger - merger *Merger - guard *CommandGuard + merger EnabledKubectlMerger + guard CmdGuard +} + +// EnabledKubectlMerger is responsible for merging enabled kubectl commands for the given namespace. +type EnabledKubectlMerger interface { + MergeForNamespace(includeBindings []string, forNamespace string) EnabledKubectl +} + +// CmdGuard is responsible for guarding kubectl commands. +type CmdGuard interface { + GetServerResourceMap() (map[string]metav1.APIResource, error) + GetResourceDetailsFromMap(selectedVerb, resourceType string, resMap map[string]metav1.APIResource) (Resource, error) } // NewCommander creates a new Commander instance. -func NewCommander(log logrus.FieldLogger, merger *Merger, guard *CommandGuard) *Commander { +func NewCommander(log logrus.FieldLogger, merger EnabledKubectlMerger, guard CmdGuard) *Commander { return &Commander{log: log, merger: merger, guard: guard} } @@ -45,7 +58,11 @@ func (c *Commander) GetCommandsForEvent(event events.Event, executorBindings []s return nil, nil } - allowedVerbs := enabledKubectls.AllowedKubectlVerb + var allowedVerbs []string + for key := range enabledKubectls.AllowedKubectlVerb { + allowedVerbs = append(allowedVerbs, key) + } + sort.Strings(allowedVerbs) resMap, err := c.guard.GetServerResourceMap() if err != nil { @@ -53,10 +70,10 @@ func (c *Commander) GetCommandsForEvent(event events.Event, executorBindings []s } var commands []Command - for verb := range allowedVerbs { + for _, verb := range allowedVerbs { res, err := c.guard.GetResourceDetailsFromMap(verb, resourceName, resMap) if err != nil { - if err == ErrVerbNotFound { + if err == ErrVerbNotSupported { c.log.Warnf("Not supported verb %q for resource %q. Skipping...", verb, resourceName) continue } diff --git a/pkg/execute/kubectl/commander_test.go b/pkg/execute/kubectl/commander_test.go index 53ba4239c..e55b0a505 100644 --- a/pkg/execute/kubectl/commander_test.go +++ b/pkg/execute/kubectl/commander_test.go @@ -1,7 +1,218 @@ package kubectl_test -import "testing" +import ( + "testing" + + logtest "github.com/sirupsen/logrus/hooks/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kubeshop/botkube/pkg/config" + "github.com/kubeshop/botkube/pkg/events" + "github.com/kubeshop/botkube/pkg/execute/kubectl" +) func TestCommander_GetCommandsForEvent(t *testing.T) { + // given + executorBindings := []string{"foo", "bar"} + testCases := []struct { + Name string + Event events.Event + MergedKubectls kubectl.EnabledKubectl + Guard kubectl.CmdGuard + + ExpectedResult []kubectl.Command + ExpectedErrMessage string + }{ + { + Name: "Skip delete event", + Event: events.Event{ + Resource: "apps/v1/deployments", + Name: "foo", + Namespace: "default", + Type: config.DeleteEvent, + }, + Guard: nil, + MergedKubectls: kubectl.EnabledKubectl{}, + ExpectedResult: nil, + ExpectedErrMessage: "", + }, + { + Name: "Resource not allowed", + Event: events.Event{ + Resource: "apps/v1/deployments", + Name: "foo", + Namespace: "default", + Type: config.CreateEvent, + }, + Guard: nil, + MergedKubectls: kubectl.EnabledKubectl{ + AllowedKubectlResource: map[string]struct{}{ + "services": {}, + "pods": {}, + }, + AllowedKubectlVerb: map[string]struct{}{ + "get": {}, + "describe": {}, + }, + }, + ExpectedResult: nil, + ExpectedErrMessage: "", + }, + { + Name: "Namespaced resource", + Event: events.Event{ + Resource: "v1/pods", + Name: "foo", + Namespace: "default", + Type: config.CreateEvent, + }, + Guard: &fakeGuard{ + resMap: map[string]metav1.APIResource{ + "pods": {Name: "pods", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + "nodes": {Name: "nodes", Namespaced: false, Kind: "Node", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}, ShortNames: []string{"no"}}, + }, + verbMap: fixVerbMapForFakeGuard(), + }, + MergedKubectls: kubectl.EnabledKubectl{ + AllowedKubectlResource: map[string]struct{}{ + "services": {}, + "pods": {}, + "deployments": {}, + }, + AllowedKubectlVerb: map[string]struct{}{ + "get": {}, + "describe": {}, + "logs": {}, + }, + }, + ExpectedResult: []kubectl.Command{ + {Name: "describe", Cmd: "describe pods foo --namespace default"}, + {Name: "get", Cmd: "get pods foo --namespace default"}, + {Name: "logs", Cmd: "logs pods/foo --namespace default"}, + }, + ExpectedErrMessage: "", + }, + { + Name: "Cluster-wide resource", + Event: events.Event{ + Resource: "v1/nodes", + Name: "foo", + Type: config.UpdateEvent, + }, + Guard: &fakeGuard{ + resMap: map[string]metav1.APIResource{ + "pods": {Name: "pods", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + "nodes": {Name: "nodes", Namespaced: false, Kind: "Node", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}, ShortNames: []string{"no"}}, + }, + verbMap: fixVerbMapForFakeGuard(), + }, + MergedKubectls: kubectl.EnabledKubectl{ + AllowedKubectlResource: map[string]struct{}{ + "services": {}, + "pods": {}, + "nodes": {}, + "deployments": {}, + }, + AllowedKubectlVerb: map[string]struct{}{ + "get": {}, + "describe": {}, + "logs": {}, + }, + }, + ExpectedResult: []kubectl.Command{ + {Name: "describe", Cmd: "describe nodes foo"}, + {Name: "get", Cmd: "get nodes foo"}, + }, + ExpectedErrMessage: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + logger, _ := logtest.NewNullLogger() + cmder := kubectl.NewCommander(logger, &fakeMerger{res: tc.MergedKubectls}, tc.Guard) + + // when + result, err := cmder.GetCommandsForEvent(tc.Event, executorBindings) + + // then + if tc.ExpectedErrMessage != "" { + require.Error(t, err) + assert.Equal(t, tc.ExpectedErrMessage, err.Error()) + return + } + + require.NoError(t, err) + assert.Equal(t, tc.ExpectedResult, result) + }) + } +} + +type fakeMerger struct { + res kubectl.EnabledKubectl +} + +func (f *fakeMerger) MergeForNamespace(includeBindings []string, forNamespace string) kubectl.EnabledKubectl { + return f.res +} + +type fakeGuard struct { + resMap map[string]metav1.APIResource + verbMap map[string]map[string]kubectl.Resource +} + +func (f *fakeGuard) GetServerResourceMap() (map[string]metav1.APIResource, error) { + return f.resMap, nil +} + +func (f *fakeGuard) GetResourceDetailsFromMap(selectedVerb, resourceType string, resMap map[string]metav1.APIResource) (kubectl.Resource, error) { + resources, ok := f.verbMap[selectedVerb] + if !ok { + return kubectl.Resource{}, kubectl.ErrVerbNotSupported + } + + res, ok := resources[resourceType] + if !ok { + return kubectl.Resource{}, kubectl.ErrVerbNotSupported + } + + return res, nil +} +func fixVerbMapForFakeGuard() map[string]map[string]kubectl.Resource { + return map[string]map[string]kubectl.Resource{ + "get": { + "pods": { + Name: "pods", + SlashSeparatedInCommand: false, + Namespaced: true, + }, + "nodes": { + Name: "nodes", + Namespaced: false, + SlashSeparatedInCommand: false, + }, + }, + "describe": { + "pods": { + Name: "pods", + SlashSeparatedInCommand: false, + Namespaced: true, + }, + "nodes": { + Name: "nodes", + Namespaced: false, + SlashSeparatedInCommand: false, + }, + }, + "logs": { + "pods": { + Name: "pods", + SlashSeparatedInCommand: true, + Namespaced: true, + }, + }, + } } diff --git a/pkg/execute/kubectl/guard.go b/pkg/execute/kubectl/guard.go index ba9c2f168..95c7f8f48 100644 --- a/pkg/execute/kubectl/guard.go +++ b/pkg/execute/kubectl/guard.go @@ -6,7 +6,6 @@ import ( "github.com/sirupsen/logrus" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/discovery" "k8s.io/utils/strings/slices" ) @@ -21,15 +20,20 @@ type Resource struct { SlashSeparatedInCommand bool } +// K8sDiscoveryInterface describes an interface for getting K8s server resources. +type K8sDiscoveryInterface interface { + ServerPreferredResources() ([]*v1.APIResourceList, error) +} + // CommandGuard is responsible for getting allowed resources for a given command. type CommandGuard struct { log logrus.FieldLogger - discoveryCli discovery.DiscoveryInterface + discoveryCli K8sDiscoveryInterface } var ( - // ErrVerbNotFound is returned when the verb is not supported for the resource. - ErrVerbNotFound = errors.New("verb not found") + // ErrVerbNotSupported is returned when the verb is not supported for the resource. + ErrVerbNotSupported = errors.New("verb not supported") // ErrResourceNotFound is returned when the resource is not found on the server. ErrResourceNotFound = errors.New("resource not found") @@ -46,6 +50,7 @@ var ( "jobs": {"logs"}, "deployments": {"logs"}, "statefulsets": {"logs"}, + "replicasets": {"logs"}, } // resourcelessVerbs contains verbs which are not resource-specific. @@ -57,11 +62,14 @@ var ( } // unsupportedGlobalVerbs contains verbs returned by K8s API which are not supported for event-related operations. - unsupportedGlobalVerbs = []string{"create", "update", "list", "patch", "watch", "deletecollection"} + unsupportedGlobalVerbs = []string{ + "create", "update", "patch", // valid kubectl verbs, but not supported by interactive kubectl + events actions + "list", "watch", "deletecollection", // invalid kubectl verbs returned by K8s API + } ) // NewCommandGuard creates a new CommandGuard instance. -func NewCommandGuard(log logrus.FieldLogger, discoveryCli discovery.DiscoveryInterface) *CommandGuard { +func NewCommandGuard(log logrus.FieldLogger, discoveryCli K8sDiscoveryInterface) *CommandGuard { return &CommandGuard{log: log, discoveryCli: discoveryCli} } @@ -81,7 +89,7 @@ func (g *CommandGuard) GetAllowedResourcesForVerb(verb string, allConfiguredReso for _, configuredRes := range allConfiguredResources { res, err := g.GetResourceDetailsFromMap(verb, configuredRes, resMap) if err != nil { - if err == ErrVerbNotFound { + if err == ErrVerbNotSupported { continue } @@ -91,6 +99,10 @@ func (g *CommandGuard) GetAllowedResourcesForVerb(verb string, allConfiguredReso resources = append(resources, res) } + if len(resources) == 0 { + return nil, ErrVerbNotSupported + } + return resources, nil } @@ -160,7 +172,7 @@ func (g *CommandGuard) GetResourceDetailsFromMap(selectedVerb, resourceType stri }, nil } - return Resource{}, ErrVerbNotFound + return Resource{}, ErrVerbNotSupported } func (g *CommandGuard) getAllSupportedVerbs(resourceType string, inVerbs v1.Verbs) v1.Verbs { diff --git a/pkg/execute/kubectl/guard_test.go b/pkg/execute/kubectl/guard_test.go index 2fd88bb7d..45a1c8449 100644 --- a/pkg/execute/kubectl/guard_test.go +++ b/pkg/execute/kubectl/guard_test.go @@ -1,19 +1,291 @@ package kubectl_test -import "testing" +import ( + "errors" + "testing" + + logtest "github.com/sirupsen/logrus/hooks/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kubeshop/botkube/pkg/execute/kubectl" +) func TestCommandGuard_GetAllowedResourcesForVerb(t *testing.T) { + // given + fakeDiscoClient := &fakeDisco{ + list: []*v1.APIResourceList{ + { + GroupVersion: "v1", + APIResources: []v1.APIResource{ + {Name: "pods", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + {Name: "services", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + {Name: "nodes", Namespaced: false, Kind: "Node", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}, ShortNames: []string{"no"}}, + }, + }, + { + GroupVersion: "authentication.k8s.io/v1", + APIResources: []v1.APIResource{ + {Name: "tokenreviews", Namespaced: false, Kind: "TokenReview", Verbs: []string{"create"}}, + }, + }, + }, + } + testCases := []struct { + Name string + SelectedVerb string + AllConfiguredResources []string + FakeDiscoClient *fakeDisco + + ExpectedResult []kubectl.Resource + ExpectedErrMessage string + }{ + { + Name: "Resourceless verb", + SelectedVerb: "api-resources", + AllConfiguredResources: []string{}, + FakeDiscoClient: fakeDiscoClient, + ExpectedResult: nil, + ExpectedErrMessage: "", + }, + { + Name: "Discovery API Error", + SelectedVerb: "get", + AllConfiguredResources: []string{"pods", "services", "nodes"}, + FakeDiscoClient: &fakeDisco{err: errors.New("test")}, + ExpectedResult: nil, + ExpectedErrMessage: "while getting server resources: test", + }, + { + Name: "Verb not supported", + SelectedVerb: "create", + AllConfiguredResources: []string{"pods", "services", "nodes"}, + FakeDiscoClient: fakeDiscoClient, + ExpectedErrMessage: "verb not supported", + }, + { + Name: "Filter out resources that don't support the verb", + SelectedVerb: "get", + AllConfiguredResources: []string{"pods", "services", "nodes", "tokenreviews"}, + FakeDiscoClient: fakeDiscoClient, + ExpectedResult: []kubectl.Resource{ + {Name: "pods", Namespaced: true, SlashSeparatedInCommand: false}, + {Name: "services", Namespaced: true, SlashSeparatedInCommand: false}, + {Name: "nodes", Namespaced: false, SlashSeparatedInCommand: false}, + }, + ExpectedErrMessage: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + logger, _ := logtest.NewNullLogger() + cmdGuard := kubectl.NewCommandGuard(logger, tc.FakeDiscoClient) + + // when + result, err := cmdGuard.GetAllowedResourcesForVerb(tc.SelectedVerb, tc.AllConfiguredResources) + + // then + if tc.ExpectedErrMessage != "" { + require.Error(t, err) + assert.Equal(t, tc.ExpectedErrMessage, err.Error()) + return + } + require.NoError(t, err) + assert.Equal(t, tc.ExpectedResult, result) + }) + } } -func TestCommandGuard_GetResourceDetails(t *testing.T) { +func TestCommandGuard_GetResourceDetails_HappyPath(t *testing.T) { + // given + expectedRes := kubectl.Resource{ + Name: "pods", + Namespaced: true, + SlashSeparatedInCommand: false, + } + fakeDisco := &fakeDisco{ + list: []*v1.APIResourceList{ + {GroupVersion: "v1", APIResources: []v1.APIResource{ + {Name: "pods", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + }}, + }, + } + logger, _ := logtest.NewNullLogger() + cmdGuard := kubectl.NewCommandGuard(logger, fakeDisco) + + // when + + res, err := cmdGuard.GetResourceDetails("get", "pods") + + // then + require.NoError(t, err) + assert.Equal(t, expectedRes, res) } -func TestCommandGuard_GetServerResourceMap(t *testing.T) { +func TestCommandGuard_GetServerResourceMap_HappyPath(t *testing.T) { + // given + expectedResMap := map[string]v1.APIResource{ + "pods": {Name: "pods", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + "nodes": {Name: "nodes", Namespaced: false, Kind: "Node", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}, ShortNames: []string{"no"}}, + "services": {Name: "services", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + "tokenreviews": {Name: "tokenreviews", Namespaced: false, Kind: "TokenReview", Verbs: []string{"create"}}, + } + fakeDisco := &fakeDisco{ + list: []*v1.APIResourceList{ + { + GroupVersion: "v1", + APIResources: []v1.APIResource{ + {Name: "pods", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + {Name: "services", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + {Name: "nodes", Namespaced: false, Kind: "Node", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}, ShortNames: []string{"no"}}, + }, + }, + { + GroupVersion: "authentication.k8s.io/v1", + APIResources: []v1.APIResource{ + {Name: "tokenreviews", Namespaced: false, Kind: "TokenReview", Verbs: []string{"create"}}, + }, + }, + { + GroupVersion: "metrics.k8s.io/v1beta1", + APIResources: []v1.APIResource{ + {Name: "pods", Namespaced: false, Kind: "PodMetrics", Verbs: []string{"get", "list"}}, + }, + }, + }, + } + logger, _ := logtest.NewNullLogger() + + cmdGuard := kubectl.NewCommandGuard(logger, fakeDisco) + + // when + resMap, err := cmdGuard.GetServerResourceMap() + + // then + require.NoError(t, err) + assert.Equal(t, expectedResMap, resMap) } func TestCommandGuard_GetResourceDetailsFromMap(t *testing.T) { + // given + resMap := map[string]v1.APIResource{ + "pods": {Name: "pods", Namespaced: true, Kind: "Pod", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}}, + "nodes": {Name: "nodes", Namespaced: false, Kind: "Node", Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"}, ShortNames: []string{"no"}}, + } + testCases := []struct { + Name string + SelectedVerb string + ResourceType string + ResourceMap map[string]v1.APIResource + + ExpectedResult kubectl.Resource + ExpectedErrMessage string + }{ + { + Name: "Namespaced", + SelectedVerb: "get", + ResourceType: "pods", + ResourceMap: resMap, + ExpectedResult: kubectl.Resource{ + Name: "pods", + Namespaced: true, + SlashSeparatedInCommand: false, + }, + }, + { + Name: "Slash-separated command", + SelectedVerb: "logs", + ResourceType: "pods", + ResourceMap: resMap, + ExpectedResult: kubectl.Resource{ + Name: "pods", + Namespaced: true, + SlashSeparatedInCommand: true, + }, + }, + { + Name: "Cluster-wide", + SelectedVerb: "get", + ResourceType: "nodes", + ResourceMap: resMap, + ExpectedResult: kubectl.Resource{ + Name: "nodes", + Namespaced: false, + SlashSeparatedInCommand: false, + }, + }, + { + Name: "Resource doesn't exist", + SelectedVerb: "get", + ResourceType: "drillbinding", + ResourceMap: resMap, + ExpectedResult: kubectl.Resource{}, + ExpectedErrMessage: "resource not found", + }, + { + Name: "Unsupported verb", + SelectedVerb: "check", + ResourceType: "pods", + ResourceMap: resMap, + ExpectedResult: kubectl.Resource{}, + ExpectedErrMessage: "verb not supported", + }, + { + Name: "Unsupported verb, but was returned by K8s API", + SelectedVerb: "patch", + ResourceType: "pods", + ResourceMap: resMap, + ExpectedResult: kubectl.Resource{}, + ExpectedErrMessage: "verb not supported", + }, + { + Name: "Additional verb", + SelectedVerb: "describe", + ResourceType: "nodes", + ResourceMap: resMap, + ExpectedResult: kubectl.Resource{ + Name: "nodes", + Namespaced: false, + SlashSeparatedInCommand: false, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + logger, _ := logtest.NewNullLogger() + cmdGuard := kubectl.NewCommandGuard(logger, nil) + + // when + result, err := cmdGuard.GetResourceDetailsFromMap(tc.SelectedVerb, tc.ResourceType, tc.ResourceMap) + + // then + if tc.ExpectedErrMessage != "" { + require.Error(t, err) + assert.Equal(t, tc.ExpectedErrMessage, err.Error()) + return + } + + require.NoError(t, err) + assert.Equal(t, tc.ExpectedResult, result) + }) + } +} + +type fakeDisco struct { + list []*v1.APIResourceList + err error +} + +func (f *fakeDisco) ServerPreferredResources() ([]*v1.APIResourceList, error) { + if f.err != nil { + return nil, f.err + } + return f.list, nil } From e44706c7b8c34eb1bf8d80c48411fa8eff314dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Kosiec?= Date: Tue, 11 Oct 2022 14:02:13 +0200 Subject: [PATCH 3/5] Add message on empty command response --- pkg/bot/teams.go | 1 + pkg/execute/executor.go | 14 +++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/bot/teams.go b/pkg/bot/teams.go index 1404a6fd5..638cf2d95 100644 --- a/pkg/bot/teams.go +++ b/pkg/bot/teams.go @@ -580,4 +580,5 @@ var emojiMapping = map[string]string{ ":white_check_mark:": "✅", ":arrows_counterclockwise:": "🔄", ":exclamation:": "❗", + ":eyes:": "👀", } diff --git a/pkg/execute/executor.go b/pkg/execute/executor.go index 6b4c3f8f8..206d2263c 100644 --- a/pkg/execute/executor.go +++ b/pkg/execute/executor.go @@ -30,6 +30,7 @@ const ( filterEnabled = "I have enabled '%s' filter on '%s' cluster." filterDisabled = "Done. I won't run '%s' filter on '%s' cluster." internalErrorMsgFmt = "Sorry, an internal error occurred while executing your command for the '%s' cluster :( See the logs for more details." + emptyResponseMsg = "The command returned empty response :eyes:" // incompleteCmdMsg incomplete command response message incompleteCmdMsg = "You missed to pass options for the command. Please use 'help' to see command options." @@ -145,13 +146,20 @@ func (e *DefaultExecutor) Execute() interactive.Message { cmd = overrideCommand } + msgBody := interactive.Body{ + CodeBlock: msg, + } + if msg == "" { + msgBody = interactive.Body{ + Plaintext: emptyResponseMsg, + } + } + header := fmt.Sprintf("%s on `%s`", cmd, clusterName) return interactive.Message{ Base: interactive.Base{ Description: e.appendByUserOnlyIfNeeded(header), - Body: interactive.Body{ - CodeBlock: msg, - }, + Body: msgBody, }, } } From 425d6fb5800801e9c55e2625a244f6e506e97f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Kosiec?= Date: Tue, 11 Oct 2022 14:15:10 +0200 Subject: [PATCH 4/5] Update empty response message --- pkg/execute/executor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/execute/executor.go b/pkg/execute/executor.go index 206d2263c..c163e6a86 100644 --- a/pkg/execute/executor.go +++ b/pkg/execute/executor.go @@ -30,7 +30,7 @@ const ( filterEnabled = "I have enabled '%s' filter on '%s' cluster." filterDisabled = "Done. I won't run '%s' filter on '%s' cluster." internalErrorMsgFmt = "Sorry, an internal error occurred while executing your command for the '%s' cluster :( See the logs for more details." - emptyResponseMsg = "The command returned empty response :eyes:" + emptyResponseMsg = ".. an empty response :eyes:" // incompleteCmdMsg incomplete command response message incompleteCmdMsg = "You missed to pass options for the command. Please use 'help' to see command options." From 7e42a39a089392326fedd9d30929eda34f0afcbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Kosiec?= Date: Tue, 11 Oct 2022 14:43:57 +0200 Subject: [PATCH 5/5] Change empty response message once again --- pkg/bot/teams.go | 2 +- pkg/execute/executor.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/bot/teams.go b/pkg/bot/teams.go index 638cf2d95..e84d4d711 100644 --- a/pkg/bot/teams.go +++ b/pkg/bot/teams.go @@ -580,5 +580,5 @@ var emojiMapping = map[string]string{ ":white_check_mark:": "✅", ":arrows_counterclockwise:": "🔄", ":exclamation:": "❗", - ":eyes:": "👀", + ":cricket:": "🦗", } diff --git a/pkg/execute/executor.go b/pkg/execute/executor.go index c163e6a86..179d46cfd 100644 --- a/pkg/execute/executor.go +++ b/pkg/execute/executor.go @@ -30,7 +30,7 @@ const ( filterEnabled = "I have enabled '%s' filter on '%s' cluster." filterDisabled = "Done. I won't run '%s' filter on '%s' cluster." internalErrorMsgFmt = "Sorry, an internal error occurred while executing your command for the '%s' cluster :( See the logs for more details." - emptyResponseMsg = ".. an empty response :eyes:" + emptyResponseMsg = ".... empty response _**_ :cricket: :cricket: :cricket:" // incompleteCmdMsg incomplete command response message incompleteCmdMsg = "You missed to pass options for the command. Please use 'help' to see command options."