From 9a872ebcd7d8f549919beec1cd5fd008cdaabd38 Mon Sep 17 00:00:00 2001 From: Zhiwei Yin Date: Tue, 28 Feb 2023 22:50:58 +0800 Subject: [PATCH] the manifest size limit can be configured (#186) Signed-off-by: Zhiwei Yin --- pkg/cmd/webhook/option.go | 10 ++- pkg/cmd/webhook/start.go | 3 + pkg/webhook/common/validator.go | 62 +++++++++++++++++++ pkg/webhook/common/validator_test.go | 59 ++++++++++++++++++ pkg/webhook/manifestwork_webhook.go | 47 +------------- pkg/webhook/manifestwork_webhook_test.go | 12 ++-- pkg/webhook/v1/manifestwork_validating.go | 51 +-------------- pkg/webhook/v1/webhook.go | 2 +- .../v1alpha1/placemanifestwork_validating.go | 4 +- test/e2e/webhook_test.go | 10 +-- 10 files changed, 149 insertions(+), 111 deletions(-) create mode 100644 pkg/webhook/common/validator.go create mode 100644 pkg/webhook/common/validator_test.go diff --git a/pkg/cmd/webhook/option.go b/pkg/cmd/webhook/option.go index f8dbd282e..ea46acc08 100644 --- a/pkg/cmd/webhook/option.go +++ b/pkg/cmd/webhook/option.go @@ -4,14 +4,16 @@ import "github.com/spf13/pflag" // Config contains the server (the webhook) cert and key. type Options struct { - Port int - CertDir string + Port int + CertDir string + ManifestLimit int } // NewOptions constructs a new set of default options for webhook. func NewOptions() *Options { return &Options{ - Port: 9443, + Port: 9443, + ManifestLimit: 500 * 1024, // the default manifest limit is 500k. } } @@ -20,4 +22,6 @@ func (c *Options) AddFlags(fs *pflag.FlagSet) { "Port is the port that the webhook server serves at.") fs.StringVar(&c.CertDir, "certdir", c.CertDir, "CertDir is the directory that contains the server key and certificate. If not set, webhook server would look up the server key and certificate in {TempDir}/k8s-webhook-server/serving-certs") + fs.IntVar(&c.ManifestLimit, "manifestLimit", c.ManifestLimit, + "ManifestLimit is the max size of manifests in a manifestWork. If not set, the default is 500k.") } diff --git a/pkg/cmd/webhook/start.go b/pkg/cmd/webhook/start.go index 902ed3e16..acff7fb42 100644 --- a/pkg/cmd/webhook/start.go +++ b/pkg/cmd/webhook/start.go @@ -2,6 +2,7 @@ package webhook import ( "k8s.io/klog/v2" + "open-cluster-management.io/work/pkg/webhook/common" "k8s.io/apimachinery/pkg/runtime" @@ -52,6 +53,8 @@ func (c *Options) RunWebhookServer() error { return err } + common.ManifestValidator.WithLimit(c.ManifestLimit) + if err = (&webhookv1.ManifestWorkWebhook{}).Init(mgr); err != nil { klog.Error(err, "unable to create ManagedCluster webhook") return err diff --git a/pkg/webhook/common/validator.go b/pkg/webhook/common/validator.go new file mode 100644 index 000000000..c8af0f2d2 --- /dev/null +++ b/pkg/webhook/common/validator.go @@ -0,0 +1,62 @@ +package common + +import ( + "fmt" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + workv1 "open-cluster-management.io/api/work/v1" +) + +type Validator struct { + limit int +} + +var ManifestValidator = &Validator{limit: 500 * 1024} // the default manifest limit is 500k. + +func (m *Validator) WithLimit(limit int) { + m.limit = limit +} + +func (m *Validator) ValidateManifests(manifests []workv1.Manifest) error { + if len(manifests) == 0 { + return apierrors.NewBadRequest("Workload manifests should not be empty") + } + + totalSize := 0 + for _, manifest := range manifests { + totalSize = totalSize + manifest.Size() + } + + if totalSize > m.limit { + return fmt.Errorf("the size of manifests is %v bytes which exceeds the %v limit", totalSize, m.limit) + } + + for _, manifest := range manifests { + err := validateManifest(manifest.Raw) + if err != nil { + return err + } + } + + return nil +} + +func validateManifest(manifest []byte) error { + // If the manifest cannot be decoded, return err + unstructuredObj := &unstructured.Unstructured{} + err := unstructuredObj.UnmarshalJSON(manifest) + if err != nil { + return err + } + + // The object must have name specified, generateName is not allowed in manifestwork + if unstructuredObj.GetName() == "" { + return fmt.Errorf("name must be set in manifest") + } + + if unstructuredObj.GetGenerateName() != "" { + return fmt.Errorf("generateName must not be set in manifest") + } + + return nil +} diff --git a/pkg/webhook/common/validator_test.go b/pkg/webhook/common/validator_test.go new file mode 100644 index 000000000..ae0deaf17 --- /dev/null +++ b/pkg/webhook/common/validator_test.go @@ -0,0 +1,59 @@ +package common + +import ( + "fmt" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + workv1 "open-cluster-management.io/api/work/v1" + "reflect" + "testing" +) + +func newManifest(size int) workv1.Manifest { + data := "" + for i := 0; i < size; i++ { + data += "a" + } + + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "namespace": "test", + "name": "test", + }, + "data": data, + }, + } + objectStr, _ := obj.MarshalJSON() + manifest := workv1.Manifest{} + manifest.Raw = objectStr + return manifest +} +func Test_Validator(t *testing.T) { + cases := []struct { + name string + manifests []workv1.Manifest + expectedError error + }{ + { + name: "not exceed the limit", + manifests: []workv1.Manifest{newManifest(100 * 1024), newManifest(100 * 1024)}, + expectedError: nil, + }, + { + name: "exceed the limit", + manifests: []workv1.Manifest{newManifest(300 * 1024), newManifest(200 * 1024)}, + expectedError: fmt.Errorf("the size of manifests is 512192 bytes which exceeds the 512000 limit"), + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := ManifestValidator.ValidateManifests(c.manifests) + if !reflect.DeepEqual(err, c.expectedError) { + t.Errorf("expected %#v but got: %#v", c.expectedError, err) + } + }) + } +} diff --git a/pkg/webhook/manifestwork_webhook.go b/pkg/webhook/manifestwork_webhook.go index 603b874de..02a7a992a 100644 --- a/pkg/webhook/manifestwork_webhook.go +++ b/pkg/webhook/manifestwork_webhook.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "open-cluster-management.io/work/pkg/webhook/common" "reflect" ocmfeature "open-cluster-management.io/api/feature" @@ -14,7 +15,6 @@ import ( authenticationv1 "k8s.io/api/authentication/v1" authorizationv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -23,9 +23,6 @@ import ( "k8s.io/klog/v2" ) -// ManifestLimit is the max size of manifests data which is 50k bytes. -const ManifestLimit = 50 * 1024 - // ManifestWorkAdmissionHook will validate the creating/updating manifestwork request. type ManifestWorkAdmissionHook struct { kubeClient kubernetes.Interface @@ -102,7 +99,7 @@ func (a *ManifestWorkAdmissionHook) validateManifestWorkObj(request *admissionv1 return fmt.Errorf("manifests should not be empty") } - if err := a.validateManifests(work); err != nil { + if err := common.ManifestValidator.ValidateManifests(work.Spec.Workload.Manifests); err != nil { return err } @@ -125,46 +122,6 @@ func (a *ManifestWorkAdmissionHook) validateManifestWorkObj(request *admissionv1 return a.validateExecutor(work, request.UserInfo) } -func (a *ManifestWorkAdmissionHook) validateManifests(work *workv1.ManifestWork) error { - totalSize := 0 - for _, manifest := range work.Spec.Workload.Manifests { - totalSize = totalSize + manifest.Size() - } - - if totalSize > ManifestLimit { - return fmt.Errorf("the size of manifests is %v bytes which exceeds the 50k limit", totalSize) - } - - for _, manifest := range work.Spec.Workload.Manifests { - err := a.validateManifest(manifest.Raw) - if err != nil { - return err - } - } - - return nil -} - -func (a *ManifestWorkAdmissionHook) validateManifest(manifest []byte) error { - // If the manifest cannot be decoded, return err - unstructuredObj := &unstructured.Unstructured{} - err := unstructuredObj.UnmarshalJSON(manifest) - if err != nil { - return err - } - - // The object must have name specified, generateName is not allowed in manifestwork - if unstructuredObj.GetName() == "" { - return fmt.Errorf("name must be set in manifest") - } - - if unstructuredObj.GetGenerateName() != "" { - return fmt.Errorf("generateName must not be set in manifest") - } - - return nil -} - func (a *ManifestWorkAdmissionHook) validateExecutor(work *workv1.ManifestWork, userInfo authenticationv1.UserInfo) error { executor := work.Spec.Executor diff --git a/pkg/webhook/manifestwork_webhook_test.go b/pkg/webhook/manifestwork_webhook_test.go index 32fb7ebbd..ed3b33a9b 100644 --- a/pkg/webhook/manifestwork_webhook_test.go +++ b/pkg/webhook/manifestwork_webhook_test.go @@ -162,17 +162,17 @@ func TestManifestWorkValidate(t *testing.T) { UserInfo: authenticationv1.UserInfo{Username: "tester"}, }, manifests: []*unstructured.Unstructured{ - spoketesting.NewUnstructuredSecretBySize("test1", "testns", 10*1024), - spoketesting.NewUnstructuredSecretBySize("test2", "testns", 10*1024), - spoketesting.NewUnstructuredSecretBySize("test3", "testns", 10*1024), - spoketesting.NewUnstructuredSecretBySize("test4", "testns", 10*1024), - spoketesting.NewUnstructuredSecretBySize("test5", "testns", 10*1024), + spoketesting.NewUnstructuredSecretBySize("test1", "testns", 100*1024), + spoketesting.NewUnstructuredSecretBySize("test2", "testns", 100*1024), + spoketesting.NewUnstructuredSecretBySize("test3", "testns", 100*1024), + spoketesting.NewUnstructuredSecretBySize("test4", "testns", 100*1024), + spoketesting.NewUnstructuredSecretBySize("test5", "testns", 100*1024), }, expectedResponse: &admissionv1beta1.AdmissionResponse{ Allowed: false, Result: &metav1.Status{ Status: metav1.StatusFailure, Code: http.StatusBadRequest, Reason: metav1.StatusReasonBadRequest, - Message: "the size of manifests is 51685 bytes which exceeds the 50k limit", + Message: "the size of manifests is 512490 bytes which exceeds the 512000 limit", }, }, }, diff --git a/pkg/webhook/v1/manifestwork_validating.go b/pkg/webhook/v1/manifestwork_validating.go index f0187c0ae..be4817fc2 100644 --- a/pkg/webhook/v1/manifestwork_validating.go +++ b/pkg/webhook/v1/manifestwork_validating.go @@ -3,13 +3,13 @@ package v1 import ( "context" "fmt" + "open-cluster-management.io/work/pkg/webhook/common" "reflect" authenticationv1 "k8s.io/api/authentication/v1" authorizationv1 "k8s.io/api/authorization/v1" apierrors "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/runtime" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/kubernetes" @@ -20,9 +20,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) -// ManifestLimit is the max size of manifests data which is 50k bytes. -const ManifestLimit = 50 * 1024 - var _ webhook.CustomValidator = &ManifestWorkWebhook{} // ValidateCreate implements webhook.Validator so a webhook will be registered for the type @@ -59,7 +56,7 @@ func (r *ManifestWorkWebhook) validateRequest(newWork, oldWork *workv1.ManifestW return apierrors.NewBadRequest("manifests should not be empty") } - if err := ValidateManifests(newWork.Spec.Workload.Manifests); err != nil { + if err := common.ManifestValidator.ValidateManifests(newWork.Spec.Workload.Manifests); err != nil { return apierrors.NewBadRequest(err.Error()) } @@ -75,50 +72,6 @@ func (r *ManifestWorkWebhook) validateRequest(newWork, oldWork *workv1.ManifestW return validateExecutor(r.kubeClient, newWork, req.UserInfo) } -func ValidateManifests(manifests []workv1.Manifest) error { - if len(manifests) == 0 { - return apierrors.NewBadRequest("Workload manifests should not be empty") - } - - totalSize := 0 - for _, manifest := range manifests { - totalSize = totalSize + manifest.Size() - } - - if totalSize > ManifestLimit { - return apierrors.NewBadRequest(fmt.Sprintf("the size of manifests is %v bytes which exceeds the 50k limit", totalSize)) - } - - for _, manifest := range manifests { - err := validateManifest(manifest.Raw) - if err != nil { - return err - } - } - - return nil -} - -func validateManifest(manifest []byte) error { - // If the manifest cannot be decoded, return err - unstructuredObj := &unstructured.Unstructured{} - err := unstructuredObj.UnmarshalJSON(manifest) - if err != nil { - return err - } - - // The object must have name specified, generateName is not allowed in manifestwork - if unstructuredObj.GetName() == "" { - return fmt.Errorf("name must be set in manifest") - } - - if unstructuredObj.GetGenerateName() != "" { - return fmt.Errorf("generateName must not be set in manifest") - } - - return nil -} - func validateExecutor(kubeClient kubernetes.Interface, work *workv1.ManifestWork, userInfo authenticationv1.UserInfo) error { executor := work.Spec.Executor if !utilfeature.DefaultMutableFeatureGate.Enabled(ocmfeature.NilExecutorValidating) { diff --git a/pkg/webhook/v1/webhook.go b/pkg/webhook/v1/webhook.go index f8e3bc9f8..8fe2d0b2b 100644 --- a/pkg/webhook/v1/webhook.go +++ b/pkg/webhook/v1/webhook.go @@ -19,7 +19,7 @@ func (r *ManifestWorkWebhook) Init(mgr ctrl.Manager) error { return err } -// SetExternalKubeClientSet is function to enable the webhook injecting to kube admssion +// SetExternalKubeClientSet is function to enable the webhook injecting to kube admission func (r *ManifestWorkWebhook) SetExternalKubeClientSet(client kubernetes.Interface) { r.kubeClient = client } diff --git a/pkg/webhook/v1alpha1/placemanifestwork_validating.go b/pkg/webhook/v1alpha1/placemanifestwork_validating.go index 27584b153..2fd3ea20c 100644 --- a/pkg/webhook/v1alpha1/placemanifestwork_validating.go +++ b/pkg/webhook/v1alpha1/placemanifestwork_validating.go @@ -2,11 +2,11 @@ package v1alpha1 import ( "context" + "open-cluster-management.io/work/pkg/webhook/common" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" workv1alpha1 "open-cluster-management.io/api/work/v1alpha1" - webhookv1 "open-cluster-management.io/work/pkg/webhook/v1" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -56,5 +56,5 @@ func (r *PlaceManifestWorkWebhook) validateRequest(newPlaceWork *workv1alpha1.Pl } func validatePlaceManifests(placeWork *workv1alpha1.PlaceManifestWork) error { - return webhookv1.ValidateManifests(placeWork.Spec.ManifestWorkTemplate.Workload.Manifests) + return common.ManifestValidator.ValidateManifests(placeWork.Spec.ManifestWorkTemplate.Workload.Manifests) } diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 415797eaf..6806f133b 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -64,27 +64,27 @@ var _ = ginkgo.Describe("ManifestWork admission webhook", func() { manifests := []workapiv1.Manifest{ { RawExtension: runtime.RawExtension{ - Object: newSecretBySize("default", "test1", 10*1024), + Object: newSecretBySize("default", "test1", 100*1024), }, }, { RawExtension: runtime.RawExtension{ - Object: newSecretBySize("default", "test2", 10*1024), + Object: newSecretBySize("default", "test2", 100*1024), }, }, { RawExtension: runtime.RawExtension{ - Object: newSecretBySize("default", "test3", 10*1024), + Object: newSecretBySize("default", "test3", 100*1024), }, }, { RawExtension: runtime.RawExtension{ - Object: newSecretBySize("default", "test4", 10*1024), + Object: newSecretBySize("default", "test4", 100*1024), }, }, { RawExtension: runtime.RawExtension{ - Object: newSecretBySize("default", "test5", 10*1024), + Object: newSecretBySize("default", "test5", 100*1024), }, }, }