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

mutating webhook: audit log mutation existence and patch #77824

Merged
merged 4 commits into from Aug 24, 2019

Conversation

@roycaihw
Copy link
Member

commented May 13, 2019

Issue:

Mutating webhooks can silently mutate objects and cause API requests to fail validation (#62666). Currently when a request goes through admission chain, kube-apiserver doesn’t log / store trace of 1. which mutating webhooks mutated the request; 2. what was changed in the request.

Moreover, when a mutated request fails validation, the validation error message doesn’t necessarily reveal the actual cause (mutating webhooks) for the request to be invalid (#65569 (comment)). This makes it hard to debug a Kubernetes cluster, when (misconfigured) mutating webhooks silently invalid valid requests and put the cluster in undesired states.

What this PR does:

This PR records name of mutating webhooks in audit log to help answer the question "which mutating webhooks mutated the request".

Does this PR introduce a user-facing change?:

Audit events now log the existence and patch of mutating webhooks. 
* At Metadata audit level or higher, an annotation with key "mutation.webhook.admission.k8s.io/round_{round idx}_index_{order idx}" gets logged with JSON payload indicating a webhook gets invoked for given request and whether it mutated the object or not.
* At Request audit level or higher, an annotation with key "patch.webhook.admission.k8s.io/round_{round idx}_index_{order idx}" get logged with the JSON payload logging the patch sent by a webhook for given request.

/sig api-machinery

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

/cc @tallclair
could you take a look to this one please?

@k8s-ci-robot k8s-ci-robot requested a review from tallclair May 13, 2019

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

/cc @caesarxuchao

I'm adding an e2e test use integration test instead

@k8s-ci-robot k8s-ci-robot requested a review from caesarxuchao May 13, 2019

@roycaihw roycaihw force-pushed the roycaihw:webhook-trace branch from 824f5af to 4c81f89 May 14, 2019

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

/retest

@sttts

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Is this blocked on anything?

@roycaihw roycaihw force-pushed the roycaihw:webhook-trace branch from 4c81f89 to 0958c79 May 28, 2019

@k8s-ci-robot k8s-ci-robot added size/M sig/auth and removed size/S labels May 28, 2019

@roycaihw roycaihw force-pushed the roycaihw:webhook-trace branch from 0958c79 to 0e8f6e4 May 28, 2019

@sttts

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

staging/src/k8s.io/apiserver/pkg/audit/request.go:230:28: "presist" is a misspelling of "persist"

@roycaihw roycaihw force-pushed the roycaihw:webhook-trace branch from 0e8f6e4 to b349e8e May 29, 2019

@k8s-ci-robot k8s-ci-robot removed the size/M label May 29, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

/hold

sorry for the hold, json construction needs a slight rework so we don't build potentially unsafe json

@liggitt liggitt moved this from Triage to Not required for GA in Admission Webhooks Aug 21, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 23, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

lgtm, go ahead and squash fixup commits

roycaihw added 3 commits May 31, 2019

@roycaihw roycaihw force-pushed the roycaihw:webhook-trace branch from efc4083 to 98ad20c Aug 23, 2019

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

squashed. Thanks for the review

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, roycaihw, sttts

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

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

/hold cancel

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

/retest

1 similar comment
@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 24, 2019

/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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

@roycaihw: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-conformance-kind-ipv6 98ad20c link /test pull-kubernetes-conformance-kind-ipv6

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit e2f57be into kubernetes:master Aug 24, 2019

23 of 24 checks passed

pull-kubernetes-conformance-kind-ipv6 Job failed.
Details
cla/linuxfoundation roycaihw authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@tedyu

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

@roycaihw
If you think the above two comments make sense, I can send out a PR.

Thanks

@liggitt liggitt moved this from Not required for GA to Complete in Admission Webhooks Aug 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.