Only log incorrect k8s buttons definitions (#1413)

* Add unit-test, make error more descriptive

Author: mszostok

CONTRIBUTING.md

@@ -189,7 +189,7 @@ For faster development, you can also build and run Botkube outside K8s cluster.
    > Each time you make a change to the [source](cmd/source) or [executors](cmd/executor) plugins re-run the above command.
 
    > **Note**
-   > To build specific plugin binaries, use `PLUGIN_TARGETS`. For example `PLUGIN_TARGETS="x, kubectl" make build-plugins-single`.
+   > To build specific plugin binaries, use `PLUGIN_TARGETS`. For example `PLUGIN_TARGETS="kubernetes,echo" make build-plugins-single`.
 
 ## Making A Change
 

Makefile

@@ -1,10 +1,13 @@
 .DEFAULT_GOAL := build
-.PHONY: container-image test test-integration-slack test-integration-discord build pre-build publish lint lint-fix go-import-fmt system-check save-images load-and-push-images gen-grpc-resources gen-plugins-index build-plugins build-plugins-single gen-docs-cli gen-plugins-goreleaser
+.PHONY: container-image test test-integration-slack test-integration-discord build pre-build publish lint lint-fix go-import-fmt system-check save-images load-and-push-images gen-grpc-resources gen-plugins-index build-plugins build-plugins-single gen-docs-cli gen-plugins-goreleaser serve-local-plugins
 
 # Show this help.
 help:
 	@awk '/^#/{c=substr($$0,3);next}c&&/^[[:alpha:]][[:alnum:]_-]+:/{print substr($$1,1,index($$1,":")),c}1{c=0}' $(MAKEFILE_LIST) | column -s: -t
 
+serve-local-plugins: ## Serve local plugins
+	go run hack/target/serve-plugins/main.go
+
 lint-fix: go-import-fmt
 	@go mod tidy
 	@go mod verify

internal/source/kubernetes/config/config.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"errors"
 	"fmt"
 	"regexp"
 	"strings"
@@ -38,10 +39,40 @@ type (
 		DisplayName string `yaml:"displayName"`
 	}
 	Trigger struct {
-		Type []string `yaml:"type"`
+		Type []EventType `yaml:"type"`
 	}
 )
 
