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

Filter annotation "annotation_kubectl_kubernetes_io_last_applied_configuration" #854

Closed
towolf opened this issue Aug 1, 2019 · 8 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@towolf
Copy link

towolf commented Aug 1, 2019

Kube-state-metrics 1.7.1 adds new kube_*_annotations metrics.

I disabled most of them because that sounded really spammy.

--metric-blacklist="kube_[^i][a-z]+_annotations"

But I wanted to keep one exception: kube_ingress_annotations. There is no other way to capture the ingess class, than via the annotations: https://kubernetes.github.io/ingress-nginx/user-guide/multiple-ingress/

We use kubectl apply in our CI/CD pipeline and now every _annotations metric contains the full object itself in the annotation_kubectl_kubernetes_io_last_applied_configuration label.

Now, we don't store sensitive stuff in the objects, but I think this specific annotation should be excluded.

Is this a BUG REPORT or FEATURE REQUEST?:

/kind bug

What happened:

see above

...

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 1, 2019
@towolf
Copy link
Author

towolf commented Aug 1, 2019

Also, annotations are an uncontrolled field, where anything can go, very few contraints are placed on them. Just exposing them across the board is a very bold move.

@towolf towolf changed the title Filter "annotation_kubectl_kubernetes_io_last_applied_configuration" Filter annotation "annotation_kubectl_kubernetes_io_last_applied_configuration" Aug 1, 2019
@lilic
Copy link
Member

lilic commented Aug 2, 2019

Yes agreed, I am seeing those as well, we should filter the annotation_kubectl_kubernetes_io_last_applied_configuration out in the all the annotations. Unless anyone finds those useful? cc @tariq1890 @brancz

A full example of how the metric looks like with the annotation_kubectl_kubernetes_io_last_applied_configuration annotation included:

kube_deployment_annotations{namespace="default",deployment="hello-world",annotation_kubectl_kubernetes_io_last_applied_configuration="{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"metadata\":{\"annotations\":{},\"name\":\"hello-world\",\"namespace\":\"default\"},\"spec\":{\"replicas\":2,\"selector\":{\"matchLabels\":{\"run\":\"load-balancer-example\"}},\"template\":{\"metadata\":{\"labels\":{\"run\":\"load-balancer-example\"}},\"spec\":{\"containers\":[{\"image\":\"gcr.io/google-samples/node-hello:1.0\",\"name\":\"hello-world\",\"ports\":[{\"containerPort\":8080,\"protocol\":\"TCP\"}]}]}}}}\n",annotation_deployment_kubernetes_io_revision="1"} 1

@towolf
Copy link
Author

towolf commented Aug 2, 2019

Just one example, if you hardcode a password in env: then this would pop up in your Prometheus metrics. You shouldn't do that, bless your heart, but it could happen.

And there's probably no utility in having this field anyway other than to test the breaking point of Prometheus.

@tariq1890
Copy link
Contributor

Your concerns are valid @towolf. Here are some options that I can think of:

I) Perhaps, we could disable annotation metrics by default in kube-state-metrics and provide users the knobs to enable the specific annotation metrics they need. One way of achieving this is the white black listing approach.

ii) We could choose to only expose certain annotation metrics(create specific metric families) that are if significance to Kubernetes and useful to the users.

@lilic
Copy link
Member

lilic commented Aug 2, 2019

I would say for now we filter out the annotation_kubectl_kubernetes_io_last_applied_configuration from annotation metric and backport this to the 1.7 branch and cut a v1.7.2 release. I don't think this annotation was meant to be in that metric in the first place. This would at least solve the immediate problem. WDYT @tariq1890?

As for the actual solution, it would probably end up in the next major release, I would go with something along the lines of number 2. What were you thinking, something similar to black/whitelist flag we have? Lets maybe open a new issue to discuss this.

@tariq1890
Copy link
Contributor

Sounds good @lilic . We can create the patch to filter that specific annotation metric out.

@lilic
Copy link
Member

lilic commented Aug 6, 2019

Think we can close this, as the PR to remove the annotation metrics was merged. Thanks for the issue!

@lilic lilic closed this as completed Aug 6, 2019
@towolf
Copy link
Author

towolf commented Aug 6, 2019

Thanks for the fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

4 participants