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

Refactor and enrich cloudbeat events #69

Merged
merged 17 commits into from
May 2, 2022

Conversation

uri-weisman
Copy link
Contributor

Refactor and add a name, sub-type, and type to the resource struct.

@uri-weisman uri-weisman requested a review from eyalkraft May 1, 2022 11:50
@uri-weisman uri-weisman marked this pull request as ready for review May 1, 2022 11:50
@uri-weisman uri-weisman requested a review from a team as a code owner May 1, 2022 11:50
func (res ECRResource) GetData() interface{} {
return res
}

func (res ECRResource) GetMetadata() fetching.ResourceMetadata {
//TODO implement me
Copy link
Contributor

Choose a reason for hiding this comment

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

please open an issue for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A task for implementing the missing GetMetadata method - #71


func (r EKSResource) GetMetadata() fetching.ResourceMetadata {
//TODO implement me
return fetching.ResourceMetadata{
Copy link
Contributor

Choose a reason for hiding this comment

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

in the issue you open include a list of fetchers to fix

}
}

func getSubType(isDir bool) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a bit weird.
what about passing info?

@@ -31,13 +31,20 @@ import (
"github.com/elastic/cloudbeat/resources/fetching"
)

const (
FSResourceType = "file"
Copy link
Contributor

Choose a reason for hiding this comment

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

ID: r.Inode,
Type: FSResourceType,
SubType: r.SubType,
Name: r.Path,
Copy link
Contributor

Choose a reason for hiding this comment

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

leave a note about this being the path from the container and not from the host

func (r K8sResource) GetData() interface{} {
return r.Data
func (r K8sResource) GetSubType() string {
k8sObj := reflect.Indirect(reflect.ValueOf(r.Data))
Copy link
Contributor

Choose a reason for hiding this comment

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

try and redesign to avoid calling reflect.Indirect(reflect.ValueOf(r.Data)) twice

}
}
}

func (c *Transformer) createBeatEvents(fetchedResource fetching.Resource, metadata ResourceMetadata) error {
func (c *Transformer) createBeatEvents(fetchedResource fetching.Resource, metadata fetching.ResourceMetadata, cycleMetadata CycleMetadata) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

now since fetchedResource itself has .GetMetadata() why do you pass both?

Raw: fetcherResult.Resource,
resource := fetching.ResourceFields{
ResourceMetadata: metadata,
Raw: fetcherResult.Resource,
Copy link
Contributor

Choose a reason for hiding this comment

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

type and subtype?
or are they waiting for another PR?

Copy link
Contributor Author

@uri-weisman uri-weisman May 1, 2022

Choose a reason for hiding this comment

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

No, they are part of the ResourceMetadata type.
plz see fetcher.go

@@ -60,3 +60,13 @@ func (r IAMResource) GetID() (string, error) {
func (r IAMResource) GetData() interface{} {
return r.Data
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You did not remove GetID() in iam_fetcher.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 💪

}

func (r K8sResource) GetData() interface{} {
return r.Data
func (r K8sResource) GetSubType() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to fail and continue with an empty string value instead of the kind?
Just wondering if we are interested in sending data without kind that might create problems down the line of the pipeline that expects to groupBy or do any logic on these values?

Copy link
Contributor

Choose a reason for hiding this comment

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

good question👆🏼
I think i'd prefer events with subtype missing than no events at all,
but the possibility for problems in Kibana resulting from this field missing is interesting...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think sending with empty subtype matches the current design pattern and is a good start (best effort).
for example currently if a fetcher registration fails cloudbeat will log an error and continue.
anyways it's a good idea to ask @kfirpeled / @ari-aviran or someone else from the FE, or even comment here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, except for bugs I don't see any reason for this method ever failing so I wouldn't waste too much time thinking/debating it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats a good point,
In case of an issue - we can define it to be empty. And we would know to treat empty values as error.

What happens when there's no subtype? For example, in process.
Just to make things easier, I think it is better to have value we would like to see. And not solve this in the UI only or assume if it is process there's no error and it is empty. Just so we can allow our users to built their own dashboard over the data without having some sort of logic.

@uri-weisman uri-weisman requested a review from eyalkraft May 1, 2022 13:01
@uri-weisman uri-weisman requested a review from a team as a code owner May 2, 2022 05:24
@github-actions
Copy link

github-actions bot commented May 2, 2022

Cloudbeat CI 🤖

Integration tests status: success
Tests Summary:

=========================== short test summary info ============================
PASSED integration/tests/test_output_to_elasticsearch.py::test_elastic_index_exists[file]
PASSED integration/tests/test_output_to_elasticsearch.py::test_elastic_index_exists[process]
PASSED integration/tests/test_output_to_elasticsearch.py::test_elastic_index_exists[k8s_object]
PASSED product/tests/test_cloudbeat.py::test_cloudbeat_pod_exist
PASSED product/tests/test_cloudbeat.py::test_cloudbeat_pods_running
================= 5 passed, 4 deselected, 6 warnings in 0.90s ==================

Link to detailed report: https://elastic.github.io/cloudbeat/428

CMDArgumentMatcher = "\\b%s[\\s=]\\/?(\\S+)"
CMDArgumentMatcher = "\\b%s[\\s=]\\/?(\\S+)"
ProcessResourceType = "process"
ProcessSubType = "process"
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

return r.Data
func getK8sSubType(k8sObj reflect.Value) string {
typeMeta, ok := k8sObj.FieldByName(k8sTypeMetadataField).Interface().(metav1.TypeMeta)
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

now i see what you meant...

@uri-weisman uri-weisman merged commit 15a339b into elastic:main May 2, 2022
@uri-weisman uri-weisman deleted the refactor_resource_struct branch May 2, 2022 09:27
orouz pushed a commit to orouz/cloudbeat that referenced this pull request Sep 6, 2023
orestisfl pushed a commit to orestisfl/cloudbeat that referenced this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants