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

[PodSecurity] Update monitoring proposal #2990

Merged
merged 3 commits into from
Oct 5, 2021
Merged

[PodSecurity] Update monitoring proposal #2990

merged 3 commits into from
Oct 5, 2021

Conversation

tallclair
Copy link
Member

Update the PodSecurity KEP with some monitoring changes. These changes are motivated by corner cases encountered in the implementation: kubernetes/kubernetes#104217

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 23, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Sep 23, 2021
liggitt
liggitt reviewed Sep 23, 2021
View reviewed changes
keps/sig-auth/2579-psp-replacement/README.md
Comment on lines +627 to +629
request can increment this metric 3 times, once for each mode. `audit` and `warn` mode metrics
are only incremented for violations. If this admission controller is enabled, every
evaluated request will at least increment the `enforce` total.
Copy link
Member

Choose a reason for hiding this comment

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

if someone wanted to figure out the proportion of allowed/denied audit or warn requests, they'd now have to compare the number of denied audit or warn requests to the total number of mode=enforce requests, right? that could be ok, but is non-obvious

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. We could have a separate metric for tracking total evaluations, but that seems unnecessary. I agree it's non-obvious, but maybe it's something we can just add to the playbook...

@jyz0309 jyz0309 mentioned this pull request Sep 25, 2021
Merged
liggitt
liggitt reviewed Oct 5, 2021
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

lgtm

keps/sig-auth/2579-psp-replacement/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2579-psp-replacement/README.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor

dashpole commented Oct 5, 2021

/lgtm for instrumentation

@tallclair @liggitt
Apply suggestions from code review
cb1068c 
Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
@tallclair tallclair added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 5, 2021
@liggitt
Copy link
Member

liggitt commented Oct 5, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Oct 5, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1140e3b into kubernetes:master Oct 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Oct 5, 2021
hh pushed a commit to ii/keps that referenced this pull request Dec 7, 2021
@tallclair @liggitt @hh
[PodSecurity] Update monitoring proposal (kubernetes#2990)
4cfbe28 
* [PodSecurity] Update monitoring proposal

* fixup! [PodSecurity] Update monitoring proposal

* Apply suggestions from code review

Co-authored-by: Jordan Liggitt <jordan@liggitt.net>

Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
rikatz pushed a commit to rikatz/enhancements that referenced this pull request Feb 1, 2022
@tallclair @liggitt @rikatz
[PodSecurity] Update monitoring proposal (kubernetes#2990)
9fb16d7 
* [PodSecurity] Update monitoring proposal

* fixup! [PodSecurity] Update monitoring proposal

* Apply suggestions from code review

Co-authored-by: Jordan Liggitt <jordan@liggitt.net>

Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
@palnabarun palnabarun mentioned this pull request Jun 12, 2022
Closed
@palnabarun palnabarun mentioned this pull request Jul 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-urbaniak s-urbaniak s-urbaniak left review comments

@liggitt liggitt liggitt left review comments

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@liggitt liggitt

@dashpole dashpole

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/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
SIG Auth
Archived in project
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.
5 participants
@tallclair @dashpole @liggitt @k8s-ci-robot @s-urbaniak