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

fix(falcosidekick): Allow overriding the Prometheus scrape Service annotation #779

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TiagoJMartins
Copy link

@TiagoJMartins TiagoJMartins commented Nov 11, 2024

What type of PR is this?

/kind bug
/kind chart-release

Any specific area of the project related to this PR?

/area falcosidekick-chart

What this PR does / why we need it:

We are working on a migration from a deprecated Prometheus setup to one based on the Prometheus Kubernetes Operator.
Since metrics scraped by the operator-managed instances are targeting ServiceMonitor and PodMonitor resources, we need to be able to configure the prometheus.io/scrape Service annotation to gradually disable scraping from our deprecated instances.

Which issue(s) this PR fixes:

Because the prometheus.io/scrape annotation is hard-coded in the Service template and it's the last entry in the annotations map, there's no way to include custom values after it.

This PR simply moves the hard-coded annotation to before the user-provided values, so at least they have an escape hatch in case they need to override the value.

Special notes for your reviewer:

You can test with:

helm template falcosidekick charts/falcosidekick -s templates/service.yaml -f - <<EOF | yq '.metadata.annotations["prometheus.io/scrape"]'
service:
  annotations:
    prometheus.io/scrape: "false"
EOF

The final template will include duplicate values, but the override will take precedence when the yaml gets parsed/applied to the cluster.

apiVersion: v1
kind: Service
metadata:
  name: falcosidekick
  annotations:
    prometheus.io/scrape: "true"
    prometheus.io/scrape: "false"
  ...

The duplicated keys will be removed by the cluster.

I considered going with a different approach based on something like .Values.service.prometheusScrape, but decided against it due to it requiring changes to the values.yaml structure.

Checklist

  • Chart Version bumped
  • CHANGELOG.md updated

@poiana poiana added do-not-merge/work-in-progress kind/bug Something isn't working dco-signoff: no kind/chart-release Add this label when the chart version has been bumped area/falcosidekick-chart labels Nov 11, 2024
@poiana
Copy link
Contributor

poiana commented Nov 11, 2024

Welcome @TiagoJMartins! It looks like this is your first PR to falcosecurity/charts 🎉

@poiana
Copy link
Contributor

poiana commented Nov 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TiagoJMartins
Once this PR has been reviewed and has the lgtm label, please assign issif for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

… on the Service resource

Signed-off-by: Tiago Martins <tiago.martins@hotjar.com>
@TiagoJMartins TiagoJMartins force-pushed the fix/sidekick-service-prom-annotation branch from bad73ed to f45a308 Compare November 11, 2024 13:46
@TiagoJMartins TiagoJMartins marked this pull request as ready for review November 11, 2024 13:46
@poiana poiana requested a review from Issif November 11, 2024 13:46
@rsicart
Copy link

rsicart commented Nov 13, 2024

And what about removing the hard-coded annotation and putting it by default in .Values.service.annotations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/falcosidekick-chart dco-signoff: yes kind/bug Something isn't working kind/chart-release Add this label when the chart version has been bumped size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants