diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go index 4aa6b56c2..b19a0cfe6 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go +++ b/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) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go index 3fb857995..81f763866 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go +++ b/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 diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go index d4839fa0f..e1bc407ba 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go +++ b/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 } diff --git a/pkg/oam/util/test_utils.go b/pkg/oam/util/test_utils.go index 01bfdb2b8..e4f304a09 100644 --- a/pkg/oam/util/test_utils.go +++ b/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) diff --git a/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/handler_test.go b/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/handler_test.go index dd46ebdd6..9745fb53c 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/handler_test.go +++ b/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()) }) }) 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 e25ece462..a693d6fd8 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler.go @@ -170,13 +170,15 @@ func ValidateTraitAppliableToWorkloadFn(_ context.Context, v ValidatingAppConfig for _, c := range v.validatingComps { // TODO(roywang) consider a CRD group could have multiple versions // and maybe we need to specify the minimum version here in the future - workloadDefRefName := c.workloadDefinition.Spec.Reference.Name - workloadDefName := c.workloadDefinition.GetName() - workloadGroup := schema.ParseGroupResource(workloadDefRefName).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", - workloadDefRefName, workloadDefName, workloadGroup)) + crdName, workloadTypeName, workloadGroup)) ValidateApplyTo: for _, t := range c.validatingTraits { klog.Info("validate trait is appliable to workload: ", @@ -194,8 +196,8 @@ func ValidateTraitAppliableToWorkloadFn(_ context.Context, v ValidatingAppConfig if strings.HasPrefix(applyTo, "*.") && workloadGroup == applyTo[2:] { continue ValidateApplyTo } - if workloadDefRefName == applyTo || - workloadDefName == applyTo { + if crdName == applyTo || + workloadTypeName == applyTo { continue ValidateApplyTo } } diff --git a/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler_test.go b/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler_test.go index 36c768175..c05037c4e 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler_test.go +++ b/pkg/webhook/core.oam.dev/v1alpha2/applicationconfiguration/validating_handler_test.go @@ -229,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{ { @@ -243,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 }, }}, }, @@ -258,12 +258,12 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) { want: nil, }, { - caseName: "apply trait to workload with specific workloadDefinition name", + caseName: "validate succeed: apply trait to workload with specific workloadDefinition name", validatingAppConfig: ValidatingAppConfig{ validatingComps: []ValidatingComponent{ { workloadDefinition: v1alpha2.WorkloadDefinition{ - ObjectMeta: v1.ObjectMeta{Name: "TestWorkload"}, + ObjectMeta: v1.ObjectMeta{Name: "TestWorkload"}, // matched workload def(type) nmae }, validatingTraits: []ValidatingTrait{ {traitDefinition: v1alpha2.TraitDefinition{ @@ -278,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 }, }, }, @@ -302,14 +302,14 @@ 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{ Spec: v1alpha2.WorkloadDefinitionSpec{ Reference: v1alpha2.DefinitionReference{ - Name: "testworkloads.example.com", + Name: "testworkloads.example.com", // matched CRD group }, }, }, @@ -342,7 +342,7 @@ 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{ { @@ -351,7 +351,7 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) { ObjectMeta: v1.ObjectMeta{Name: "TestWorkload"}, Spec: v1alpha2.WorkloadDefinitionSpec{ Reference: v1alpha2.DefinitionReference{ - Name: "TestWorkload1", + Name: "TestWorkload1.example.foo", }, }, }, @@ -370,6 +370,94 @@ func TestValidateTraitAppliableToWorkloadFn(t *testing.T) { "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"})}, + }, } for _, tc := range tests {