Merge pull request #670 from captainroy-hy/fix-webhook-applyto

fix webhook about Trait-Applyto

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go

@@ -86,7 +86,7 @@ const (
 func Setup(mgr ctrl.Manager, args core.Args, l logging.Logger) error {
 	dm, err := discoverymapper.New(mgr.GetConfig())
 	if err != nil {
-		return fmt.Errorf("create discovery dm fail %v", err)
+		return fmt.Errorf("create discovery dm fail %w", err)
 	}
 	name := "oam/" + strings.ToLower(v1alpha2.ApplicationConfigurationGroupKind)
 

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go

@@ -84,6 +84,7 @@ type workloads struct {
 	dm             discoverymapper.DiscoveryMapper
 }
 
+//nolint:errorlint
 func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...resource.ApplyOption) error {
 	// they are all in the same namespace
 	var namespace = w[0].Workload.GetNamespace()
@@ -91,6 +92,7 @@ func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus,
 		if !wl.HasDep {
 			err := a.patchingClient.Apply(ctx, wl.Workload, ao...)
 			if err != nil {
+				// TODO(roywang) use errors.As() insteand of type assertion on error
 				if _, ok := err.(*GenerationUnchanged); !ok {
 					// GenerationUnchanged only aborts applying current workload
 					// but not blocks the whole reconciliation through returning an error
@@ -104,6 +106,7 @@ func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus,
 			}
 			t := trait.Object
 			if err := a.updatingClient.Apply(ctx, &trait.Object, ao...); err != nil {
+				// TODO(roywang) use errors.As() insteand of type assertion on error
 				if _, ok := err.(*GenerationUnchanged); !ok {
 					// GenerationUnchanged only aborts applying current trait
 					// but not blocks the whole reconciliation through returning an error

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go

@@ -615,7 +615,7 @@ func getExpectVal(m v1alpha2.ConditionRequirement, ac *fieldpath.Paved) (string,
 	var err error
 	value, err := ac.GetString(m.ValueFrom.FieldPath)
 	if err != nil {
-		return "", fmt.Errorf("get valueFrom.fieldPath fail: %v", err)
+		return "", fmt.Errorf("get valueFrom.fieldPath fail: %w", err)
 	}
 	return value, nil
 }

pkg/oam/util/test_utils.go

@@ -82,6 +82,8 @@ func (matcher ErrorMatcher) Match(actual interface{}) (success bool, err error)
 }
 
 // FailureMessage builds an error message.
+//nolint:errorlint
+// TODO(roywang) use errors.As() instead of type assertion on error
 func (matcher ErrorMatcher) FailureMessage(actual interface{}) (message string) {
 	actualError, actualOK := actual.(error)
 	expectedError, expectedOK := matcher.ExpectedError.(error)
@@ -102,6 +104,8 @@ func (matcher ErrorMatcher) FailureMessage(actual interface{}) (message string)
 }
 
 // NegatedFailureMessage builds an error message.
+//nolint:errorlint
+// TODO(roywang) use errors.As() instead of type assertion on error
 func (matcher ErrorMatcher) NegatedFailureMessage(actual interface{}) (message string) {
 	actualError, actualOK := actual.(error)
 	expectedError, expectedOK := matcher.ExpectedError.(error)

pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/handler_test.go

@@ -268,6 +268,17 @@ var _ = Describe("ApplicationConfiguration Admission controller Test", func() {
 		By(string(resp.Result.Reason))
 		Expect(resp.Allowed).Should(BeTrue())
 
+		By("Test delete operation request")
+		req = admission.Request{
+			AdmissionRequest: admissionv1beta1.AdmissionRequest{
+				Operation: admissionv1beta1.Delete,
+				Resource:  reqResource,
+				Object:    runtime.RawExtension{Raw: util.JSONMarshal(appConfig)},
+			},
+		}
+		resp = handler.Handle(context.TODO(), req)
+		Expect(resp.Allowed).Should(BeTrue())
+
 		By("Test bad admission request format")
 		req = admission.Request{
 			AdmissionRequest: admissionv1beta1.AdmissionRequest{
@@ -278,5 +289,50 @@ var _ = Describe("ApplicationConfiguration Admission controller Test", func() {
 		}
 		resp = handler.Handle(context.TODO(), req)
 		Expect(resp.Allowed).Should(BeFalse())
+
+		By("Prepare for a bad admission resource")
+		badReqResource := metav1.GroupVersionResource{Group: "core.oam.dev", Version: "v1alpha2", Resource: "foo"}
+		req = admission.Request{
+			AdmissionRequest: admissionv1beta1.AdmissionRequest{
+				Operation: admissionv1beta1.Create,
+				Resource:  badReqResource,
+				Object:    runtime.RawExtension{Raw: util.JSONMarshal(appConfig)},
+			},
+		}
+		resp = handler.Handle(context.TODO(), req)
+		Expect(resp.Allowed).Should(BeFalse())
+
+		By("reject the request for error occurs when prepare data for validation")
+		errClientInstance := &test.MockClient{
+			MockGet: func(ctx context.Context, key types.NamespacedName, obj runtime.Object) error {
+				return fmt.Errorf("cannot prepare data for validation")
+			},
+		}
+		req = admission.Request{
+			AdmissionRequest: admissionv1beta1.AdmissionRequest{
+				Operation: admissionv1beta1.Create,
+				Resource:  reqResource,
+				Object:    runtime.RawExtension{Raw: util.JSONMarshal(appConfig)},
+			},
+		}
+		injc.InjectClient(errClientInstance)
+		resp = handler.Handle(context.TODO(), req)
+		Expect(resp.Allowed).Should(BeFalse())
+
+		By("reject the request for validation fails")
+		var rejectHandler admission.Handler = &ValidatingHandler{
+			Mapper: mapper,
+			Validators: []AppConfigValidator{
+				AppConfigValidateFunc(func(c context.Context, vac ValidatingAppConfig) []error {
+					return []error{fmt.Errorf("validation fails")}
+				}),
+			},
+		}
+		rejectDeecoderInjector := rejectHandler.(admission.DecoderInjector)
+		rejectDeecoderInjector.InjectDecoder(decoder)
+		rejectClientInjector := rejectHandler.(inject.Client)
+		rejectClientInjector.InjectClient(clientInstance)
+		resp = rejectHandler.Handle(context.TODO(), req)
+		Expect(resp.Allowed).Should(BeFalse())
 	})
 })

pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler.go

@@ -8,10 +8,10 @@ import (
 
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2"
 
-	"github.com/oam-dev/kubevela/pkg/oam"
 	"github.com/oam-dev/kubevela/pkg/oam/discoverymapper"
 
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/klog"
@@ -168,13 +168,22 @@ func ValidateTraitAppliableToWorkloadFn(_ context.Context, v ValidatingAppConfig
 	klog.Info("validate trait is appliable to workload", "name", v.appConfig.Name)
 	var allErrs []error
 	for _, c := range v.validatingComps {
-		workloadType := c.component.GetLabels()[oam.WorkloadTypeLabel]
-		workloadDefRefName := c.workloadDefinition.Spec.Reference.Name
 		// TODO(roywang) consider a CRD group could have multiple versions
 		// and maybe we need to specify the minimum version here in the future
-		workloadGroup := c.workloadDefinition.GetObjectKind().GroupVersionKind().Group
+		// according to OAM convention, Spec.Reference.Name in workloadDefinition is CRD name
+		crdName := c.workloadDefinition.Spec.Reference.Name
+		// according to OAM convention, name of workloadDefinition is the workload type.
+		workloadTypeName := c.workloadDefinition.GetName()
+		workloadGroup := schema.ParseGroupResource(crdName).Group
+
+		klog.Info("validate trait is appliable to workload: ",
+			fmt.Sprintf("workloadDefRefName:%s, workloadDefName(type):%s, workloadGroup:%s",
+				crdName, workloadTypeName, workloadGroup))
 	ValidateApplyTo:
 		for _, t := range c.validatingTraits {
+			klog.Info("validate trait is appliable to workload: ",
+				fmt.Sprintf("trait %q is allowed to apply to %s",
+					t.traitDefinition.GetName(), t.traitDefinition.Spec.AppliesToWorkloads))
 			if len(t.traitDefinition.Spec.AppliesToWorkloads) == 0 {
 				// AppliesToWorkloads is empty, the trait can be applied to ANY workload
 				continue
@@ -187,14 +196,14 @@ func ValidateTraitAppliableToWorkloadFn(_ context.Context, v ValidatingAppConfig
 				if strings.HasPrefix(applyTo, "*.") && workloadGroup == applyTo[2:] {
 					continue ValidateApplyTo
 				}
-				if workloadType == applyTo ||
-					workloadDefRefName == applyTo {
+				if crdName == applyTo ||
+					workloadTypeName == applyTo {
 					continue ValidateApplyTo
 				}
 			}
 			allErrs = append(allErrs, fmt.Errorf(errFmtUnappliableTrait,
-				t.traitDefinition.GetObjectKind().GroupVersionKind().String(),
-				c.workloadDefinition.GetObjectKind().GroupVersionKind().String(),
+				t.traitDefinition.GetName(),
+				c.workloadDefinition.GetName(),
 				c.compName, t.traitDefinition.Spec.AppliesToWorkloads))
 		}
 	}

pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler_test.go

@@ -9,8 +9,6 @@ import (
 
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2"
 
-	"github.com/oam-dev/kubevela/pkg/oam"
-
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -231,7 +229,7 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) {
 		want                []error
 	}{
 		{
-			caseName: "apply trait to any workload",
+			caseName: "validate succeed: apply trait to any workload",
 			validatingAppConfig: ValidatingAppConfig{
 				validatingComps: []ValidatingComponent{
 					{
@@ -245,12 +243,12 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) {
 						validatingTraits: []ValidatingTrait{
 							{traitDefinition: v1alpha2.TraitDefinition{
 								Spec: v1alpha2.TraitDefinitionSpec{
-									AppliesToWorkloads: []string{"*"},
+									AppliesToWorkloads: []string{"*"}, // "*" means apply to any
 								},
 							}},
 							{traitDefinition: v1alpha2.TraitDefinition{
 								Spec: v1alpha2.TraitDefinitionSpec{
-									AppliesToWorkloads: []string{},
+									AppliesToWorkloads: []string{}, // empty means apply to any
 								},
 							}},
 						},
@@ -260,13 +258,13 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) {
 			want: nil,
 		},
 		{
-			caseName: "apply trait to workload with specific type",
+			caseName: "validate succeed: apply trait to workload with specific workloadDefinition name",
 			validatingAppConfig: ValidatingAppConfig{
 				validatingComps: []ValidatingComponent{
 					{
-						component: v1alpha2.Component{ObjectMeta: v1.ObjectMeta{
-							Labels: map[string]string{oam.WorkloadTypeLabel: "TestWorkload"},
-						}},
+						workloadDefinition: v1alpha2.WorkloadDefinition{
+							ObjectMeta: v1.ObjectMeta{Name: "TestWorkload"}, // matched workload def(type) nmae
+						},
 						validatingTraits: []ValidatingTrait{
 							{traitDefinition: v1alpha2.TraitDefinition{
 								Spec: v1alpha2.TraitDefinitionSpec{
@@ -280,14 +278,14 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) {
 			want: nil,
 		},
 		{
-			caseName: "apply trait to workload with specific definition reference name",
+			caseName: "validate succeed: apply trait to workload with specific definition reference name",
 			validatingAppConfig: ValidatingAppConfig{
 				validatingComps: []ValidatingComponent{
 					{
 						workloadDefinition: v1alpha2.WorkloadDefinition{
 							Spec: v1alpha2.WorkloadDefinitionSpec{
 								Reference: v1alpha2.DefinitionReference{
-									Name: "TestWorkload",
+									Name: "TestWorkload", // matched CRD name
 								},
 							},
 						},
@@ -304,14 +302,31 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) {
 			want: nil,
 		},
 		{
-			caseName: "apply trait to workload with specific group",
+			caseName: "validate succeed: apply trait to workload with specific group",
 			validatingAppConfig: ValidatingAppConfig{
 				validatingComps: []ValidatingComponent{
 					{
 						workloadDefinition: v1alpha2.WorkloadDefinition{
-							TypeMeta: v1.TypeMeta{
-								APIVersion: "example.com/v1",
-								Kind:       "TestWorkload",
+							Spec: v1alpha2.WorkloadDefinitionSpec{
+								Reference: v1alpha2.DefinitionReference{
+									Name: "testworkloads.example.com", // matched CRD group
+								},
+							},
+						},
+						validatingTraits: []ValidatingTrait{
+							{traitDefinition: v1alpha2.TraitDefinition{
+								Spec: v1alpha2.TraitDefinitionSpec{
+									AppliesToWorkloads: []string{"*.example.com"},
+								},
+							}},
+						},
+					},
+					{
+						workloadDefinition: v1alpha2.WorkloadDefinition{
+							Spec: v1alpha2.WorkloadDefinitionSpec{
+								Reference: v1alpha2.DefinitionReference{
+									Name: "testworkload2s.example.com",
+								},
 							},
 						},
 						validatingTraits: []ValidatingTrait{
@@ -327,33 +342,24 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) {
 			want: nil,
 		},
 		{
-			caseName: "apply trait to unappliable workload",
+			caseName: "validate fail: apply trait to unappliable workload",
 			validatingAppConfig: ValidatingAppConfig{
 				validatingComps: []ValidatingComponent{
 					{
 						compName: "example-comp",
-						component: v1alpha2.Component{ObjectMeta: v1.ObjectMeta{
-							Labels: map[string]string{oam.WorkloadTypeLabel: "TestWorkload0"},
-						}},
 						workloadDefinition: v1alpha2.WorkloadDefinition{
-							TypeMeta: v1.TypeMeta{
-								APIVersion: "unknown.group/v1",
-								Kind:       "TestWorkload1",
-							},
+							ObjectMeta: v1.ObjectMeta{Name: "TestWorkload"},
 							Spec: v1alpha2.WorkloadDefinitionSpec{
 								Reference: v1alpha2.DefinitionReference{
-									Name: "TestWorkload2",
+									Name: "TestWorkload1.example.foo",
 								},
 							},
 						},
 						validatingTraits: []ValidatingTrait{
 							{traitDefinition: v1alpha2.TraitDefinition{
-								TypeMeta: v1.TypeMeta{
-									APIVersion: "example.com/v1",
-									Kind:       "TestTrait",
-								},
+								ObjectMeta: v1.ObjectMeta{Name: "TestTrait"},
 								Spec: v1alpha2.TraitDefinitionSpec{
-									AppliesToWorkloads: []string{"example.com", "TestWorkload"},
+									AppliesToWorkloads: []string{"example.com", "TestWorkload2"},
 								},
 							}},
 						},
@@ -361,8 +367,96 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) {
 				},
 			},
 			want: []error{fmt.Errorf(errFmtUnappliableTrait,
-				"example.com/v1, Kind=TestTrait", "unknown.group/v1, Kind=TestWorkload1", "example-comp",
-				[]string{"example.com", "TestWorkload"})},
+				"TestTrait", "TestWorkload", "example-comp",
+				[]string{"example.com", "TestWorkload2"})},
+		},
+		{
+			caseName: "validate fail: applyTo has CRD group but not match workload",
+			validatingAppConfig: ValidatingAppConfig{
+				validatingComps: []ValidatingComponent{
+					{
+						compName: "example-comp",
+						workloadDefinition: v1alpha2.WorkloadDefinition{
+							ObjectMeta: v1.ObjectMeta{
+								Name: "TestWorkload",
+							},
+							Spec: v1alpha2.WorkloadDefinitionSpec{
+								Reference: v1alpha2.DefinitionReference{
+									Name: "testworkloads.example.foo", // dismatched CRD group
+								},
+							},
+						},
+						validatingTraits: []ValidatingTrait{
+							{traitDefinition: v1alpha2.TraitDefinition{
+								ObjectMeta: v1.ObjectMeta{Name: "TestTrait"},
+								Spec: v1alpha2.TraitDefinitionSpec{
+									AppliesToWorkloads: []string{"*.example.com"},
+								},
+							}},
+						},
+					},
+				},
+			},
+			want: []error{fmt.Errorf(errFmtUnappliableTrait,
+				"TestTrait", "TestWorkload", "example-comp",
+				[]string{"*.example.com"})},
+		},
+		{
+			caseName: "validate fail: applyTo has CRD name but not match",
+			validatingAppConfig: ValidatingAppConfig{
+				validatingComps: []ValidatingComponent{
+					{
+						compName: "example-comp",
+						workloadDefinition: v1alpha2.WorkloadDefinition{
+							ObjectMeta: v1.ObjectMeta{
+								Name: "TestWorkload",
+							},
+							Spec: v1alpha2.WorkloadDefinitionSpec{
+								Reference: v1alpha2.DefinitionReference{
+									Name: "bar.example.com", // dismatched CRD name
+								},
+							},
+						},
+						validatingTraits: []ValidatingTrait{
+							{traitDefinition: v1alpha2.TraitDefinition{
+								ObjectMeta: v1.ObjectMeta{Name: "TestTrait"},
+								Spec: v1alpha2.TraitDefinitionSpec{
+									AppliesToWorkloads: []string{"foo.example.com"},
+								},
+							}},
+						},
+					},
+				},
+			},
+			want: []error{fmt.Errorf(errFmtUnappliableTrait,
+				"TestTrait", "TestWorkload", "example-comp",
+				[]string{"foo.example.com"})},
+		},
+		{
+			caseName: "validate fail: applyTo has definition name but not match",
+			validatingAppConfig: ValidatingAppConfig{
+				validatingComps: []ValidatingComponent{
+					{
+						compName: "example-comp",
+						workloadDefinition: v1alpha2.WorkloadDefinition{
+							ObjectMeta: v1.ObjectMeta{
+								Name: "bar", // dismatched workload def(type) name
+							},
+						},
+						validatingTraits: []ValidatingTrait{
+							{traitDefinition: v1alpha2.TraitDefinition{
+								ObjectMeta: v1.ObjectMeta{Name: "TestTrait"},
+								Spec: v1alpha2.TraitDefinitionSpec{
+									AppliesToWorkloads: []string{"foo"},
+								},
+							}},
+						},
+					},
+				},
+			},
+			want: []error{fmt.Errorf(errFmtUnappliableTrait,
+				"TestTrait", "bar", "example-comp",
+				[]string{"foo"})},
 		},
 	}