support annotations for admission webhook by CaoShuFeng · Pull Request #58679 · kubernetes/kubernetes

CaoShuFeng · 2018-01-23T10:45:07Z

Depends on: #58143
Release note:

Support annotations for remote admission webhooks.

CaoShuFeng · 2018-01-28T11:42:36Z

@liggitt @tallclair @sttts This is also ready for review.
I had deployed a little remote admission controller to test it.

sttts · 2018-01-30T15:13:01Z

staging/src/k8s.io/api/admission/v1beta1/types.go

there should be a specified format. What are the keys?

This Annotation is a little bit different from annotation in admission attribute.
It doesn't contain the fully qualified plugin name.

I use this plugin name to call attributesRecord.SetAnnotations().

sttts · 2018-01-30T15:13:46Z

staging/src/k8s.io/apiserver/pkg/admission/attributes.go

plugin_name + ".admission.k8s.io/" + key ?

We also have to check that the key is a DNS segment.

We also have to check that the key is a DNS segment.

Done.

plugin_name + ".admission.k8s.io/" + key ?

plugin_name is already fully qualified.
Webhook plugin do not use .admission.k8s.io in their plugin name, I think.

sttts · 2018-01-30T15:15:02Z

staging/src/k8s.io/apiserver/pkg/admission/interfaces.go

can this be queried by plugins in the chain? I don't like to open up this channel without good reasons.

sttts · 2018-01-30T15:16:19Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/admission.go

this is ugly

What about we move this mutex into SetAnnotations()?

yes. And get rid of the single k/v SetAnnotations variant, but only for a whole map.

sttts · 2018-01-31T11:07:03Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/admission.go

these names can theoretically overlap with the built in names. I guess we need h.Name + ".mutatingwebhook".

This h.Name is already full qualified, like imagepolicy.kubernetes.io.
This name is set by cluster administrator.
What about we trust cluster administrator that he will not set a name overlap with the built in names?

kubernetes/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go

Line 126 in f3942e7

// Name should be fully qualified, e.g., imagepolicy.kubernetes.io, where

Not sure it's obvious that this name shows up in auditing. But imagepolicy.kubernetes.io.mutatingwebhook.admission.k8s.io is not nice either.

What about we ignore h.Name?
And allow remote admission to set pluginName themselves?
If so, remote admsssion will be consistent with the build-in admission.

we could also enforce that webhooks use *.mutatingwebhook.admission.k8s.io as key.

CaoShuFeng · 2018-07-10T13:18:24Z

/test pull-kubernetes-e2e-kops-aws

tallclair · 2018-07-12T00:59:00Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

Should we consider this an admission error, or just log the error to the debug log? I guess considering it an admission error is failing closed.

Actually, looking more closely I think this should be:

&webhookerrors.ErrCallingWebhook{WebhookName: h.name, Reason: err}

This type of error is either ignored or blocking depending on the webhook configuration. It is important to be able to ignore errors to avoid a misconfigured webhook from locking up the cluster.

or just log the error to the debug log?

Done.

&webhookerrors.ErrCallingWebhook{WebhookName: h.name, Reason: err}

ErrCallingWebhook is returned for transport-layer errors calling webhooks. It represents a failure to talk to the webhook. So I log the error to the debug log rather than use ErrCallingWebhook

Actually, looking more closely I think this should be:
&webhookerrors.ErrCallingWebhook{WebhookName: h.name, Reason: err}

Done. And I removed the transport-layer description about ErrCallingWebhook in comment.
Thanks.

tallclair · 2018-07-12T01:10:03Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

Instead of adding yet another argument to this, prefer exposing FakeAttributes and FakeAttributes.Annotations (or if you prefer, FakeAttributes.GetAnnotations)

tallclair · 2018-07-12T01:10:34Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

nit: initialize the map in this case, especially if exposing it directly.

tallclair · 2018-07-12T01:13:29Z

pkg/apis/admission/types.go

Add more about the intent, e.g. Annotations may be provided by the admission webhook to add additional context to the audit log for this request

tallclair · 2018-07-12T01:13:54Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go

ditto (ErrCallingWebhook)

CaoShuFeng · 2018-07-12T07:15:12Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

@tallclair Thanks for you review.
What do you think about current behavior?

tallclair · 2018-07-27T00:42:09Z

/milestone v1.12
/status approved-for-milestone

tallclair · 2018-07-27T00:47:05Z

/retest

tallclair · 2018-07-27T00:47:12Z

/lgtm

sttts · 2018-07-27T05:56:20Z

/assign @liggitt

for api approval.

k8s-github-robot · 2018-07-27T05:56:51Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@CaoShuFeng @liggitt @tallclair

Pull Request Labels

sig/api-machinery sig/auth: Pull Request will be escalated to these SIGs if needed.
priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
kind/feature: New functionality.

Help

liggitt · 2018-07-27T14:41:55Z

pkg/apis/admission/types.go

will this be confused with the Annotations field on the API object being admitted? If we leave this named "Annotations", the documentation needs to be very clear that this is only audit logging. Alternately we could name it something like AuditAnnotations.

We should also (in a separate PR) clarify the documentation on the audit.Event type that the map does not contain the annotations of the submitted object

Agreed. I thought we discussed calling it ExtraAuditInfo, but can't find the reference. AuditAnnotations is ok too.

AuditAnnotations is ok too.

Done.

We should also (in a separate PR) clarify the documentation on the audit.Event type that the map does not contain the annotations of the submitted object

Will do this after #65891 gets merged.

#65891 is merged, can we queue up the change to the audit.Event annotations field doc?

Done.
#67386

liggitt · 2018-07-27T14:42:48Z

