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

helm: adding optional to expose tetragon-operator metrics #1797

Merged

Conversation

hungran
Copy link
Contributor

@hungran hungran commented Nov 26, 2023

hey team, I'm new here so please correct me if I'm missing somewhere

This will fix the issue #1778

  • port in the operator Deployment + a Service exposing it
  • Helm value configuring the operator metrics port
  • optional ServiceAccount scraping operator metrics following the issue -> not sure the 3rd point as I've seen we already had SA for TetragonOperator
Add a Kubernetes service exposing Tetragon Operator metrics. Make the operator metrics port configurable via Helm values and change the default to 2113.

@hungran hungran requested a review from a team as a code owner November 26, 2023 07:29
@hungran hungran requested a review from kevsecurity November 26, 2023 07:29
@hungran hungran changed the title adding optional to expose tetragon-operator metrics helm: adding optional to expose tetragon-operator metrics Nov 26, 2023
@kkourt kkourt requested a review from lambdanis November 27, 2023 09:09
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this @hungran. I left some comments about the Helm values.

install/kubernetes/values.yaml Outdated Show resolved Hide resolved
install/kubernetes/values.yaml Show resolved Hide resolved
Copy link

netlify bot commented Nov 27, 2023

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit b22126c
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/656c327579a0fe0009d3e82c
😎 Deploy Preview https://deploy-preview-1797--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hungran hungran requested a review from lambdanis November 27, 2023 13:55
@hungran
Copy link
Contributor Author

hungran commented Nov 28, 2023

indent level was re-corrected, @lambdanis might have chance to re-run the CI?

install/kubernetes/values.yaml Show resolved Hide resolved
install/kubernetes/values.yaml Show resolved Hide resolved
@lambdanis lambdanis added release-note/minor This PR introduces a minor user-visible change area/helm Related to the Helm chart labels Nov 28, 2023
@lambdanis
Copy link
Contributor

@hungran Thanks for the update, I requested one more change (to pass port to the operator via the configmap).

Also, please run cd install/kubernetes && ./test.sh to generate Helm docs. Otherwise the CI will fail like here: https://github.com/cilium/tetragon/actions/runs/7014110773/job/19099622794

Could you also squash the commits when you make changes? Thanks!

@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from b6302fb to 564b819 Compare November 29, 2023 03:27
Copy link
Contributor Author

@hungran hungran left a comment

Choose a reason for hiding this comment

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

@lambdanis not sure why must pass port to configmap as metrics-bind-address as we changed the default value to 2113 or here we can have a way that cmd.Flag() could have metrics-bind-address from config file (config map)? is this idea here?

noted: there is a change from test.sh as below, also generated README.md

install/kubernetes/test.sh Outdated Show resolved Hide resolved
@lambdanis
Copy link
Contributor

not sure why must pass port to configmap as metrics-bind-address as we changed the default value to 2113 or here we can have a way that cmd.Flag() could have metrics-bind-address from config file (config map)? is this idea here?

The operator configmap is mounted in the operator deployment, and the flags are read from there already. So to make it possible to use a non-default port for the metrics server, we need to pass a flag via the configmap.

install/kubernetes/test.sh Outdated Show resolved Hide resolved
install/kubernetes/test.sh Outdated Show resolved Hide resolved
@hungran
Copy link
Contributor Author

hungran commented Nov 30, 2023

not sure why must pass port to configmap as metrics-bind-address as we changed the default value to 2113 or here we can have a way that cmd.Flag() could have metrics-bind-address from config file (config map)? is this idea here?

The operator configmap is mounted in the operator deployment, and the flags are read from there already. So to make it possible to use a non-default port for the metrics server, we need to pass a flag via the configmap.

Hi @lambdanis Correct me if I'm wrong, the current TetragonOperator not yet read that flag though?
I've tried to used image the v1.0.0 from the hub and also built on my local and tried to deploy by difference port along with the configmap metrics-bind-address. However, seem like the metrics server still bind to the default port by cmd Flags as hardcode under operator/cmd/serve/serve.go

