Enhance Update Events with resource spec diff and Change event messag…

…e formats (#158) * Include Namespace field only for Namespaced event notifs This Commit - includes namespace field only for namespaced objects notifs - excludes namespace field from namespace independent resource events * Enhance Update Event Notifications This Commit, - enhances update event notifications with details of what Spec is getting changed - adds custom `SpecDiffReporter` and `utils.Diff()` method. - adds update events in all config.yaml files - adds unit-test for utils.Diff() - adds update events for Deployment, Statefulset and Job by default.
kubeshop · Aug 18, 2019 · 1f9efd6 · 1f9efd6
1 parent 72f2f19
commit 1f9efd6
Show file tree

Hide file tree

Showing 11 changed files with 209 additions and 86 deletions.
diff --git a/config.yaml b/config.yaml
@@ -19,13 +19,15 @@ resources:
       - all
     events:
       - create
+      - update
       - delete
       - error
   - name: statefulset
     namespaces:
       - all
     events:
       - create
+      - update
       - delete
       - error
   - name: ingress
@@ -89,9 +91,9 @@ resources:
       - all
     events:
       - create
+      - update
       - delete
       - error
-      - update
   - name: role
     namespaces:
       - all

diff --git a/deploy-all-in-one-tls.yaml b/deploy-all-in-one-tls.yaml
@@ -29,13 +29,15 @@ data:
           - all
         events:
           - create
+          - update
           - delete
           - error
       - name: statefulset
         namespaces:
           - all
         events:
           - create
+          - update
           - delete
           - error
       - name: ingress

diff --git a/deploy-all-in-one.yaml b/deploy-all-in-one.yaml
@@ -29,13 +29,15 @@ data:
           - all
         events:
           - create
+          - update
           - delete
           - error
       - name: statefulset
         namespaces:
           - all
         events:
           - create
+          - update
           - delete
           - error
       - name: ingress

diff --git a/go.mod b/go.mod
@@ -13,6 +13,7 @@ require (
 	github.com/go-sql-driver/mysql v1.4.1 // indirect
 	github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6 // indirect
 	github.com/golang/protobuf v1.3.2 // indirect
+	github.com/google/go-cmp v0.3.0
 	github.com/google/go-github/v27 v27.0.4
 	github.com/googleapis/gnostic v0.3.0 // indirect
 	github.com/gorilla/websocket v1.4.0 // indirect

diff --git a/helm/botkube/values.yaml b/helm/botkube/values.yaml
@@ -39,13 +39,15 @@ config:
         - all
       events:
         - create
+        - update
         - delete
         - error
     - name: statefulset
       namespaces:
         - all
       events:
         - create
+        - update
         - delete
         - error
     - name: ingress
@@ -109,8 +111,8 @@ config:
         - all
       events:
         - create
-        - delete
         - update
+        - delete
         - error
     - name: role
       namespaces:

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -74,7 +74,7 @@ func RegisterInformers(c *config.Config) {
 
 			// If event type is AllowedEventType and configured for the resource
 			if strings.ToLower(eventObj.Type) == config.AllowedEventType.String() {
-				sendEvent(obj, c, kind, config.ErrorEvent)
+				sendEvent(obj, nil, c, kind, config.ErrorEvent)
 			}
 		},
 	})
@@ -95,26 +95,29 @@ func registerEventHandlers(c *config.Config, resourceType string, events []confi
 	for _, event := range events {
 		if event == config.AllEvent || event == config.CreateEvent {
 			handlerFns.AddFunc = func(obj interface{}) {
-				sendEvent(obj, c, resourceType, config.CreateEvent)
+				log.Logger.Debugf("Processing add to %v", resourceType)
+				sendEvent(obj, nil, c, resourceType, config.CreateEvent)
 			}
 		}
 
 		if event == config.AllEvent || event == config.UpdateEvent {
 			handlerFns.UpdateFunc = func(old, new interface{}) {
-				sendEvent(new, c, resourceType, config.UpdateEvent)
+				log.Logger.Debugf("Processing update to %v\n Object: %+v\n", resourceType, new)
+				sendEvent(new, old, c, resourceType, config.UpdateEvent)
 			}
 		}
 
 		if event == config.AllEvent || event == config.DeleteEvent {
 			handlerFns.DeleteFunc = func(obj interface{}) {
-				sendEvent(obj, c, resourceType, config.DeleteEvent)
+				log.Logger.Debugf("Processing delete to %v", resourceType)
+				sendEvent(obj, nil, c, resourceType, config.DeleteEvent)
 			}
 		}
 	}
 	return handlerFns
 }
 
-func sendEvent(obj interface{}, c *config.Config, kind string, eventType config.EventType) {
+func sendEvent(obj, oldObj interface{}, c *config.Config, kind string, eventType config.EventType) {
 	// Filter namespaces
 	objectMeta := utils.GetObjectMetaData(obj)
 	if !utils.AllowedEventKindsMap[utils.EventKind{Resource: kind, Namespace: "all", EventType: eventType}] &&
@@ -140,12 +143,16 @@ func sendEvent(obj interface{}, c *config.Config, kind string, eventType config.
 		}
 	}
 
-	// After resync, Informer gets OnUpdate call, even if nothing changed.
-	// We need to skip update event if that is happened before current time.
-	// As a workaround, we will be ignoring update events older than 5s of current time.
-	if eventType == config.UpdateEvent && time.Now().Sub(event.TimeStamp).Seconds() > 5 {
-		log.Logger.Debug("Skipping older events")
-		return
+	// check for siginificant Update Events in objects
+	if eventType == config.UpdateEvent {
+		updateMsg := utils.Diff(oldObj, obj)
+		if len(updateMsg) > 0 {
+			event.Messages = append(event.Messages, updateMsg)
+		} else {
+			// skipping least significant update
+			log.Logger.Debug("skipping least significant Update event")
+			event.Skip = true
+		}
 	}
 
 	event = filterengine.DefaultFilterEngine.Run(obj, event)

diff --git a/pkg/events/events.go b/pkg/events/events.go
@@ -109,40 +109,16 @@ func New(object interface{}, eventType config.EventType, kind string) Event {
 		event.TimeStamp = obj.LastTimestamp.Time
 	case *apiV1.Pod:
 		event.Kind = "Pod"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
 	case *apiV1.Node:
 		event.Kind = "Node"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
 	case *apiV1.Namespace:
 		event.Kind = "Namespace"
 	case *apiV1.PersistentVolume:
 		event.Kind = "PersistentVolume"
 	case *apiV1.PersistentVolumeClaim:
 		event.Kind = "PersistentVolumeClaim"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
 	case *apiV1.ReplicationController:
 		event.Kind = "ReplicationController"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
 	case *apiV1.Service:
 		event.Kind = "Service"
 	case *apiV1.Secret:
@@ -157,37 +133,14 @@ func New(object interface{}, eventType config.EventType, kind string) Event {
 		event.Kind = "DaemonSet"
 	case *appsV1.ReplicaSet:
 		event.Kind = "ReplicaSet"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
 	case *appsV1.Deployment:
 		event.Kind = "Deployment"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
 	case *appsV1.StatefulSet:
 		event.Kind = "StatefulSet"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
 
 	case *batchV1.Job:
 		event.Kind = "Job"
-		if eventType == config.UpdateEvent {
-			condLen := len(obj.Status.Conditions)
-			if condLen != 0 {
-				event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
-			}
-		}
+
 	case *rbacV1.Role:
 		event.Kind = "Role"
 	case *rbacV1.RoleBinding:
@@ -220,33 +173,67 @@ func (event *Event) Message() (msg string) {
 
 	switch event.Type {
 	case config.CreateEvent, config.DeleteEvent, config.UpdateEvent:
-		msg = fmt.Sprintf(
-			"%s `%s` of cluster `%s`, namespace `%s` has been %s:\n```%s```",
-			event.Kind,
-			event.Name,
-			event.Cluster,
-			event.Namespace,
-			event.Type+"d",
-			message,
-		)
+		switch event.Kind {
+		case "Namespace", "Node", "PersistentVolume", "ClusterRole", "ClusterRoleBinding":
+			msg = fmt.Sprintf(
+				"%s `%s` in of cluster `%s` has been %s:\n```%s```",
+				event.Kind,
+				event.Name,
+				event.Cluster,
+				event.Type+"d",
+				message,
+			)
+		default:
+			msg = fmt.Sprintf(
+				"%s `%s` in of cluster `%s`, namespace `%s` has been %s:\n```%s```",
+				event.Kind,
+				event.Name,
+				event.Cluster,
+				event.Namespace,
+				event.Type+"d",
+				message,
+			)
+		}
 	case config.ErrorEvent:
-		msg = fmt.Sprintf(
-			"Error Occurred in %s: `%s` of cluster `%s`, namespace `%s`:\n```%s``` ",
-			event.Kind,
-			event.Name,
-			event.Cluster,
-			event.Namespace,
-			message,
-		)
+		switch event.Kind {
+		case "Namespace", "Node", "PersistentVolume", "ClusterRole", "ClusterRoleBinding":
+			msg = fmt.Sprintf(
+				"Error Occurred in %s: `%s` of cluster `%s`:\n```%s``` ",
+				event.Kind,
+				event.Name,
+				event.Cluster,
+				message,
+			)
+		default:
+			msg = fmt.Sprintf(
+				"Error Occurred in %s: `%s` of cluster `%s`, namespace `%s`:\n```%s``` ",
+				event.Kind,
+				event.Name,
+				event.Cluster,
+				event.Namespace,
+				message,
+			)
+		}
 	case config.WarningEvent:
-		msg = fmt.Sprintf(
-			"Warning %s: `%s` of cluster `%s`, namespace `%s`:\n```%s``` ",
-			event.Kind,
-			event.Name,
-			event.Cluster,
-			event.Namespace,
-			message,
-		)
+		switch event.Kind {
+		case "Namespace", "Node", "PersistentVolume", "ClusterRole", "ClusterRoleBinding":
+			msg = fmt.Sprintf(
+				"Warning %s: `%s` of cluster `%s`:\n```%s``` ",
+				event.Kind,
+				event.Name,
+				event.Cluster,
+				message,
+			)
+		default:
+			msg = fmt.Sprintf(
+				"Warning %s: `%s` of cluster `%s`, namespace `%s`:\n```%s``` ",
+				event.Kind,
+				event.Name,
+				event.Cluster,
+				event.Namespace,
+				message,
+			)
+		}
 	}
 	return msg
 }
diff --git a/pkg/filterengine/filters/ingress_validator.go b/pkg/filterengine/filters/ingress_validator.go
@@ -3,6 +3,7 @@ package filters
 import (
 	"github.com/infracloudio/botkube/pkg/events"
 	"github.com/infracloudio/botkube/pkg/filterengine"
+	log "github.com/infracloudio/botkube/pkg/logging"
 	extV1beta1 "k8s.io/api/extensions/v1beta1"
 )
 
@@ -56,6 +57,7 @@ func (iv IngressValidator) Run(object interface{}, event *events.Event) {
 			event.Recommendations = append(event.Recommendations, "TLS secret "+tls.SecretName+"does not exist")
 		}
 	}
+	log.Logger.Debug("Ingress Validator filter successful!")
 }
 
 // Describe filter

diff --git a/pkg/filterengine/filters/job_status_checker.go b/pkg/filterengine/filters/job_status_checker.go
@@ -45,6 +45,8 @@ func (f JobStatusChecker) Run(object interface{}, event *events.Event) {
 	if c.Type == batchV1.JobComplete {
 		event.Messages = []string{"Job succeeded!"}
 		event.TimeStamp = c.LastTransitionTime.Time
+		// overwrite event.Skip in case of Job succeeded (Job update) events
+		event.Skip = false
 	} else {
 		event.Skip = true
 		return
-Original file line number
+Diff line change
@@ Expand Up / @@ -29,13 +29,15 @@ data: @@
               - all
             events:
               - create
+              - update
               - delete
               - error
           - name: statefulset
             namespaces:
               - all
             events:
               - create
+              - update
               - delete
               - error
           - name: ingress
@@ Expand Down @@