diff --git a/apis/core/v1alpha2/core_types.go b/apis/core/v1alpha2/core_types.go index 77c4294e..6e08465c 100644 --- a/apis/core/v1alpha2/core_types.go +++ b/apis/core/v1alpha2/core_types.go @@ -114,6 +114,12 @@ type TraitDefinitionSpec struct { // +optional AppliesToWorkloads []string `json:"appliesToWorkloads,omitempty"` + // ConflictsWith specifies the list of traits which could not apply to the + // same workloads with this trait. Traits that omit this field can work with + // any other traits. + // +optional + ConflictsWith []string `json:"conflictsWith,omitempty"` + // Extension is used for extension needs by OAM platform builders // +optional // +kubebuilder:pruning:PreserveUnknownFields diff --git a/apis/core/v1alpha2/zz_generated.deepcopy.go b/apis/core/v1alpha2/zz_generated.deepcopy.go index e8cc77c0..dfda1e42 100644 --- a/apis/core/v1alpha2/zz_generated.deepcopy.go +++ b/apis/core/v1alpha2/zz_generated.deepcopy.go @@ -1451,6 +1451,11 @@ func (in *TraitDefinitionSpec) DeepCopyInto(out *TraitDefinitionSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.ConflictsWith != nil { + in, out := &in.ConflictsWith, &out.ConflictsWith + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Extension != nil { in, out := &in.Extension, &out.Extension *out = new(runtime.RawExtension) diff --git a/charts/oam-kubernetes-runtime/crds/core.oam.dev_traitdefinitions.yaml b/charts/oam-kubernetes-runtime/crds/core.oam.dev_traitdefinitions.yaml index a88b261e..04e24038 100644 --- a/charts/oam-kubernetes-runtime/crds/core.oam.dev_traitdefinitions.yaml +++ b/charts/oam-kubernetes-runtime/crds/core.oam.dev_traitdefinitions.yaml @@ -44,6 +44,11 @@ spec: items: type: string type: array + conflictsWith: + description: ConflictsWith specifies the list of traits which could not apply to the same workloads with this trait. Traits that omit this field can work with any other traits. + items: + type: string + type: array definitionRef: description: Reference to the CustomResourceDefinition that defines this trait kind. properties: diff --git a/legacy/charts/oam-kubernetes-runtime-legacy/crds/core.oam.dev_traitdefinitions.yaml b/legacy/charts/oam-kubernetes-runtime-legacy/crds/core.oam.dev_traitdefinitions.yaml index d5860429..ab5f0593 100644 --- a/legacy/charts/oam-kubernetes-runtime-legacy/crds/core.oam.dev_traitdefinitions.yaml +++ b/legacy/charts/oam-kubernetes-runtime-legacy/crds/core.oam.dev_traitdefinitions.yaml @@ -43,6 +43,11 @@ spec: items: type: string type: array + conflictsWith: + description: ConflictsWith specifies the list of traits which could not apply to the same workloads with this trait. Traits that omit this field can work with any other traits. + items: + type: string + type: array definitionRef: description: Reference to the CustomResourceDefinition that defines this trait kind. properties: diff --git a/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration.go b/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration.go index 2b83a600..600650db 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration.go @@ -179,7 +179,7 @@ func NewReconciler(m ctrl.Manager, dm discoverymapper.DiscoveryMapper, o ...Reco dm: dm, params: ParameterResolveFn(resolve), workload: ResourceRenderFn(renderWorkload), - trait: ResourceRenderFn(renderTrait), + trait: ResourceRenderFn(RenderTrait), }, workloads: &workloads{ // NOTE(roywang) PatchingApplicator@v0.10.0 only use "application/merge-patch+json" type patch diff --git a/pkg/controller/v1alpha2/applicationconfiguration/render.go b/pkg/controller/v1alpha2/applicationconfiguration/render.go index 6350f488..2b842933 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/render.go @@ -325,7 +325,8 @@ func renderWorkload(data []byte, p ...Parameter) (*unstructured.Unstructured, er return &unstructured.Unstructured{Object: w.UnstructuredContent()}, nil } -func renderTrait(data []byte, _ ...Parameter) (*unstructured.Unstructured, error) { +// RenderTrait renders Trait to *unstructured.Unstructured format +func RenderTrait(data []byte, _ ...Parameter) (*unstructured.Unstructured, error) { // TODO(negz): Is there a better decoder to use here? u := &unstructured.Unstructured{} if err := json.Unmarshal(data, u); err != nil { diff --git a/pkg/controller/v1alpha2/applicationconfiguration/render_test.go b/pkg/controller/v1alpha2/applicationconfiguration/render_test.go index e74db97a..bc7b17c7 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/render_test.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/render_test.go @@ -645,7 +645,7 @@ func TestRenderTrait(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { - got, err := renderTrait(tc.args.data) + got, err := RenderTrait(tc.args.data) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nrenderTrait(...): -want error, +got error:\n%s\n", tc.reason, diff) } diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index aa23db7e..0fc8ee83 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -450,3 +450,13 @@ func MergeMapOverrideWithDst(src, dst map[string]string) map[string]string { } return r } + +// Contains whether a slice contains an item +func Contains(slice []string, elem string) bool { + for _, s := range slice { + if s == elem { + return true + } + } + return false +} diff --git a/pkg/webhook/v1alpha2/applicationconfiguration/handler_test.go b/pkg/webhook/v1alpha2/applicationconfiguration/handler_test.go index 3627ae9b..2c519836 100644 --- a/pkg/webhook/v1alpha2/applicationconfiguration/handler_test.go +++ b/pkg/webhook/v1alpha2/applicationconfiguration/handler_test.go @@ -251,7 +251,7 @@ var _ = Describe("ApplicationConfiguration Admission controller Test", func() { reason: "", client: clientInstance, }, - "malformat appConfig": { + "malformed appConfig": { trait: "bad format", operation: admissionv1beta1.Create, pass: false, diff --git a/pkg/webhook/v1alpha2/applicationconfiguration/validating_handler.go b/pkg/webhook/v1alpha2/applicationconfiguration/validating_handler.go index cbb13daa..8471cc1e 100644 --- a/pkg/webhook/v1alpha2/applicationconfiguration/validating_handler.go +++ b/pkg/webhook/v1alpha2/applicationconfiguration/validating_handler.go @@ -6,11 +6,8 @@ import ( "fmt" "net/http" - "github.com/crossplane/oam-kubernetes-runtime/apis/core/v1alpha2" - "github.com/crossplane/oam-kubernetes-runtime/pkg/oam/discoverymapper" - "github.com/crossplane/oam-kubernetes-runtime/pkg/oam/util" - "github.com/crossplane/crossplane-runtime/pkg/fieldpath" + "github.com/pkg/errors" admissionv1beta1 "k8s.io/api/admission/v1beta1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/validation/field" @@ -20,6 +17,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/crossplane/oam-kubernetes-runtime/apis/core/v1alpha2" + "github.com/crossplane/oam-kubernetes-runtime/pkg/controller/v1alpha2/applicationconfiguration" + "github.com/crossplane/oam-kubernetes-runtime/pkg/oam/discoverymapper" + "github.com/crossplane/oam-kubernetes-runtime/pkg/oam/util" ) const ( @@ -31,6 +33,12 @@ const ( // WorkloadNamePath indicates field path of workload name WorkloadNamePath = "metadata.name" + + // ErrConflictsWith marks as the identifier of Trait conflict error + ErrConflictsWith = "TraitConflictError" + + // ErrFmtConflictsTrait formats the error message for Traits conflict + ErrFmtConflictsTrait = "cannot apply trait %q %q %q whose conflictsWith is %q" ) var appConfigResource = v1alpha2.SchemeGroupVersion.WithResource("applicationconfigurations") @@ -67,7 +75,7 @@ func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) a if err != nil { return admission.Errored(http.StatusBadRequest, err) } - if allErrs := ValidateTraitObject(obj); len(allErrs) > 0 { + if allErrs := ValidateTraitObject(ctx, h.Client, h.Mapper, obj); len(allErrs) > 0 { klog.Info("create or update failed", "name", obj.Name, "errMsg", allErrs.ToAggregate().Error()) return admission.Denied(allErrs.ToAggregate().Error()) } @@ -83,10 +91,18 @@ func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) a } // ValidateTraitObject validates the ApplicationConfiguration on creation/update -func ValidateTraitObject(obj *v1alpha2.ApplicationConfiguration) field.ErrorList { +func ValidateTraitObject(ctx context.Context, client client.Reader, dm discoverymapper.DiscoveryMapper, obj *v1alpha2.ApplicationConfiguration) field.ErrorList { klog.Info("validate applicationConfiguration", "name", obj.Name) var allErrs field.ErrorList for cidx, comp := range obj.Spec.Components { + componentName := comp.ComponentName + compatibleTraits, err := getAppliedTraits(obj, componentName) + if err != nil { + fldPath := field.NewPath("spec").Child("components").Index(cidx).Child("traits") + allErrs = append(allErrs, field.NotFound(fldPath, componentName)) + return allErrs + } + var preAppliedTraits = make([]unstructured.Unstructured, 0) for idx, tr := range comp.Traits { fldPath := field.NewPath("spec").Child("components").Index(cidx).Child("traits").Index(idx).Child("trait") var content map[string]interface{} @@ -110,6 +126,15 @@ func ValidateTraitObject(obj *v1alpha2.ApplicationConfiguration) field.ErrorList allErrs = append(allErrs, field.Invalid(fldPath, content, fmt.Sprintf("the trait data missing GVK, api = %s, kind = %s,", trait.GetAPIVersion(), trait.GetKind()))) } + compatibleTraits = append(compatibleTraits, preAppliedTraits...) + t, err := CheckTraitsConflict(ctx, client, dm, tr, compatibleTraits) + if err != nil { + allErrs = append(allErrs, field.Invalid(fldPath, err, "the trait is conflicted")) + return allErrs + } + if t != nil { + preAppliedTraits = append(preAppliedTraits, *t) + } } } @@ -188,3 +213,55 @@ func RegisterValidatingHandler(mgr manager.Manager) error { }}) return nil } + +// CheckTraitsConflict checks whether traits are conflicted +func CheckTraitsConflict(ctx context.Context, client client.Reader, dm discoverymapper.DiscoveryMapper, + ct v1alpha2.ComponentTrait, compatibleTraits []unstructured.Unstructured) (*unstructured.Unstructured, error) { + t, err := applicationconfiguration.RenderTrait(ct.Trait.Raw) + if err != nil { + return nil, nil + } + traitDef, err := util.FetchTraitDefinition(ctx, client, dm, t) + if err != nil { + return nil, err + } + + for _, conflict := range traitDef.Spec.ConflictsWith { + for j := range compatibleTraits { + compatibleTraitDef, err := util.FetchTraitDefinition(ctx, client, dm, &compatibleTraits[j]) + if err != nil { + return nil, err + } + if conflict == compatibleTraitDef.Name { + err := errors.New(ErrConflictsWith) + return nil, errors.Wrapf(err, ErrFmtConflictsTrait, t.GetAPIVersion(), t.GetKind(), t.GetName(), conflict) + } + } + } + return t, nil +} + +// GetAppliedTraits gets all the traits which is already applied to a specified component +func getAppliedTraits(ac *v1alpha2.ApplicationConfiguration, componentName string) ([]unstructured.Unstructured, error) { + var traits []v1alpha2.ComponentTrait + var appliedTraits = make([]unstructured.Unstructured, 0) + deployedComponents := make([]string, 0) + for _, w := range ac.Status.Workloads { + deployedComponents = append(deployedComponents, w.ComponentName) + } + for _, c := range ac.Spec.Components { + if c.ComponentName != componentName || !util.Contains(deployedComponents, c.ComponentName) { + continue + } + traits = c.Traits + break + } + for _, t := range traits { + unstructuredTrait, err := applicationconfiguration.RenderTrait(t.Trait.Raw) + if err != nil { + return nil, err + } + appliedTraits = append(appliedTraits, *unstructuredTrait) + } + return appliedTraits, nil +} diff --git a/test/e2e-test/traits_conflict_test.go b/test/e2e-test/traits_conflict_test.go new file mode 100644 index 00000000..eb9663a9 --- /dev/null +++ b/test/e2e-test/traits_conflict_test.go @@ -0,0 +1,184 @@ +package controllers_test + +import ( + "context" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + v1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/crossplane/oam-kubernetes-runtime/apis/core/v1alpha2" + "github.com/crossplane/oam-kubernetes-runtime/pkg/oam" +) + +var _ = Describe("Trait conflict", func() { + ctx := context.Background() + apiVersion := "core.oam.dev/v1alpha2" + namespace := "default" + componentName := "conflict" + appConfigName := "conflict" + workload := v1.Deployment{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1", Kind: "Deployment"}, + ObjectMeta: metav1.ObjectMeta{Namespace: namespace}, + Spec: v1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "nginx", + }, + }, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "nginx", + Image: "nginx:1.9.4"}, + }, + }, + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "nginx"}}, + }, + }, + } + component := v1alpha2.Component{ + TypeMeta: metav1.TypeMeta{ + APIVersion: apiVersion, + Kind: "Component", + }, + ObjectMeta: metav1.ObjectMeta{Name: componentName, Namespace: namespace}, + Spec: v1alpha2.ComponentSpec{ + Workload: runtime.RawExtension{Object: workload.DeepCopyObject()}, + }, + } + + traitTypeMeta := metav1.TypeMeta{ + APIVersion: apiVersion, + Kind: "TraitDefinition", + } + + var traitDefinition1Name = "trait1.core.oam.dev" + traitDefinition1 := v1alpha2.TraitDefinition{ + TypeMeta: traitTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: traitDefinition1Name, + Namespace: namespace, + }, + Spec: v1alpha2.TraitDefinitionSpec{ + Reference: v1alpha2.DefinitionReference{ + Name: traitDefinition1Name, + }, + WorkloadRefPath: "spec.workloadRef", + }, + } + + var traitDefinition2Name = "trait2.core.oam.dev" + traitDefinition2 := v1alpha2.TraitDefinition{ + TypeMeta: traitTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: traitDefinition2Name, + Namespace: namespace, + }, + Spec: v1alpha2.TraitDefinitionSpec{ + Reference: v1alpha2.DefinitionReference{ + Name: traitDefinition2Name, + }, + WorkloadRefPath: "spec.workloadRef", + ConflictsWith: []string{traitDefinition1Name}, + }, + } + + var traitDefinition3Name = "trait3.core.oam.dev" + traitDefinition3 := v1alpha2.TraitDefinition{ + TypeMeta: traitTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: traitDefinition3Name, + Namespace: namespace, + }, + Spec: v1alpha2.TraitDefinitionSpec{ + Reference: v1alpha2.DefinitionReference{ + Name: traitDefinition3Name, + }, + WorkloadRefPath: "spec.workloadRef", + }, + } + + var trait1, trait2, trait3 unstructured.Unstructured + trait1.SetAPIVersion(apiVersion) + trait1.SetKind("Trait1") + trait1.SetLabels(map[string]string{ + oam.TraitTypeLabel: traitDefinition1Name, + }) + + trait2.SetAPIVersion(apiVersion) + trait2.SetKind("Trait2") + trait2.SetLabels(map[string]string{ + oam.TraitTypeLabel: traitDefinition2Name, + }) + + trait3.SetAPIVersion(apiVersion) + trait3.SetKind("Trait3") + trait3.SetLabels(map[string]string{ + oam.TraitTypeLabel: traitDefinition3Name, + }) + + appConfig := v1alpha2.ApplicationConfiguration{ + TypeMeta: metav1.TypeMeta{ + APIVersion: apiVersion, + Kind: "ApplicationConfiguration", + }, + ObjectMeta: metav1.ObjectMeta{Name: appConfigName, Namespace: namespace}, + Spec: v1alpha2.ApplicationConfigurationSpec{ + Components: []v1alpha2.ApplicationConfigurationComponent{{ + ComponentName: componentName, + Traits: []v1alpha2.ComponentTrait{ + {Trait: runtime.RawExtension{Object: trait1.DeepCopyObject()}}, + {Trait: runtime.RawExtension{Object: trait2.DeepCopyObject()}}, + {Trait: runtime.RawExtension{Object: trait3.DeepCopyObject()}}, + }, + }, + }, + }, + } + + appConfigObjKey := client.ObjectKey{Name: appConfigName, Namespace: namespace} + + BeforeEach(func() { + Expect(k8sClient.Create(ctx, &traitDefinition1)).Should(Succeed()) + Expect(k8sClient.Create(ctx, &traitDefinition2)).Should(Succeed()) + Expect(k8sClient.Create(ctx, &traitDefinition3)).Should(Succeed()) + Expect(k8sClient.Create(ctx, &component)).Should(Succeed()) + }) + + Context("Apply conflicted traits", func() { + It("Apply conflicted traits when there is no existing traits", func() { + By("submit ApplicationConfiguration") + err := k8sClient.Create(ctx, &appConfig) + Expect(err).Should(HaveOccurred()) + }) + + It("Apply a trait which conflicts to an existing traits", func() { + By("submit ApplicationConfiguration") + appConfig.Spec.Components[0].Traits = []v1alpha2.ComponentTrait{{Trait: runtime.RawExtension{Object: trait1.DeepCopyObject()}}} + Expect(k8sClient.Create(ctx, &appConfig)).Should(Succeed()) + + By("apply new ApplicationConfiguration with a conflicted trait") + Expect(k8sClient.Get(ctx, appConfigObjKey, &appConfig)).Should(Succeed()) + appConfig.Spec.Components[0].Traits = []v1alpha2.ComponentTrait{ + {Trait: runtime.RawExtension{Object: trait1.DeepCopyObject()}}, + {Trait: runtime.RawExtension{Object: trait2.DeepCopyObject()}}, + {Trait: runtime.RawExtension{Object: trait3.DeepCopyObject()}}, + } + Expect(k8sClient.Update(ctx, &appConfig)).Should(HaveOccurred()) + }) + }) + + AfterEach(func() { + k8sClient.Delete(ctx, &appConfig) + k8sClient.Delete(ctx, &component) + k8sClient.Delete(ctx, &traitDefinition1) + k8sClient.Delete(ctx, &traitDefinition2) + k8sClient.Delete(ctx, &traitDefinition3) + }) +})