cmd.Flags().StringVar(&metricsAddr, "metrics-bind-address", ":2113", "The address the metric endpoint binds to.")

@hungran hungran requested a review from lambdanis December 1, 2023 10:13
@lambdanis
Copy link
Contributor

Hi @lambdanis Correct me if I'm wrong, the current TetragonOperator not yet read that flag though?
I've tried to used image the v1.0.0 from the hub and also built on my local and tried to deploy by difference port along with the configmap metrics-bind-address. However, seem like the metrics server still bind to the default port by cmd Flags as hardcode under operator/cmd/serve/serve.go

You're right, currently the operator doesn't read this flag from the configmap, thank you for testing it 🙇‍♀️ It does, however read this flag if it's passed as a command line argument, so we can pass it here in the deployment spec.

It seems some changes merged to main are conflicting with your changes (sorry about that), could you rebase your branch with main and force push? Please remove the merge commits and rebase instead, it seems that CI doesn't like these merges.

@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from 036efef to 40da16e Compare December 1, 2023 16:49
@hungran
Copy link
Contributor Author

hungran commented Dec 1, 2023

Hi @lambdanis Correct me if I'm wrong, the current TetragonOperator not yet read that flag though?
I've tried to used image the v1.0.0 from the hub and also built on my local and tried to deploy by difference port along with the configmap metrics-bind-address. However, seem like the metrics server still bind to the default port by cmd Flags as hardcode under operator/cmd/serve/serve.go

You're right, currently the operator doesn't read this flag from the configmap, thank you for testing it 🙇‍♀️ It does, however read this flag if it's passed as a command line argument, so we can pass it here in the deployment spec.

It seems some changes merged to main are conflicting with your changes (sorry about that), could you rebase your branch with main and force push? Please remove the merge commits and rebase instead, it seems that CI doesn't like these merges.

Thanks, it was run from my local environment by the container argument
rebase and updated, should be one more chance to run the full CI flow tho

@hungran hungran marked this pull request as draft December 1, 2023 17:10
@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from 40da16e to 1be331f Compare December 1, 2023 18:03
@hungran hungran marked this pull request as ready for review December 1, 2023 18:04
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

One indent in the service seems too big, but other than that changes look good :)

I see that the commit message contains multiple sign-offs. Could you amend it to keep only one at the end?

install/kubernetes/templates/operator_service.yaml Outdated Show resolved Hide resolved
@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from 1be331f to 870b69d Compare December 2, 2023 01:55
@hungran
Copy link
Contributor Author

hungran commented Dec 2, 2023

One indent in the service seems too big, but other than that changes look good :)

I see that the commit message contains multiple sign-offs. Could you amend it to keep only one at the end?

ah right, done

@hungran hungran requested a review from lambdanis December 2, 2023 01:59
@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from 870b69d to f47d778 Compare December 2, 2023 07:01
@lambdanis
Copy link
Contributor

@hungran It looks like you need to run install/kubernetes/test.sh once again, sorry.

@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from f47d778 to b22126c Compare December 3, 2023 07:46
@hungran
Copy link
Contributor Author

hungran commented Dec 3, 2023

@hungran It looks like you need to run install/kubernetes/test.sh once again, sorry.

no worries, a new local environment not yet has gnu-sed so the install/kubernetes/test.sh was failed btw :) updated

@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from b22126c to aee9508 Compare December 3, 2023 08:08
@hungran hungran force-pushed the pr/hungran-ISSUE-1778/helm-operator-metrics branch from aee9508 to efa7ca7 Compare December 3, 2023 08:09
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Thank you @hungran!

@lambdanis lambdanis merged commit a37a56f into cilium:main Dec 4, 2023
33 checks passed
@lambdanis lambdanis mentioned this pull request Dec 4, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Related to the Helm chart release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants