Add live k8s API collector #34

jt-dd · 2023-06-05T16:42:09Z

This collector connect directly using the default config path to find the configuration (using controller-runtime lib):

--kubeconfig flag pointing at a file
KUBECONFIG environment variable pointing at a file
In-cluster config if running in cluster
$HOME/.kube/config if exists.

Ref: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.15.0/pkg/client/config/config.go#L76

It uses the configuration to set 3 variables needed for crawling the API:

PageSize: Number of entry being retrieving by each call on the API (same for all Kubernetes entry types)
PageBufferSize: Number of pages to buffer
RateLimitPerSecond: Rate limiting per second across all calls (same for all Kubernetes entry types) against the Kubernetes API

Note: for the rate limiting, a simple leaky bucket has been implemented (using ratelimit lib from Uber). The rate limiting affect simultaneously all the calls to the Kub API regardless of the type being collected.

edznux-dd

WIP review, gotta go :D

pkg/collector/collector.go

pkg/collector/k8s_api_test.go

edznux-dd · 2023-06-05T16:50:23Z

pkg/collector/k8s_api.go

+	}
+
+	if cfg.Collector.Live.PageSize == nil {
+		cfg.Collector.Live.PageSize = globals.Ptr(K8sAPIDefaultPageSize)


Can't you use * or & or in the type instead of doing a generic in here?
For some types (like bool and all, where you don't have a variable, I see why, but i'm not sure I understand here)

I can not take address of a constant, so using for me was the most elegant way. But I guess I can also do the tricks, but for me it was less elegant:

PageSize := K8sAPIDefaultPageSize cfg.Collector.Live.PageSize = &PageSize

ah yep, makes sense didn't see the constant.
I would have used a var instead of const so it has a pointer available. 😅

Also tbh, this is config related, and we are already using viper so we should not use custom things to set default imo:
something like viper.SetDefault("k8s.collector.PageSize", 5000) would replace this function entirely. You would just need to call viper.GetInt("k8s.collector.PageSize")

In any case, K8sAPIDefaultPageSize should be renamed K8sAPIDefaultPageSizeDefault to make it clear that it's customizable later on and that this value isn't the "final" one.

K8sAPIDefaultPageSizeDefault has 2 default which does not make sense. If you prefer I can change it to K8sAPIPageSizeDefault but all the current variables are to DefaultBlabla.
Also the SetDefault() seems to be a pain:

Default value for nested key spf13/viper#71 (comment)
I will look to see how to work with it.

ah woops, can't read, guess it shows that it needs to be prefix/suffix and not in the middle 😅
DefaultK8sAPIPageSize sounds fine to me! Having the default in the middle breaks the convention we have in other variables of DefaultBlabla as you mention yourself :D

Is the issue mentionned still valid (from 2015, and marked as fixed by spf13/viper#195 (comment))?

It did not brake the convention because to me it made more sense to have PackageDefaultValue. The package was never mentioned in the other values. Do we want to name the "package" in those constant ? I find it easier to follow.

You should "never" put the package's name in the name of the var. Otherwise you need to do "package.PackageVar" which is redundant (and also create more work if you want to update / move the package, but that's 🤷).

speaking of package, If possible, I think having the whole k8s_api related in a subpackage of collector would be nicer to use (if there's no circular deps with the rest of the project) :D

no we can not due to the CollectorClient interface and ClientFactory() method. This is why I added the name of the "subpackage" (if you prefer).

pkg/collector/k8s_api.go

pkg/collector/k8s_api_test.go

edznux-dd · 2023-06-06T07:49:49Z

pkg/collector/k8s_api.go

+	}
+
+	if cfg.Collector.Live.PageSize == nil {
+		cfg.Collector.Live.PageSize = globals.Ptr(K8sAPIDefaultPageSize)


ah yep, makes sense didn't see the constant.
I would have used a var instead of const so it has a pointer available. 😅

Also tbh, this is config related, and we are already using viper so we should not use custom things to set default imo:
something like viper.SetDefault("k8s.collector.PageSize", 5000) would replace this function entirely. You would just need to call viper.GetInt("k8s.collector.PageSize")

In any case, K8sAPIDefaultPageSize should be renamed K8sAPIDefaultPageSizeDefault to make it clear that it's customizable later on and that this value isn't the "final" one.

pkg/collector/k8s_api.go

pkg/collector/k8s_api_test.go

pkg/collector/k8s_api.go

pkg/collector/k8s_api_test.go

jt-dd · 2023-06-06T08:20:41Z

This whole thing can be replaced by
assert.Equal(cfg.Collector.Live, tt.args,value) I believe
The pretty print and all will be done automatically, example go playground: https://goplay.tools/snippet/n_yR-edgBMq

=== RUN   TestSomething
    prog_test.go:19: 
        	Error Trace:	/tmp/sandbox3381284915/prog_test.go:19
        	Error:      	Not equal: 
        	            	expected: main.testlol{FieldA:"Hellolola", FieldB:123}
        	            	actual  : main.testlol{FieldA:"Hellolol", FieldB:123}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,3 @@
        	            	 (main.testlol) {
        	            	- FieldA: (string) (len=9) "Hellolola",
        	            	+ FieldA: (string) (len=8) "Hellolol",
        	            	  FieldB: (int) 123
        	Test:       	TestSomething
        	Messages:   	The two words should be the same.
--- FAIL: TestSomething (0.00s)
FAIL

I did not know that trick. It is really nice.

jt-dd · 2023-06-06T10:55:05Z

This should be a switch based on the config instead of being hardcoded

Yes implemented.

d0g0x01 · 2023-06-06T12:08:12Z

There are a couple of features missing from the spec https://datadoghq.atlassian.net/browse/ASENG-362 - maybe it would be worth adding these as a subsequent PR?

When this is complete do you want to switch the integration tests over to use this collector? you'll need to update the code at test/system/setup_test.go

d0g0x01 · 2023-06-06T11:57:21Z

pkg/collector/k8s_api.go

+	if cfg.Collector.Type != config.CollectorTypeK8sAPI {
+		return fmt.Errorf("invalid collector type in config: %s", cfg.Collector.Type)
+	}
+	return nil


should you not do then attach the config to the collector instance? also you dont use the logger?

Also should this be a private method?

Not sure to understand what you mean by attaching the config to the collector instance. The config is attached to the collector instance.
Same for the logger, it is being used by the collector instance.

Yes this should be private.

pkg/collector/k8s_api.go

d0g0x01 · 2023-06-06T11:59:44Z

pkg/collector/k8s_api.go

+type k8sAPICollector struct {
+	clientset kubernetes.Interface
+	log       *log.KubehoundLogger
+	rl        ratelimit.Limiter


d0g0x01 · 2023-06-06T12:01:58Z

pkg/collector/k8s_api.go

+		return nil, err
+	}
+
+	kubeConfig, err := ctrl.GetConfig()


I think we should be able to specify a target cluster in the configuration. I think currently this will default to the current kubectx which might not be what the user wnats/expects?

This support an env variable KUBECONFIG (and other standard way) . But I can try to path it over through the config file in another PR.

d0g0x01 · 2023-06-06T12:03:32Z

pkg/collector/k8s_api.go

+
+// checkNamespaceExists checks if a namespace exists
+func (c *k8sAPICollector) checkNamespaceExists(ctx context.Context, namespace string) error {
+	_, err := c.clientset.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{})


it might be worth getting all the namespaces once then caching for quick lookups as should not be that much data

*no need to use the formal cache provider here. just a hashmap in memory attached to the collector would be fine

Same for this, maybe I should do the caching in a different PR since it is purely optimisation?

sounds good

d0g0x01 · 2023-06-06T12:14:37Z

pkg/collector/k8s_api.go

+}
+
+// streamPodsNamespace streams the pod objects corresponding to a cluster namespace.
+func (c *k8sAPICollector) streamPodsNamespace(ctx context.Context, namespace string, ingestor PodIngestor) error {


We discussed in the original desing meeting that it might help ensure better consistency if we pull all the objects namespace by namespace for pods/roles/rolebindings. What do you think of tweaking the logic slightly to be

pull all namespaces

cache namespaces in the collector instance

call streamXXXNamespace for each namespace in order

You have most of the pieces to do it already so should not be too tricky. @edznux-dd any thoughts?

This is "why" we though about the WithOption() pattern in the Stream* functions.
Caching the list of namespace seems fine to me, I don't see it grow that significantly for it to cause any issue.

Doing the iteration in the Steam* function may make it a bit difficult to parallelize more in the future? (multiple namespace concurrently, so non concurently). I don't know how the API/Storage of k8s handles the load balancing/sharding of this kind of resources, and I don't know if it matters :D (I don't think that could cause hot spots to appears if we do purely sequential reads, but I don't know)

What is the actual gain of iterating manually over the namespace vs the "automated way" ?

On second thoughts I think this might be some premature optimization. Suggest we leave as is for now and revisit if we see any issues materialize

d0g0x01 · 2023-06-06T12:15:21Z

pkg/globals/collector.go

+const (
+	DefaultK8sAPIPageSize           int64 = 5000
+	DefaultK8sAPIPageBufferSize     int32 = 10
+	DefaultK8sAPIRateLimitPerSecond int   = 100


is compute ok with this internally?

I did not ping them, but those a the default value (expect the rate limiting). But 100 req/sec seems pretty low to me (I used to try to keep under this value why I was pentesting API). But I will ask them.

I think thats fine for me then

d0g0x01 · 2023-06-06T12:16:09Z

pkg/collector/testdata/kubehound-test-live-mixed-spec.yaml

@@ -0,0 +1,5 @@
+collector:


might be worth editing the default config to use this (configs/etc/kubehound.yaml)

d0g0x01

LGTM

jt-dd added 2 commits June 5, 2023 18:32

first draft

235d1a8

typo

e9058ec

jt-dd requested a review from edznux-dd June 5, 2023 16:42

jt-dd marked this pull request as ready for review June 5, 2023 16:42

jt-dd added 2 commits June 5, 2023 18:55

fix CI

1e527eb

typo

40197ef

edznux-dd reviewed Jun 5, 2023

View reviewed changes

jt-dd added 2 commits June 5, 2023 21:34

typo %w error

f063359

unit test simplified

d0b3c1c

edznux-dd reviewed Jun 6, 2023

View reviewed changes

typo error

98c0479

edznux-dd reviewed Jun 6, 2023

View reviewed changes

pkg/collector/k8s_api_test.go Outdated Show resolved Hide resolved

unit test parrallel

b8a5895

jt-dd added 3 commits June 6, 2023 10:21

simplified unit test againg

e54561b

fix factory

b626a1d

Using setdefault from viper

8a97020

Rename default var

ced0e87

jt-dd requested review from d0g0x01 and edznux-dd June 6, 2023 10:58

remove util

7e12317

d0g0x01 reviewed Jun 6, 2023

View reviewed changes

jt-dd and others added 4 commits June 6, 2023 15:35

last PR commrnyd

cff0776

typo

ce35134

pushed to default config

72952a7

Merge branch 'main' into jt-dd/k8sAPILocalCollector

27a6429

jt-dd requested a review from d0g0x01 June 6, 2023 13:45

d0g0x01 approved these changes Jun 6, 2023

View reviewed changes

jt-dd merged commit 3204a58 into main Jun 6, 2023

jt-dd deleted the jt-dd/k8sAPILocalCollector branch June 6, 2023 13:49

Add live k8s API collector #34

Add live k8s API collector #34

Conversation

jt-dd commented Jun 5, 2023

edznux-dd left a 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

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

Choose a reason for hiding this comment

jt-dd commented Jun 6, 2023

jt-dd commented Jun 6, 2023

d0g0x01 commented Jun 6, 2023 • edited Loading

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

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

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

Choose a reason for hiding this comment

d0g0x01 left a comment

Choose a reason for hiding this comment

d0g0x01 commented Jun 6, 2023 •

edited

Loading