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 merging Kubernetes configuration #1253

Merged
merged 1 commit into from
Sep 13, 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
38 changes: 19 additions & 19 deletions internal/source/kubernetes/registration.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,19 +141,19 @@ func (r registration) matchEvent(routes []route, event event.Event) (bool, error
errs := multierror.New()
for _, rt := range routes {
// event reason
if rt.event != nil && rt.event.Reason.AreConstraintsDefined() {
match, err := rt.event.Reason.IsAllowed(event.Reason)
if rt.Event != nil && rt.Event.Reason.AreConstraintsDefined() {
match, err := rt.Event.Reason.IsAllowed(event.Reason)
if err != nil {
return false, err
}
if !match {
r.log.Debugf("Ignoring as reason %q doesn't match constraints %+v", event.Reason, rt.event.Reason)
r.log.Debugf("Ignoring as reason %q doesn't match constraints %+v", event.Reason, rt.Event.Reason)
return false, nil
}
}

// event message
if rt.event != nil && rt.event.Message.AreConstraintsDefined() {
if rt.Event != nil && rt.Event.Message.AreConstraintsDefined() {
var anyMsgMatches bool

eventMsgs := event.Messages
Expand All @@ -163,7 +163,7 @@ func (r registration) matchEvent(routes []route, event event.Event) (bool, error
}

for _, msg := range eventMsgs {
match, err := rt.event.Message.IsAllowed(msg)
match, err := rt.Event.Message.IsAllowed(msg)
if err != nil {
return false, err
}
Expand All @@ -173,42 +173,42 @@ func (r registration) matchEvent(routes []route, event event.Event) (bool, error
}
}
if !anyMsgMatches {
r.log.Debugf("Ignoring as any event message from %q doesn't match constraints %+v", strings.Join(event.Messages, ";"), rt.event.Message)
r.log.Debugf("Ignoring as any event message from %q doesn't match constraints %+v", strings.Join(event.Messages, ";"), rt.Event.Message)
return false, nil
}
}

// resource name
if rt.resourceName.AreConstraintsDefined() {
allowed, err := rt.resourceName.IsAllowed(event.Name)
if rt.ResourceName.AreConstraintsDefined() {
allowed, err := rt.ResourceName.IsAllowed(event.Name)
if err != nil {
return false, err
}
if !allowed {
r.log.Debugf("Ignoring as resource name %q doesn't match constraints %+v", event.Name, rt.resourceName)
r.log.Debugf("Ignoring as resource name %q doesn't match constraints %+v", event.Name, rt.ResourceName)
return false, nil
}
}

// namespace
if rt.namespaces != nil && rt.namespaces.AreConstraintsDefined() {
match, err := rt.namespaces.IsAllowed(event.Namespace)
if rt.Namespaces != nil && rt.Namespaces.AreConstraintsDefined() {
match, err := rt.Namespaces.IsAllowed(event.Namespace)
if err != nil {
return false, err
}
if !match {
r.log.Debugf("Ignoring as namespace %q doesn't match constraints %+v", event.Namespace, rt.namespaces)
r.log.Debugf("Ignoring as namespace %q doesn't match constraints %+v", event.Namespace, rt.Namespaces)
return false, nil
}
}

// annotations
if !kvsSatisfiedForMap(rt.annotations, event.ObjectMeta.Annotations) {
if !kvsSatisfiedForMap(rt.Annotations, event.ObjectMeta.Annotations) {
continue
}

// labels
if !kvsSatisfiedForMap(rt.labels, event.ObjectMeta.Labels) {
if !kvsSatisfiedForMap(rt.Labels, event.ObjectMeta.Labels) {
continue
}
return true, nil
Expand Down Expand Up @@ -309,26 +309,26 @@ func (r registration) qualifyEventForUpdate(
continue
}

if route.updateSetting == nil {
if route.UpdateSetting == nil {
// in theory this should never happen, as we check if it is not nil in `route.hasActionableUpdateSetting()`, but just in case
r.log.Debugf("Nil updateSetting but hasActionableUpdateSetting returned true for route: %v. This looks like a bug...", route)
continue
}

r.log.WithFields(logrus.Fields{"old": oldUnstruct.Object, "new": newUnstruct.Object}).Debug("Getting diff for objects...")
diff, err := k8sutil.Diff(oldUnstruct.Object, newUnstruct.Object, *route.updateSetting)
diff, err := k8sutil.Diff(oldUnstruct.Object, newUnstruct.Object, *route.UpdateSetting)
if err != nil {
r.log.Errorf("while getting diff: %w", err)
}
r.log.Debugf("About to qualify event for route: %v for update, diff: %s, updateSetting: %+v", route, diff, route.updateSetting)
r.log.Debugf("About to qualify event for route: %v for update, diff: %s, updateSetting: %+v", route, diff, route.UpdateSetting)

if route.updateSetting.IncludeDiff {
if route.UpdateSetting.IncludeDiff {
diffs = append(diffs, diff)
}

if len(diff) > 0 {
result = true
r.log.Debugf("Qualified for update: route: %v for update, diff: %s, updateSetting: %+v", route, diff, route.updateSetting)
r.log.Debugf("Qualified for update: route: %v for update, diff: %s, updateSetting: %+v", route, diff, route.UpdateSetting)
}
}

Expand Down
51 changes: 26 additions & 25 deletions internal/source/kubernetes/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,21 @@ type registrationHandler func(resource string) (cache.SharedIndexInformer, error
type eventHandler func(ctx context.Context, source Source, event event.Event, updateDiffs []string)

type route struct {
resourceName config.RegexConstraints
labels *map[string]string
annotations *map[string]string
namespaces *config.RegexConstraints
updateSetting *config.UpdateSetting
event *config.KubernetesEvent
ResourceName config.RegexConstraints
Labels *map[string]string
Annotations *map[string]string
Namespaces *config.RegexConstraints
UpdateSetting *config.UpdateSetting
Event *config.KubernetesEvent
}

func (r route) hasActionableUpdateSetting() bool {
return r.updateSetting != nil && len(r.updateSetting.Fields) > 0
return r.UpdateSetting != nil && len(r.UpdateSetting.Fields) > 0
}

type entry struct {
event config.EventType
routes []route
Event config.EventType
Routes []route
}

// Router maintains handled event types from registered
Expand Down Expand Up @@ -66,7 +66,7 @@ func (r *Router) BuildTable(cfg *config.Config) *Router {
for resource, resourceEvents := range mergedEvents {
eventRoutes := r.mergeEventRoutes(resource, cfg)
for evt := range resourceEvents {
r.table[resource] = append(r.table[resource], entry{event: evt, routes: eventRoutes[evt]})
r.table[resource] = append(r.table[resource], entry{Event: evt, Routes: eventRoutes[evt]})
}
}
r.log.Debugf("routing table: %+v", r.table)
Expand Down Expand Up @@ -169,25 +169,26 @@ func mergeResourceEvents(cfg *config.Config) mergedEvents {

func (r *Router) mergeEventRoutes(resource string, cfg *config.Config) map[config.EventType][]route {
out := make(map[config.EventType][]route)
for _, r := range cfg.Resources {
for idx := range cfg.Resources {
r := cfg.Resources[idx] // make sure that we work on a copy

Choose a reason for hiding this comment

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

🤯

for _, e := range flattenEventTypes(cfg.Event.Types, r.Event.Types) {
if resource != r.Type {
continue
}

route := route{
namespaces: resourceNamespaces(cfg.Namespaces, &r.Namespaces),
annotations: resourceStringMap(cfg.Annotations, r.Annotations),
labels: resourceStringMap(cfg.Labels, r.Labels),
resourceName: r.Name,
event: resourceEvent(*cfg.Event, r.Event),
Namespaces: resourceNamespaces(cfg.Namespaces, &r.Namespaces),
Annotations: resourceStringMap(cfg.Annotations, r.Annotations),
Labels: resourceStringMap(cfg.Labels, r.Labels),
ResourceName: r.Name,
Event: resourceEvent(*cfg.Event, r.Event),
}
if e == config.UpdateEvent {
route.updateSetting = &config.UpdateSetting{
route.UpdateSetting = &config.UpdateSetting{
Fields: r.UpdateSetting.Fields,
IncludeDiff: r.UpdateSetting.IncludeDiff,
}
}

out[e] = append(out[e], route)
}
}
Expand All @@ -211,8 +212,8 @@ func (r *Router) setEventRouteForRecommendationsIfShould(routeMap *map[config.Ev
}

recommRoute := route{
namespaces: cfg.Namespaces,
event: &config.KubernetesEvent{
Namespaces: cfg.Namespaces,
Event: &config.KubernetesEvent{
Reason: config.RegexConstraints{},
Message: config.RegexConstraints{},
Types: nil,
Expand All @@ -222,7 +223,7 @@ func (r *Router) setEventRouteForRecommendationsIfShould(routeMap *map[config.Ev
// Override route and get all these events for all namespaces.
// The events without recommendations will be filtered out when sending the event.
for i, r := range (*routeMap)[eventType] {
recommRoute.namespaces = resourceNamespaces(cfg.Namespaces, r.namespaces)
recommRoute.Namespaces = resourceNamespaces(cfg.Namespaces, r.Namespaces)
(*routeMap)[eventType][i] = recommRoute
return
}
Expand All @@ -234,8 +235,8 @@ func (r *Router) setEventRouteForRecommendationsIfShould(routeMap *map[config.Ev
func eventRoutes(routeTable map[string][]entry, targetResource string, targetEvent config.EventType) []route {
var out []route
for _, routedEvent := range routeTable[targetResource] {
if routedEvent.event == targetEvent {
out = append(out, routedEvent.routes...)
if routedEvent.Event == targetEvent {
out = append(out, routedEvent.Routes...)
}
}
return out
Expand All @@ -244,7 +245,7 @@ func eventRoutes(routeTable map[string][]entry, targetResource string, targetEve
func (r *Router) resourceEvents(resource string) []config.EventType {
var out []config.EventType
for _, routedEvent := range r.table[resource] {
out = append(out, routedEvent.event)
out = append(out, routedEvent.Event)
}
return out
}
Expand All @@ -254,7 +255,7 @@ func (r *Router) resourcesForEvents(targets []config.EventType) []string {
for _, target := range targets {
for resource, routedEvents := range r.table {
for _, routedEvent := range routedEvents {
if routedEvent.event == target {
if routedEvent.Event == target {
out = append(out, resource)
break
}
Expand Down
30 changes: 30 additions & 0 deletions internal/source/kubernetes/router_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
package kubernetes

import (
"fmt"
"os"
"path/filepath"
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gopkg.in/yaml.v3"
"gotest.tools/v3/golden"

"github.com/kubeshop/botkube/internal/loggerx"
"github.com/kubeshop/botkube/internal/source/kubernetes/config"
Expand Down Expand Up @@ -61,3 +68,26 @@ func TestRouter_BuildTable_CreatesRoutesWithProperEventsList(t *testing.T) {
})
}
}

func TestRouterListMergingNestedFields(t *testing.T) {
// given
router := NewRouter(nil, nil, loggerx.NewNoop())

var cfg config.Config
fixConfig, err := os.ReadFile(filepath.Join("testdata", t.Name(), "override-fields-config.yaml"))
require.NoError(t, err)

err = yaml.Unmarshal(fixConfig, &cfg)
require.NoError(t, err)

// when
router = router.BuildTable(&cfg)

// then
for key := range router.table {
out, err := yaml.Marshal(router.table[key])
require.NoError(t, err)
filename := fmt.Sprintf("route-%s.golden.yaml", strings.ReplaceAll(key, "/", "."))
golden.Assert(t, string(out), filepath.Join(t.Name(), filename))
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
event:
types:
- delete
reason:
include: ["reason-include-1-level"]
exclude: ["reason-exclude-1-level"]
message:
include: ["message-include-1-level"]
exclude: ["message-exclude-1-level"]

annotations:
test: "annotation-1-level"

labels:
test: "label-1-level"

namespaces:
include: ["namespace-include-1-level"]
exclude: ["namespace-exclude-1-level"]

resources:
- type: v1/configmaps # change all nested properties
namespaces:
include: ["namespace-include-2-level"]
exclude: ["namespace-exclude-2-level"]
event:
types:
- create
reason:
include: ["reason-include-2-level"]
exclude: ["reason-exclude-2-level"]
message:
include: ["message-include-2-level"]
exclude: ["message-exclude-2-level"]
annotations:
test: "annotation-2-level"
labels:
test: "label-2-level"

- type: apps/v1/deployments # change only ns
namespaces:
exclude:
- "other"
- type: v1/pod # use top level properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
- event: delete
routes:
- resourcename:
include: []
labels:
test: label-1-level
annotations:
test: annotation-1-level
namespaces:
include: []
exclude:
- other
updatesetting: null
event:
reason:
include:
- reason-include-1-level
exclude:
- reason-exclude-1-level
message:
include:
- message-include-1-level
exclude:
- message-exclude-1-level
types:
- delete
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
- event: create
routes:
- resourcename:
include: []
labels:
test: label-2-level
annotations:
test: annotation-2-level
namespaces:
include:
- namespace-include-2-level
exclude:
- namespace-exclude-2-level
updatesetting: null
event:
reason:
include:
- reason-include-2-level
exclude:
- reason-exclude-2-level
message:
include:
- message-include-2-level
exclude:
- message-exclude-2-level
types:
- create
Loading
Loading