From 6d979cfcabfc2ba64be5f1e2739b4239c81e4792 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 5 Aug 2022 15:05:00 +0800 Subject: [PATCH] [Backport release-1.5] Fix: reject applications with empty policy properties (#4566) * Fix: reject applications with empty policies Signed-off-by: Charlie Chiang (cherry picked from commit 337032511e160bb8d7570b7cd25449bb6c60b3a0) * Style: change err msg Signed-off-by: Charlie Chiang (cherry picked from commit 2bb5c0245ab070bb4036d9da331fd888bd392031) * Fix: use 400 instead of 422 to show err msg Signed-off-by: Charlie Chiang (cherry picked from commit 553ac92c62012c2e7d6e78788b730870a793c86b) * Test: fix tests Signed-off-by: Charlie Chiang (cherry picked from commit 0ce352d13b82199f4c2ea503b3e57d255f10ec25) Co-authored-by: Charlie Chiang --- pkg/appfile/parser.go | 6 +++++ pkg/appfile/parser_test.go | 22 +++++++++++++++++++ .../application/validating_handler.go | 6 +++-- .../application/validating_handler_test.go | 17 ++++++++++++++ .../validating_handler.go | 7 +++--- 5 files changed, 53 insertions(+), 5 deletions(-) diff --git a/pkg/appfile/parser.go b/pkg/appfile/parser.go index 8d64f5a58..07480dd21 100644 --- a/pkg/appfile/parser.go +++ b/pkg/appfile/parser.go @@ -381,6 +381,9 @@ func (p *Parser) parsePoliciesFromRevision(ctx context.Context, af *Appfile) (er return err } for _, policy := range af.Policies { + if policy.Properties == nil && policy.Type != v1alpha1.DebugPolicyType { + return fmt.Errorf("policy %s named %s must not have empty properties", policy.Type, policy.Name) + } switch policy.Type { case v1alpha1.GarbageCollectPolicyType: case v1alpha1.ApplyOncePolicyType: @@ -407,6 +410,9 @@ func (p *Parser) parsePolicies(ctx context.Context, af *Appfile) (err error) { return err } for _, policy := range af.Policies { + if policy.Properties == nil && policy.Type != v1alpha1.DebugPolicyType { + return fmt.Errorf("policy %s named %s must not have empty properties", policy.Type, policy.Name) + } switch policy.Type { case v1alpha1.GarbageCollectPolicyType: case v1alpha1.ApplyOncePolicyType: diff --git a/pkg/appfile/parser_test.go b/pkg/appfile/parser_test.go index 1c1eb8a14..40f2a6abc 100644 --- a/pkg/appfile/parser_test.go +++ b/pkg/appfile/parser_test.go @@ -269,6 +269,20 @@ spec: image: "busybox" ` +const appfileYamlEmptyPolicy = ` +apiVersion: core.oam.dev/v1beta1 +kind: Application +metadata: + name: application-sample + namespace: default +spec: + components: [] + policies: + - type: garbage-collect + name: somename + properties: +` + var _ = Describe("Test application parser", func() { It("Test we can parse an application to an appFile", func() { o := v1beta1.Application{} @@ -314,6 +328,14 @@ var _ = Describe("Test application parser", func() { Expect(err).ShouldNot(HaveOccurred()) _, err = NewApplicationParser(&tclient, dm, pd).GenerateAppFile(context.TODO(), ¬found) Expect(err).Should(HaveOccurred()) + + By("app with empty policy") + emptyPolicy := v1beta1.Application{} + err = yaml.Unmarshal([]byte(appfileYamlEmptyPolicy), &emptyPolicy) + Expect(err).ShouldNot(HaveOccurred()) + _, err = NewApplicationParser(&tclient, dm, pd).GenerateAppFile(context.TODO(), &emptyPolicy) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("have empty properties")) }) }) diff --git a/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler.go b/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler.go index 3e250db72..c5e5b127a 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler.go @@ -96,7 +96,9 @@ func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) a switch req.Operation { case admissionv1.Create: if allErrs := h.ValidateCreate(ctx, app); len(allErrs) > 0 { - return admission.Errored(http.StatusUnprocessableEntity, mergeErrors(allErrs)) + // http.StatusUnprocessableEntity will NOT report any error descriptions + // to the client, use generic http.StatusBadRequest instead. + return admission.Errored(http.StatusBadRequest, mergeErrors(allErrs)) } case admissionv1.Update: oldApp := &v1beta1.Application{} @@ -105,7 +107,7 @@ func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) a } if app.ObjectMeta.DeletionTimestamp.IsZero() { if allErrs := h.ValidateUpdate(ctx, app, oldApp); len(allErrs) > 0 { - return admission.Errored(http.StatusUnprocessableEntity, mergeErrors(allErrs)) + return admission.Errored(http.StatusBadRequest, mergeErrors(allErrs)) } } default: diff --git a/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler_test.go b/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler_test.go index 2c51f193b..63848ab3c 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler_test.go +++ b/pkg/webhook/core.oam.dev/v1alpha2/application/validating_handler_test.go @@ -452,4 +452,21 @@ var _ = Describe("Test Application Validator", func() { resp = handler.Handle(ctx, req) Expect(resp.Allowed).Should(BeFalse()) }) + + It("Test Application with empty policy", func() { + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Resource: metav1.GroupVersionResource{Group: "core.oam.dev", Version: "v1beta1", Resource: "applications"}, + Object: runtime.RawExtension{ + Raw: []byte(` +{"kind":"Application","metadata":{"name":"app-with-empty-policy-webhook-test", "namespace":"default"}, +"spec":{"components":[],"policies":[{"name":"2345","type":"garbage-collect","properties":null}]}} +`), + }, + }, + } + resp := handler.Handle(ctx, req) + Expect(resp.Allowed).Should(BeFalse()) + }) }) diff --git a/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler.go b/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler.go index 8f6cbe9ad..cb8003ffc 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler.go @@ -118,13 +118,14 @@ func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) a if err := h.Decoder.DecodeRaw(req.AdmissionRequest.OldObject, oldApp); err != nil { return admission.Errored(http.StatusBadRequest, err) } - if allErrs := h.ValidateUpdate(ctx, app, oldApp); len(allErrs) > 0 { - return admission.Errored(http.StatusUnprocessableEntity, allErrs.ToAggregate()) + // http.StatusUnprocessableEntity will NOT report any error descriptions + // to the client, use generic http.StatusBadRequest instead. + return admission.Errored(http.StatusBadRequest, allErrs.ToAggregate()) } case admissionv1.Create: if allErrs := h.ValidateCreate(ctx, app); len(allErrs) > 0 { - return admission.Errored(http.StatusUnprocessableEntity, allErrs.ToAggregate()) + return admission.Errored(http.StatusBadRequest, allErrs.ToAggregate()) } default: // Do nothing for CONNECT