From 56a5da68ee6afcded74c35d8d5f926d3093e53fd Mon Sep 17 00:00:00 2001 From: Lan Luo Date: Fri, 26 Aug 2022 09:33:09 +0800 Subject: [PATCH] Fix nil error issue admission.Errored doesn't handle nil error case, it will raise a nil pointer error when there is no ClusterSet found. Fix it with a new error with error message. Refactor unit test and add a few more test cases. Signed-off-by: Lan Luo --- .../memberclusterannounce_webhook.go | 5 +- .../memberclusterannounce_webhook_test.go | 406 +++++++----------- 2 files changed, 147 insertions(+), 264 deletions(-) diff --git a/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go b/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go index cd434079686..2820cea396e 100644 --- a/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go +++ b/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go @@ -19,6 +19,7 @@ package main import ( "context" "encoding/json" + "fmt" "net/http" admissionv1 "k8s.io/api/admission/v1" @@ -85,8 +86,8 @@ func (v *memberClusterAnnounceValidator) Handle(ctx context.Context, req admissi } if len(clusterSetList.Items) == 0 { - klog.ErrorS(err, "No ClusterSet found", "Namespace", v.namespace) - return admission.Errored(http.StatusPreconditionFailed, err) + klog.ErrorS(nil, "No ClusterSet found", "Namespace", v.namespace) + return admission.Errored(http.StatusPreconditionFailed, fmt.Errorf("no ClusterSet found in Namespace %s", v.namespace)) } clusterSet := clusterSetList.Items[0] if clusterSet.Name != memberClusterAnnounce.ClusterSetID { diff --git a/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook_test.go b/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook_test.go index bb664570fb9..ebd576215cc 100644 --- a/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook_test.go +++ b/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook_test.go @@ -39,7 +39,7 @@ import ( var mcaWebhookUnderTest *memberClusterAnnounceValidator -func setup() { +func TestMemberClusterAnnounceWebhook(t *testing.T) { existingClusterSet := &mcsv1alpha1.ClusterSet{ ObjectMeta: metav1.ObjectMeta{ Namespace: "mcs1", @@ -63,7 +63,6 @@ func setup() { Namespace: "mcs-A", }, } - existingServiceAccounts := &corev1.ServiceAccountList{ Items: []corev1.ServiceAccount{ { @@ -81,27 +80,6 @@ func setup() { }, } - newScheme := runtime.NewScheme() - utilruntime.Must(clientgoscheme.AddToScheme(newScheme)) - utilruntime.Must(k8smcsv1alpha1.AddToScheme(newScheme)) - utilruntime.Must(mcsv1alpha1.AddToScheme(newScheme)) - fakeClient := fake.NewClientBuilder().WithScheme(newScheme).WithObjects(existingClusterSet).WithLists(existingServiceAccounts).Build() - - mcaWebhookUnderTest = &memberClusterAnnounceValidator{ - Client: fakeClient, - namespace: "mcs1"} - - decoder, err := admission.NewDecoder(newScheme) - if err != nil { - klog.ErrorS(err, "Error constructing a decoder") - } - - mcaWebhookUnderTest.InjectDecoder(decoder) -} - -func TestWebhookAllow(t *testing.T) { - setup() - mca := &mcsv1alpha1.MemberClusterAnnounce{ ObjectMeta: metav1.ObjectMeta{ Name: "member-announce-from-east", @@ -111,96 +89,11 @@ func TestWebhookAllow(t *testing.T) { ClusterSetID: "clusterset1", LeaderClusterID: "leader1", } - b, _ := j.Marshal(mca) - - req := admission.Request{ - AdmissionRequest: v1.AdmissionRequest{ - UID: "07e52e8d-4513-11e9-a716-42010a800270", - Kind: metav1.GroupVersionKind{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Kind: "MemberClusterAnnounce", - }, - Resource: metav1.GroupVersionResource{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Resource: "memberclusterannounces", - }, - Name: "member-announce-from-east", - Namespace: "mcs1", - Operation: v1.Create, - Object: runtime.RawExtension{ - Raw: b, - }, - UserInfo: authenticationv1.UserInfo{ - Username: "system:serviceaccount:mcs1:east-access-sa", - UID: "4842eb60-68e3-4e38-adad-3abfd6117241", - Groups: []string{ - "system:serviceaccounts", - "system:serviceaccounts:mcs1", - "system:authenticated", - }, - }, - }, - } - - response := mcaWebhookUnderTest.Handle(context.Background(), req) - assert.Equal(t, true, response.Allowed) -} - -func TestWebhookJoinAllow(t *testing.T) { - setup() - - mca := &mcsv1alpha1.MemberClusterAnnounce{ - ObjectMeta: metav1.ObjectMeta{ - Name: "member-announce-from-south", - Namespace: "mcs1", - }, - ClusterID: "south", - ClusterSetID: "clusterset1", - LeaderClusterID: "leader1", - } - b, _ := j.Marshal(mca) - - req := admission.Request{ - AdmissionRequest: v1.AdmissionRequest{ - UID: "07e52e8d-4513-11e9-a716-42010a800270", - Kind: metav1.GroupVersionKind{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Kind: "MemberClusterAnnounce", - }, - Resource: metav1.GroupVersionResource{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Resource: "memberclusterannounces", - }, - Name: "member-announce-from-south", - Namespace: "mcs1", - Operation: v1.Create, - Object: runtime.RawExtension{ - Raw: b, - }, - UserInfo: authenticationv1.UserInfo{ - Username: "system:serviceaccount:mcs1:east-access-sa", - UID: "4842eb60-68e3-4e38-adad-3abfd6117241", - Groups: []string{ - "system:serviceaccounts", - "system:serviceaccounts:mcs1", - "system:authenticated", - }, - }, - }, - } - response := mcaWebhookUnderTest.Handle(context.Background(), req) - assert.Equal(t, true, response.Allowed) -} - -func TestWebhookDeniedDifferentClusterSet(t *testing.T) { - setup() + oldmca := mca.DeepCopy() + oldmca.ClusterSetID = "old-clusterset" - mca := &mcsv1alpha1.MemberClusterAnnounce{ + mcafromAnotherClusterSet := &mcsv1alpha1.MemberClusterAnnounce{ ObjectMeta: metav1.ObjectMeta{ Name: "member-announce-from-north", Namespace: "mcs1", @@ -209,47 +102,8 @@ func TestWebhookDeniedDifferentClusterSet(t *testing.T) { ClusterSetID: "another-clusterset", LeaderClusterID: "leader1", } - b, _ := j.Marshal(mca) - req := admission.Request{ - AdmissionRequest: v1.AdmissionRequest{ - UID: "07e52e8d-4513-11e9-a716-42010a800270", - Kind: metav1.GroupVersionKind{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Kind: "MemberClusterAnnounce", - }, - Resource: metav1.GroupVersionResource{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Resource: "memberclusterannounces", - }, - Name: "member-announce-from-north", - Namespace: "mcs1", - Operation: v1.Create, - Object: runtime.RawExtension{ - Raw: b, - }, - UserInfo: authenticationv1.UserInfo{ - Username: "system:serviceaccount:mcs1:east-access-sa", - UID: "4842eb60-68e3-4e38-adad-3abfd6117241", - Groups: []string{ - "system:serviceaccounts", - "system:serviceaccounts:mcs1", - "system:authenticated", - }, - }, - }, - } - - response := mcaWebhookUnderTest.Handle(context.Background(), req) - assert.Equal(t, false, response.Allowed) -} - -func TestWebhookDeniedDifferentLeaderCluster(t *testing.T) { - setup() - - mca := &mcsv1alpha1.MemberClusterAnnounce{ + mcaDifferentLeader := &mcsv1alpha1.MemberClusterAnnounce{ ObjectMeta: metav1.ObjectMeta{ Name: "member-announce-from-north", Namespace: "mcs1", @@ -258,9 +112,23 @@ func TestWebhookDeniedDifferentLeaderCluster(t *testing.T) { ClusterSetID: "clusterset1", LeaderClusterID: "different-leader", } - b, _ := j.Marshal(mca) - req := admission.Request{ + mcaMarshaled, _ := j.Marshal(mca) + oldmcaMarshaled, _ := j.Marshal(oldmca) + mcaAnotherMarshaled, _ := j.Marshal(mcafromAnotherClusterSet) + mcaDifferentLeaderMarshaled, _ := j.Marshal(mcaDifferentLeader) + + userInfo := authenticationv1.UserInfo{ + Username: "system:serviceaccount:mcs1:east-access-sa", + UID: "4842eb60-68e3-4e38-adad-3abfd6117241", + Groups: []string{ + "system:serviceaccounts", + "system:serviceaccounts:mcs1", + "system:authenticated", + }, + } + + reqAllow := admission.Request{ AdmissionRequest: v1.AdmissionRequest{ UID: "07e52e8d-4513-11e9-a716-42010a800270", Kind: metav1.GroupVersionKind{ @@ -273,135 +141,149 @@ func TestWebhookDeniedDifferentLeaderCluster(t *testing.T) { Version: "v1alpha1", Resource: "memberclusterannounces", }, - Name: "member-announce-from-north", + Name: "member-announce-from-east", Namespace: "mcs1", Operation: v1.Create, Object: runtime.RawExtension{ - Raw: b, - }, - UserInfo: authenticationv1.UserInfo{ - Username: "system:serviceaccount:mcs1:east-access-sa", - UID: "4842eb60-68e3-4e38-adad-3abfd6117241", - Groups: []string{ - "system:serviceaccounts", - "system:serviceaccounts:mcs1", - "system:authenticated", - }, + Raw: mcaMarshaled, }, + UserInfo: userInfo, }, } - response := mcaWebhookUnderTest.Handle(context.Background(), req) - assert.Equal(t, false, response.Allowed) -} - -func TestWebhookDeniedUnknownServiceAccount(t *testing.T) { - setup() + reqAllowCopy := reqAllow.DeepCopy() + reqDenyAnother := admission.Request{ + AdmissionRequest: *reqAllowCopy, + } + reqDenyAnother.Name = "member-announce-from-north" + reqDenyAnother.Object = runtime.RawExtension{ + Raw: mcaAnotherMarshaled, + } - mca := &mcsv1alpha1.MemberClusterAnnounce{ - ObjectMeta: metav1.ObjectMeta{ - Name: "member-announce-from-south", - Namespace: "mcs1", - }, - ClusterID: "south", - ClusterSetID: "clusterset1", - LeaderClusterID: "leader1", + reqDenyAnotherCopy := reqDenyAnother.DeepCopy() + reqDenyDifferentLeader := admission.Request{ + AdmissionRequest: *reqDenyAnotherCopy, + } + reqDenyDifferentLeader.Object = runtime.RawExtension{ + Raw: mcaDifferentLeaderMarshaled, } - b, _ := j.Marshal(mca) - req := admission.Request{ - AdmissionRequest: v1.AdmissionRequest{ - UID: "07e52e8d-4513-11e9-a716-42010a800270", - Kind: metav1.GroupVersionKind{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Kind: "MemberClusterAnnounce", - }, - Resource: metav1.GroupVersionResource{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Resource: "memberclusterannounces", - }, - Name: "member-announce-from-south", - Namespace: "mcs1", - Operation: v1.Create, - Object: runtime.RawExtension{ - Raw: b, - }, - UserInfo: authenticationv1.UserInfo{ - Username: "system:serviceaccount:mcs1:unknown-access-sa", - UID: "4842eb60-68e3-4e38-adad-3abfd6117241", - Groups: []string{ - "system:serviceaccounts", - "system:serviceaccounts:mcs1", - "system:authenticated", - }, - }, + reqDenyUnknownSA := admission.Request{ + AdmissionRequest: *reqAllowCopy, + } + reqDenyUnknownSA.UserInfo = authenticationv1.UserInfo{ + Username: "system:serviceaccount:mcs1:unknown-access-sa", + UID: "4842eb60-68e3-4e38-adad-3abfd6117241", + Groups: []string{ + "system:serviceaccounts", + "system:serviceaccounts:mcs1", + "system:authenticated", }, } - response := mcaWebhookUnderTest.Handle(context.Background(), req) - assert.Equal(t, false, response.Allowed) -} + reqDenyUpdateClusterSetID := admission.Request{ + AdmissionRequest: *reqAllowCopy, + } + reqDenyUpdateClusterSetID.OldObject = runtime.RawExtension{ + Raw: oldmcaMarshaled, + } + reqDenyUpdateClusterSetID.Operation = v1.Update -func TestUpdateClusterSetID(t *testing.T) { - setup() + reqDenyNoClusterSet := admission.Request{ + AdmissionRequest: *reqAllowCopy, + } + reqDelete := admission.Request{ + AdmissionRequest: *reqAllowCopy, + } + reqDelete.Operation = v1.Delete - mca := &mcsv1alpha1.MemberClusterAnnounce{ - ObjectMeta: metav1.ObjectMeta{ - Name: "member-announce-from-south", - Namespace: "mcs1", - }, - ClusterID: "south", - ClusterSetID: "clusterset-changed", - LeaderClusterID: "leader1", + reqInvalidUser := admission.Request{ + AdmissionRequest: *reqAllowCopy, } - oldMca := &mcsv1alpha1.MemberClusterAnnounce{ - ObjectMeta: metav1.ObjectMeta{ - Name: "member-announce-from-south", - Namespace: "mcs1", + reqInvalidUser.UserInfo = authenticationv1.UserInfo{ + Username: "system:user", + UID: "4842eb60-68e3-4e38-adad-3abfd6117241", + Groups: []string{ + "system:authenticated", }, - ClusterID: "south", - ClusterSetID: "clusterset", - LeaderClusterID: "leader1", } - b, _ := j.Marshal(mca) - old, _ := j.Marshal(oldMca) - req := admission.Request{ - AdmissionRequest: v1.AdmissionRequest{ - UID: "07e52e8d-4513-11e9-a716-42010a800270", - Kind: metav1.GroupVersionKind{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Kind: "MemberClusterAnnounce", - }, - Resource: metav1.GroupVersionResource{ - Group: "multicluster.crd.antrea.io", - Version: "v1alpha1", - Resource: "memberclusterannounces", - }, - Name: "member-announce-from-south", - Namespace: "mcs1", - Operation: v1.Update, - Object: runtime.RawExtension{ - Raw: b, - }, - OldObject: runtime.RawExtension{ - Raw: old, - }, - UserInfo: authenticationv1.UserInfo{ - Username: "system:serviceaccount:mcs1:east-access-sa", - UID: "4842eb60-68e3-4e38-adad-3abfd6117241", - Groups: []string{ - "system:serviceaccounts", - "system:serviceaccounts:mcs1", - "system:authenticated", - }, - }, + tests := []struct { + name string + existingClusterSet *mcsv1alpha1.ClusterSet + req admission.Request + isAllowed bool + }{ + { + name: "Allow MemberClusterAnnounce creation", + existingClusterSet: existingClusterSet, + req: reqAllow, + isAllowed: true, + }, + { + name: "Deny MemberClusterAnnounce creation for another ClusterSet", + existingClusterSet: existingClusterSet, + req: reqDenyAnother, + isAllowed: false, + }, + { + name: "Deny MemberClusterAnnounce creation with different Leader ID", + existingClusterSet: existingClusterSet, + req: reqDenyDifferentLeader, + isAllowed: false, + }, + { + name: "Deny MemberClusterAnnounce creation with unknown ServiceAccount", + existingClusterSet: existingClusterSet, + req: reqDenyUnknownSA, + isAllowed: false, + }, + { + name: "Deny MemberClusterAnnounce creation when no ClusterSet found", + req: reqDenyNoClusterSet, + isAllowed: false, + }, + { + name: "Deny MemberClusterAnnounce update with ClusterSet ID change", + existingClusterSet: existingClusterSet, + req: reqDenyUpdateClusterSetID, + isAllowed: false, + }, + { + name: "Allow MemberClusterAnnounce delete", + existingClusterSet: existingClusterSet, + req: reqDelete, + isAllowed: true, }, + { + name: "Deny MemberClusterAnnounce creation with invalid user info", + existingClusterSet: existingClusterSet, + req: reqInvalidUser, + isAllowed: false, + }, + } + + newScheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(newScheme)) + utilruntime.Must(k8smcsv1alpha1.AddToScheme(newScheme)) + utilruntime.Must(mcsv1alpha1.AddToScheme(newScheme)) + decoder, err := admission.NewDecoder(newScheme) + if err != nil { + klog.ErrorS(err, "Error constructing a decoder") + } + for _, tt := range tests { + fakeClient := fake.NewClientBuilder().WithScheme(newScheme).WithObjects().WithLists(existingServiceAccounts).Build() + if tt.existingClusterSet != nil { + fakeClient = fake.NewClientBuilder().WithScheme(newScheme).WithObjects(existingClusterSet).WithLists(existingServiceAccounts).Build() + } + mcaWebhookUnderTest = &memberClusterAnnounceValidator{ + Client: fakeClient, + namespace: "mcs1"} + mcaWebhookUnderTest.InjectDecoder(decoder) + t.Run(tt.name, func(t *testing.T) { + response := mcaWebhookUnderTest.Handle(context.Background(), tt.req) + assert.Equal(t, tt.isAllowed, response.Allowed) + }) } - response := mcaWebhookUnderTest.Handle(context.Background(), req) - assert.Equal(t, false, response.Allowed) }