(feat.) creation of SharedInformer from Dynamic ClientSet #253

rahulchheda · 2020-04-09T12:04:18Z

ISSUE TYPE

Feature Pull Request
Bug fix Pull Request
Docs Pull Request

Feature Pull Request

SUMMARY

- Added creation of Dynamic ClientSet
- Replaced the current k8s resources to DynamicInformer
- Added logic to create metav1.ObjectMeta/ metav1.TypeMeta from obj interface{}
- Added GKE auth pkg
- Modified fns to get ObjectMeta, and ObjectTypeMeta
- Added function to create *coreV1.Event from the obj interface{}
- Improved switch case for event.New()
- Added for loop to create informer
- Changed Name for fn GetAnnotaion to GetAnnotationFromEvent
- Changed Parsing Logic for GCR (using '/')
- Added logic in FilterEngine, to get the appropriate data
- Full deprecate kubeClient init
- Added logic to fetch resources from DynamicKubeClient
- Added a fn to transform a object form ibj interface{}
- Added REST Mapper to map all resources present on the cluster
- Declared this REST Mapper as global for further use
- Removed logic to filter out the Kubernetes resources according to Involved Object kind
- Removed constants from utils.go

In the latest commit:
- To pass the e2e tests, just replaced all kubernetes resources informer, with dynamic informer.
- Unable to pass the e2e test suite (will paste the error in the comments.)

Fixes #200

test/fake_resource_config.yaml

pkg/utils/utils.go

pkg/controller/controller.go

PrasadG193 · 2020-04-14T13:07:10Z

pkg/controller/controller.go

@@ -125,6 +125,7 @@ func registerEventHandlers(c *config.Config, notifiers []notify.Notifier, resour
 func sendEvent(obj, oldObj interface{}, c *config.Config, notifiers []notify.Notifier, kind string, eventType config.EventType) {
 	// Filter namespaces
 	objectMeta := utils.GetObjectMetaData(obj)


We can get object name and namespace with unstructured.GetName() and unstructured.GetNamespace()

pkg/utils/utils.go

PrasadG193 · 2020-05-01T07:47:44Z

pkg/config/config.go

+	//auth package for GCP
+	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"


Can you please check if it increases binary size?

Sure, let me check.

@rahulchheda Please revert this if not sure about the binary size

pkg/controller/controller.go

PrasadG193 · 2020-05-01T07:51:26Z

pkg/controller/controller.go

 			if err != nil {
-				log.Logger.Errorf("Failed to get MetaNamespaceKey from event resource")
-				return
+				log.Logger.Errorf("Unable to tranform object type: %v, into type: %v", reflect.TypeOf(obj), reflect.TypeOf(eventObj))


should we return from here?
Also could you please check what will be the behaviour if we try to register informer on unregistered resource (for eg custom resource without CRD)?

Sure, let me create such a scenario.

pkg/filterengine/filters/utils.go

pkg/utils/utils.go

PrasadG193 · 2020-05-17T09:27:28Z

@rahulchheda did you get a chance to look at the review comments?

rahulchheda · 2020-05-17T19:21:04Z

@PrasadG193 Can you start resolving the comments that are fixed, might help me in keeping a track of those.
Thanks.

pkg/controller/controller.go

PrasadG193 · 2020-05-22T05:44:19Z

pkg/controller/controller.go

 			}
-			eventObj, ok := obj.(*coreV1.Event)
-			if !ok {
+			//log.Logger.Infof("Printing Event Object: %v", eventObj)


Please remove commented code

pkg/controller/controller.go

pkg/events/events.go

PrasadG193 · 2020-05-22T05:53:45Z

pkg/utils/utils.go

+	replicationController = "v1/replicationcontrollers"
+	daemonSet             = "apps/v1/daemonsets"
+	clusterRole           = "rbac.authorization.k8s.io/v1/clusterroles"
+)


We can get GVR info from event.InvolvedObject. Since it is of type ObjectReference (https://godoc.org/k8s.io/api/core/v1#ObjectReference). One of our goals is to remove the hardcoded mappings like this so that we support any k8s resource including Custom Resources

pkg/utils/utils.go

test/fake_resource_config.yaml

PrasadG193 · 2020-05-22T06:01:58Z

@rahulchheda Please rebase with develop so that it can run CI against GitHub actions

rahulchheda · 2020-06-12T08:23:29Z

Added an example CR in resource_config.yaml to test out the changes install their respective CRD's from here: docs.litmuschaos.io

PrasadG193 · 2020-06-13T06:31:04Z

@rahulchheda Please don't force push. It becomes hard to check if the comments are resolved or not. We anyways squash all into one while merging

PrasadG193 · 2020-06-13T06:42:51Z

pkg/utils/utils.go

-		objectMeta = object.ObjectMeta
-		// pass InvolvedObject`s annotations into Event`s annotations
-		// for filtering event objects based on InvolvedObject`s annotations
+	unstructuredObject := obj.(*unstructured.Unstructured).DeepCopy()


We should check if assertion is possible first, otherwise, this would cause panic

Suggested change

unstructuredObject := obj.(*unstructured.Unstructured).DeepCopy()

unstructuredObject, ok := obj.(*unstructured.Unstructured)

if !ok {

// log and return

}

Not resolved yet.

PrasadG193 · 2020-06-13T06:47:28Z

pkg/utils/utils.go

-		typeMeta = object.TypeMeta
-	case *rbacV1.ClusterRoleBinding:
-		typeMeta = object.TypeMeta
+	k := obj.(*unstructured.Unstructured)


Please check if assertion is possible for safety

resource_config.yaml

test/resource_config.yaml

PrasadG193 · 2020-06-13T06:50:22Z

@rahulchheda Please have a look at CI failures. Let me know if you have any doubts

PrasadG193 · 2020-06-15T10:59:52Z

@rahulchheda Please updated deploy-all-in-one*.yaml and helm chart as per the new config syntax

resource_config.yaml

pkg/utils/utils.go

PrasadG193 · 2020-07-08T05:16:21Z

@rahulchheda Could you please also confirm if we are able to monitor custom resource events?
Ideally, we should add a test case in our e2e test framework.

- Added creation of Dynamic ClientSet - Replaced the current k8s resources to DynamicInformer - Added logic to create metav1.ObjectMeta/ metav1.TypeMeta from obj interface{} - Added GKE auth pkg - Modified fns to get ObjectMeta, and ObjectTypeMeta - Added function to create *coreV1.Event from the obj interface{} - Improved switch case for event.New() - Added for loop to create informer - Changed Name for fn GetAnnotaion to GetAnnotationFromEvent - Changed Parsing Logic for GCR (using '/') - Added logic in FilterEngine, to get the appropriate data - Full deprecate kubeClient init - Added logic to fetch resources from DynamicKubeClient - Added a fn to transform a object form ibj interface{} - Added REST Mapper to map all resources present on the cluster - Declared this REST Mapper as global for further use - Removed logic to filter out the Kubernetes resources according to Involved Object kind - Removed constants from utils.go Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

PrasadG193 · 2020-08-11T08:01:35Z

@rahulchheda The build is failing. Can you please take a look?

Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

…o dynamicInformer Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

PrasadG193 · 2020-09-06T06:41:05Z

@rahulchheda Have you tested this with Custom Resource? Are we able to monitor CRs?

PrasadG193 · 2020-09-06T06:43:19Z

pkg/utils/utils.go

-		objectMeta = object.ObjectMeta
-		// pass InvolvedObject`s annotations into Event`s annotations
-		// for filtering event objects based on InvolvedObject`s annotations
+	unstructuredObject := obj.(*unstructured.Unstructured).DeepCopy()


Not resolved yet.

resource_config.yaml

test/resource_config.yaml

pkg/config/config.go

Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

PrasadG193 · 2020-09-07T03:29:48Z

pkg/utils/utils.go

-		// pass InvolvedObject`s annotations into Event`s annotations
-		// for filtering event objects based on InvolvedObject`s annotations
+	unstructuredObject, ok := obj.(*unstructured.Unstructured)
+	if !ok {


We should log here

resource_config.yaml

test/e2e/env/env.go

test/resource_config.yaml

PrasadG193 · 2020-09-07T03:38:16Z

Let's merge this now and address pending items in the followup.

Things to address:

Pending review comments from this PR
Add e2e test for Custom Resources

rahulchheda changed the title ~~(feat) Creation of SharedInformer from Dynamic ClientSet~~ [WIP] (feat) Creation of SharedInformer from Dynamic ClientSet Apr 9, 2020

PrasadG193 reviewed Apr 10, 2020

View reviewed changes

test/fake_resource_config.yaml Outdated Show resolved Hide resolved

PrasadG193 reviewed Apr 10, 2020

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

rahulchheda force-pushed the dynamicInformer branch 2 times, most recently from 0c5c579 to 1642253 Compare April 13, 2020 22:42

rahulchheda requested a review from PrasadG193 April 14, 2020 08:54

PrasadG193 reviewed Apr 14, 2020

View reviewed changes

PrasadG193 mentioned this pull request Apr 18, 2020

Disable Read Attempts for Unmonitored Resources #248

Closed

PrasadG193 reviewed May 1, 2020

View reviewed changes

pkg/utils/utils.go Show resolved Hide resolved

PrasadG193 reviewed May 1, 2020

View reviewed changes

PrasadG193 added this to In progress in v1.0.0 May 9, 2020

PrasadG193 moved this from In progress to Review in progress in v1.0.0 May 9, 2020

PrasadG193 reviewed May 22, 2020

View reviewed changes

rahulchheda force-pushed the dynamicInformer branch from 808d355 to d17fdbe Compare June 12, 2020 08:20

PrasadG193 reviewed Jun 13, 2020

View reviewed changes

rahulchheda changed the title ~~[WIP] (feat) Creation of SharedInformer from Dynamic ClientSet~~ (feat.) creation of SharedInformer from Dynamic ClientSet Jun 13, 2020

PrasadG193 reviewed Jun 15, 2020

View reviewed changes

resource_config.yaml Show resolved Hide resolved

PrasadG193 reviewed Jun 15, 2020

View reviewed changes

pkg/utils/utils.go Show resolved Hide resolved

PrasadG193 added this to In progress in v0.11.0 via automation Aug 3, 2020

PrasadG193 removed this from Review in progress in v1.0.0 Aug 3, 2020

PrasadG193 removed this from In progress in v0.11.0 Aug 3, 2020

Fixes needed to run E2E tests

925f3cd

Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

rahulchheda force-pushed the dynamicInformer branch from cdad04a to 925f3cd Compare August 4, 2020 10:30

Rahul M Chheda added 2 commits September 5, 2020 23:47

Fixed the tests, and build issues

bb64721

Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

Merge branch 'develop' of https://github.com/infracloudio/botkube int…

3b252fe

…o dynamicInformer Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

PrasadG193 reviewed Sep 6, 2020

View reviewed changes

Resolved review comments

db6b850

Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>

PrasadG193 reviewed Sep 7, 2020

View reviewed changes

PrasadG193 added 3 commits September 7, 2020 09:04

Update resource_config.yaml

469ce3a

Update test/e2e/env/env.go

bc89f26

Update test/resource_config.yaml

cafe7d1

PrasadG193 approved these changes Sep 7, 2020

View reviewed changes

PrasadG193 added the ready-to-merge label Sep 7, 2020

mergify bot merged commit c326015 into kubeshop:develop Sep 7, 2020

PrasadG193 mentioned this pull request Sep 7, 2020

[BUG] BotKube resource configuration in deployment manifests pointing to older syntax #331

Closed

This was referenced Sep 26, 2020

How can I create botkube without giving "clusterrole" #227

Closed

Errors spamming logs when using a service account with limited access to resources #318

Closed

kelvinwijaya mentioned this pull request Oct 2, 2020

Develop kelvinwijaya/botkube#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat.) creation of SharedInformer from Dynamic ClientSet #253

(feat.) creation of SharedInformer from Dynamic ClientSet #253

rahulchheda commented Apr 9, 2020 •

edited

PrasadG193 Apr 14, 2020

PrasadG193 May 1, 2020

rahulchheda May 17, 2020

PrasadG193 Sep 6, 2020

PrasadG193 May 1, 2020

rahulchheda May 17, 2020

PrasadG193 commented May 17, 2020

rahulchheda commented May 17, 2020

PrasadG193 May 22, 2020

PrasadG193 May 22, 2020

PrasadG193 commented May 22, 2020

rahulchheda commented Jun 12, 2020

PrasadG193 commented Jun 13, 2020

PrasadG193 Jun 13, 2020

PrasadG193 Sep 6, 2020

PrasadG193 Jun 13, 2020

PrasadG193 commented Jun 13, 2020

PrasadG193 commented Jun 15, 2020

PrasadG193 commented Jul 8, 2020

PrasadG193 commented Aug 11, 2020

PrasadG193 commented Sep 6, 2020

PrasadG193 Sep 6, 2020

PrasadG193 Sep 7, 2020

PrasadG193 commented Sep 7, 2020

		//auth package for GCP
		_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"

(feat.) creation of SharedInformer from Dynamic ClientSet #253

(feat.) creation of SharedInformer from Dynamic ClientSet #253

Conversation

rahulchheda commented Apr 9, 2020 • edited

ISSUE TYPE

SUMMARY

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrasadG193 commented May 17, 2020

rahulchheda commented May 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrasadG193 commented May 22, 2020

rahulchheda commented Jun 12, 2020

PrasadG193 commented Jun 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrasadG193 commented Jun 13, 2020

PrasadG193 commented Jun 15, 2020

PrasadG193 commented Jul 8, 2020

PrasadG193 commented Aug 11, 2020

PrasadG193 commented Sep 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrasadG193 commented Sep 7, 2020

rahulchheda commented Apr 9, 2020 •

edited