Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Implement traits ConflictsWith #309

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions apis/core/v1alpha2/core_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// ConflictsWith specifies the list of traits which could not apply to the
// ConflictsWith specifies the list of traits(CRD name, Definition name, CRD group) 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
Expand Down
5 changes: 5 additions & 0 deletions apis/core/v1alpha2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) [email protected] only use "application/merge-patch+json" type patch
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/v1alpha2/applicationconfiguration/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should create a common function for it instead of changing it public, and this common function could be in helper lib or anywhere else instead of the main logic lib.

u := &unstructured.Unstructured{}
if err := json.Unmarshal(data, u); err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/oam/util/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 (
Expand All @@ -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")
Expand Down Expand Up @@ -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())
}
Expand All @@ -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{}
Expand All @@ -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...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

根本不需要那个 preAppliedTraits . 每次 compatibbleTraits append 一个当前检查通过的trait就行了。

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)
}
}
}

Expand Down Expand Up @@ -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])
Copy link
Member

@wonderflow wonderflow Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里每次重复去fetch调用RPC请求性能太差了,预先获取所有涉及到的trait definition

if err != nil {
return nil, err
}
if conflict == compatibleTraitDef.Name {
Copy link
Member

@wonderflow wonderflow Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

四个条件都要有,你这里只检查了第一条:

  1. CRD 名称定义冲突
    - services.k8s.io # API resource/crd name
  1. api group 定义冲突,这个是CRD 名称的增强版,前面用 *. 前缀
    - *.networking.k8s.io # API group
  1. label 定义冲突,前面固定前缀 labelSelector , label检查的是trait definition的label
    - labelSelector:foo=bar

  1. Definition 名称定义冲突,与第一条类似,只不过这里写的是definition的名字而不是CRD名字

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
}
Loading