pkg/apis/admission/types.go

replace "will fill in" with "will prefix the keys"

liggitt · 2018-07-27T15:16:12Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

~~if the key already is already namespaced to the webhook, noop?~~

edit: that would require the webhook to know the name by which it was registered, which isn't great, so forget that

erroring on an existing key means that a mutating and validating webhook with the same name cannot set the same key, since they share a namespace

erroring on an existing key means that a mutating and validating webhook with the same name cannot set the same key, since they share a namespace

Local mutating and validating admission may have same problem.
What about we trust the cluster admin here?

liggitt · 2018-07-27T15:20:22Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go

message should say "validating webhook"

liggitt · 2018-07-27T15:21:48Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

I'm not sure this should be a fatal error... the in-tree annotation error handling just logs a warning:

kubernetes/plugin/pkg/admission/security/podsecuritypolicy/admission.go

Lines 168 to 170 in 4d5d266

if err := a.AddAnnotation(key, pspName); err != nil {

glog.Warningf("failed to set admission audit annotation %s to %s: %v", key, pspName, err)

}

Updated to glog.Warningf and make them consistent with each other.

CaoShuFeng · 2018-07-31T06:51:27Z

/test pull-kubernetes-e2e-kops-aws

liggitt · 2018-08-10T20:23:32Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

should probably indicate the reason why in the warning message like the other places we do this (e.g. glog.Warningf("Failed to set annotations[%q] to %q for audit:%q, it has already been set to %q", key, value, ae.AuditID, ae.Annotations[key]))

jimangel · 2018-08-17T02:32:29Z

@CaoShuFeng do you have a PR for the 1.12 docs branch for this? Thanks!

CaoShuFeng · 2018-08-17T02:50:30Z

@CaoShuFeng do you have a PR for the 1.12 docs branch for this? Thanks!

Will work on it.

k8s-ci-robot · 2018-08-17T05:51:37Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	ad223e87a26cc4fd0f1017a93d55a5e10b879215	link	`/test pull-kubernetes-unit`

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.

Details

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.

liggitt · 2018-08-22T20:11:02Z

/lgtm

k8s-ci-robot · 2018-08-22T20:11:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]

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

k8s-github-robot · 2018-08-22T21:15:57Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-08-22T22:31:33Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from deads2k, derekwaynecarr, lavalamp, liggitt, mikedanese, php-coder, soltysh, tallclair and vishh January 23, 2018 10:45

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 23, 2018

CaoShuFeng mentioned this pull request Jan 23, 2018

Add PodSecurityPolicy information to audit logs #58143

Merged

CaoShuFeng force-pushed the admission_webhook branch 2 times, most recently from 1d0b1cd to 0c96dec Compare January 24, 2018 06:30

CaoShuFeng mentioned this pull request Jan 25, 2018

Add PodSecurityPolicy admission information to audit logs #56209

Closed

CaoShuFeng force-pushed the admission_webhook branch from 0c96dec to 9def9cf Compare January 27, 2018 08:46

CaoShuFeng changed the title ~~[WIP] support annotations for admission webhook~~ support annotations for admission webhook Jan 28, 2018

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 Jan 28, 2018

CaoShuFeng force-pushed the admission_webhook branch from 9def9cf to 7db77c5 Compare January 28, 2018 11:45

sttts reviewed Jan 30, 2018

View reviewed changes

CaoShuFeng force-pushed the admission_webhook branch from 7db77c5 to bdb4e11 Compare January 31, 2018 09:53

sttts reviewed Jan 31, 2018

View reviewed changes

CaoShuFeng force-pushed the admission_webhook branch from bdb4e11 to 43d7def Compare February 1, 2018 10:42

CaoShuFeng force-pushed the admission_webhook branch from 57bfb50 to 88b0dba Compare July 5, 2018 01:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2018

CaoShuFeng force-pushed the admission_webhook branch from 88b0dba to dc9b1e8 Compare July 10, 2018 11:48

tallclair reviewed Jul 12, 2018

View reviewed changes

CaoShuFeng force-pushed the admission_webhook branch from dc9b1e8 to 1eaa39d Compare July 12, 2018 06:07

CaoShuFeng commented Jul 12, 2018

View reviewed changes

liggitt reviewed Jul 27, 2018

View reviewed changes

liggitt reviewed Aug 10, 2018

View reviewed changes

CaoShuFeng mentioned this pull request Aug 14, 2018

update Annotations description about audit.Event #67386

Merged

CaoShuFeng added 3 commits August 18, 2018 17:40

add annotations to admission webhook

b4a71bb

run make update

edc3e40

support annotations for admission webhook

0ebfc3e

	if err := a.AddAnnotation(key, pspName); err != nil {
	glog.Warningf("failed to set admission audit annotation %s to %s: %v", key, pspName, err)
	}

Conversation

CaoShuFeng commented Jan 23, 2018 • edited by sttts Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaoShuFeng commented Jan 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaoShuFeng Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaoShuFeng commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaoShuFeng Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tallclair commented Jul 27, 2018

Uh oh!

tallclair commented Jul 27, 2018

Uh oh!

tallclair commented Jul 27, 2018

Uh oh!

sttts commented Jul 27, 2018

Uh oh!

k8s-github-robot commented Jul 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaoShuFeng commented Jan 23, 2018 •

edited by sttts

Loading

CaoShuFeng Jan 31, 2018 •

edited

Loading

CaoShuFeng Jul 12, 2018 •

edited

Loading

liggitt Jul 27, 2018 •

edited

Loading

liggitt Aug 10, 2018 •

edited

Loading

liggitt Jul 27, 2018 •

edited

Loading