+func (b *ExtraButtons) NormalizeAndValidate() error {
+	// the multierr pkg is not used as it breaks the final error indent making it hard to read
+	var issues []string
+	if b.Button.DisplayName == "" {
+		issues = append(issues, "displayName cannot be empty")
+	}
+
+	if b.Button.CommandTpl == "" {
+		issues = append(issues, "commandTpl cannot be empty")
+	}
+
+	if b.Trigger.Type == nil {
+		issues = append(issues, "trigger.type cannot be empty")
+	}
+	for idx, t := range b.Trigger.Type {
+		// we can't normalize it on custom 'UnmarshalYAML' because koanf uses map[string]any
+		// that causes it to drop Go struct as custom UnmarshalYAML.
+		t = EventType(strings.ToLower(string(t)))
+		if !t.IsValid() {
+			issues = append(issues, fmt.Sprintf("unknown trigger.type[%q]", t))
+		}
+		b.Trigger.Type[idx] = t
+	}
+
+	if len(issues) > 0 {
+		return errors.New(strings.Join(issues, ", "))
+	}
+	return nil
+}
+
 // Commands contains allowed verbs and resources
 type Commands struct {
 	Verbs     []string `yaml:"verbs"`
@@ -179,8 +210,24 @@ const (
 	AllEvent EventType = "all"
 )
 
-func (eventType EventType) String() string {
-	return string(eventType)
+// IsValid checks if the event type is valid.
+func (eventType *EventType) IsValid() bool {
+	if eventType == nil {
+		return false
+	}
+	switch *eventType {
+	case CreateEvent, UpdateEvent, DeleteEvent, ErrorEvent, WarningEvent, NormalEvent, InfoEvent, AllEvent:
+		return true
+	}
+	return false
+}
+
+// String returns the string representation of the event type.
+func (eventType *EventType) String() string {
+	if eventType == nil {
+		return ""
+	}
+	return string(*eventType)
 }
 
 // Resource contains resources to watch

internal/source/kubernetes/filterengine/filterengine.go

@@ -69,7 +69,7 @@ func (f *DefaultFilterEngine) Run(ctx context.Context, event event.Event) event.
 // Register filter(s) to engine.
 func (f *DefaultFilterEngine) Register(filters ...RegisteredFilter) {
 	for _, filter := range filters {
-		f.log.Infof("Registering filter %q (enabled: %t)...", filter.Name(), filter.Enabled)
+		f.log.Debugf("Registering filter %q (enabled: %t)...", filter.Name(), filter.Enabled)
 		f.filters[filter.Name()] = filter
 	}
 }

internal/source/kubernetes/msg.go

@@ -7,12 +7,13 @@ import (
 
 	sprig "github.com/go-task/slim-sprig"
 	"github.com/sirupsen/logrus"
-	"k8s.io/kubectl/pkg/util/slice"
+	"golang.org/x/exp/slices"
 
 	"github.com/kubeshop/botkube/internal/source/kubernetes/commander"
 	"github.com/kubeshop/botkube/internal/source/kubernetes/config"
 	"github.com/kubeshop/botkube/internal/source/kubernetes/event"
 	"github.com/kubeshop/botkube/pkg/api"
+	multierrx "github.com/kubeshop/botkube/pkg/multierror"
 )
 
 var emojiForLevel = map[config.Level]string{
@@ -53,12 +54,12 @@ func (m *MessageBuilder) FromEvent(event event.Event, actions []config.ExtraButt
 
 	cmdSection, err := m.getCommandSelectIfShould(event)
 	if err != nil {
-		return api.Message{}, err
+		m.log.Errorf("Failed to get commands buttons assigned to %q event. Those buttons will be omitted. Issues:\n%s", event.Type.String(), err)
 	}
 
-	btns, err := m.getExternalActions(actions, event)
+	btns, err := m.getExtraButtonsAssignedToEvent(actions, event)
 	if err != nil {
-		return api.Message{}, err
+		m.log.Errorf("Failed to convert extra buttons assigned to %q event. Those buttons will be omitted. Issues:\n%s", event.Type.String(), err)
 	}
 	if cmdSection != nil || len(btns) > 0 {
 		msg.Sections = append(msg.Sections, api.Section{
@@ -70,24 +71,33 @@ func (m *MessageBuilder) FromEvent(event event.Event, actions []config.ExtraButt
 	return msg, nil
 }
 
-func (m *MessageBuilder) getExternalActions(actions []config.ExtraButtons, e event.Event) (api.Buttons, error) {
+func (m *MessageBuilder) getExtraButtonsAssignedToEvent(actions []config.ExtraButtons, e event.Event) (api.Buttons, error) {
 	var actBtns api.Buttons
-	for _, act := range actions {
+	issues := multierrx.New()
+	for idx, act := range actions {
 		if !act.Enabled {
 			continue
 		}
-		if !slice.ContainsString(act.Trigger.Type, e.Type.String(), nil) {
+
+		err := act.NormalizeAndValidate()
+		if err != nil {
+			issues = multierrx.Append(issues, fmt.Errorf("invalid extraButtons[%d]: %s", idx, err))
+			continue
+		}
+
+		if !slices.Contains(act.Trigger.Type, e.Type) {
 			continue
 		}
 
 		btn, err := m.renderActionButton(act, e)
 		if err != nil {
-			return nil, err
+			issues = multierrx.Append(issues, fmt.Errorf("invalid extraButtons[%d].commandTpl: %s", idx, err))
+			continue
 		}
 		actBtns = append(actBtns, btn)
 	}
 
-	return actBtns, nil
+	return actBtns, issues.ErrorOrNil()
 }
 
 func ptrSection(s *api.Selects) api.Selects {
@@ -175,7 +185,7 @@ func (m *MessageBuilder) appendBulletListIfNotEmpty(bulletLists api.BulletLists,
 }
 
 func (m *MessageBuilder) renderActionButton(act config.ExtraButtons, e event.Event) (api.Button, error) {
-	tmpl, err := template.New("example").Funcs(sprig.FuncMap()).Parse(act.Button.CommandTpl)
+	tmpl, err := template.New(act.Button.DisplayName).Funcs(sprig.FuncMap()).Parse(act.Button.CommandTpl)
 	if err != nil {
 		return api.Button{}, err
 	}

internal/source/kubernetes/msg_test.go

@@ -0,0 +1,81 @@
+package kubernetes
+
+import (
+	"testing"
+
+	"github.com/MakeNowJust/heredoc"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/kubeshop/botkube/internal/source/kubernetes/config"
+	"github.com/kubeshop/botkube/internal/source/kubernetes/event"
+)
+
+func TestGetExtraButtonsAssignedToEvent(t *testing.T) {
+	// given
+	builder := MessageBuilder{}
+	givenButtons := []config.ExtraButtons{
+		{
+			// This is fully valid
+			Enabled: true,
+			Trigger: config.Trigger{
+				Type: []config.EventType{"error"},
+			},
+			Button: config.Button{
+				DisplayName: "Ask AI",
+				CommandTpl:  "ai --resource={{ .Namespace }}/{{ .Kind | lower }}/{{ .Name }} --error={{ .Reason }} --bk-cmd-header='AI assistance'",
+			},
+		},
+		{
+			// This is valid, as the 'ERROR' type should be normalized to "error"
+			Enabled: true,
+			Trigger: config.Trigger{
+				Type: []config.EventType{"ERROR"},
+			},
+			Button: config.Button{
+				DisplayName: "Get",
+				CommandTpl:  "kubectl get {{ .Kind | lower }}",
+			},
+		},
+		{
+			// This is invalid, as we can't render `.Event`
+			Enabled: true,
+			Trigger: config.Trigger{
+				Type: []config.EventType{"error"},
+			},
+			Button: config.Button{
+				DisplayName: "Ask AI v2",
+				CommandTpl:  "ai {{.Event.Namespace}} this one is wrong",
+			},
+		},
+		{
+			// This is invalid, as the DisplayName and Trigger is not set
+			Enabled: true,
+			Trigger: config.Trigger{},
+			Button: config.Button{
+				CommandTpl: "ai {{.Event.Namespace}} this one is wrong",
+			},
+		},
+		{
+			// This is invalid, but should be ignored as it's disabled
+			Enabled: false,
+			Trigger: config.Trigger{},
+			Button: config.Button{
+				CommandTpl: "ai {{.Event.Namespace}} this one is wrong",
+			},
+		},
+	}
+
+	// when
+	gotBtns, err := builder.getExtraButtonsAssignedToEvent(givenButtons, event.Event{Type: "error"})
+	assert.EqualError(t, err, heredoc.Doc(`
+        2 errors occurred:
+        	* invalid extraButtons[2].commandTpl: template: Ask AI v2:1:11: executing "Ask AI v2" at <.Event.Namespace>: can't evaluate field Event in type event.Event
+        	* invalid extraButtons[3]: displayName cannot be empty, trigger.type cannot be empty`))
+
+	// then
+	require.Len(t, gotBtns, 2)
+	for idx, btn := range gotBtns {
+		assert.Equal(t, givenButtons[idx].Button.DisplayName, btn.Name)
+	}
+}

internal/source/kubernetes/source.go

@@ -84,13 +84,19 @@ func (*Source) Stream(ctx context.Context, input source.StreamInput) (source.Str
 	if err != nil {
 		return source.StreamOutput{}, fmt.Errorf("while merging input configs: %w", err)
 	}
+
+	// In Kubernetes, we have an "info" level by default. We should aim to minimize info logging and consider using
+	// the debug level instead. This approach will prevent flooding the Agent logs with irrelevant information,
+	// as the Agent logs everything that plugin writes to stderr.
+	log := loggerx.NewStderr(pkgConfig.Logger{
+		Level: cfg.Log.Level,
+	})
+
 	s := Source{
-		startTime: time.Now(),
-		eventCh:   make(chan source.Event),
-		config:    cfg,
-		logger: loggerx.New(pkgConfig.Logger{
-			Level: cfg.Log.Level,
-		}),
+		startTime:                time.Now(),
+		eventCh:                  make(chan source.Event),
+		config:                   cfg,
+		logger:                   log,
 		clusterName:              input.Context.ClusterName,
 		kubeConfig:               input.Context.KubeConfig,
 		isInteractivitySupported: input.Context.IsInteractivitySupported,
@@ -135,7 +141,7 @@ func consumeEvents(ctx context.Context, s Source) {
 	}, func(resource string) (cache.SharedIndexInformer, error) {
 		gvr, err := parseResourceArg(resource, client.mapper)
 		if err != nil {
-			s.logger.Infof("Unable to parse resource: %s to register with informer\n", resource)
+			s.logger.Errorf("Unable to parse resource: %s to register with informer\n", resource)
 			return nil, err
 		}
 		return dynamicKubeInformerFactory.ForResource(gvr).Informer(), nil

pkg/config/config.go

@@ -620,8 +620,8 @@ const (
 	// FormatterText text formatter for logging
 	FormatterText Formatter = "text"
 
-	// FormatterJson json formatter for logging
-	FormatterJson Formatter = "json"
+	// FormatterJSON json formatter for logging
+	FormatterJSON Formatter = "json"
 )
 
 // Logger holds logger configuration parameters.

pkg/loggerx/logger.go

@@ -1,6 +1,7 @@
 package loggerx
 
 import (
+	"io"
 	"os"
 
 	"github.com/sirupsen/logrus"
@@ -9,10 +10,24 @@ import (
 )
 
 // New returns a new logger based on a given configuration.
+// It logs to stdout by default. It's a helper function to maintain backward compatibility.
 func New(cfg config.Logger) logrus.FieldLogger {
+	return NewStdout(cfg)
+}
+
+// NewStderr returns a new logger based on a given configuration. It logs to stderr.
+func NewStderr(cfg config.Logger) logrus.FieldLogger {
+	return newWithOutput(cfg, os.Stderr)
+}
+
+// NewStdout returns a new logger based on a given configuration. It logs to stdout.
+func NewStdout(cfg config.Logger) logrus.FieldLogger {
+	return newWithOutput(cfg, os.Stdout)
+}
+
+func newWithOutput(cfg config.Logger, output io.Writer) logrus.FieldLogger {
 	logger := logrus.New()
-	// Output to stdout instead of the default stderr
-	logger.SetOutput(os.Stdout)
+	logger.SetOutput(output)
 
 	// Only logger the warning severity or above.
 	logLevel, err := logrus.ParseLevel(cfg.Level)
@@ -21,7 +36,7 @@ func New(cfg config.Logger) logrus.FieldLogger {
 		logLevel = logrus.InfoLevel
 	}
 	logger.SetLevel(logLevel)
-	if cfg.Formatter == config.FormatterJson {
+	if cfg.Formatter == config.FormatterJSON {
 		logger.Formatter = &logrus.JSONFormatter{}
 	} else {
 		logger.Formatter = &logrus.TextFormatter{FullTimestamp: true, DisableColors: cfg.DisableColors, ForceColors: true}