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

Ensure that the k8s annotation-based discovery cannot target other endpoints #36595

Open
dmitryax opened this issue Nov 29, 2024 · 2 comments
Open
Labels

Comments

@dmitryax
Copy link
Member

dmitryax commented Nov 29, 2024

Is your feature request related to a problem? Please describe.

#35617 introduced a new way to create scrapers from particular k8s annotations. The annotations can include the configuration of the scraper to be created. It's important that the configuration provided in the annotation cannot create a scraper that would scrape data from any sources other than the annotated pod. Currently, we have a validation relying on the assumption that every scraper has endpoint configuration field. However, it's not always the case. For example, TLS Check Receiver has targets.url field for that purpose. In this particular case, the validation must check that only one target is set and the target matches the endpoint with the annotation.

Describe the solution you'd like

Every scrape should explicitly support the discovery and provide a validation function, even if it's primarily identical for scrapers with the endpoint config field. Scrapers that do not explicitly provide declare that should not be supported by the discovery mechanism.

We can introduce a new interface that scrapers supporting discovery have to introduce.

type Discoverable interface {
	// Validate validates the rawCfg unmarshals to a config making the scraper receiving data only from discoveredEndpoint.
	Validate(rawCfg map[string]any, discoveredEndpoint string)
}

Most of the scrapers should be able to use a common function built on https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/35617/files#diff-2db19e270904db9b7d87966c13aaa399b48566a81f520f91fa8af8032fdc9827R184

cc @ChrsMark

Copy link
Contributor

Pinging code owners for receiver/receivercreator: @dmitryax. See Adding Labels via Comments if you do not have permissions to add labels yourself. For example, comment '/label priority:p2 -needs-triaged' to set the priority and remove the needs-triaged label.

@dmitryax dmitryax changed the title Ensure that the k8s annotation-based discovery works with all scrapers Ensure that the k8s annotation-based discovery cannot target other endpoints Nov 29, 2024
@ChrsMark
Copy link
Member

Thank's for checking this @dmitryax!

While I see the point of introducing such an interface, I'm not 100% sure if this is an actual problem right now though.

In the current implementation we set as default endpoint the discovered Pod's ip:port at

conf[endpointConfigKey] = defaultEndpoint
. This is also mentioned in the docs. Users can override this through the annotation but all generated configs come with endpoint: setting anyways.

Hence, scrapers that do not support the endpoint setting will fail to get validated anyways because endpoint: key will be invalid key for their configuration.

For example for a redis config that includes a random setting like the following:

io.opentelemetry.discovery.metrics.6379/config: |
  collection_interval: "20s"
  timeout: "10s"
  some: foo

We get an error like this:

2024-11-29T09:14:53.107Z	info	[email protected]/observerhandler.go:201	starting receiver	{"kind": "receiver", "name": "receiver_creator/metrics", "data_type": "metrics", "name": "redis/0f2c06b9-bef5-4245-b31e-5263097ff45e_6379", "endpoint": "10.244.0.7:6379", "endpoint_id": "k8s_observer/0f2c06b9-bef5-4245-b31e-5263097ff45e/redis(6379)", "config": {"collection_interval":"20s","endpoint":"10.244.0.7:6379","some":"foo","timeout":"10s"}}
2024-11-29T09:14:53.116Z	error	[email protected]/observerhandler.go:217	failed to start receiver	{"kind": "receiver", "name": "receiver_creator/metrics", "data_type": "metrics", "receiver": "redis/0f2c06b9-bef5-4245-b31e-5263097ff45e_6379", "error": "failed to load \"redis/0f2c06b9-bef5-4245-b31e-5263097ff45e_6379\" template config: decoding failed due to the following error(s):\n\n'' has invalid keys: some"}
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/receivercreator.(*observerHandler).startReceiver
	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/[email protected]/observerhandler.go:217
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/receivercreator.(*observerHandler).OnAdd
	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/[email protected]/observerhandler.go:93
github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer.(*EndpointsWatcher).updateAndNotifyOfEndpoints
	github.com/open-telemetry/opentelemetry-collector-contrib/extension/[email protected]/endpointswatcher.go:111

Not all receivers make sense for monitoring K8s Pod workloads, TLS Check Receiver feels a bit like one of them (it's still under development and not even part of the contrib distro). Curious though if we have more like this one 🤔 .

At the same time Collector operators can always use the ignore_receivers setting to disable receivers/scrapers that do not make sense to them or come with security risks.

Either way, maybe we can evaluate this better once we know more about open-telemetry/opentelemetry-collector#11238 (comment)?

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

No branches or pull requests

2 participants