diff --git a/internal/config/reloader/remote_test.go b/internal/config/reloader/remote_test.go index c3cd57c83..0d79a65fa 100644 --- a/internal/config/reloader/remote_test.go +++ b/internal/config/reloader/remote_test.go @@ -126,6 +126,7 @@ func fixConfig(actionEnabled bool) config.Config { BotToken: "xoxb-123", Channels: map[string]config.ChannelBindingsByName{ "foo": { + Name: "foo", Notification: config.ChannelNotification{ Disabled: false, }, @@ -174,6 +175,7 @@ func fixConfigStr(actionEnabled bool) string { botToken: xoxb-123 channels: foo: + name: foo notification: disabled: false actions: diff --git a/pkg/bot/cloudslack.go b/pkg/bot/cloudslack.go index 809c8cb38..728873f9a 100644 --- a/pkg/bot/cloudslack.go +++ b/pkg/bot/cloudslack.go @@ -85,13 +85,13 @@ func NewCloudSlack(log logrus.FieldLogger, return nil, err } - channels := slackChannelsConfigFrom(cfg.Channels) + channels := slackChannelsConfigFrom(log, cfg.Channels) if err != nil { return nil, fmt.Errorf("while producing channels configuration map by ID: %w", err) } return &CloudSlack{ - log: log.WithField("integration", config.CloudSlackCommPlatformIntegration), + log: log, cfg: cfg, executorFactory: executorFactory, reporter: reporter, diff --git a/pkg/bot/discord.go b/pkg/bot/discord.go index c8db61be8..89c81717e 100644 --- a/pkg/bot/discord.go +++ b/pkg/bot/discord.go @@ -14,6 +14,7 @@ import ( "github.com/kubeshop/botkube/pkg/api" "github.com/kubeshop/botkube/pkg/bot/interactive" "github.com/kubeshop/botkube/pkg/config" + conversationx "github.com/kubeshop/botkube/pkg/conversation" "github.com/kubeshop/botkube/pkg/execute" "github.com/kubeshop/botkube/pkg/execute/command" "github.com/kubeshop/botkube/pkg/multierror" @@ -67,7 +68,7 @@ func NewDiscord(log logrus.FieldLogger, commGroupName string, cfg config.Discord return nil, fmt.Errorf("while creating Discord session: %w", err) } - channelsCfg, err := discordChannelsConfigFrom(api, cfg.Channels) + channelsCfg, err := discordChannelsConfigFrom(log, api, cfg.Channels) if err != nil { return nil, fmt.Errorf("while creating Discord channels config: %w", err) } @@ -343,9 +344,15 @@ func (b *Discord) formatMessage(msg interactive.CoreMessage) (*discordgo.Message }, nil } -func discordChannelsConfigFrom(api *discordgo.Session, channelsCfg config.IdentifiableMap[config.ChannelBindingsByID]) (map[string]channelConfigByID, error) { +func discordChannelsConfigFrom(log logrus.FieldLogger, api *discordgo.Session, channelsCfg config.IdentifiableMap[config.ChannelBindingsByID]) (map[string]channelConfigByID, error) { res := make(map[string]channelConfigByID) for channAlias, channCfg := range channelsCfg { + normalizedChannelID, changed := conversationx.NormalizeChannelIdentifier(channCfg.ID) + if changed { + log.Warnf("Channel ID %q has been normalized to %q", channCfg.ID, normalizedChannelID) + } + channCfg.ID = normalizedChannelID + channelData, err := api.Channel(channCfg.Identifier()) if err != nil { return nil, fmt.Errorf("while getting channel name for ID %q: %w", channCfg.Identifier(), err) diff --git a/pkg/bot/mattermost.go b/pkg/bot/mattermost.go index 4c45c61c1..cff7456aa 100644 --- a/pkg/bot/mattermost.go +++ b/pkg/bot/mattermost.go @@ -491,6 +491,8 @@ func getBotUserID(client *model.Client4, teamID, botName string) (string, error) func mattermostChannelsCfgFrom(client *model.Client4, teamID string, channelsCfg config.IdentifiableMap[config.ChannelBindingsByName]) (map[string]channelConfigByID, error) { res := make(map[string]channelConfigByID) for channAlias, channCfg := range channelsCfg { + // do not normalize channel as Mattermost allows virtually all characters in channel names + // See https://docs.mattermost.com/channels/channel-naming-conventions.html fetchedChannel, _, err := client.GetChannelByName(channCfg.Identifier(), teamID, "") if err != nil { return nil, fmt.Errorf("while getting channel by name %q: %w", channCfg.Name, err) diff --git a/pkg/bot/slack.go b/pkg/bot/slack.go index 8d04bef25..9469bb8ec 100644 --- a/pkg/bot/slack.go +++ b/pkg/bot/slack.go @@ -73,7 +73,7 @@ func NewSlack(log logrus.FieldLogger, commGroupName string, cfg config.Slack, ex return nil, err } - channels := slackChannelsConfigFrom(cfg.Channels) + channels := slackChannelsConfigFrom(log, cfg.Channels) if err != nil { return nil, fmt.Errorf("while producing channels configuration map by ID: %w", err) } diff --git a/pkg/bot/slack_utils.go b/pkg/bot/slack_utils.go index e3bf00d50..d54281956 100644 --- a/pkg/bot/slack_utils.go +++ b/pkg/bot/slack_utils.go @@ -4,14 +4,23 @@ import ( "fmt" "regexp" + "github.com/sirupsen/logrus" + "github.com/kubeshop/botkube/pkg/config" + conversationx "github.com/kubeshop/botkube/pkg/conversation" ) const slackBotMentionPrefixFmt = "^<@%s>" -func slackChannelsConfigFrom(channelsCfg config.IdentifiableMap[config.ChannelBindingsByName]) map[string]channelConfigByName { +func slackChannelsConfigFrom(log logrus.FieldLogger, channelsCfg config.IdentifiableMap[config.ChannelBindingsByName]) map[string]channelConfigByName { channels := make(map[string]channelConfigByName) for channAlias, channCfg := range channelsCfg { + normalizedChannelName, changed := conversationx.NormalizeChannelIdentifier(channCfg.Name) + if changed { + log.Warnf("Channel name %q has been normalized to %q", channCfg.Name, normalizedChannelName) + } + channCfg.Name = normalizedChannelName + channels[channCfg.Identifier()] = channelConfigByName{ ChannelBindingsByName: channCfg, alias: channAlias, diff --git a/pkg/bot/socketslack.go b/pkg/bot/socketslack.go index f5b7ed939..ee0bf5118 100644 --- a/pkg/bot/socketslack.go +++ b/pkg/bot/socketslack.go @@ -82,7 +82,7 @@ func NewSocketSlack(log logrus.FieldLogger, commGroupName string, cfg config.Soc return nil, err } - channels := slackChannelsConfigFrom(cfg.Channels) + channels := slackChannelsConfigFrom(log, cfg.Channels) if err != nil { return nil, fmt.Errorf("while producing channels configuration map by ID: %w", err) } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 2d36a3620..1e1fe645b 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -104,6 +104,7 @@ func TestLoadedConfigValidationErrors(t *testing.T) { name string expErrMsg string configs [][]byte + isWarning bool }{ { name: "no config files", @@ -218,6 +219,19 @@ func TestLoadedConfigValidationErrors(t *testing.T) { readTestdataFile(t, "sources-rbac.yaml"), }, }, + { + name: "Invalid channel names", + expErrMsg: heredoc.Doc(` + 4 errors occurred: + * Key: 'Config.Communications[default-workspace].SocketSlack.alias2.Name' The channel name 'INCORRECT' seems to be invalid. See the documentation to learn more: https://api.slack.com/methods/conversations.rename#naming. + * Key: 'Config.Communications[default-workspace].CloudSlack.alias2.Name' The channel name 'INCORRECT' seems to be invalid. See the documentation to learn more: https://api.slack.com/methods/conversations.rename#naming. + * Key: 'Config.Communications[default-workspace].Mattermost.alias.Name' The channel name 'too-long name really really really really really really really really really really really really long' seems to be invalid. See the documentation to learn more: https://docs.mattermost.com/channels/channel-naming-conventions.html. + * Key: 'Config.Communications[default-workspace].Discord.alias2.ID' The channel name 'incorrect' seems to be invalid. See the documentation to learn more: https://support.discord.com/hc/en-us/articles/206346498-Where-can-I-find-my-User-Server-Message-ID-.`), + configs: [][]byte{ + readTestdataFile(t, "invalid-channels.yaml"), + }, + isWarning: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -225,6 +239,14 @@ func TestLoadedConfigValidationErrors(t *testing.T) { cfg, details, err := config.LoadWithDefaults(tc.configs) // then + if tc.isWarning { + assert.NoError(t, err) + assert.NotNil(t, cfg) + assert.Error(t, details.ValidateWarnings) + assert.EqualError(t, details.ValidateWarnings, tc.expErrMsg) + return + } + assert.Nil(t, cfg) assert.NoError(t, details.ValidateWarnings) assert.EqualError(t, err, tc.expErrMsg) diff --git a/pkg/config/testdata/TestLoadedConfigValidationErrors/invalid-channels.yaml b/pkg/config/testdata/TestLoadedConfigValidationErrors/invalid-channels.yaml new file mode 100644 index 000000000..a6128303c --- /dev/null +++ b/pkg/config/testdata/TestLoadedConfigValidationErrors/invalid-channels.yaml @@ -0,0 +1,85 @@ +communications: # req 1 elm. + 'default-workspace': + cloudSlack: + enabled: true + channels: + 'alias': + name: '#correct' + bindings: + executors: + - kubectl-read-only + sources: + - k8s-events + 'alias2': + name: 'INCORRECT' + bindings: + executors: + - kubectl-read-only + sources: + - k8s-events + + token: 'TOKEN' + + socketSlack: + enabled: true + channels: + 'alias': + name: '#correct' + bindings: + executors: + - kubectl-read-only + sources: + - k8s-events + 'alias2': + name: 'INCORRECT' + bindings: + executors: + - kubectl-read-only + sources: + - k8s-events + + appToken: 'xapp-SLACK_APP_TOKEN' + botToken: 'xoxb-SLACK_BOT_TOKEN' + + discord: + enabled: true + token: 'DISCORD_TOKEN' + botID: 'DISCORD_BOT_ID' + channels: + 'alias': + id: '976789916447019068' # correct + bindings: + executors: + - kubectl-read-only + sources: + - k8s-events + 'alias2': + id: 'incorrect' + bindings: + executors: + - kubectl-read-only + sources: + - k8s-events + + mattermost: + enabled: true + url: 'MATTERMOST_SERVER_URL' + token: 'MATTERMOST_TOKEN' + team: 'MATTERMOST_TEAM' + channels: + 'alias': + name: "too-long name really really really really really really really really really really really really long" + notification: + disabled: true + bindings: + executors: + - kubectl-read-only + sources: + - k8s-events + notification: + type: short + +executors: + kubectl-read-only: {} +sources: + k8s-events: {} diff --git a/pkg/config/validator.go b/pkg/config/validator.go index e881cd710..d299e5d9a 100644 --- a/pkg/config/validator.go +++ b/pkg/config/validator.go @@ -3,6 +3,7 @@ package config import ( "fmt" "reflect" + "regexp" "strings" "github.com/go-playground/locales/en" @@ -11,6 +12,7 @@ import ( en_translations "github.com/go-playground/validator/v10/translations/en" "github.com/hashicorp/go-multierror" + "github.com/kubeshop/botkube/pkg/conversation" "github.com/kubeshop/botkube/pkg/execute/command" multierrx "github.com/kubeshop/botkube/pkg/multierror" ) @@ -18,6 +20,8 @@ import ( const ( regexConstraintsIncludeTag = "rs-include-regex" invalidBindingTag = "invalid_binding" + invalidChannelNameTag = "invalid_channel_name" + invalidChannelIDTag = "invalid_channel_id" conflictingPluginRepoTag = "conflicting_plugin_repo" conflictingPluginVersionTag = "conflicting_plugin_version" invalidPluginDefinitionTag = "invalid_plugin_definition" @@ -28,9 +32,22 @@ const ( botTokenPrefix = "xoxb-" ) -var warnsOnlyTags = map[string]struct{}{ - regexConstraintsIncludeTag: {}, -} +var ( + warnsOnlyTags = map[string]struct{}{ + regexConstraintsIncludeTag: {}, + invalidChannelNameTag: {}, + invalidChannelIDTag: {}, + } + + slackChannelNameRegex = regexp.MustCompile(`^[a-z0-9-_]{1,79}$`) + + discordChannelIDRegex = regexp.MustCompile(`^[0-9]*$`) + mattermostChannelNameRegex = regexp.MustCompile(`^[\s\S]{1,64}$`) + + slackDocsURL = "https://api.slack.com/methods/conversations.rename#naming" + discordDocsURL = "https://support.discord.com/hc/en-us/articles/206346498-Where-can-I-find-my-User-Server-Message-ID-" + mattermostDocsURL = "https://docs.mattermost.com/channels/channel-naming-conventions.html" +) // ValidateResult holds the validation results. type ValidateResult struct { @@ -67,7 +84,11 @@ func ValidateStruct(in any) (ValidateResult, error) { } validate.RegisterStructValidation(slackStructTokenValidator, Slack{}) - validate.RegisterStructValidation(socketSlackStructTokenValidator, SocketSlack{}) + validate.RegisterStructValidation(socketSlackValidator, SocketSlack{}) + validate.RegisterStructValidation(discordValidator, Discord{}) + validate.RegisterStructValidation(cloudSlackValidator, CloudSlack{}) + validate.RegisterStructValidation(mattermostValidator, Mattermost{}) + validate.RegisterStructValidation(sourceStructValidator, Sources{}) validate.RegisterStructValidation(executorStructValidator, Executors{}) @@ -101,6 +122,7 @@ func ValidateStruct(in any) (ValidateResult, error) { func registerCustomTranslations(validate *validator.Validate, trans ut.Translator) error { return registerTranslation(validate, trans, map[string]string{ "invalid_slack_token": "{0} {1}", + invalidChannelNameTag: "The channel name '{0}' seems to be invalid. See the documentation to learn more: {1}.", }) } @@ -155,7 +177,7 @@ func slackStructTokenValidator(sl validator.StructLevel) { } } -func socketSlackStructTokenValidator(sl validator.StructLevel) { +func socketSlackValidator(sl validator.StructLevel) { slack, ok := sl.Current().Interface().(SocketSlack) if !ok || !slack.Enabled { @@ -179,6 +201,61 @@ func socketSlackStructTokenValidator(sl validator.StructLevel) { msg := fmt.Sprintf("must have the %s prefix. Learn more at https://docs.botkube.io/installation/socketslack/#generate-and-obtain-app-level-token", appTokenPrefix) sl.ReportError(slack.AppToken, "AppToken", "AppToken", "invalid_slack_token", msg) } + + validateChannels(sl, slackChannelNameRegex, true, slack.Channels, "Name", slackDocsURL) +} + +func cloudSlackValidator(sl validator.StructLevel) { + slack, ok := sl.Current().Interface().(CloudSlack) + + if !ok || !slack.Enabled { + return + } + + validateChannels(sl, slackChannelNameRegex, true, slack.Channels, "Name", slackDocsURL) +} + +func discordValidator(sl validator.StructLevel) { + discord, ok := sl.Current().Interface().(Discord) + + if !ok || !discord.Enabled { + return + } + + validateChannels(sl, discordChannelIDRegex, true, discord.Channels, "ID", discordDocsURL) +} + +func mattermostValidator(sl validator.StructLevel) { + mattermost, ok := sl.Current().Interface().(Mattermost) + + if !ok || !mattermost.Enabled { + return + } + + validateChannels(sl, mattermostChannelNameRegex, false, mattermost.Channels, "Name", mattermostDocsURL) +} + +func validateChannels[T Identifiable](sl validator.StructLevel, regex *regexp.Regexp, shouldNormalize bool, channels IdentifiableMap[T], fieldName, docsURL string) { + if len(channels) == 0 { + sl.ReportError(channels, "Channels", "Channels", "required", "") + } + + for channelAlias, channel := range channels { + if channel.Identifier() == "" { + sl.ReportError(channel.Identifier(), fieldName, fieldName, "required", "") + return + } + + channelIdentifier := channel.Identifier() + if shouldNormalize { + channelIdentifier, _ = conversation.NormalizeChannelIdentifier(channel.Identifier()) + } + valid := regex.MatchString(channelIdentifier) + if !valid { + fieldNameWithAliasProp := fmt.Sprintf("%s.%s", channelAlias, fieldName) + sl.ReportError(fieldName, channel.Identifier(), fieldNameWithAliasProp, invalidChannelNameTag, docsURL) + } + } } func regexConstraintsStructValidator(sl validator.StructLevel) { diff --git a/pkg/conversation/normalize.go b/pkg/conversation/normalize.go new file mode 100644 index 000000000..e925f82d9 --- /dev/null +++ b/pkg/conversation/normalize.go @@ -0,0 +1,13 @@ +package conversation + +import "strings" + +// NormalizeChannelIdentifier removes leading and trailing spaces and # from the channel name. +// this is platform-agnostic, as different platforms use different rules: +// Slack - channel name: https://api.slack.com/methods/conversations.rename#naming +// Mattermost - channel name: https://docs.mattermost.com/channels/channel-naming-conventions.html +// Discord - channel ID: https://support.discord.com/hc/en-us/articles/206346498-Where-can-I-find-my-User-Server-Message-ID- +func NormalizeChannelIdentifier(in string) (string, bool) { + out := strings.TrimLeft(strings.TrimSpace(in), "#") + return out, out != in +} diff --git a/pkg/conversation/normalize_test.go b/pkg/conversation/normalize_test.go new file mode 100644 index 000000000..4740b8bda --- /dev/null +++ b/pkg/conversation/normalize_test.go @@ -0,0 +1,68 @@ +package conversation + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNormalizeChannelName(t *testing.T) { + // given + testCases := []struct { + name string + input string + expected string + }{ + { + name: "no hash", + input: "test", + expected: "test", + }, + { + name: "hash", + input: "#test", + expected: "test", + }, + { + name: "hash 2", + input: "###test", + expected: "test", + }, + { + name: "Discord", + input: "#1045026984801091655", + expected: "1045026984801091655", + }, + { + name: "whitespace 1", + input: " test ", + expected: "test", + }, + { + name: "whitespace 2", + input: " #test ", + expected: "test", + }, + { + name: "whitespace between (Mattermost)", + input: " Town Square ", + expected: "Town Square", + }, + { + name: "whitespace between (Mattermost) 2", + input: "Town Square", + expected: "Town Square", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // when + got, changed := NormalizeChannelIdentifier(tc.input) + + // then + assert.Equal(t, tc.expected, got) + assert.Equal(t, tc.expected != tc.input, changed) + }) + } +}