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

annotation metrics, what is the future direction? #941

Closed
kirkharris1 opened this issue Oct 9, 2019 · 37 comments
Closed

annotation metrics, what is the future direction? #941

kirkharris1 opened this issue Oct 9, 2019 · 37 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@kirkharris1
Copy link

kirkharris1 commented Oct 9, 2019

Is this a BUG REPORT or FEATURE REQUEST?:

/kind feature

What happened:

We make extensive use of kube_namespace_annotations which has been removed. This PR #859 says "rethink the annotation metrics in a separate PR". Reading around the PRs and Issues, the main concern seems to be that annotations can be numerous/large and security concerns. Would it be acceptable to reinstate annotation metrics, but with them off by default and enabled in the startup args? Making it optional would allow users that don't have security concerns and do not have a large number of annotations to use it.

If metrics carrying annotations will not be returning, then we will have to find another way internally so that we can upgrade beyond v1.6.0. For example, write an annotations exporter to run alongside kube-state. Before starting on that, i thought I would check to see if there are any plans/timescales that you can share.

Thanks

Environment:

  • Kubernetes version (use kubectl version): v1.15.3
  • Kube-state-metrics image version: > v1.8.0+
@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 9, 2019
@nugasescuserbun
Copy link

Should this be a BUG actually? The commit that was reverted is this one, which alongside the older kube_namespace_annotations added various other metrics for different resource (deployment, ingress etc). But the revert also removed the kube_namespace_annotations which is an older metric that we've ended up relying on.

@lilic
Copy link
Member

lilic commented Nov 4, 2019

Thanks for this, sorry we missed to respond to this. We should find a solution for this ASAP.

I believe allow-listing like we do with other things might be good approach. Thoughts? cc @tariq1890 @brancz

@brancz
Copy link
Member

brancz commented Nov 4, 2019

I think listing individual annotations that would be converted into metrics sounds appropriate.

@kirkharris1
Copy link
Author

I think listing individual annotations that would be converted into metrics sounds appropriate.

That would be great, it would fix the issue for our use case.

@lilic
Copy link
Member

lilic commented Nov 4, 2019

@kirkharris1 with conferences coming up, we probably won't have time to work on this right now. But if you want to write up a short proposal for this and then start working on it, you are most welcome to! Do you want me to assign this issue for you? :)

@kirkharris1
Copy link
Author

@kirkharris1 with conferences coming up, we probably won't have time to work on this right now. But if you want to write up a short proposal for this and then start working on it, you are most welcome to! Do you want me to assign this issue for you? :)

I can't do it, but i will ask around internally.

@jw-s
Copy link
Contributor

jw-s commented Nov 7, 2019

Hi @lilic @kirkharris1, in our company we are also affected by this. I am more than happy to work on this if you can't find anyone etc

@kirkharris1
Copy link
Author

@jw-s thank you

@lilic
Copy link
Member

lilic commented Nov 7, 2019

@jw-s sgtm, as agreed above we want to just allow individual annotations that do not contain any sensitive information and do not produce high unbound cardinality and convert those to metrics e.g. whitelist them in code. Let me know if you need any more info on this. Thank you!

/assign @jw-s

@jw-s
Copy link
Contributor

jw-s commented Nov 8, 2019

@lilic I suggest sticking with the previous (old) implementation of having one metric with each whitelisted annotation included as labels in this metric. With this proposal, we'd have to create a new cli flag specifically for this annotation unfortunately, or we go with a new metric per annotation then we can re-use the existing whitelist/blacklist cli flags. Thoughts?

@lilic
Copy link
Member

lilic commented Nov 8, 2019

@jw-s Yes we would need at least one new flag to specify the resource/annotation to whitelist, we do not want to blacklist approach here.

cc @brancz @tariq1890 for thoughts

@tariq1890
Copy link
Contributor

Yes. I agree with @lilic in that we choose the approach of having a white(Allow) list. That way, users are not at the mercy of Kubernetes implementations that use annotations to store sensitive information. This provides users more control over what annotation metrics they want to allow/block as opposed to exposing all annotations and risk leaking sensitive data when they least expect it. While adding a new CLI flag may be unfortunate, it's worth making that tradeoff for security.

@lpereiraa
Copy link

Hello there, good people at Kubernetes :)
Any development on this?
Thanks in advance!

@george-angel
Copy link

@jw-s
Copy link
Contributor

jw-s commented Jan 23, 2020

Hello there, good people at Kubernetes :)
Any development on this?
Thanks in advance!

I should have it finished this weekend!

@brancz
Copy link
Member

brancz commented Jan 24, 2020

Note we do want to reintroduce this feature but due to the various security concerns we want to make sure each annotation exposed is explicitly white listed.

@jw-s
Copy link
Contributor

jw-s commented Jan 24, 2020

@brancz My understanding was we introduce a new cli flag which is a whitelist for all annotation related metrics and of course, re-introduce the annotation related metrics as discussed. Is this not the case?

@brancz
Copy link
Member

brancz commented Jan 24, 2020

Yes as that way the user consciously chooses the annotations to expose.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 23, 2020
@george-angel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 23, 2020
@maran-maran
Copy link

Hello, we are using old(not so reliable) version due to lack of annotation metrics. Is there any chance to implement solution described above? Should we find another way how to get annotations?
Thanks

@brancz
Copy link
Member

brancz commented May 18, 2020

There are various approaches outlined/in progress that are linked to this issue, please contribute to move this forward :)

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 16, 2020
@george-angel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 17, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2020
@george-angel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2020
@lilic
Copy link
Member

lilic commented Nov 18, 2020

My two cents on this:

Adding annotation metrics can be possible if we use the same approach as we did with labels metrics, so e.g. kube_pod_labels. So we could have the same allow mechanism as is being now implemented for those metrics, see PR #1301 (comment). So for annotation metrics it could be --annotation-metric-allow-list flag and you pass resource name and annotation as it appears in kubernetes. By default this would be all disabled and users could just enable annotations they are interested in.

Note that this will not go into 2.0 release but we can already start the work and can be merged into master and can be included into the next release.

@brancz @tariq1890 and anyone on this thread, would this work for you?

@calexandre
Copy link

Completely agree on this.
Currently we use labels to annotate some namespaces with information regarding the owner of the namespace - an email for example. That information is then used on Prometheus + Alertmanager to provide hints of whom should be alerted for anything that happens beneath the namespace.

I don't think using Labels for this is the best approach and I would rather use annotations... so +1 for this feature.

@brancz
Copy link
Member

brancz commented Nov 26, 2020

@lilic your suggestion sounds perfect. 💯

@Mepemotrarial
Copy link

What is the status of this issue? I have exactly the same use-case as @calexandre (only for cron jobs).

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 6, 2021
@george-angel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 6, 2021
sepich added a commit to sepich/kube-state-metrics that referenced this issue Jun 4, 2021
@sepich
Copy link

sepich commented Jun 4, 2021

We're also affected by missing annotation metric (Labels are not enough, as they can only store [a-z0-9]).
I've took a chance to continue work of @jw-s, also addressing @lilic concerns from this thread and created new PR.
Would be glad somebody to review it, thanks.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 2, 2021
@george-angel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 9, 2021
@dgrisonnet
Copy link
Member

This feature was implemented in #1468 and is now available in kube-state-metrics v2.2.0 so we can close this issue.

/close

@k8s-ci-robot
Copy link
Contributor

@dgrisonnet: Closing this issue.

In response to this:

This feature was implemented in #1468 and is now available in kube-state-metrics v2.2.0 so we can close this issue.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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