exec credential provider: add rest_client_exec_plugin_call_total metric #98892

ankeesler · 2021-02-08T20:27:07Z

Signed-off-by: Andrew Keesler akeesler@vmware.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add rest_client_exec_plugin_call_total metric to client-go which tracks the number of calls to any exec plugin and partitions the calls by exit code.

Which issue(s) this PR fixes:

This is in service of getting the client-go credential plugin feature to GA: External client-go credential providers enhancements#541.
Here are the metrics described in the KEP: https://github.com/kubernetes/enhancements/tree/7e69f3be9783e33f597b02de8c952b6eca1e8273/keps/sig-auth/541-external-credential-providers#metrics.
- Note that 2 of the metrics have already been implemented, and this PR is adding the 3rd.

Does this PR introduce a user-facing change?:

A client-go metric, rest_client_exec_plugin_call_total, has been added to track total calls to client-go credential plugins.

k8s-ci-robot · 2021-02-08T20:27:15Z

Hi @ankeesler. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ankeesler · 2021-02-08T20:27:19Z

/sig auth

ankeesler · 2021-02-09T14:51:37Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

+	execPluginCalls = k8smetrics.NewCounterVec(
+		&k8smetrics.CounterOpts{
+			Name: "rest_client_exec_plugin_call_total",
+			Help: "Number of calls to an exec plugin, partitioned by exit code. A special exit code of -1 " +


I wanted to document a contract for what the exec authenticator does when it encounters exec plugin errors that it doesn't know how to process (e.g., the go stdlib failed to copy IO streams (very unlikely), the user does not have permission to exec the plugin). I figured this was a good enough place to document it since this help text will most likely be displayed to prometheus metric users in some fashion? For users that have a different metrics system...not sure where to document this contract...

I think we are safe with using negative numbers as special values, since POSIX uses the low-order 8 (unsigned) bits of an int return value to indicate the status, and Windows treats return values as unsigned integers. See https://en.wikipedia.org/wiki/Exit_status.

I think we do this elsewhere too actually.

@liggitt may know?

Another question here - do we need to document this metric publicly in any k/website (or elsewhere) docs? I am not familiar with how kube metrics are documented.

I think we are safe with using negative numbers as special values, since POSIX uses the low-order 8 (unsigned) bits of an int return value to indicate the status, and Windows treats return values as unsigned integers. See https://en.wikipedia.org/wiki/Exit_status.

...
I think we do this elsewhere too actually.
@liggitt may know?

umm.... I ... don't know

do we need to document this metric publicly in any k/website (or elsewhere) docs?

In the KEP is a good starting point (https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/541-external-credential-providers/README.md#metrics). including in the user-doc as well would make sense to me

ankeesler · 2021-02-09T15:00:42Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go

+	"k8s.io/klog/v2"
+)
+
+// The following constants shadow the special values used in the prometheus metrics implementation.


We can lock this contract in place with an integration test that uses prometheus metrics?

ankeesler · 2021-02-09T15:10:41Z

/cc @liggitt @enj @logicalhan @ehashman

ankeesler · 2021-02-18T16:23:23Z

/test pull-kubernetes-integration

logicalhan

The metrics looks okay to me, but I find the test a bit difficult to parse (mostly because of there is so much state).

logicalhan · 2021-02-18T20:05:29Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go

+	pluginNotFoundExitCode = -1
+	// pluginReturnedStrangeErrorExitCode is the exit code passed to the metrics.ExecPluginCalls counter when a
+	// call to an exec plugin returns an error that the code does not understand.
+	pluginReturnedStrangeErrorExitCode = -2


nit: s/pluginReturnedStrangeErrorExitCode/unrecognizedPluginExitCode/

logicalhan · 2021-02-18T20:17:14Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

+	execPluginCalls = k8smetrics.NewCounterVec(
+		&k8smetrics.CounterOpts{
+			Name: "rest_client_exec_plugin_call_total",
+			Help: "Number of calls to an exec plugin, partitioned by exit code. A special exit code of -1 " +


I think we do this elsewhere too actually.

@liggitt may know?

ankeesler · 2021-02-22T14:08:44Z

/test pull-kubernetes-e2e-kind-ipv6

I think that test flake was this: #99147.

ankeesler · 2021-02-22T14:09:08Z

@liggitt - any chance you have time this week to do a sweep of this PR?

liggitt · 2021-02-23T18:38:01Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

+	execPluginCalls = k8smetrics.NewCounterVec(
+		&k8smetrics.CounterOpts{
+			Name: "rest_client_exec_plugin_call_total",
+			Help: "Number of calls to an exec plugin, partitioned by exit code. A special exit code of -1 " +


I think we are safe with using negative numbers as special values, since POSIX uses the low-order 8 (unsigned) bits of an int return value to indicate the status, and Windows treats return values as unsigned integers. See https://en.wikipedia.org/wiki/Exit_status.

...
I think we do this elsewhere too actually.
@liggitt may know?

umm.... I ... don't know

do we need to document this metric publicly in any k/website (or elsewhere) docs?

In the KEP is a good starting point (https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/541-external-credential-providers/README.md#metrics). including in the user-doc as well would make sense to me

liggitt · 2021-02-23T18:45:33Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go

+	// pluginNotFoundExitCode is the exit code passed to the metrics.ExecPluginCalls counter when a
+	// call to an exec plugin fails because the exec plugin is not found.
+	pluginNotFoundExitCode = -1
+	// unrecognizedPluginExitCode is the exit code passed to the metrics.ExecPluginCalls counter when
+	// a call to an exec plugin returns an error that the code does not understand.
+	unrecognizedPluginExitCode = -2


this seems overly clever to me... admission webhooks have a similar issue where an API call can be rejected in admission because 1) there was an API server error, 2) there was an error invoking the webhook, or 3) the admission webhook was called and correctly rejected the request.

They represent those cases with an extra label in the webhook_rejection_count metric (error_type = calling_webhook_error | apiserver_internal_error | no_error), rather than try to overload the code label

thanks @liggitt - i took a pass at this in e0b334b4204d62a3e7b13d6c20e15c226438624c

i tried to take the concept you shared and adapt it to this particular metric that we are mucking with. hopefully i got it somewhere close to where we want it.

logicalhan

/lgtm
/approve

liggitt · 2021-03-03T13:35:24Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go

+// incrementCallsMetric increments a global metrics counter for the number of calls to an exec
+// plugin, partitioned by exit code. The provided err should be the return value from
+// exec.Cmd.Run().
+func incrementCallsMetric(err error) {


suggest using https://golang.org/pkg/errors/#As for these two error types to be able to handle wrapped errors

18b42aeab316651173d904b57827099712e6493f

liggitt · 2021-03-03T13:37:41Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go

+		metrics.ExecPluginCalls.Increment(err.(*exec.ExitError).ExitCode(), pluginCalledStatus)
+
+	case *exec.Error: // Binary does not exist (see exec.Error).
+		metrics.ExecPluginCalls.Increment(0, pluginNotFoundStatus)


suggest setting code to 1 for pluginNotFoundStatus and clientInternalError... that allows someone to monitor non-0 codes for all unsuccessful invocations, and partition by the event type to understand the reason for the failure

2da2732ae9cd5e83b01fceb50ce1c4606e0add28

liggitt · 2021-03-03T13:41:35Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

+				"client_internal_error) and an optional exit code which is set with " +
+				"plugin_called.",
+		},
+		[]string{"code", "call_status"},


is call_status a good description for this label, or would error_type be clearer (and set to no_error when err is nil)?

i went back and forth with myself on call_status vs error_type. i think i landed on call_status since it better matched the metric name (rest_client_exec_plugin_*call*_total) and so we could get away with 3 enum values for this field instead of 4. i figured that if someone wanted to filter down to all the unsuccessful executions, they could just say exitCode != 0 && call_status == plugin_called.

i'm not totally wed to this. i'll switch it over to no_error and just let me know if you change your mind: 9e947251851a1b44ae4c7c11457932cf151b1467

liggitt · 2021-03-03T13:44:02Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go

+		metrics.ExecPluginCalls.Increment(0, pluginNotFoundStatus)
+
+	default: // We don't know about this error type.
+		klog.Warningf("unexpected exec plugin return error type: %T (error = %q)", err, err.Error())


log lines in a metrics record method are a little unexpected... I'd probably drop this... the only thing it affects is the recorded metric, and people who care will be watching the metric, right?

edit: I guess we'd maybe want to know the type so we can fix the unexpected type, but this seems like it could be a lower-level message... Info level 2 or something

e921dc9049ed37fd53424c2aeaea42df9f4db718

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

liggitt · 2021-03-03T15:19:11Z

pull-kubernetes-dependencies failure is unrelated to this PR and is being worked on

/lgtm
/approve

k8s-ci-robot · 2021-03-03T15:19:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankeesler, liggitt, logicalhan

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~staging/src/k8s.io/component-base/metrics/OWNERS~~ [liggitt,logicalhan]

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

fejta-bot · 2021-03-03T17:58:23Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

ankeesler · 2021-03-03T21:11:51Z

i heard a rumor these test issues were resolved now? let's see

/retest

dims · 2021-03-04T02:07:59Z

/hold

dims · 2021-03-04T02:12:16Z

/hold cancel
/retest

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 8, 2021

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 8, 2021

k8s-ci-robot requested review from coffeepac and lavalamp February 8, 2021 20:27

ankeesler mentioned this pull request Feb 8, 2021

External client-go credential providers kubernetes/enhancements#541

Closed

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Feb 8, 2021

ankeesler force-pushed the exec-plugin-metrics branch from 7d4ae6b to b2f3409 Compare February 9, 2021 14:42

ankeesler commented Feb 9, 2021

View reviewed changes

ankeesler force-pushed the exec-plugin-metrics branch from b2f3409 to 7308c50 Compare February 9, 2021 15:00

ankeesler commented Feb 9, 2021

View reviewed changes

ankeesler changed the title ~~WIP: exec credential provider: add rest_client_exec_plugin_call_total metric~~ exec credential provider: add rest_client_exec_plugin_call_total metric Feb 9, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2021

k8s-ci-robot requested review from ehashman, enj, liggitt and logicalhan February 9, 2021 15:10

k8s-ci-robot assigned RainbowMango Feb 10, 2021

liggitt added this to the v1.21 milestone Feb 18, 2021

logicalhan reviewed Feb 18, 2021

View reviewed changes

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 19, 2021

liggitt reviewed Feb 23, 2021

View reviewed changes

ankeesler force-pushed the exec-plugin-metrics branch from dd21c7c to e0b334b Compare March 1, 2021 20:21

logicalhan approved these changes Mar 2, 2021

View reviewed changes

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

liggitt reviewed Mar 3, 2021

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2021

exec credential provider: add rest_client_exec_plugin_call_total metric

31eec29

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

ankeesler force-pushed the exec-plugin-metrics branch from 9e94725 to 31eec29 Compare March 3, 2021 14:54

k8s-ci-robot assigned liggitt Mar 3, 2021

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

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2021

k8s-ci-robot merged commit cd54b19 into kubernetes:master Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exec credential provider: add rest_client_exec_plugin_call_total metric #98892

exec credential provider: add rest_client_exec_plugin_call_total metric #98892

ankeesler commented Feb 8, 2021

k8s-ci-robot commented Feb 8, 2021

ankeesler commented Feb 8, 2021

ankeesler Feb 9, 2021 •

edited

logicalhan Feb 18, 2021

ankeesler Feb 22, 2021

liggitt Feb 23, 2021

ankeesler Feb 9, 2021

ankeesler commented Feb 9, 2021

ankeesler commented Feb 18, 2021

logicalhan left a comment

logicalhan Feb 18, 2021

ankeesler Feb 19, 2021

logicalhan Feb 18, 2021

ankeesler commented Feb 22, 2021

ankeesler commented Feb 22, 2021

liggitt Feb 23, 2021

liggitt Feb 23, 2021

ankeesler Mar 1, 2021

logicalhan left a comment

liggitt Mar 3, 2021

ankeesler Mar 3, 2021

liggitt Mar 3, 2021

ankeesler Mar 3, 2021

liggitt Mar 3, 2021

ankeesler Mar 3, 2021

liggitt Mar 3, 2021 •

edited

ankeesler Mar 3, 2021

liggitt commented Mar 3, 2021 •

edited

k8s-ci-robot commented Mar 3, 2021

fejta-bot commented Mar 3, 2021

ankeesler commented Mar 3, 2021

dims commented Mar 4, 2021

dims commented Mar 4, 2021

exec credential provider: add rest_client_exec_plugin_call_total metric #98892

exec credential provider: add rest_client_exec_plugin_call_total metric #98892

Conversation

ankeesler commented Feb 8, 2021

k8s-ci-robot commented Feb 8, 2021

ankeesler commented Feb 8, 2021

ankeesler Feb 9, 2021 • edited

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

ankeesler commented Feb 9, 2021

ankeesler commented Feb 18, 2021

logicalhan left a 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

ankeesler commented Feb 22, 2021

ankeesler commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan left a 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

liggitt Mar 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Mar 3, 2021 • edited

k8s-ci-robot commented Mar 3, 2021

fejta-bot commented Mar 3, 2021

ankeesler commented Mar 3, 2021

dims commented Mar 4, 2021

dims commented Mar 4, 2021

ankeesler Feb 9, 2021 •

edited

liggitt Mar 3, 2021 •

edited

liggitt commented Mar 3, 2021 •

edited