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

Add possibility to add annotations to the metrics service #737

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

afreyermuth98
Copy link

@afreyermuth98 afreyermuth98 commented Sep 10, 2024

I want to add the possibilty to put annotations on the metrics service to be scraped by my OTEL collector

What type of PR is this?

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

/kind design

/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

What this PR does / why we need it:

We need it to scrape the metrics with an external service monitor

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

@Issif
Copy link
Member

Issif commented Sep 10, 2024

Hi,

To be a valid PR, a few steps remain:

  • bump up the chart version in Chart.yaml
  • add an entry in the CHANGELOG.md to explain reason of the new version
  • run make docs-falco to keep up to date the README.md

Moreover, wdyt to also add the possibility to ad custom labels, useful for the prometheus operator and else?

Thanks

@afreyermuth98
Copy link
Author

Of course, doing it !

@Issif
Copy link
Member

Issif commented Sep 10, 2024

Sorry, I forgot to mention you need to sign off your commits, you can squash them and sign only the remaining

@afreyermuth98
Copy link
Author

yes done ! @Issif ;)

@Issif
Copy link
Member

Issif commented Sep 10, 2024

There's an error in the CI:

TestPluginConfigurationInFalcoConfig/defaultValues 2024-09-10T15:36:45Z logger.go:66: Running command helm with args [template --set collectors.kubernetes.enabled=true --show-only templates/configmap.yaml rendered-resources /home/runner/work/charts/charts/charts/falco]
TestMetricsConfigInFalcoConfig/metricsEnabled 2024-09-10T15:36:45Z logger.go:66: Error: template: falco/templates/service.yaml:9:8: executing "falco/templates/service.yaml" at <include ".Values.metrics.service.labels" .>: error calling include: template: no template ".Values.metrics.service.labels" associated with template "gotpl"
TestMetricsConfigInFalcoConfig/metricsEnabled 2024-09-10T15:36:45Z logger.go:66: 
TestMetricsConfigInFalcoConfig/metricsEnabled 2024-09-10T15:36:45Z logger.go:66: Use --debug flag to render out invalid YAML
--- FAIL: TestMetricsConfigInFalcoConfig (0.00s)
    --- FAIL: TestMetricsConfigInFalcoConfig/Flip/Change_Values (0.06s)
        template.go:21: 
            	Error Trace:	/home/runner/go/pkg/mod/github.com/gruntwork-io/[email protected]/modules/helm/template.go:21
            	            				/home/runner/work/charts/charts/charts/falco/tests/unit/metricsConfig_test.go:165
            	Error:      	Received unexpected error:
            	            	error while running command: exit status 1; Error: template: falco/templates/service.yaml:9:8: executing "falco/templates/service.yaml" at <include ".Values.metrics.service.labels" .>: error calling include: template: no template ".Values.metrics.service.labels" associated with template "gotpl"
            	            	
            	            	Use --debug flag to render out invalid YAML
            	Test:       	TestMetricsConfigInFalcoConfig/Flip/Change_Values
    --- FAIL: TestMetricsConfigInFalcoConfig/metricsEnabled (0.09s)
        template.go:21: 
            	Error Trace:	/home/runner/go/pkg/mod/github.com/gruntwork-io/[email protected]/modules/helm/template.go:21
            	            				/home/runner/work/charts/charts/charts/falco/tests/unit/metricsConfig_test.go:165
            	Error:      	Received unexpected error:
            	            	error while running command: exit status 1; Error: template: falco/templates/service.yaml:9:8: executing "falco/templates/service.yaml" at <include ".Values.metrics.service.labels" .>: error calling include: template: no template ".Values.metrics.service.labels" associated with template "gotpl"
            	            	
            	            	Use --debug flag to render out invalid YAML
            	Test:       	TestMetricsConfigInFalcoConfig/metricsEnabled
TestDriverConfigInFalcoConfig/kind=auto 2024-09-10T15:36:45Z logger.go:66: Running command helm with args [template --set driver.kind=auto --show-only templates/configmap.yaml rendered-resources /home/runner/work/charts/charts/charts/falco]

charts/falco/templates/service.yaml Outdated Show resolved Hide resolved
@poiana
Copy link
Contributor

poiana commented Sep 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afreyermuth98
Once this PR has been reviewed and has the lgtm label, please ask for approval from issif. 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

Signed-off-by: afreyermuth98 <[email protected]>
Signed-off-by: afreyermuth98 <[email protected]>
@afreyermuth98
Copy link
Author

I have no idea 😭

Copy link
Member

@alacuku alacuku left a comment

Choose a reason for hiding this comment

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

Hey @afreyermuth98, thanks for the PR. I left some suggestions.

charts/falco/templates/service.yaml Show resolved Hide resolved
charts/falco/templates/service.yaml Show resolved Hide resolved
@afreyermuth98
Copy link
Author

@alacuku Reviews applied !

Signed-off-by: afreyermuth98 <[email protected]>
Signed-off-by: afreyermuth98 <[email protected]>
@alacuku
Copy link
Member

alacuku commented Sep 13, 2024

@afreyermuth98, the CI is now green 🥳

