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.

Sign up for GitHub

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

Jump to bottom

Added rest client metrics for client TTL and rot. #84382

Merged
merged 7 commits into from
Nov 23, 2019
Merged

Added rest client metrics for client TTL and rot. #84382

merged 7 commits into from
Nov 23, 2019

Conversation

sambdavidson
Copy link
Contributor

@sambdavidson sambdavidson commented Oct 25, 2019 •
edited
Loading

/sig auth
/sig instrumentation

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds prometheus metrics to the client-go rest transport to report the TTL and rotation cadence of the client TLS certificates. Improves monitoring capabilities into client certificates nearing expiry and/or issues with certificate refreshing.

Does this PR introduce a user-facing change?:

Added two client certificate metrics for exec auth:
    - `rest_client_certificate_expiration_seconds` a gauge reporting the lifetime of the current client certificate. Reports the time of expiry in seconds since January 1, 1970 UTC.
    - `rest_client_certificate_rotation_age` a histogram reporting the age of a just rotated client certificate in seconds.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 25, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @sambdavidson. 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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 25, 2019
@k8s-ci-robot 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. labels Oct 25, 2019
@sambdavidson
Copy link
Contributor Author

/assign @awly @logicalhan
/cc @mikedanese @liggitt @dashpole

@k8s-ci-robot k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 25, 2019
@awly
Copy link
Contributor

awly commented Oct 28, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 28, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2019
awly
awly reviewed Oct 28, 2019
View reviewed changes
Copy link
Contributor

@awly awly left a comment

Choose a reason for hiding this comment

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

/lgtm

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Nov 14, 2019

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2019
@liggitt liggitt added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, sambdavidson

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2019
@liggitt
Copy link
Member

liggitt commented Nov 15, 2019

just a BUILD file change

/lgtm
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 9dcb3bf into kubernetes:master Nov 23, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Nov 23, 2019
@liggitt
Copy link
Member

liggitt commented Nov 23, 2019

@logicalhan @sambdavidson master reopened for 1.18, you can update this with the gauge func we talked about in a follow up

@logicalhan
Copy link
Member

@logicalhan @sambdavidson master reopened for 1.18, you can update this with the gauge func we talked about in a follow up

Oh, it actually looks like the GaugeFunc was ported over already by @RainbowMango #83830

@RainbowMango
Copy link
Member

Yes. It is applicable to replace rest_client_exec_plugin_ttl_seconds with GaugeFunc.

Let me try this case and see if GaugeFunc works.

@sambdavidson sambdavidson mentioned this pull request Dec 4, 2019
Merged
plsmaop pushed a commit to plsmaop/kubernetes that referenced this pull request Jan 19, 2020
@sambdavidson @plsmaop
Added rest client metrics for client TTL and rot. (kubernetes#84382)
6fa1f14 
* Added rest client metrics for client TTL and rot.

* Fixed foo bar comment, added nil checks

* Moved rotation observation inside of old cert nil check block

* Fixed rotation age logic.

* fixed BUILD for exec plugin package

* fixed null pointer dereference in exec.go

* Updated metric name, bucket, used oldest cert.
@liggitt liggitt mentioned this pull request Feb 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@logicalhan logicalhan logicalhan left review comments

@liggitt liggitt liggitt left review comments

@brancz brancz brancz left review comments

@awly awly awly approved these changes

@jayunit100 jayunit100 Awaiting requested review from jayunit100

@lavalamp lavalamp Awaiting requested review from lavalamp

@dashpole dashpole Awaiting requested review from dashpole

@mikedanese mikedanese Awaiting requested review from mikedanese

Assignees

@logicalhan logicalhan

@liggitt liggitt

@awly awly

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.18
Development

Successfully merging this pull request may close these issues.
7 participants
@sambdavidson @k8s-ci-robot @awly @liggitt @logicalhan @RainbowMango @brancz