From 32182ea39422825dfe17833ed48329e1d5ecba07 Mon Sep 17 00:00:00 2001 From: Pasquale Congiusti Date: Thu, 7 Mar 2024 17:34:16 +0100 Subject: [PATCH] fix(traits): use Comparable matches Reverting #4512 which introduced a function diverging the match from the original design --- .../ROOT/pages/architecture/traits.adoc | 11 ++- pkg/controller/integration/kits.go | 67 +------------------ pkg/controller/integration/kits_test.go | 15 ----- pkg/trait/builder.go | 16 ++++- pkg/trait/builder_test.go | 58 ++++++++++++++++ pkg/trait/camel.go | 10 ++- pkg/trait/camel_test.go | 22 ++++++ pkg/trait/quarkus.go | 20 +++--- pkg/trait/quarkus_test.go | 28 ++++++++ pkg/trait/registry.go | 5 -- pkg/trait/trait_types.go | 13 +--- pkg/trait/util.go | 64 ++++++++++++++++++ pkg/trait/util_test.go | 34 ++++++++++ 13 files changed, 250 insertions(+), 113 deletions(-) diff --git a/docs/modules/ROOT/pages/architecture/traits.adoc b/docs/modules/ROOT/pages/architecture/traits.adoc index fc56b11928..dee146ccb3 100644 --- a/docs/modules/ROOT/pages/architecture/traits.adoc +++ b/docs/modules/ROOT/pages/architecture/traits.adoc @@ -49,6 +49,15 @@ type Trait interface { IsAllowedInProfile(v1.TraitProfile) bool Order() int } + +type Comparable interface { + Matches(trait Trait) bool +} + +type ComparableTrait interface { + Trait + Comparable +} ---- Each trait will implement this interface. The most important methods that will be invoked by the xref:architecture/operator.adoc[Operator] are `Configure()` and `Apply()`. Basically, the `Configure()` method will set those inputs aforementioned (each trait has its own). The method is in charge to verify also the correctness of those expected parameters, where it makes sense (i.e., a well expected `Kubernetes` resource name). The function can return a `TraitCondition` object containing any informative or warning condition to be attached to the resulting Integration (for instance, traits forcefully altered by the platform, or deprecation notices). @@ -59,6 +68,6 @@ The `Order()` method helps in resolving the order of execution of different trai The `InfluencesKit()`, `IsPlatformTrait()` and `RequiresIntegrationPlatform()` methods are easy to understand. They are used to determine if a trait has to influence an `IntegrationKit` build/initialization, if it's a platform trait (ie, needed by the platform itself) or are requiring the presence of an `IntegrationPlatform`. -The presence of `InfluencesBuild()` will let specify the level of granularity of a trait down to its properties for a rebuild. So, if you need, you can compare the traits properties coming from the `prev` (previous) Integration to decide if it is worth to rebuild an Integration or the trait can reuse the one already provided in `this` version. +For those traits that `InfluencesKit()` you may need to provide a `Matches(trait Trait)` func in order to specify those trait parameters that influences a build. This is required by the platform to decide if it is worth to rebuild an Integration or the trait can reuse the one already provided. Finally, through the `IsAllowedInProfile()` method we can override the default behavior (allow the trait for any profile). We must specify the profile we expect for this trait to be executed properly. diff --git a/pkg/controller/integration/kits.go b/pkg/controller/integration/kits.go index fd1a106ddf..5c0e34c6c2 100644 --- a/pkg/controller/integration/kits.go +++ b/pkg/controller/integration/kits.go @@ -19,8 +19,6 @@ package integration import ( "context" - "fmt" - "reflect" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" @@ -132,7 +130,7 @@ func integrationMatches(ctx context.Context, c client.Client, integration *v1.In return false, err } - if match, err := hasMatchingTraits(itc, ikc); !match || err != nil { + if match, err := trait.HasMatchingTraits(itc, ikc); !match || err != nil { ilog.Debug("Integration and integration-kit traits do not match", "integration", integration.Name, "integration-kit", kit.Name, "namespace", integration.Namespace) return false, err } @@ -195,7 +193,7 @@ func kitMatches(kit *v1.IntegrationKit, target *v1.IntegrationKit) (bool, error) return false, err } - if match, err := hasMatchingTraits(c1, c2); !match || err != nil { + if match, err := trait.HasMatchingTraits(c1, c2); !match || err != nil { return false, err } if !util.StringSliceContains(kit.Spec.Dependencies, target.Spec.Dependencies) { @@ -205,67 +203,6 @@ func kitMatches(kit *v1.IntegrationKit, target *v1.IntegrationKit) (bool, error) return true, nil } -func hasMatchingTraits(traitMap trait.Options, kitTraitMap trait.Options) (bool, error) { - catalog := trait.NewCatalog(nil) - - for _, t := range catalog.AllTraits() { - if t == nil { - continue - } - - id := string(t.ID()) - it, ok1 := traitMap.Get(id) - kt, ok2 := kitTraitMap.Get(id) - - if !ok1 && !ok2 { - continue - } - - if t.InfluencesKit() && t.InfluencesBuild(it, kt) { - if ct, ok := t.(trait.ComparableTrait); ok { - // if it's match trait use its matches method to determine the match - if match, err := matchesComparableTrait(ct, it, kt); !match || err != nil { - return false, err - } - } else { - if !matchesTrait(it, kt) { - return false, nil - } - } - } - } - - return true, nil -} - -func matchesComparableTrait(ct trait.ComparableTrait, it map[string]interface{}, kt map[string]interface{}) (bool, error) { - t1 := reflect.New(reflect.TypeOf(ct).Elem()).Interface() - if err := trait.ToTrait(it, &t1); err != nil { - return false, err - } - - t2 := reflect.New(reflect.TypeOf(ct).Elem()).Interface() - if err := trait.ToTrait(kt, &t2); err != nil { - return false, err - } - - ct2, ok := t2.(trait.ComparableTrait) - if !ok { - return false, fmt.Errorf("type assertion failed: %v", t2) - } - tt1, ok := t1.(trait.Trait) - if !ok { - return false, fmt.Errorf("type assertion failed: %v", t1) - } - - return ct2.Matches(tt1), nil -} - -func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool { - // perform exact match on the two trait maps - return reflect.DeepEqual(it, kt) -} - func hasMatchingSourcesForNative(it *v1.Integration, kit *v1.IntegrationKit) bool { if len(it.UserDefinedSources()) != len(kit.Spec.Sources) { return false diff --git a/pkg/controller/integration/kits_test.go b/pkg/controller/integration/kits_test.go index 58852be167..406976ecfb 100644 --- a/pkg/controller/integration/kits_test.go +++ b/pkg/controller/integration/kits_test.go @@ -27,7 +27,6 @@ import ( traitv1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1/trait" "github.com/apache/camel-k/v2/pkg/trait" - "github.com/apache/camel-k/v2/pkg/util/log" "github.com/apache/camel-k/v2/pkg/util/test" "github.com/stretchr/testify/assert" @@ -94,10 +93,6 @@ func TestLookupKitForIntegration_DiscardKitsInError(t *testing.T) { require.NoError(t, err) - a := buildKitAction{} - a.InjectLogger(log.Log) - a.InjectClient(c) - kits, err := lookupKitsForIntegration(context.TODO(), c, &v1.Integration{ TypeMeta: metav1.TypeMeta{ APIVersion: v1.SchemeGroupVersion.String(), @@ -234,10 +229,6 @@ func TestLookupKitForIntegration_DiscardKitsWithIncompatibleTraits(t *testing.T) require.NoError(t, err) - a := buildKitAction{} - a.InjectLogger(log.Log) - a.InjectClient(c) - kits, err := lookupKitsForIntegration(context.TODO(), c, &v1.Integration{ TypeMeta: metav1.TypeMeta{ APIVersion: v1.SchemeGroupVersion.String(), @@ -316,9 +307,6 @@ func TestHasMatchingTraits_KitNoTraitShouldNotBePicked(t *testing.T) { }, } - a := buildKitAction{} - a.InjectLogger(log.Log) - ok, err := integrationAndKitHaveSameTraits(integration, kit) require.NoError(t, err) assert.False(t, ok) @@ -387,9 +375,6 @@ func TestHasMatchingTraits_KitSameTraitShouldBePicked(t *testing.T) { }, } - a := buildKitAction{} - a.InjectLogger(log.Log) - ok, err := integrationAndKitHaveSameTraits(integration, kit) require.NoError(t, err) assert.True(t, ok) diff --git a/pkg/trait/builder.go b/pkg/trait/builder.go index 6b93751262..2027dcc4aa 100644 --- a/pkg/trait/builder.go +++ b/pkg/trait/builder.go @@ -20,6 +20,7 @@ package trait import ( "fmt" "regexp" + "slices" "sort" "strconv" "strings" @@ -56,9 +57,18 @@ func (t *builderTrait) InfluencesKit() bool { return true } -// InfluencesBuild overrides base class method. -func (t *builderTrait) InfluencesBuild(this, prev map[string]interface{}) bool { - return true +func (t *builderTrait) Matches(trait Trait) bool { + otherTrait, ok := trait.(*builderTrait) + if !ok { + return false + } + if t.BaseImage != otherTrait.BaseImage || len(t.Properties) != len(otherTrait.Properties) { + return false + } + // More sofisticated check if len is the same. Sort and compare via slices equal func. + slices.Sort(t.Properties) + slices.Sort(otherTrait.Properties) + return slices.Equal(t.Properties, otherTrait.Properties) } func (t *builderTrait) Configure(e *Environment) (bool, *TraitCondition, error) { diff --git a/pkg/trait/builder_test.go b/pkg/trait/builder_test.go index 2448a28778..c8d1f8c11d 100644 --- a/pkg/trait/builder_test.go +++ b/pkg/trait/builder_test.go @@ -28,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1" + traitv1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1/trait" "github.com/apache/camel-k/v2/pkg/util/camel" "github.com/apache/camel-k/v2/pkg/util/defaults" "github.com/apache/camel-k/v2/pkg/util/kubernetes" @@ -603,3 +604,60 @@ func tasksByName(tasks []v1.Task) []string { } return pipelineTasks } + +func TestBuilderMatches(t *testing.T) { + t1 := builderTrait{ + BasePlatformTrait: NewBasePlatformTrait("builder", 600), + BuilderTrait: traitv1.BuilderTrait{ + OrderStrategy: "dependencies", + }, + } + t2 := builderTrait{ + BasePlatformTrait: NewBasePlatformTrait("builder", 600), + BuilderTrait: traitv1.BuilderTrait{ + OrderStrategy: "dependencies", + }, + } + assert.True(t, t1.Matches(&t2)) + // This is a property that does not influence the build + t2.OrderStrategy = "fifo" + assert.True(t, t1.Matches(&t2)) + // Changing properties which influences build + t1.Properties = []string{"hello=world"} + assert.False(t, t1.Matches(&t2)) + t2.Properties = []string{"hello=world"} + assert.True(t, t1.Matches(&t2)) + t1.Properties = []string{"hello=world", "weare=theworld"} + assert.False(t, t1.Matches(&t2)) + // should detect swap + t2.Properties = []string{"weare=theworld", "hello=world"} + assert.True(t, t1.Matches(&t2)) +} + +func TestBuilderTraitDoesntInfluencesBuild(t *testing.T) { + opt1 := Options{ + "builder": map[string]interface{}{ + "orderStrategy": "dependencies", + }, + } + opt2 := Options{ + "builder": map[string]interface{}{ + "builder": map[string]interface{}{ + "orderStrategy": "dependencies", + }, + }, + } + opt3 := Options{ + "builder": map[string]interface{}{ + "builder": map[string]interface{}{ + "orderStrategy": "fifo", + }, + }, + } + b1, err := HasMatchingTraits(opt1, opt2) + assert.Nil(t, err) + assert.True(t, b1, "Traits should match") + b2, err := HasMatchingTraits(opt1, opt3) + assert.Nil(t, err) + assert.True(t, b2, "Traits should match") +} diff --git a/pkg/trait/camel.go b/pkg/trait/camel.go index f56c2bd1f5..3d2a991d8d 100644 --- a/pkg/trait/camel.go +++ b/pkg/trait/camel.go @@ -53,9 +53,13 @@ func (t *camelTrait) InfluencesKit() bool { return true } -// InfluencesBuild only when the runtime has changed. -func (t *camelTrait) InfluencesBuild(this, prev map[string]interface{}) bool { - return this["runtimeVersion"] != prev["runtimeVersion"] +func (t *camelTrait) Matches(trait Trait) bool { + otherTrait, ok := trait.(*camelTrait) + if !ok { + return false + } + + return otherTrait.RuntimeVersion == t.RuntimeVersion } func (t *camelTrait) Configure(e *Environment) (bool, *TraitCondition, error) { diff --git a/pkg/trait/camel_test.go b/pkg/trait/camel_test.go index e60e7cf994..a3fc3688d6 100644 --- a/pkg/trait/camel_test.go +++ b/pkg/trait/camel_test.go @@ -25,6 +25,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1" + traitv1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1/trait" "github.com/apache/camel-k/v2/pkg/util/camel" "github.com/apache/camel-k/v2/pkg/util/kubernetes" "github.com/apache/camel-k/v2/pkg/util/test" @@ -208,3 +209,24 @@ func TestApplyCamelTraitWithSources(t *testing.T) { "content": "XML Source Code", }, sourceCm.Data) } + +func TestCamelMatches(t *testing.T) { + t1 := camelTrait{ + BasePlatformTrait: NewBasePlatformTrait("camel", 600), + CamelTrait: traitv1.CamelTrait{ + RuntimeVersion: "1.2.3", + }, + } + t2 := camelTrait{ + BasePlatformTrait: NewBasePlatformTrait("camel", 600), + CamelTrait: traitv1.CamelTrait{ + RuntimeVersion: "1.2.3", + }, + } + + assert.True(t, t1.Matches(&t2)) + t1.Properties = []string{"hello=world"} + assert.True(t, t1.Matches(&t2)) + t2.RuntimeVersion = "3.2.1" + assert.False(t, t1.Matches(&t2)) +} diff --git a/pkg/trait/quarkus.go b/pkg/trait/quarkus.go index bf7fdcc684..3803cb24fb 100644 --- a/pkg/trait/quarkus.go +++ b/pkg/trait/quarkus.go @@ -109,23 +109,14 @@ func (t *quarkusTrait) InfluencesKit() bool { return true } -// InfluencesBuild overrides base class method. -func (t *quarkusTrait) InfluencesBuild(this, prev map[string]interface{}) bool { - return true -} - -var _ ComparableTrait = &quarkusTrait{} - func (t *quarkusTrait) Matches(trait Trait) bool { qt, ok := trait.(*quarkusTrait) if !ok { return false } - if len(t.Modes) == 0 && len(qt.Modes) != 0 && !qt.containsMode(traitv1.JvmQuarkusMode) { return false } - for _, md := range t.Modes { if md == traitv1.JvmQuarkusMode && len(qt.Modes) == 0 { continue @@ -135,8 +126,17 @@ func (t *quarkusTrait) Matches(trait Trait) bool { } return false } + // We need to check if the native base image used is the same + thisNativeBaseImage := t.NativeBaseImage + if thisNativeBaseImage == "" { + thisNativeBaseImage = QuarkusNativeDefaultBaseImageName + } + otherNativeBaseImage := qt.NativeBaseImage + if otherNativeBaseImage == "" { + otherNativeBaseImage = QuarkusNativeDefaultBaseImageName + } - return true + return thisNativeBaseImage == otherNativeBaseImage } func (t *quarkusTrait) Configure(e *Environment) (bool, *TraitCondition, error) { diff --git a/pkg/trait/quarkus_test.go b/pkg/trait/quarkus_test.go index 752e57fc02..5d681a3097 100644 --- a/pkg/trait/quarkus_test.go +++ b/pkg/trait/quarkus_test.go @@ -225,3 +225,31 @@ func TestGetLanguageSettingsWithLoaders(t *testing.T) { assert.Equal(t, languageSettings{native: false, sourcesRequiredAtBuildTime: true}, getLanguageSettings(environment, v1.LanguageKotlin)) assert.Equal(t, languageSettings{native: true, sourcesRequiredAtBuildTime: false}, getLanguageSettings(environment, v1.LanguageJavaShell)) } + +func TestQuarkusMatches(t *testing.T) { + qt := quarkusTrait{ + BasePlatformTrait: NewBasePlatformTrait("quarkus", 600), + QuarkusTrait: traitv1.QuarkusTrait{ + Modes: []traitv1.QuarkusMode{traitv1.JvmQuarkusMode}, + }, + } + qt2 := quarkusTrait{ + BasePlatformTrait: NewBasePlatformTrait("quarkus", 600), + QuarkusTrait: traitv1.QuarkusTrait{ + Modes: []traitv1.QuarkusMode{traitv1.JvmQuarkusMode}, + NativeBaseImage: QuarkusNativeDefaultBaseImageName, + }, + } + + assert.True(t, qt.Matches(&qt2)) + qt2.Modes = append(qt2.Modes, traitv1.NativeQuarkusMode) + assert.True(t, qt.Matches(&qt2)) + qt2.Modes = []traitv1.QuarkusMode{traitv1.NativeQuarkusMode} + assert.False(t, qt.Matches(&qt2)) + qt2.Modes = nil + assert.True(t, qt.Matches(&qt2)) + qt2.Modes = []traitv1.QuarkusMode{} + assert.True(t, qt.Matches(&qt2)) + qt2.NativeBaseImage = "docker.io/my-new-native-base" + assert.False(t, qt.Matches(&qt2)) +} diff --git a/pkg/trait/registry.go b/pkg/trait/registry.go index 98d9f88e2e..906e6f7fa9 100644 --- a/pkg/trait/registry.go +++ b/pkg/trait/registry.go @@ -54,11 +54,6 @@ func (t *registryTrait) InfluencesKit() bool { return true } -// InfluencesBuild overrides base class method. -func (t *registryTrait) InfluencesBuild(this, prev map[string]interface{}) bool { - return true -} - func (t *registryTrait) Configure(e *Environment) (bool, *TraitCondition, error) { // disabled by default if e.IntegrationKit == nil || !pointer.BoolDeref(t.Enabled, false) { diff --git a/pkg/trait/trait_types.go b/pkg/trait/trait_types.go index 1d41da6320..85d152e47a 100644 --- a/pkg/trait/trait_types.go +++ b/pkg/trait/trait_types.go @@ -73,11 +73,6 @@ type Trait interface { // InfluencesKit determines if the trait has any influence on Integration Kits InfluencesKit() bool - // InfluencesBuild defines a low level of granularity for those traits which influences the build. - // The trait can specify if any particular trait configuration influences a build or not. - // Note: You must override this method if you override `InfluencesKit()` - InfluencesBuild(this, prev map[string]interface{}) bool - // IsPlatformTrait marks all fundamental traits that allow the platform to work IsPlatformTrait() bool @@ -91,10 +86,12 @@ type Trait interface { Order() int } +// Comparable is the interface exposing comparable funcs. type Comparable interface { Matches(trait Trait) bool } +// ComparableTrait is the interface used to compare two traits between them. type ComparableTrait interface { Trait Comparable @@ -154,12 +151,6 @@ func (trait *BaseTrait) InfluencesKit() bool { return false } -// InfluencesBuild defines a low level of granularity for those traits which influences the build. -// The trait can specify if any particular trait configuration influences a build or not. -func (trait *BaseTrait) InfluencesBuild(this, prev map[string]interface{}) bool { - return false -} - // IsPlatformTrait marks all fundamental traits that allow the platform to work. func (trait *BaseTrait) IsPlatformTrait() bool { return false diff --git a/pkg/trait/util.go b/pkg/trait/util.go index 19089baac3..b7bddd1ddf 100644 --- a/pkg/trait/util.go +++ b/pkg/trait/util.go @@ -597,3 +597,67 @@ func containsEndpoint(name string, e *Environment, c client.Client) (bool, error } return hasKnativeEndpoint, nil } + +// HasMatchingTraits verifies if two traits options match. +func HasMatchingTraits(traitMap Options, kitTraitMap Options) (bool, error) { + catalog := NewCatalog(nil) + + for _, t := range catalog.AllTraits() { + if t == nil || !t.InfluencesKit() { + // We don't store the trait configuration if the trait cannot influence the kit behavior + continue + } + + id := string(t.ID()) + it, ok1 := traitMap.Get(id) + kt, ok2 := kitTraitMap.Get(id) + + if (!ok1 || len(it) == 0) && (!ok2 || len(kt) == 0) { + continue + } + + if !ok1 || !ok2 { + return false, nil + } + if ct, ok := t.(ComparableTrait); ok { + // if it's match trait use its matches method to determine the match + if match, err := matchesComparableTrait(ct, it, kt); !match || err != nil { + return false, err + } + } else { + if !matchesTrait(it, kt) { + return false, nil + } + } + } + + return true, nil +} + +func matchesComparableTrait(ct ComparableTrait, it map[string]interface{}, kt map[string]interface{}) (bool, error) { + t1 := reflect.New(reflect.TypeOf(ct).Elem()).Interface() + if err := ToTrait(it, &t1); err != nil { + return false, err + } + + t2 := reflect.New(reflect.TypeOf(ct).Elem()).Interface() + if err := ToTrait(kt, &t2); err != nil { + return false, err + } + + ct2, ok := t2.(ComparableTrait) + if !ok { + return false, fmt.Errorf("type assertion failed: %v", t2) + } + tt1, ok := t1.(Trait) + if !ok { + return false, fmt.Errorf("type assertion failed: %v", t1) + } + + return ct2.Matches(tt1), nil +} + +func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool { + // perform exact match on the two trait maps + return reflect.DeepEqual(it, kt) +} \ No newline at end of file diff --git a/pkg/trait/util_test.go b/pkg/trait/util_test.go index 69767785b1..72a4f945b8 100644 --- a/pkg/trait/util_test.go +++ b/pkg/trait/util_test.go @@ -345,3 +345,37 @@ func TestSameTraits(t *testing.T) { assert.False(t, ok) }) } + +func TestHasMathchingTraitsEmpty(t *testing.T) { + opt1 := Options{ + "builder": {}, + "camel": { + "runtimeVersion": "1.2.3", + }, + "quarkus": {}, + } + opt2 := Options{ + "camel": { + "runtimeVersion": "1.2.3", + }, + } + opt3 := Options{ + "camel": { + "runtimeVersion": "1.2.3", + }, + } + opt4 := Options{ + "camel": { + "runtimeVersion": "3.2.1", + }, + } + b1, err := HasMatchingTraits(opt1, opt2) + assert.Nil(t, err) + assert.True(t, b1) + b2, err := HasMatchingTraits(opt1, opt4) + assert.Nil(t, err) + assert.False(t, b2) + b3, err := HasMatchingTraits(opt2, opt3) + assert.Nil(t, err) + assert.True(t, b3) +}