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

feat: added dahsboard and template #795

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

cobcan
Copy link
Contributor

@cobcan cobcan commented Dec 11, 2024

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

If this PR will release a new chart version please make sure to also uncomment the following line:

/kind chart-release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area falco-chart

/area falco-exporter-chart

/area falcosidekick-chart

/area falco-talon

/area event-generator-chart

/area k8s-metacollector

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Checklist

  • Chart Version bumped
  • Variables are documented in the README.md
  • CHANGELOG.md updated

@poiana poiana added dco-signoff: no kind/feature New feature or request kind/chart-release Add this label when the chart version has been bumped area/falcosidekick-chart labels Dec 11, 2024
@poiana poiana requested review from bencer and leogr December 11, 2024 15:58
@cobcan
Copy link
Contributor Author

cobcan commented Dec 11, 2024

@alacuku @Issif Sorry, I closed the other PR accidentally #792

@cobcan cobcan force-pushed the feat-falcosidekick-dashboard branch from ab8a88e to 1451af1 Compare December 11, 2024 16:15
@leogr
Copy link
Member

leogr commented Dec 12, 2024

@cobcan Thank you 🙏

I just noticed this, may you fix it please?

image

@leogr
Copy link
Member

leogr commented Dec 12, 2024

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

Hey @cobcan

To fix the DCO sign-off, just run the following:

git fetch upstream
git rebase --signoff -i upstream/main

Then git push --force-with-lease.

It's the last thing to fix. Then we can merge the PR. Thanks!

@leogr
Copy link
Member

leogr commented Dec 12, 2024

Hey @cobcan

I noticed the rebase did not work. I will fix it for you, please hold on.

@cobcan
Copy link
Contributor Author

cobcan commented Dec 12, 2024

@leogr

okay thank you

@leogr leogr force-pushed the feat-falcosidekick-dashboard branch from 34326d7 to f42fb0a Compare December 12, 2024 16:57
@leogr leogr force-pushed the feat-falcosidekick-dashboard branch 2 times, most recently from 46cf273 to 141277a Compare December 12, 2024 17:04
cobcan and others added 3 commits December 12, 2024 18:09
Co-authored-by: Alfonso Cobo <[email protected]>
Signed-off-by: Alfonso Cobo <[email protected]>
Co-authored-by: Alfonso Cobo <[email protected]>
Signed-off-by: Alfonso Cobo <[email protected]>
Co-authored-by: Alfonso Cobo <[email protected]>
Signed-off-by: Alfonso Cobo <[email protected]>
@leogr leogr force-pushed the feat-falcosidekick-dashboard branch from 141277a to 932e38d Compare December 12, 2024 17:09
@leogr
Copy link
Member

leogr commented Dec 12, 2024

@cobcan we should be ok now.

summoning @Issif for the last check and approval 🙏

@cobcan
Copy link
Contributor Author

cobcan commented Dec 12, 2024

@leogr

thanks for doing your git-magic 😄

Comment on lines 726 to 737
dashboards:
falcosidekickLoki:
# -- enabled specifies whether this dashboard should be deployed.
enabled: false
# --configmaps to be deployed that contain a grafana dashboard.
configMap:
# -- name specifies the name for the configmap.
name: falcosidekick-loki-dashboard-grafana
# -- namespace specifies the namespace for the configmap.
namespace: ""
# -- folder where the dashboard is stored by grafana.
folder: ""
Copy link
Member

Choose a reason for hiding this comment

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

there's a mismatch here, this output grafana creates annotations, this block should in the Loki section.

something like:

  loki:
    # -- Loki <http://host:port>, if not `empty`, Loki is *enabled*
    hostport: ""
    # -- user for Grafana Logs
    user: ""
    # -- API Key for Grafana Logs
    apikey: ""
    # -- Loki endpoint URL path, more info: <https://grafana.com/docs/loki/latest/api/#post-apiprompush>
    endpoint: "/loki/api/v1/push"
    # -- Loki tenant, if not `empty`, Loki tenant is *enabled*
    tenant: ""
    # -- comma separated list of fields to use as labels additionally to rule, source, priority, tags and custom_fields
    extralabels: ""
    # -- a list of comma separated custom headers to add, syntax is "key:value,key:value"
    customheaders: ""
    # -- minimum priority of event to use this output, order is `emergency\|alert\|critical\|error\|warning\|notice\|informational\|debug or ""`
    minimumpriority: ""
    # -- if true, checkcert flag will be ignored (server cert will always be checked)
    mutualtls: false
    # -- check if ssl certificate of the output is valid
    checkcert: true
    # -- dashboard for Grafana
    grafanaDashboard:
      # -- enabled specifies whether this dashboard should be deployed.
      enabled: false
      # --configmaps to be deployed that contain a grafana dashboard.
      configMap:
        # -- name specifies the name for the configmap.
        name: falcosidekick-loki-dashboard-grafana
        # -- namespace specifies the namespace for the configmap.
        namespace: ""
        # -- folder where the dashboard is stored by grafana.
        folder: ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the change suggested is done, but one question. What do you mean that "grafana create annotations"??

Copy link
Member

Choose a reason for hiding this comment

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

the section grafana is to configure the output of falcosidekick which creates annotations for the graphs with the falco events: https://grafana.com/docs/grafana/latest/dashboards/build-dashboards/annotate-visualizations/

Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

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

I see some points to improve:

  • this dashboard is for Loki but also relies on Prometheus to fill the variables for templating, can you have just Loki as dependency?
  • the dashboard is named Falco Pods, Falco logs should be more relevant
  • you use the label cluster for the template, are you sure it's always available, it's not preset either by Falco either by Falcosidekick
  • some panels don't have any name, 1 is called Panel Title
  • your count by for the table should also count by namespace to be exhaustive

@leogr
Copy link
Member

leogr commented Dec 12, 2024

@leogr

thanks for doing your git-magic 😄

np! If you plan to add more commits, I recommend you remove your local repo, and git clone it again (so you will be sure to be aligned with your fork)

Signed-off-by: Alfonso Cobo Canela <[email protected]>
@cobcan
Copy link
Contributor Author

cobcan commented Dec 12, 2024

@Issif

I see some points to improve:

  • this dashboard is for Loki but also relies on Prometheus to fill the variables for templating, can you have just Loki as dependency?
  • the dashboard is named Falco Pods, Falco logs should be more relevant
  • you use the label cluster for the template, are you sure it's always available, it's not preset either by Falco either by Falcosidekick
  • some panels don't have any name, 1 is called Panel Title
  • your count by for the table should also count by namespace to be exhaustive
  1. I've change it to get the namespaces from loki
  2. It's changed to Falco logs
  3. "cluster" was a variable from Prometheus to make de dashboard multi-cluster, but since Prometheus is removed is no longer needed
  4. I've added the label 'k8s_ns' to the table to check where namespace is each pod.
  5. Extra: I've modified the rates panels, the ones at the bottom of the dashboard, to be also filtered by namespace

@Issif
Copy link
Member

Issif commented Dec 13, 2024

/lgtm

@poiana
Copy link
Contributor

poiana commented Dec 13, 2024

LGTM label has been added.

Git tree hash: 297cb24ab3d5012275eac50e4e0fb55eb4754132

@poiana
Copy link
Contributor

poiana commented Dec 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cobcan, Issif, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit f3a8b2e into falcosecurity:master Dec 16, 2024
6 checks passed
@cobcan cobcan deleted the feat-falcosidekick-dashboard branch December 27, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/falcosidekick-chart dco-signoff: yes kind/chart-release Add this label when the chart version has been bumped kind/feature New feature or request lgtm size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants