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

support annotations for admission webhook #58679

Merged
merged 3 commits into from Aug 22, 2018

Conversation

@CaoShuFeng
Copy link
Member

commented Jan 23, 2018

Depends on: #58143
Release note:

Support annotations for remote admission webhooks.

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch 2 times, most recently from 1d0b1cd to 0c96dec Jan 23, 2018

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from 0c96dec to 9def9cf Jan 27, 2018

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

@CaoShuFeng

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2018

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

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from 9def9cf to 7db77c5 Jan 28, 2018

@@ -94,6 +94,10 @@ type AdmissionResponse struct {
// The type of Patch. Currently we only allow "JSONPatch".
// +optional
PatchType *PatchType `json:"patchType,omitempty" protobuf:"bytes,5,opt,name=patchType"`

// Annotations set by remote admission controller.

This comment has been minimized.

Copy link
@sttts

sttts Jan 30, 2018

Contributor

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

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Jan 31, 2018

Author Member

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().

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Feb 4, 2018

Author Member

Done.

if record.annotations == nil {
return false
}
key = plugin_name + "/" + key

This comment has been minimized.

Copy link
@sttts

sttts Jan 30, 2018

Contributor

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

This comment has been minimized.

Copy link
@sttts

sttts Jan 30, 2018

Contributor

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

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Jan 31, 2018

Author Member

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.

@@ -49,6 +49,12 @@ type Attributes interface {
GetKind() schema.GroupVersionKind
// GetUserInfo is information about the requesting user
GetUserInfo() user.Info
// GetAnnotations returns annotation map for the admission chain.
GetAnnotations() map[string]string

This comment has been minimized.

Copy link
@sttts

sttts Jan 30, 2018

Contributor

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

@@ -299,7 +300,7 @@ func (a *ValidatingAdmissionWebhook) shouldCallHook(h *v1beta1.Webhook, attr adm
return a.namespaceMatcher.MatchNamespaceSelector(h, attr)
}

func (a *ValidatingAdmissionWebhook) callHook(ctx context.Context, h *v1beta1.Webhook, attr admission.Attributes) error {
func (a *ValidatingAdmissionWebhook) callHook(ctx context.Context, h *v1beta1.Webhook, attr admission.Attributes, annotationLock sync.Mutex) error {

This comment has been minimized.

Copy link
@sttts

sttts Jan 30, 2018

Contributor

this is ugly

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Jan 31, 2018

Author Member

What about we move this mutex into SetAnnotations()?

This comment has been minimized.

Copy link
@sttts

sttts Jan 31, 2018

Contributor

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

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Feb 1, 2018

Author Member

Done.

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from 7db77c5 to bdb4e11 Jan 31, 2018

@@ -297,6 +297,10 @@ func (a *MutatingWebhook) callAttrMutatingHook(ctx context.Context, h *v1beta1.W
return &webhookerrors.ErrCallingWebhook{WebhookName: h.Name, Reason: err}
}

for k, v := range response.Response.Annotations {
attr.SetAnnotations(h.Name, k, v)

This comment has been minimized.

Copy link
@sttts

sttts Jan 31, 2018

Contributor

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

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Feb 1, 2018

Author Member

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?

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

This comment has been minimized.

Copy link
@sttts

sttts Feb 1, 2018

Contributor

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

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Feb 1, 2018

Author Member

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.

This comment has been minimized.

Copy link
@sttts

sttts Feb 1, 2018

Contributor

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

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Feb 4, 2018

Author Member

Done.

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from bdb4e11 to 43d7def Feb 1, 2018

for k, v := range response.Response.Annotations {
if err := attr.AddAnnotation(h.Name+"/"+k, v); err != nil {
return &webhookerrors.ErrCallingWebhook{WebhookName: h.Name,
Reason: fmt.Errorf("Failed to set annotations for mutating webhook %s: %v.", h.Name, err)}

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 27, 2018

Member

message should say "validating webhook"

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Jul 31, 2018

Author Member

Done.

@@ -97,6 +97,13 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta
return &webhookerrors.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")}
}

for k, v := range response.Response.Annotations {
if err := attr.AddAnnotation(h.Name+"/"+k, v); err != nil {
return &webhookerrors.ErrCallingWebhook{WebhookName: h.Name,

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 27, 2018

Member

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

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

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Jul 31, 2018

Author Member

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

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from 56785ed to 0c4fa2b Jul 31, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 31, 2018

@CaoShuFeng

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

/test pull-kubernetes-e2e-kops-aws

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from 0c4fa2b to df62d48 Aug 8, 2018

@@ -102,6 +102,12 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta
return &webhookerrors.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")}
}

for k, v := range response.Response.AuditAnnotations {
if err := attr.AddAnnotation(h.Name+"/"+k, v); err != nil {
glog.Warningf("Failed to set annotations for mutating webhook %s: %v.", h.Name, err)

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 10, 2018

Member

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]))

This comment has been minimized.

Copy link
@CaoShuFeng

CaoShuFeng Aug 14, 2018

Author Member

Done.

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from df62d48 to ade4f52 Aug 14, 2018

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from ade4f52 to c903285 Aug 14, 2018

@jimangel

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

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

@CaoShuFeng

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

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

Will work on it.

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from c903285 to cd2307c Aug 17, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

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

Test name Commit Details Rerun command
pull-kubernetes-unit ad223e8 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.

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.

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:admission_webhook branch from cd2307c to 0ebfc3e Aug 20, 2018

CaoShuFeng added some commits Aug 18, 2018

k8s-github-robot pushed a commit that referenced this pull request Aug 20, 2018

Kubernetes Submit Queue
Merge pull request #67386 from CaoShuFeng/audit_annotation_object
Automatic merge from submit-queue (batch tested with PRs 55600, 67386). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

update Annotations description about audit.Event

ref: #58679 (comment)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:
/assign @liggitt 

**Release note**:

```release-note
NONE
```

k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Aug 20, 2018

Merge pull request #67386 from CaoShuFeng/audit_annotation_object
Automatic merge from submit-queue (batch tested with PRs 55600, 67386). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

update Annotations description about audit.Event

ref: kubernetes/kubernetes#58679 (comment)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:
/assign @liggitt

**Release note**:

```release-note
NONE
```

Kubernetes-commit: 4c5e6156525b96b72961b86ff5bd82c44ea0cd96
@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 22, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

[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

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-github-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

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

@k8s-github-robot k8s-github-robot merged commit 4e76bb4 into kubernetes:master Aug 22, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation CaoShuFeng authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

sttts pushed a commit to sttts/apiserver that referenced this pull request Sep 5, 2018

Merge pull request #67386 from CaoShuFeng/audit_annotation_object
Automatic merge from submit-queue (batch tested with PRs 55600, 67386). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

update Annotations description about audit.Event

ref: kubernetes/kubernetes#58679 (comment)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:
/assign @liggitt

**Release note**:

```release-note
NONE
```

Kubernetes-commit: 4c5e6156525b96b72961b86ff5bd82c44ea0cd96

k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Sep 6, 2018

Merge pull request #67386 from CaoShuFeng/audit_annotation_object
Automatic merge from submit-queue (batch tested with PRs 55600, 67386). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

update Annotations description about audit.Event

ref: kubernetes/kubernetes#58679 (comment)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:
/assign @liggitt

**Release note**:

```release-note
NONE
```

Kubernetes-commit: 4c5e6156525b96b72961b86ff5bd82c44ea0cd96
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.