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

Cherry pick of #64255: fix field removal in mutating admission webhooks #64971

Merged
merged 1 commit into from Jun 13, 2018
Merged

Cherry pick of #64255: fix field removal in mutating admission webhooks #64971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/BUILD
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@ go_library(
"//vendor/k8s.io/api/admission/v1beta1:go_default_library",
"//vendor/k8s.io/api/admissionregistration/v1beta1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/serializer/json:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
Expand Down Expand Up @@ -42,6 +43,7 @@ go_test(
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
Expand Down
22 changes: 19 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/admission.go
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@ import (
admissionv1beta1 "k8s.io/api/admission/v1beta1"
"k8s.io/api/admissionregistration/v1beta1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer/json"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
Expand Down Expand Up @@ -211,7 +212,7 @@ func (a *MutatingWebhook) Admit(attr admission.Attributes) error {
}

// convert the object to the external version before sending it to the webhook
versionedAttr := versioned.Attributes{
versionedAttr := &versioned.Attributes{
Attributes: attr,
}
if oldObj := attr.GetOldObject(); oldObj != nil {
Expand Down Expand Up @@ -271,7 +272,7 @@ func (a *MutatingWebhook) shouldCallHook(h *v1beta1.Webhook, attr admission.Attr
}

// note that callAttrMutatingHook updates attr
func (a *MutatingWebhook) callAttrMutatingHook(ctx context.Context, h *v1beta1.Webhook, attr versioned.Attributes) error {
func (a *MutatingWebhook) callAttrMutatingHook(ctx context.Context, h *v1beta1.Webhook, attr *versioned.Attributes) error {
// Make the webhook request
request := request.CreateAdmissionReview(attr)
client, err := a.clientManager.HookClient(h)
Expand Down Expand Up @@ -303,9 +304,24 @@ func (a *MutatingWebhook) callAttrMutatingHook(ctx context.Context, h *v1beta1.W
if err != nil {
return apierrors.NewInternalError(err)
}
if _, _, err := a.jsonSerializer.Decode(patchedJS, nil, attr.Object); err != nil {

var newObject runtime.Object
if _, ok := attr.Object.(*unstructured.Unstructured); ok {
// Custom Resources don't have corresponding Go struct's.
// They are represented as Unstructured.
newObject = &unstructured.Unstructured{}
} else {
newObject, err = a.convertor.Scheme.New(attr.GetKind())
if err != nil {
return apierrors.NewInternalError(err)
}
}
newObject, _, err = a.jsonSerializer.Decode(patchedJS, nil, newObject)
if err != nil {
return apierrors.NewInternalError(err)
}
attr.Object = newObject

a.defaulter.Default(attr.Object)
return nil
}
193 changes: 160 additions & 33 deletions staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/admission_test.go
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ import (
"net/http"
"net/http/httptest"
"net/url"
"reflect"
"strings"
"sync/atomic"
"testing"
Expand All @@ -33,6 +34,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/admission"
Expand Down Expand Up @@ -155,40 +157,16 @@ func TestAdmit(t *testing.T) {
},
}

// Set up a test object for the call
kind := corev1.SchemeGroupVersion.WithKind("Pod")
name := "my-pod"
object := corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
"pod.name": name,
},
Name: name,
Namespace: namespace,
},
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Pod",
},
}
oldObject := corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
}
operation := admission.Update
resource := corev1.Resource("pods").WithVersion("v1")
subResource := ""
userInfo := user.DefaultInfo{
Name: "webhook-test",
UID: "webhook-test",
}

ccfgURL := urlConfigGenerator{serverURL}.ccfgURL

type test struct {
hookSource fakeHookSource
path string
expectAllow bool
errorContains string
hookSource fakeHookSource
path string
isCRD bool
additionalLabels map[string]string
errorContains string
expectAllow bool
expectLabels map[string]string
}

matchEverythingRules := []registrationv1beta1.RuleWithOperations{{
Expand Down Expand Up @@ -226,6 +204,71 @@ func TestAdmit(t *testing.T) {
},
expectAllow: true,
},

"match & remove label": {
hookSource: fakeHookSource{
hooks: []registrationv1beta1.Webhook{{
Name: "removeLabel",
ClientConfig: ccfgSVC("removeLabel"),
Rules: matchEverythingRules,
NamespaceSelector: &metav1.LabelSelector{},
}},
},
expectAllow: true,
additionalLabels: map[string]string{"remove": "me"},
expectLabels: map[string]string{"pod.name": "my-pod"},
},
"match & add label": {
hookSource: fakeHookSource{
hooks: []registrationv1beta1.Webhook{{
Name: "addLabel",
ClientConfig: ccfgSVC("addLabel"),
Rules: matchEverythingRules,
NamespaceSelector: &metav1.LabelSelector{},
}},
},
expectAllow: true,
expectLabels: map[string]string{"pod.name": "my-pod", "added": "test"},
},
"match CRD & add label": {
hookSource: fakeHookSource{
hooks: []registrationv1beta1.Webhook{{
Name: "addLabel",
ClientConfig: ccfgSVC("addLabel"),
Rules: matchEverythingRules,
NamespaceSelector: &metav1.LabelSelector{},
}},
},
isCRD: true,
expectAllow: true,
expectLabels: map[string]string{"crd.name": "my-test-crd", "added": "test"},
},
"match CRD & remove label": {
hookSource: fakeHookSource{
hooks: []registrationv1beta1.Webhook{{
Name: "removeLabel",
ClientConfig: ccfgSVC("removeLabel"),
Rules: matchEverythingRules,
NamespaceSelector: &metav1.LabelSelector{},
}},
},
isCRD: true,
expectAllow: true,
additionalLabels: map[string]string{"remove": "me"},
expectLabels: map[string]string{"crd.name": "my-test-crd"},
},
"match & invalid mutation": {
hookSource: fakeHookSource{
hooks: []registrationv1beta1.Webhook{{
Name: "invalidMutation",
ClientConfig: ccfgSVC("invalidMutation"),
Rules: matchEverythingRules,
NamespaceSelector: &metav1.LabelSelector{},
}},
},
errorContains: "invalid character",
},

"match & disallow": {
hookSource: fakeHookSource{
hooks: []registrationv1beta1.Webhook{{
Expand Down Expand Up @@ -366,14 +409,68 @@ func TestAdmit(t *testing.T) {

for name, tt := range table {
if !strings.Contains(name, "no match") {
continue
//continue
}
t.Run(name, func(t *testing.T) {
wh.hookSource = &tt.hookSource
err = wh.Admit(admission.NewAttributesRecord(&object, &oldObject, kind, namespace, name, resource, subResource, operation, &userInfo))

// Set up a test object for the call
var (
object, oldObject runtime.Object
resource string
)
if tt.isCRD {
resource = "crd"
u := &unstructured.Unstructured{}
u.SetKind("TestCRD")
u.SetAPIVersion("custom.resource/v1")
u.SetName("my-test-crd")
object = u
oldObject = u.DeepCopyObject()
} else {
resource = "pod"
name := "my-pod"
object = &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
},
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Pod",
},
}
oldObject = &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Pod"},
}
}
lbls := map[string]string{
resource + ".name": object.(metav1.Object).GetName(),
}
for k, v := range tt.additionalLabels {
lbls[k] = v
}
object.(metav1.Object).SetLabels(lbls)
operation := admission.Update
subResource := ""
userInfo := user.DefaultInfo{
Name: "webhook-test",
UID: "webhook-test",
}
kind := object.GetObjectKind().GroupVersionKind()
attr := admission.NewAttributesRecord(object, oldObject, kind, namespace, object.(metav1.Object).GetName(), kind.GroupVersion().WithResource(resource), subResource, operation, &userInfo)

err = wh.Admit(attr)
if tt.expectAllow != (err == nil) {
t.Errorf("expected allowed=%v, but got err=%v", tt.expectAllow, err)
}
if tt.expectLabels != nil {
if !reflect.DeepEqual(tt.expectLabels, attr.GetObject().(metav1.Object).GetLabels()) {
t.Errorf("%s: expected labels '%v', but got '%v'", name, tt.expectLabels, attr.GetObject().(metav1.Object).GetLabels())
}
}

// ErrWebhookRejected is not an error for our purposes
if tt.errorContains != "" {
if err == nil || !strings.Contains(err.Error(), tt.errorContains) {
Expand Down Expand Up @@ -608,6 +705,36 @@ func webhookHandler(w http.ResponseWriter, r *http.Request) {
Allowed: true,
},
})
case "/removeLabel":
w.Header().Set("Content-Type", "application/json")
pt := v1beta1.PatchTypeJSONPatch
json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{
Response: &v1beta1.AdmissionResponse{
Allowed: true,
PatchType: &pt,
Patch: []byte(`[{"op": "remove", "path": "/metadata/labels/remove"}]`),
},
})
case "/addLabel":
w.Header().Set("Content-Type", "application/json")
pt := v1beta1.PatchTypeJSONPatch
json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{
Response: &v1beta1.AdmissionResponse{
Allowed: true,
PatchType: &pt,
Patch: []byte(`[{"op": "add", "path": "/metadata/labels/added", "value": "test"}]`),
},
})
case "/invalidMutation":
w.Header().Set("Content-Type", "application/json")
pt := v1beta1.PatchTypeJSONPatch
json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{
Response: &v1beta1.AdmissionResponse{
Allowed: true,
PatchType: &pt,
Patch: []byte(`[{"op": "add", "CORRUPTED_KEY":}]`),
},
})
default:
http.NotFound(w, r)
}
Expand Down