From d83206d5af076f6ed2f4bd31c9e80d0b515d5d95 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Fri, 2 Aug 2024 07:17:18 +0200 Subject: [PATCH 1/5] Added more granular matchers --- hack/shoot-comparator/pkg/shoot/matcher.go | 117 ++++++++++++++++++++- 1 file changed, 114 insertions(+), 3 deletions(-) diff --git a/hack/shoot-comparator/pkg/shoot/matcher.go b/hack/shoot-comparator/pkg/shoot/matcher.go index 4bf39f2c..51fe1db7 100644 --- a/hack/shoot-comparator/pkg/shoot/matcher.go +++ b/hack/shoot-comparator/pkg/shoot/matcher.go @@ -90,9 +90,120 @@ func (m *Matcher) Match(actual interface{}) (success bool, err error) { path: "metadata/annotations", }, { - GomegaMatcher: gomega.Equal(eShoot.Spec), - expected: aShoot.Spec, - path: "spec", + GomegaMatcher: gomega.Equal(eShoot.Spec.Addons), + expected: aShoot.Spec.Addons, + path: "spec/Addons", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.CloudProfileName), + expected: aShoot.Spec.CloudProfileName, + path: "spec/CloudProfileName", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.DNS), + expected: aShoot.Spec.DNS, + path: "spec/DNS", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Extensions), + expected: aShoot.Spec.Extensions, + path: "spec/Extensions", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Hibernation), + expected: aShoot.Spec.Hibernation, + path: "spec/Hibernation", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Kubernetes), + expected: aShoot.Spec.Kubernetes, + path: "spec/Kubernetes", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Networking), + expected: aShoot.Spec.Networking, + path: "spec/Networking", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Maintenance), + expected: aShoot.Spec.Maintenance, + path: "spec/Maintenance", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Monitoring), + expected: aShoot.Spec.Monitoring, + path: "spec/Monitoring", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Provider), + expected: aShoot.Spec.Provider, + path: "spec/Provider", + }, + + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Purpose), + expected: aShoot.Spec.Purpose, + path: "spec/Purpose", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Region), + expected: aShoot.Spec.Region, + path: "spec/Region", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.SecretBindingName), + expected: aShoot.Spec.SecretBindingName, + path: "spec/SecretBindingName", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.SeedName), + expected: aShoot.Spec.SeedName, + path: "spec/SeedName", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.SeedSelector), + expected: aShoot.Spec.SeedSelector, + path: "spec/SeedSelector", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Resources), + expected: aShoot.Spec.Resources, + path: "spec/Resources", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.Tolerations), + expected: aShoot.Spec.Tolerations, + path: "spec/Tolerations", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.ExposureClassName), + expected: aShoot.Spec.ExposureClassName, + path: "spec/ExposureClassName", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.SystemComponents), + expected: aShoot.Spec.SystemComponents, + path: "spec/SystemComponents", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.ControlPlane), + expected: aShoot.Spec.ControlPlane, + path: "spec/ControlPlane", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.SchedulerName), + expected: aShoot.Spec.SchedulerName, + path: "spec/SchedulerName", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.CloudProfile), + expected: aShoot.Spec.CloudProfile, + path: "spec/CloudProfile", + }, + { + GomegaMatcher: gomega.Equal(eShoot.Spec.CredentialsBindingName), + expected: aShoot.Spec.CredentialsBindingName, + path: "spec/CredentialsBindingName", }, } { ok, err := matcher.Match(matcher.expected) From 2a7326e0b092245304bc6e2180df2a0ae7b5165e Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Fri, 2 Aug 2024 07:38:10 +0200 Subject: [PATCH 2/5] Shoot from the converter is stored on volume. --- .../runtime/fsm/runtime_fsm_persist_shoot.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_persist_shoot.go b/internal/controller/runtime/fsm/runtime_fsm_persist_shoot.go index dd1a597d..afef364b 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_persist_shoot.go +++ b/internal/controller/runtime/fsm/runtime_fsm_persist_shoot.go @@ -38,11 +38,16 @@ func persist(path string, s interface{}, saveFunc writerGetter) error { func sFnDumpShootSpec(_ context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { paths := createFilesPath(m.PVCPath, s.shoot.Namespace, s.shoot.Name) - shootCp := s.shoot.DeepCopy() + // To make comparison easier we don't store object obtained from the cluster as it contains additional fields that are not relevant for the comparison. + // We use object created by the converter instead (the Provisioner uses the same approach) + convertedShoot, err := convertShoot(&s.instance, m.ConverterConfig) + if err != nil { + return updateStatusAndStopWithError(err) + } + runtimeCp := s.instance.DeepCopy() - shootCp.ManagedFields = nil - if err := persist(paths["shoot"], shootCp, m.writerProvider); err != nil { + if err := persist(paths["shoot"], convertedShoot, m.writerProvider); err != nil { return updateStatusAndStopWithError(err) } From 1eb5953edbcfbd084bfc82f3b3ad79c419adfd41 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Fri, 2 Aug 2024 11:22:15 +0200 Subject: [PATCH 3/5] Unit test fixed --- .../fsm/runtime_fsm_persist_shoot_test.go | 19 +++++++++++++++---- .../controller/runtime/fsm/testing/shoot.go | 14 ++++++++++++++ .../controller/runtime/fsm/utilz_for_test.go | 8 ++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_persist_shoot_test.go b/internal/controller/runtime/fsm/runtime_fsm_persist_shoot_test.go index cd12d927..d959b76a 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_persist_shoot_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_persist_shoot_test.go @@ -3,6 +3,7 @@ package fsm import ( "bytes" "context" + "github.com/kyma-project/infrastructure-manager/internal/gardener/shoot" "io" "time" @@ -24,15 +25,25 @@ var _ = Describe("KIM sFnPersist", func() { testCtx, cancel := context.WithTimeout(context.Background(), time.Second) defer cancel() - shootWrite, err := yaml.Marshal(&testing.ShootNoDNS) - runtimeWrite, err := yaml.Marshal(&testing.RuntimeOnlyName) + expectedRuntime := testing.RuntimeOnlyName.DeepCopy() + expectedRuntime.Spec.Shoot.Provider.Type = "aws" + + expectedShoot, err := convertShoot(expectedRuntime, shoot.ConverterConfig{}) + Expect(err).To(BeNil()) + + shootWrite, err := yaml.Marshal(&expectedShoot) + Expect(err).To(BeNil()) + + runtimeWrite, err := yaml.Marshal(expectedRuntime) + Expect(err).To(BeNil()) + expectedData := append(shootWrite, runtimeWrite...) Expect(err).ShouldNot(HaveOccurred()) It("should persist shoot data", func() { - next, _, err := sFnDumpShootSpec(testCtx, must(newFakeFSM, withStorageWriter(testWriterGetter)), &systemState{shoot: &testing.ShootNoDNS, instance: testing.RuntimeOnlyName}) + next, _, err := sFnDumpShootSpec(testCtx, must(newFakeFSM, withStorageWriter(testWriterGetter), withConverterConfig(shoot.ConverterConfig{})), &systemState{shoot: &testing.ShootNoDNS, instance: *expectedRuntime}) Expect(err).To(BeNil()) Expect(next).To(haveName("sFnUpdateStatus")) - Expect(expectedData).To(Equal(b.Bytes())) + Expect(b.Bytes()).To(Equal(expectedData)) }) }) diff --git a/internal/controller/runtime/fsm/testing/shoot.go b/internal/controller/runtime/fsm/testing/shoot.go index 20379c6b..14230c01 100644 --- a/internal/controller/runtime/fsm/testing/shoot.go +++ b/internal/controller/runtime/fsm/testing/shoot.go @@ -18,6 +18,20 @@ var ( ObjectMeta: metav1.ObjectMeta{ Name: "test-instance", Namespace: "default", + Labels: map[string]string{ + "kyma-project.io/instance-id": "instance-id", + "kyma-project.io/runtime-id": "runtime-id", + "kyma-project.io/shoot-name": "shoot-name", + "kyma-project.io/region": "region", + "operator.kyma-project.io/kyma-name": "kyma-name", + "kyma-project.io/broker-plan-id": "broker-plan-id", + "kyma-project.io/broker-plan-name": "broker-plan-name", + "kyma-project.io/global-account-id": "global-account-id", + "kyma-project.io/subaccount-id": "subaccount-id", + "operator.kyma-project.io/managed-by": "managed-by", + "operator.kyma-project.io/internal": "false", + "kyma-project.io/platform-region": "platform-region", + }, }, Spec: v1.RuntimeSpec{ Shoot: v1.RuntimeShoot{Name: "test-shoot"}, diff --git a/internal/controller/runtime/fsm/utilz_for_test.go b/internal/controller/runtime/fsm/utilz_for_test.go index 257f6378..10239bf1 100644 --- a/internal/controller/runtime/fsm/utilz_for_test.go +++ b/internal/controller/runtime/fsm/utilz_for_test.go @@ -2,6 +2,7 @@ package fsm import ( "fmt" + "github.com/kyma-project/infrastructure-manager/internal/gardener/shoot" . "github.com/onsi/gomega" //nolint:revive "k8s.io/apimachinery/pkg/runtime" @@ -35,6 +36,13 @@ var ( } } + withConverterConfig = func(config shoot.ConverterConfig) fakeFSMOpt { + return func(fsm *fsm) error { + fsm.ConverterConfig = config + return nil + } + } + withFakedK8sClient = func( scheme *runtime.Scheme, objs ...client.Object) fakeFSMOpt { From 69493839cc4610ed838428cb08916d10f6e73765 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Fri, 2 Aug 2024 11:40:01 +0200 Subject: [PATCH 4/5] Stopped saving shoot on upgrade --- .../controller/runtime/fsm/runtime_fsm_patch_shoot.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go b/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go index 5fd72721..df5ab25d 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go +++ b/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go @@ -49,11 +49,6 @@ func sFnPatchExistingShoot(ctx context.Context, m *fsm, s *systemState) (stateFn "Shoot is pending", ) - shouldDumpShootSpec := m.PVCPath != "" - if shouldDumpShootSpec { - return switchState(sFnDumpShootSpec) - } - return updateStatusAndRequeueAfter(gardenerRequeueDuration) } @@ -63,13 +58,13 @@ func convertShoot(instance *imv1.Runtime, cfg shoot.ConverterConfig) (gardener.S } converter := gardener_shoot.NewConverter(cfg) - shoot, err := converter.ToShoot(*instance) + newShoot, err := converter.ToShoot(*instance) if err == nil { - setObjectFields(&shoot) + setObjectFields(&newShoot) } - return shoot, err + return newShoot, err } // workaround From 8de7e50b214a21313871b223cfbe705bf51e4b42 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Sun, 4 Aug 2024 21:10:12 +0200 Subject: [PATCH 5/5] Removed annotations from matcher --- hack/shoot-comparator/pkg/shoot/matcher.go | 7 ++----- hack/shoot-comparator/pkg/shoot/matcher_test.go | 12 ------------ 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/hack/shoot-comparator/pkg/shoot/matcher.go b/hack/shoot-comparator/pkg/shoot/matcher.go index 51fe1db7..4202f08d 100644 --- a/hack/shoot-comparator/pkg/shoot/matcher.go +++ b/hack/shoot-comparator/pkg/shoot/matcher.go @@ -64,6 +64,8 @@ func (m *Matcher) Match(actual interface{}) (success bool, err error) { return false, err } + // Note: we define separate matchers for each field to make input more readable + // Annotations are not matched as they are not relevant for the comparison ; both KIM, and Provisioner have different set of annotations for _, matcher := range []matcher{ { GomegaMatcher: gomega.Equal(eShoot.TypeMeta), @@ -84,11 +86,6 @@ func (m *Matcher) Match(actual interface{}) (success bool, err error) { expected: aShoot.Labels, path: "metadata/labels", }, - { - GomegaMatcher: gomega.Equal(eShoot.Annotations), - expected: aShoot.Annotations, - path: "metadata/annotations", - }, { GomegaMatcher: gomega.Equal(eShoot.Spec.Addons), expected: aShoot.Spec.Addons, diff --git a/hack/shoot-comparator/pkg/shoot/matcher_test.go b/hack/shoot-comparator/pkg/shoot/matcher_test.go index 82dfaadd..5cacc68a 100644 --- a/hack/shoot-comparator/pkg/shoot/matcher_test.go +++ b/hack/shoot-comparator/pkg/shoot/matcher_test.go @@ -26,12 +26,6 @@ func withLabels(labels map[string]string) deepCpOpts { } } -func withAnnotations(annotations map[string]string) deepCpOpts { - return func(s *v1beta1.Shoot) { - s.Annotations = annotations - } -} - func withShootSpec(spec v1beta1.ShootSpec) deepCpOpts { return func(s *v1beta1.Shoot) { s.Spec = spec @@ -103,12 +97,6 @@ var _ = Describe(":: shoot matcher :: ", func() { deepCp(empty, withLabels(map[string]string{})), false, ), - Entry( - "should detect difference in annotations", - deepCp(empty, withAnnotations(map[string]string{"test": "me"})), - deepCp(empty, withAnnotations(map[string]string{"test": "it"})), - false, - ), Entry( "should detect differences in spec", deepCp(empty, withShootSpec(v1beta1.ShootSpec{