You need to squash the commits, and if you feel comfortable with golang you should add some unit tests. It should be simple, here are the tests for the serviceMonitor: https://github.com/falcosecurity/charts/blob/master/charts/falco/tests/unit/serviceMonitorTemplate_test.go

Let me know if you need help with the tests.

@afreyermuth98
Copy link
Author

afreyermuth98 commented Sep 13, 2024

@alacuku Thx 🙏
If would be happy to add some tests but yeah I'll maybe need some help ^^'
I need to create a new file such as serviceMetricsTemplate_test.go for example ?
Btw the commits can be squashed at PR merging isn't it ?

@alacuku
Copy link
Member

alacuku commented Sep 13, 2024

@alacuku Thx 🙏 If would be happy to add some tests but yeah I'll maybe need some help ^^' I need to create a new file such as serviceMetricsTemplate_test.go for example ?

Yeah, create the file and then you can adapt the tests from: https://github.com/falcosecurity/charts/blob/master/charts/falco/tests/unit/serviceMonitorTemplate_test.go

In this case, we need to check that:

  1. Using the default values the service is rendered and labels/annotations are the default one;
  2. Add labels/annotations and check that the rendered service has the custom labels/annotations that we added;

I suggest you test the labels and annotations separately.

Signed-off-by: afreyermuth98 <[email protected]>
@poiana poiana added size/L and removed size/S labels Sep 13, 2024
@afreyermuth98
Copy link
Author

afreyermuth98 commented Sep 13, 2024

I'm working on tests @alacuku but I have few questions :

  • Am I on the right way here ?
  • Do I need to then reapply values inside the same function to test with user inputted labels ?
  • How can I pass extra variables such as .metrics.service.labels = {"hello": "world"} etc ...
  • How can I get the values such as 0.38.2, falco-4.8.1 etc ... as they are dynamic
    Just need some help / hints here :)

@alacuku
Copy link
Member

alacuku commented Sep 13, 2024

  • How can I get the values such as 0.38.2, falco-4.8.1 etc ... as they are dynamic
    Just need some help / hints here :)

Here you go:

  • the function to get the charts info: https://github.com/falcosecurity/charts/blob/master/charts/k8s-metacollector/tests/unit/chartInfo.go. Copy it as is in the falco chart. In the future we will refactor the common code in it' s own package.
  • Function to test the default labels:
    func (s *commonMetaFieldsTest) TestLabels() {
    // Get chart info.
    cInfo, err := chartInfo(s.T(), s.chartPath)
    s.NoError(err)
    // Get app version.
    appVersion, found := cInfo["appVersion"]
    s.True(found, "should find app version in chart info")
    appVersion = appVersion.(string)
    // Get chart version.
    chartVersion, found := cInfo["version"]
    s.True(found, "should find chart version in chart info")
    // Get chart name.
    chartName, found := cInfo["name"]
    s.True(found, "should find chart name in chart info")
    chartName = chartName.(string)
    expectedLabels := map[string]string{
    "helm.sh/chart": fmt.Sprintf("%s-%s", chartName, chartVersion),
    "app.kubernetes.io/name": chartName.(string),
    "app.kubernetes.io/instance": s.releaseName,
    "app.kubernetes.io/version": appVersion.(string),
    "app.kubernetes.io/managed-by": "Helm",
    "app.kubernetes.io/component": "metadata-collector",
    }
    // Adjust the values to render all components.
    options := helm.Options{SetValues: map[string]string{"serviceMonitor.create": "true"}}
    for _, template := range s.templates {
    // Render the current template.
    output := helm.RenderTemplate(s.T(), &options, s.chartPath, s.releaseName, []string{template})
    // Unmarshal output to a map.
    var resource unstructured.Unstructured
    helm.UnmarshalK8SYaml(s.T(), output, &resource)
    labels := resource.GetLabels()
    s.Len(labels, len(expectedLabels), "should have the same number of labels")
    for key, value := range labels {
    expectedVal := expectedLabels[key]
    s.Equal(expectedVal, value)
    }
    }
    }

@alacuku
Copy link
Member

alacuku commented Sep 13, 2024

  • How can I pass extra variables such as .metrics.service.labels = {"hello": "world"} etc ...

An example how can you set custom variable:

options := &helm.Options{SetValues: map[string]string{"serviceMonitor.create": "true"}}

Signed-off-by: afreyermuth98 <[email protected]>
@afreyermuth98
Copy link
Author

@alacuku Here is the first draft for labels.
Maybe it's possible to factorize this ?
If it's ok for you I'll replicate for annotations ! :)

@alacuku
Copy link
Member

alacuku commented Sep 17, 2024

@alacuku Here is the first draft for labels. Maybe it's possible to factorize this ? If it's ok for you I'll replicate for annotations ! :)

The CI is failing. Could you have a look?

@afreyermuth98
Copy link
Author

Yes but I wanted to first know if the code seems ok at a first look for you :) @alacuku

@alacuku
Copy link
Member

alacuku commented Sep 17, 2024

Yes but I wanted to first know if the code seems ok at a first look for you :) @alacuku

Yeah, it's ok!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants