[ASENG-835] Add puller package for s3 #157

edznux-dd · 2023-12-26T18:12:56Z

WIP / Discussion

PoC of a gRPC API for a remote ingestor.
This is one possible solution for pulling the data from an s3 bucket that was stored previously.

This PR includes 3 blocks:

A Puller: pulls the data from a remote source, in our case a generic implementation of a blob storage (e.g: s3)
An API: allows users and automated system to execute command from that "ingestor"
A notifier: allows the API to notify user and system of processing completion. This can be done via a queuing system, webhooks or something else.

Notes:

Not a huge fan of the "ingestor" name, we probably need to convey that it's a remote API that may have extended use later on?
- I thing the API needs to be extended for it to not be a bit over engineered here.

Alternative solution:

Should we replace the puller by simply a custom collector (e.g: blob_collector)?
- This would need to fit the Collector interface to pull data to s3 but could simplify a few things maybe?
- This will match the same structure as the file collector (bundled json files with all resource, per type)

pkg/ingestor/puller/puller.go

…d/implement-ingestor-api

edznux-dd

thanks for taking over the PR :D
I left some comments!

I guess we could have reused the tar package from our secure go sdk for create/extract and remove some of the manual setup!

Small, overall nit: (it was maybe in my code tbh haha), there's a bit of back and forth between the inline vs 2 lines if err !=nil checks. it's a bit weird, but I'm alright with it.

edznux-dd · 2024-03-14T08:52:01Z

pkg/config/dynamic.go

+		log.I.Errorf("error loading run id: %v", err)
+		return failure(fmt.Errorf("loading run id: %v", err))


Do we really want to log AND return the error?
Sounds like we should just return the error and print in the caller to me if we don't want to duplicate the log.

edznux-dd · 2024-03-14T09:17:30Z

pkg/ingestor/api/api.go

+	runCtx = context.WithValue(runCtx, span.ContextLogFieldRunID, runID)
+
+	spanJob, runCtx := span.SpanIngestRunFromContext(runCtx, span.IngestorStartJob)
+	defer spanJob.Finish()


I think we'll want so we can propagate the errors in the span.

Suggested change

defer spanJob.Finish()

defer func() {

spanJob.Finish(tracer.WithError(err))

}()

(might need to declare err before hand tho)

yes nice catch need to update it like 20x

(might need to declare err before hand tho)

You can use a named parameter for the return value, iirc that's how the tracing doc advises to do it

edznux-dd · 2024-03-14T09:19:08Z

pkg/ingestor/api/api.go

+
+	// Run the ingest pipeline
+	log.I.Info("Starting Kubernetes raw data ingest")
+	if err := ingestor.IngestData(runCtx, runCfg, collect, g.cache, g.storedb, g.graphdb); err != nil {


Any reason to switch style of err := ...; err != nil vs err := ... \n if err !=nil {} ?

No not really. Changing it.

edznux-dd · 2024-03-14T09:23:53Z

pkg/ingestor/ingestor.go

+
+	start := time.Now()
+	span, ctx := tracer.StartSpanFromContext(ctx, span.IngestData, tracer.Measured())
+	defer span.Finish()


Suggested change

defer span.Finish()

defer func() {

span.Finish(tracer.WithError(err))

}()

same here, you'll need var err error above (or name the return value err)

pkg/ingestor/puller/blob/blob.go

edznux-dd · 2024-03-14T11:47:11Z

pkg/kubehound/core/core_live.go

+// Launch will launch the KubeHound application to ingest data from a collector and create an attack graph.
+func Launch(ctx context.Context, opts ...LaunchOption) error {
+	span, ctx := tracer.StartSpanFromContext(ctx, span.Launch, tracer.Measured())
+	defer span.Finish()


Suggested change

defer span.Finish()

defer func() {

span.Finish(tracer.WithError(err))

}()

edznux-dd · 2024-03-14T11:47:45Z

pkg/kubehound/core/core_live.go

+
+	// We update the base tags to include that run id, so we have it available for metrics
+	tag.AppendBaseTags(tag.RunID(runID.String()))
+	// tag.BaseTags = append(tag.BaseTags, tag.RunID(runID.String()))


Suggested change

// tag.BaseTags = append(tag.BaseTags, tag.RunID(runID.String()))

I believe this is a leftover?

edznux-dd · 2024-03-14T11:52:11Z

pkg/telemetry/tag/tags.go

+	}
+}
+
+func AppendBaseTags(tags ...string) {


log uses "GlobalTags" instead of base tags, would that make sense to unify? (it feels a bit weird on the caller side because it suggest 2 different semantic meaning and I don't think there should be?)

Overall, I wonder if we shouldn't have all the tags in both metrics and logs shared onto a single config? (it could be "pulled" from here into the logger package)

edznux-dd · 2024-03-14T11:56:40Z

pkg/kubehound/core/core_live.go

+	// We define a unique run id this so we can measure run by run in addition of version per version.
+	// Useful when rerunning the same binary (same version) on different dataset or with different databases...
+	runID := config.NewRunID()
+	span.SetBaggageItem("run_id", runID.String())


Should we use tags.RunIdTag here to unify?

yes this will get rebranded. I did not want to touch yet the "live" dumping. I will rework it after.

edznux-dd · 2024-03-14T12:10:30Z

pkg/kubehound/core/core.go

-		return fmt.Errorf("raw data ingest: %w", err)
-	}
+type LaunchConfig struct {
+	Cfg      *config.KubehoundConfig


Do we want to expose Cfg to other package?
I don't think it's used anywhere outside this pacakge by looking at the code locally.

yes it is being used, you will see in the next PRs

Add puller package for s3

c39cacd

edznux-dd temporarily deployed to devenv December 26, 2023 18:13 — with GitHub Actions Inactive

Add base GRPC interfaces

d7283de

edznux-dd temporarily deployed to devenv December 26, 2023 18:14 — with GitHub Actions Inactive

github-advanced-security bot found potential problems Dec 26, 2023

View reviewed changes

pkg/ingestor/puller/puller.go Fixed Show fixed Hide fixed

edznux-dd added 3 commits December 27, 2023 10:22

Move grpcs

63587eb

Add gitignore for kubehound-ingestor binary

bb80766

Fix linters

3aeb590

edznux-dd temporarily deployed to devenv December 27, 2023 11:30 — with GitHub Actions Inactive

edznux-dd added 3 commits December 27, 2023 15:01

Use global config

0364790

Add & fix tests

53a898d

Fix linter & use parallel tests

2b5f9a7

edznux-dd temporarily deployed to devenv December 27, 2023 15:48 — with GitHub Actions Inactive

linter

c602bfb

edznux-dd temporarily deployed to devenv December 27, 2023 15:58 — with GitHub Actions Inactive

edznux-dd added 2 commits December 27, 2023 18:07

Add notifier package

e9c2b09

Refactor api / notifier / puller architecture

e87b2f2

edznux-dd temporarily deployed to devenv December 27, 2023 17:45 — with GitHub Actions Inactive

Refactor API & add mockings test

f3801c9

edznux-dd temporarily deployed to devenv December 28, 2023 12:23 — with GitHub Actions Inactive

edznux-dd temporarily deployed to devenv January 12, 2024 14:43 — with GitHub Actions Inactive

jt-dd added 3 commits February 28, 2024 15:44

Merge branch 'main' of ssh://github.com/DataDog/kubehound into edouar…

e26cd27

…d/implement-ingestor-api

fixing merge

8ec286a

rework API struct from new dumper

679ccce

jt-dd temporarily deployed to devenv March 13, 2024 15:21 — with GitHub Actions Inactive

wip

e9d6f19

jt-dd temporarily deployed to devenv March 13, 2024 16:26 — with GitHub Actions Inactive

fixing tags

2c56ea3

jt-dd temporarily deployed to devenv March 13, 2024 16:30 — with GitHub Actions Inactive

fix linter issue

23fd6f2

fix sherry pick

823c753

jt-dd temporarily deployed to devenv March 13, 2024 17:57 — with GitHub Actions Inactive

wrong PR

6445480

jt-dd temporarily deployed to devenv March 13, 2024 18:03 — with GitHub Actions Inactive

jt-dd marked this pull request as ready for review March 13, 2024 18:09

jt-dd requested a review from a team as a code owner March 13, 2024 18:09

fix linter

b2ed623

jt-dd temporarily deployed to devenv March 14, 2024 09:09 — with GitHub Actions Inactive

fix linter

154b02e

jt-dd temporarily deployed to devenv March 14, 2024 09:29 — with GitHub Actions Inactive

edznux-dd commented Mar 14, 2024

View reviewed changes

PR comment

dfe97f8

jt-dd temporarily deployed to devenv March 14, 2024 15:54 — with GitHub Actions Inactive

fix linter

fd6999f

jt-dd temporarily deployed to devenv March 14, 2024 16:01 — with GitHub Actions Inactive

jt-dd previously approved these changes Mar 14, 2024

View reviewed changes

jt-dd added 2 commits March 14, 2024 18:06

fix comment PR

b359bca

fix linter

2356dd4

jt-dd dismissed their stale review via 2356dd4 March 14, 2024 17:08

jt-dd temporarily deployed to devenv March 14, 2024 17:08 — with GitHub Actions Inactive

fix linter

03cc099

jt-dd temporarily deployed to devenv March 14, 2024 17:28 — with GitHub Actions Inactive

fix linter

87f6a06

jt-dd temporarily deployed to devenv March 14, 2024 17:46 — with GitHub Actions Inactive

fix linter

b112061

jt-dd temporarily deployed to devenv March 14, 2024 17:52 — with GitHub Actions Inactive

jt-dd self-requested a review March 14, 2024 18:00

jt-dd approved these changes Mar 14, 2024

View reviewed changes

jt-dd merged commit 4f34295 into main Mar 14, 2024
7 checks passed

jt-dd deleted the edouard/implement-ingestor-api branch March 14, 2024 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASENG-835] Add puller package for s3 #157

[ASENG-835] Add puller package for s3 #157

edznux-dd commented Dec 26, 2023 •

edited

Loading

edznux-dd left a comment

edznux-dd Mar 14, 2024

edznux-dd Mar 14, 2024

edznux-dd Mar 14, 2024

jt-dd Mar 14, 2024

Minosity-VR Mar 14, 2024 •

edited

Loading

edznux-dd Mar 14, 2024

jt-dd Mar 14, 2024

edznux-dd Mar 14, 2024

edznux-dd Mar 14, 2024

edznux-dd Mar 14, 2024

edznux-dd Mar 14, 2024

edznux-dd Mar 14, 2024

edznux-dd Mar 14, 2024

jt-dd Mar 14, 2024

edznux-dd Mar 14, 2024

jt-dd Mar 14, 2024

		log.I.Errorf("error loading run id: %v", err)
		return failure(fmt.Errorf("loading run id: %v", err))

-	defer spanJob.Finish()
+	defer func() {
+		spanJob.Finish(tracer.WithError(err))
+	}()

[ASENG-835] Add puller package for s3 #157

[ASENG-835] Add puller package for s3 #157

Conversation

edznux-dd commented Dec 26, 2023 • edited Loading

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

Minosity-VR Mar 14, 2024 • 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

edznux-dd commented Dec 26, 2023 •

edited

Loading

Minosity-VR Mar 14, 2024 •

edited

Loading