Add collector for csr #650

wanghaoran1988 · 2019-01-31T11:34:03Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #649

mxinden · 2019-02-04T08:16:45Z

internal/collector/csr.go

+
+// addCSRConditionMetrics generates one metric for each possible csr condition status
+func addCSRConditionMetrics(cs certv1beta1.CertificateSigningRequestStatus) []*metric.Metric {
+	approved := false


A CSR can either be approved or denied, right? If so, I would encode this here by only having either a approved or denied boolean, and derive the opposite via the ! operator.

yes, this should be treated as an enum metric

mxinden · 2019-02-04T08:17:48Z

internal/collector/csr.go

+	approved := false
+	denied := false
+	if len(cs.Conditions) > 0 {
+		for _, s := range cs.Conditions {


Just to make sure, if a CSR has multiple conditions, do all of the apply or only the most recent one?

If it has multiple condition, then do all the apply, it can has "approve" and "denied" condition at the same time now.

wanghaoran1988 · 2019-02-05T12:26:16Z

@mxinden @brancz Updated according to https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/certificates/certificate_controller_utils.go#L23

brancz · 2019-02-05T15:38:02Z

internal/collector/csr.go

+	}
+	//"approved" if a certificate request has the
+	// "Approved" condition and no "Denied" conditions; false otherwise.
+	approved = approved && !denied


interpretation of data should be done at query time, not in kube-state-metrics, we should just expose each state with a 0 or 1 depending on whether it is in that state

A little confused, what do you mean by "query time" ? do you mean I can just remove this line? when the csr have "approved" and "denied" condition at the same time, should I expose the metrics both 1 like this:
kube_csr_condition{csr="certificate-test",csr_condition="denied"} 1
kube_csr_condition{csr="certificate-test",csr_condition="approved"} 1

@brancz if we don't interpret the data, there is the possibility for both approved and denied being true.

This upstream section seems to be the source of truth: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/certificates/certificate_controller_utils.go#L23

Correct, but here we want to expose the raw data. Essentially the approved && !denied is something that is up to a user to do in PromQL.

This make sense, and it's how I did originally

mxinden · 2019-02-06T09:01:53Z

internal/collector/csr.go

+func addCSRConditionMetrics(cs certv1beta1.CertificateSigningRequestStatus) []*metric.Metric {
+	approved := false
+	denied := false
+	if len(cs.Conditions) > 0 {


No need for this, if it is 0, the for loop will loop 0 times.

mxinden · 2019-02-06T09:02:19Z

internal/collector/csr.go

+	approved := false
+	denied := false
+	if len(cs.Conditions) > 0 {
+		for _, s := range cs.Conditions {


How about using GetCertApprovalCondition here instead?

I'd like a duplication here, or it will bring go dependency here, WDYT?

mxinden · 2019-02-06T09:02:31Z

internal/collector/csr.go

+	approved := false
+	denied := false
+	if len(cs.Conditions) > 0 {
+		for _, s := range cs.Conditions {


mxinden · 2019-02-06T14:55:43Z

In regards to the outdated discussion here: #650 (comment)

If we want to expose the raw data, how about also exposing the count of denied and approved conditions instead of just true or false?

@brancz @wanghaoran1988 what do you think?

brancz · 2019-02-06T15:04:08Z

Just true and false. Count is then something a user can do in PromQL 🙂 .

brancz · 2019-02-06T15:05:43Z

internal/collector/csr.go

+	return []*metric.Metric{
+		&metric.Metric{
+			LabelValues: []string{"issued"},
+			Value:       boolFloat64(len(cs.Certificate) > 0),


this is a state assessed based on data, not raw data, this should be done in PromQL

@brancz So how about expose a metrics kube_csr_cert_length ?

that works 👍

brancz · 2019-02-06T15:05:54Z

internal/collector/csr.go

+		},
+		&metric.Metric{
+			LabelValues: []string{"pending"},
+			Value:       boolFloat64(!(approved || denied)),


Same as abbove

wanghaoran1988 · 2019-02-06T15:06:31Z

Yes, we can do the "count" in PromQL

mxinden · 2019-02-06T15:10:38Z

I am not quite sure we are on the same page @brancz.

For the following CertificateSigningRequestStatus

CertificateSigningRequestStatus {
    []Condition {
        { approved }, { approved }, { denied }, { denied }, { denied },
    }
}

I am suggesting to expose the number of approvals and denies like:

kube_csr_condition{ condition="approved" } 2
kube_csr_condition{ conditiion="denied" } 3

Instead of what we currently do:

kube_csr_condition{ condition="approved" } 1
kube_csr_condition{ conditiion="denied" } 1

What do you think?

wanghaoran1988 · 2019-02-06T15:12:20Z

{ approved }, { approved }, { denied }, { denied }, { denied },

@mxinden This will not happen, we can only have one for each like: { approved }, { denied }

mxinden · 2019-02-06T15:21:30Z

The comment here states that there can be multiple denied conditions:

// IsCertificateRequestApproved returns true if a certificate request has the
// "Approved" condition and no "Denied" conditions; false otherwise.

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/certificates/certificate_controller_utils.go#L21-L22

Is there a specification somewhere?

wanghaoran1988 · 2019-02-06T15:24:37Z

@mxinden Did some test with running "kubectl certificate approve/deny" many time against on one csr, finally, it only have one "approve" and one "denied" condition.

wanghaoran1988 · 2019-02-12T08:42:45Z

@mxinden @brancz Could you take another look? I had a rebase again and addressed the comments.

tariq1890 · 2019-02-15T03:18:02Z

Documentation/README.md

@@ -62,7 +62,7 @@ Per group of metrics there is one file for each metrics. See each file for speci
 * [Secret Metrics](secret-metrics.md)
 * [ConfigMap Metrics](configmap-metrics.md)
 * [Ingress Metrics](ingress-metrics.md)
-
+* [CSR Metrics](csr-metrics.md)


Suggested change

* [CSR Metrics](csr-metrics.md)

* [CertificateSigningRequest Metrics](certificatesigningrequest-metrics.md)

Suggesting this, as the convention seems to be the resource type name rather than resource abbreviation

tariq1890 · 2019-02-15T03:18:56Z

internal/collector/csr.go

@@ -0,0 +1,152 @@
+/*


This should be renamed to certificatesigningrequest.go IMO

tariq1890 · 2019-02-15T03:19:44Z

internal/collector/csr.go

+)
+
+var (
+	descCSRLabelsName          = "kube_csr_labels"


Suggested change

descCSRLabelsName = "kube_csr_labels"

descCSRLabelsName = "kube_certificatesigningrequest_labels"

tariq1890 · 2019-02-15T03:21:35Z

Documentation/csr-metrics.md

+
+| Metric name| Metric type | Labels/tags | Status |
+| ---------- | ----------- | ----------- | ----------- |
+| kube_csr_created| Gauge | `csr`=&lt;csr-name&gt;| STABLE |


tariq1890 · 2019-02-15T03:22:01Z

internal/collector/builder.go

@@ -134,6 +135,7 @@ var availableCollectors = map[string]func(f *Builder) *coll.Collector{
 	"secrets":                  func(b *Builder) *coll.Collector { return b.buildSecretCollector() },
 	"services":                 func(b *Builder) *coll.Collector { return b.buildServiceCollector() },
 	"statefulsets":             func(b *Builder) *coll.Collector { return b.buildStatefulSetCollector() },
+	"csr":                      func(b *Builder) *coll.Collector { return b.buildCsrCollector() },


Suggested change

"csr": func(b *Builder) *coll.Collector { return b.buildCsrCollector() },

"certificatesigningrequests": func(b *Builder) *coll.Collector { return b.buildCsrCollector() },

tariq1890 · 2019-02-15T03:23:32Z

@mxinden @brancz I've added my non-maintainer review comments (apologize if it's inappropriate) for ensuring nomenclature consistency.

tariq1890 · 2019-02-15T03:24:45Z

@wanghaoran1988 make sure you run either go mod tidy or go mod vendor. That would fix one of the travic CI fails :).

wanghaoran1988 · 2019-03-12T05:50:10Z

Sorry, I just back from paternity leave, will have an update this week.

wanghaoran1988 · 2019-03-18T09:05:29Z

@mxinden @tariq1890 Could you take a look ? I rebased and renamed.

tariq1890 · 2019-03-18T16:28:30Z

docs/certificatessigningrequest-metrics.md

@@ -0,0 +1,8 @@
+# CSR Metrics


Suggested change

# CSR Metrics

# CertificateSigningRequest Metrics

@wanghaoran1988 Let's change this as well.

tariq1890 · 2019-03-18T16:29:31Z

docs/certificatessigningrequest-metrics.md

+
+| Metric name| Metric type | Labels/tags | Status |
+| ---------- | ----------- | ----------- | ----------- |
+| kube_certificatesigningrequest_created| Gauge | `csr`=&lt;csr-name&gt;| STABLE |


s/csr/certificatesigningrequest

tariq1890 · 2019-03-18T16:30:33Z

internal/collector/certificatesigningrequest.go

+var (
+	descCSRLabelsName          = "kube_certificatesigningrequest_labels"
+	descCSRLabelsHelp          = "Kubernetes labels converted to Prometheus labels."
+	descCSRLabelsDefaultLabels = []string{"csr"}


Suggested change

descCSRLabelsDefaultLabels = []string{"csr"}

descCSRLabelsDefaultLabels = []string{"certificatesigningrequest"}

wanghaoran1988 · 2019-03-20T12:06:05Z

comments addressed, @tariq1890 PTAL

tariq1890 · 2019-03-20T13:59:10Z

internal/collector/builder.go

+	"secrets":                    func(b *Builder) *coll.Collector { return b.buildSecretCollector() },
+	"services":                   func(b *Builder) *coll.Collector { return b.buildServiceCollector() },
+	"statefulsets":               func(b *Builder) *coll.Collector { return b.buildStatefulSetCollector() },
+	"certificatesigningrequests": func(b *Builder) *coll.Collector { return b.buildCsrCollector() },


This list actually follows alphabetical order. Let's continue that :)

Sorry to have missed this earlier.

tariq1890 · 2019-03-20T14:00:41Z

internal/collector/certificatesigningrequest_test.go

+	for i, c := range cases {
+		c.Func = metric.ComposeMetricGenFuncs(csrMetricFamilies)
+		if err := c.run(); err != nil {
+			t.Errorf("unexpected collecting result in %vth run:\n%s", i, err)


Suggested change

t.Errorf("unexpected collecting result in %vth run:\n%s", i, err)

t.Errorf("unexpected error when collecting result in %vth run:\n%s", i, err)

tariq1890 · 2019-03-20T14:02:28Z

@wanghaoran1988 Thanks for your patience! This should be my last round of review comments.

LGTM pending those comments and @brancz 's final word of approval.

wanghaoran1988 · 2019-03-21T00:26:45Z

@tariq1890 comments addressed

tariq1890 · 2019-03-21T00:33:41Z

docs/certificatessigningrequest-metrics.md

@@ -0,0 +1,8 @@
+# CertificatesSigningRequest Metrics


Suggested change

# CertificatesSigningRequest Metrics

# CertificateSigningRequest Metrics

there is an extra s.

tariq1890 · 2019-03-21T00:34:57Z

pkg/options/collector.go

-		"statefulsets":             struct{}{},
+		"configmaps":                 struct{}{},
+		"cronjobs":                   struct{}{},
+		"certificatesigningrequests": struct{}{},


can this go to the top?

tariq1890

/lgtm

tariq1890 · 2019-03-21T01:08:45Z

/assign @mxinden for final approval.

Possible to fit this into release 1.6 once merged?

brancz · 2019-03-21T15:01:28Z

lgtm but leaving final decision up to @mxinden whether to include in 1.6 or not

mxinden

Thanks for the work @wanghaoran1988!

mxinden · 2019-03-24T20:05:16Z

Let's include this in kube state metrics 1.6.

/lgtm

/approve

k8s-ci-robot · 2019-03-24T20:05:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mxinden, tariq1890, wanghaoran1988

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mxinden]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tariq1890 · 2019-03-24T20:08:08Z

Yes! Thanks once again @wanghaoran1988 :)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 31, 2019

k8s-ci-robot requested review from andyxning and zouyee January 31, 2019 11:34

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 31, 2019

mxinden reviewed Feb 4, 2019

View reviewed changes

wanghaoran1988 force-pushed the add_crs_collector branch from 0997048 to c917158 Compare February 5, 2019 12:37

brancz reviewed Feb 5, 2019

View reviewed changes

mxinden reviewed Feb 6, 2019

View reviewed changes

wanghaoran1988 force-pushed the add_crs_collector branch 2 times, most recently from de2dcea to d6ed2ee Compare February 6, 2019 14:24

brancz reviewed Feb 6, 2019

View reviewed changes

wanghaoran1988 force-pushed the add_crs_collector branch 2 times, most recently from 6989976 to 782083e Compare February 12, 2019 08:40

wanghaoran1988 force-pushed the add_crs_collector branch from 782083e to 5965270 Compare February 15, 2019 03:04

tariq1890 reviewed Feb 15, 2019

View reviewed changes

internal/collector/csr.go Outdated

@@ -0,0 +1,152 @@

/*

Copy link

Contributor

tariq1890 Feb 15, 2019

Choose a reason for hiding this comment

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

This should be renamed to certificatesigningrequest.go IMO

tariq1890 reviewed Feb 15, 2019

View reviewed changes

tariq1890 mentioned this pull request Mar 7, 2019

Discuss cutting kube-state-metrics v1.6.0 #697

Closed

wanghaoran1988 force-pushed the add_crs_collector branch 2 times, most recently from ff995cf to 4e5a9b1 Compare March 18, 2019 05:23

tariq1890 reviewed Mar 18, 2019

View reviewed changes

wanghaoran1988 force-pushed the add_crs_collector branch from 4e5a9b1 to df7727f Compare March 19, 2019 00:44

tariq1890 reviewed Mar 20, 2019

View reviewed changes

wanghaoran1988 force-pushed the add_crs_collector branch from df7727f to cac0765 Compare March 21, 2019 00:19

tariq1890 reviewed Mar 21, 2019

View reviewed changes

Add collector for csr

6793b68

wanghaoran1988 force-pushed the add_crs_collector branch from cac0765 to 6793b68 Compare March 21, 2019 01:04

k8s-ci-robot assigned tariq1890 Mar 21, 2019

tariq1890 approved these changes Mar 21, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2019

mxinden approved these changes Mar 24, 2019

View reviewed changes

k8s-ci-robot assigned mxinden Mar 24, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2019

k8s-ci-robot merged commit 907c526 into kubernetes:master Mar 24, 2019

dghubble mentioned this pull request Apr 10, 2019

Add list/watch access to ClusterRole for CSR collector #717

Merged

	* [CSR Metrics](csr-metrics.md)
	* [CertificateSigningRequest Metrics](certificatesigningrequest-metrics.md)

	descCSRLabelsName = "kube_csr_labels"
	descCSRLabelsName = "kube_certificatesigningrequest_labels"

	\| kube_csr_created\| Gauge \| `csr`=<csr-name>\| STABLE \|
	\| kube_certificatesigningrequest_created\| Gauge \| `csr`=<csr-name>\| STABLE \|

	"csr": func(b Builder) coll.Collector { return b.buildCsrCollector() },
	"certificatesigningrequests": func(b Builder) coll.Collector { return b.buildCsrCollector() },

	descCSRLabelsDefaultLabels = []string{"csr"}
	descCSRLabelsDefaultLabels = []string{"certificatesigningrequest"}

	t.Errorf("unexpected collecting result in %vth run:\n%s", i, err)
	t.Errorf("unexpected error when collecting result in %vth run:\n%s", i, err)

	# CertificatesSigningRequest Metrics
	# CertificateSigningRequest Metrics

Add collector for csr #650

Add collector for csr #650

Conversation

wanghaoran1988 commented Jan 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanghaoran1988 commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Feb 6, 2019

brancz commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanghaoran1988 commented Feb 6, 2019

mxinden commented Feb 6, 2019

wanghaoran1988 commented Feb 6, 2019

mxinden commented Feb 6, 2019

wanghaoran1988 commented Feb 6, 2019

wanghaoran1988 commented Feb 12, 2019

tariq1890 Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq1890 commented Feb 15, 2019

tariq1890 commented Feb 15, 2019

wanghaoran1988 commented Mar 12, 2019

wanghaoran1988 commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanghaoran1988 commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq1890 commented Mar 20, 2019

wanghaoran1988 commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq1890 left a comment

Choose a reason for hiding this comment

tariq1890 commented Mar 21, 2019

brancz commented Mar 21, 2019

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Mar 24, 2019

k8s-ci-robot commented Mar 24, 2019

tariq1890 commented Mar 24, 2019

tariq1890 Feb 15, 2019 •

edited

Loading