Re: [kubernetes/kubernetes] support annotations for admission webhook (#58679)
Dr. Stefan Schimanski
/cc @kubernetes/sig-api-machinery-api-reviews
Is everybody happy with this API extension for admission webhook responses?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Needs Approval
@CaoShuFeng @kubernetes/sig-api-machinery-misc @kubernetes/sig-auth-misc
Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 1 day, the pull request will be moved out of the v1.12 milestone.
sig/api-machinerysig/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.
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Removed From Pull Request
@CaoShuFeng @kubernetes/sig-api-machinery-misc @kubernetes/sig-auth-misc
Important: This pull request was missing the status/approved-for-milestone label for more than 4 days.
Tim Allclair (St. Clair)
@tallclair commented on this pull request.
In staging/src/k8s.io/api/admission/v1beta1/types.go:
> @@ -94,6 +94,12 @@ 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 is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // fully qualified name (e.g. imagepolicy.example.com.mutatingwebhook.admissionregistration.k8s.io/error=image-blacklisted)
I don't know about this key... maybe it should just be prefixed with the webhok name? I.e. imagepolicy.example.com/error=image-blacklisted
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/api/admission/v1beta1/types.go:
> @@ -94,6 +94,12 @@ 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 is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // fully qualified name (e.g. imagepolicy.example.com.mutatingwebhook.admissionregistration.k8s.io/error=image-blacklisted)
webhook.Name can theoretically overlap with the built in names.
FYI: #58679 (comment)
What do you think?
k8s-ci-robot
@CaoShuFeng: PR needs rebase.
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
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-unit | ad223e8 | link | /test pull-kubernetes-unit |
| pull-kubernetes-e2e-kops-aws | 88b0dba | link | /test pull-kubernetes-e2e-kops-aws |
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.
Tim Allclair (St. Clair)
@tallclair commented on this pull request.
In staging/src/k8s.io/api/admission/v1beta1/types.go:
> @@ -94,6 +94,12 @@ 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 is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // fully qualified name (e.g. imagepolicy.example.com.mutatingwebhook.admissionregistration.k8s.io/error=image-blacklisted)
IMO we can just trust the admins to ensure that there aren't collisions. I think validating that the annotations are fo the expected form is the next best thing.
@sttts
Dr. Stefan Schimanski
@sttts commented on this pull request.
In staging/src/k8s.io/api/admission/v1beta1/types.go:
> @@ -94,6 +94,12 @@ 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 is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // fully qualified name (e.g. imagepolicy.example.com.mutatingwebhook.admissionregistration.k8s.io/error=image-blacklisted)
I could live without the postfix, i.e. what @tallclair suggests in the first comment.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: CaoShuFeng
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.
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
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | dc9b1e8 | link | /test pull-kubernetes-e2e-kops-aws |
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
/test pull-kubernetes-e2e-kops-aws
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/api/admission/v1beta1/types.go:
> @@ -94,6 +94,12 @@ 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 is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // fully qualified name (e.g. imagepolicy.example.com.mutatingwebhook.admissionregistration.k8s.io/error=image-blacklisted)
Done.
Tim Allclair (St. Clair)
@tallclair commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -94,6 +94,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.Annotations {
+ if err := attr.AddAnnotation(h.Name+"/"+k, v); err != nil {
+ return err
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.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -94,6 +94,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.Annotations {
+ if err := attr.AddAnnotation(h.Name+"/"+k, v); err != nil {
+ return err
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.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go:
> @@ -76,7 +77,7 @@ func NewFakeDataSource(name string, webhooks []registrationv1beta1.Webhook, muta
return client, informerFactory
}
-func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind schema.GroupVersionKind, namespace string, name string, resource string, labels map[string]string) admission.Attributes {
+func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind schema.GroupVersionKind, namespace string, name string, resource string, labels map[string]string, annotations map[string]string) admission.Attributes {
Instead of adding yet another argument to this, prefer exposing FakeAttributes and FakeAttributes.Annotations (or if you prefer, FakeAttributes.GetAnnotations)
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go:
> + Attributes: admission.NewAttributesRecord(object.(runtime.Object), oldObject.(runtime.Object), kind, namespace, name, gvr, subResource, admission.Update, &userInfo),
+ annotations: annotations,
+ }
+}
+
+// fakeAttributes decorate admission.Attributes. It's used to trace the added annotations.
+type fakeAttributes struct {
+ admission.Attributes
+ annotations map[string]string
+ mutex sync.Mutex
+}
+
+func (f fakeAttributes) AddAnnotation(k, v string) error {
+ f.mutex.Lock()
+ defer f.mutex.Unlock()
+ if f.annotations != nil {
nit: initialize the map in this case, especially if exposing it directly.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,11 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
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
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go:
> @@ -111,6 +111,11 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.Webhook,
if response.Response == nil {
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 err
ditto (ErrCallingWebhook)
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-verify | 1eaa39d | link | /test pull-kubernetes-verify |
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 commented on this pull request.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,11 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
Done.
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go:
> @@ -76,7 +77,7 @@ func NewFakeDataSource(name string, webhooks []registrationv1beta1.Webhook, muta
return client, informerFactory
}
-func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind schema.GroupVersionKind, namespace string, name string, resource string, labels map[string]string) admission.Attributes {
+func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind schema.GroupVersionKind, namespace string, name string, resource string, labels map[string]string, annotations map[string]string) admission.Attributes {
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -94,6 +94,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.Annotations {
+ if err := attr.AddAnnotation(h.Name+"/"+k, v); err != nil {
+ return err
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
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go:
> @@ -404,6 +438,18 @@ func NewTestCases(url *url.URL) []Test {
}},
ErrorContains: "Webhook response was absent",
},
+ {
+ Name: "illegal annotation format",
+ Webhooks: []registrationv1beta1.Webhook{{
+ Name: "invalidAnnotation",
+ ClientConfig: ccfgURL("invalidAnnotation"),
+ Rules: matchEverythingRules,
+ NamespaceSelector: &metav1.LabelSelector{},
+ }},
+ // misconfigured webhook should not lock up the cluster
+ ExpectAllow: true,
+ ExpectAnnotations: nil,
+ },
@tallclair Thanks for you review.
What do you think about current behavior?
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | ad223e8 | link | /test pull-kubernetes-unit |
| pull-kubernetes-verify | 1eaa39d | link | /test pull-kubernetes-verify |
| pull-kubernetes-kubemark-e2e-gce-big | dcf34c1 | link | /test pull-kubernetes-kubemark-e2e-gce-big |
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 commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -94,6 +94,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.Annotations {
+ if err := attr.AddAnnotation(h.Name+"/"+k, v); err != nil {
+ return err
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.
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-integration | 56785ed | link | /test pull-kubernetes-integration |
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
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | ad223e8 | link | /test pull-kubernetes-unit |
| pull-kubernetes-kubemark-e2e-gce-big | dcf34c1 | link | /test pull-kubernetes-kubemark-e2e-gce-big |
| pull-kubernetes-integration | 56785ed | link | /test pull-kubernetes-integration |
| pull-kubernetes-e2e-gce | 56785ed | link | /test pull-kubernetes-e2e-gce |
Tim Allclair (St. Clair)
/milestone v1.12
/status approved-for-milestone
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process
sig/api-machinerysig/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.
—
Tim Allclair (St. Clair)
/retest
Tim Allclair (St. Clair)
/lgtm
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process
sig/api-machinerysig/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.
—
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: CaoShuFeng, tallclair
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.
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
—
Dr. Stefan Schimanski
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process
@CaoShuFeng @liggitt @tallclair
sig/api-machinerysig/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.
—
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // admission webhook name (e.g. imagepolicy.example.com/error=image-blacklisted). Annotations may be provided by
+ // the admission webhook to add additional context to the audit log for this request.
+ // +optional
+ Annotations map[string]string
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.
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
replace "will fill in" with "will prefix the keys"
Jordan Liggitt
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // admission webhook name (e.g. imagepolicy.example.com/error=image-blacklisted). Annotations may be provided by
+ // the admission webhook to add additional context to the audit log for this request.
+ // +optional
+ Annotations map[string]string
We should also clarify the documentation on the audit.Event type that the map does not contain the annotations of the submitted object
Jordan Liggitt
@liggitt commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -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 {
if the key already is already namespaced to the webhook, noop?
Jordan Liggitt
@liggitt commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -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 {
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
Jordan Liggitt
@liggitt commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go:
> @@ -111,6 +111,12 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.Webhook,
if response.Response == nil {
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,
+ Reason: fmt.Errorf("Failed to set annotations for mutating webhook %s: %v.", h.Name, err)}
message should say "validating webhook"
Jordan Liggitt
@liggitt commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -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,
I'm not sure this should be a fatal error... the in-tree annotation error handling just logs a warning
Tim Allclair (St. Clair)
@tallclair commented on this pull request.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // admission webhook name (e.g. imagepolicy.example.com/error=image-blacklisted). Annotations may be provided by
+ // the admission webhook to add additional context to the audit log for this request.
+ // +optional
+ Annotations map[string]string
Agreed. I thought we discussed calling it ExtraAuditInfo, but can't find the reference. AuditAnnotations is ok too.
Kubernetes Submit Queue
/test all
Tests are more than 96 hours old. Re-running tests.
k8s-ci-robot
New changes are detected. LGTM label has been removed.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: CaoShuFeng, tallclair
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: liggitt
If they are not already assigned, you can assign the PR to them by writing /assign @liggitt in a comment when ready.
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
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | 0c4fa2b | link | /test pull-kubernetes-e2e-kops-aws |
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
/test pull-kubernetes-e2e-kops-aws
CaoShuFeng
@CaoShuFeng commented on this pull request.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
Done.
CaoShuFeng
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // admission webhook name (e.g. imagepolicy.example.com/error=image-blacklisted). Annotations may be provided by
+ // the admission webhook to add additional context to the audit log for this request.
+ // +optional
+ Annotations map[string]string
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.
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -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 {
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?
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go:
> @@ -111,6 +111,12 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.Webhook,
if response.Response == nil {
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,
+ Reason: fmt.Errorf("Failed to set annotations for mutating webhook %s: %v.", h.Name, err)}
Done.
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -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,
Updated to glog.Warningf and make them consistent with each other.
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-integration | df62d48 | link | /test pull-kubernetes-integration |
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.
—
Jordan Liggitt
@liggitt commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -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)
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]))
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // admission webhook name (e.g. imagepolicy.example.com/error=image-blacklisted). Annotations may be provided by
+ // the admission webhook to add additional context to the audit log for this request.
+ // +optional
+ Annotations map[string]string
#65891 is merged, can we queue this up?
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | ade4f52 | link | /test pull-kubernetes-e2e-kops-aws |
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 commented on this pull request.
In pkg/apis/admission/types.go:
> @@ -92,6 +92,12 @@ type AdmissionResponse struct {
// PatchType indicates the form the Patch will take. Currently we only support "JSONPatch".
// +optional
PatchType *PatchType
+ // Annotations is an unstructured key value map set by remote admission controller (e.g. error=image-blacklisted).
+ // MutatingAdmissionWebhook and ValidatingAdmissionWebhook admission controller will fill in the Annotations with
+ // admission webhook name (e.g. imagepolicy.example.com/error=image-blacklisted). Annotations may be provided by
+ // the admission webhook to add additional context to the audit log for this request.
+ // +optional
+ Annotations map[string]string
Done.
#67386
CaoShuFeng
@CaoShuFeng commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go:
> @@ -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)
Done.
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | ad223e8 | link | /test pull-kubernetes-unit |
| pull-kubernetes-e2e-kops-aws | ade4f52 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | c903285 | link | /test pull-kubernetes-e2e-gce |
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
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | c903285 | link | /test pull-kubernetes-e2e-kops-aws |
Jim Angel
@CaoShuFeng do you have a PR for the 1.12 docs branch for this? Thanks!
CaoShuFeng
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce | cd2307c | link | /test pull-kubernetes-e2e-gce |
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
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | ad223e8 | link | /test pull-kubernetes-unit |
| pull-kubernetes-e2e-kops-aws | c903285 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | cd2307c | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-e2e-gce-device-plugin-gpu | cd2307c | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
k8s-ci-robot
@CaoShuFeng: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce | cd2307c | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-e2e-gce-device-plugin-gpu | cd2307c | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | cd2307c | link | /test pull-kubernetes-e2e-kops-aws |
Jordan Liggitt
/lgtm
k8s-ci-robot
[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: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
—
Kubernetes Submit Queue
/test all [submit-queue is verifying that this PR is safe to merge]
Kubernetes Submit Queue
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.
Kubernetes Submit Queue
Merged #58679 into master.