From 2ecd78f3e96a47bc46032dddc171f39b7e7e94cd Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Fri, 21 Apr 2023 10:10:41 +0200 Subject: [PATCH 1/4] feat(operator): consider corner cases in KACR controller Signed-off-by: Florian Bacher --- .../keptnappcreationrequest/controller.go | 39 ++- .../controller_test.go | 253 +++++++++++++++++- .../app-creation-request/00-assert.yaml | 2 +- .../app-creation-request/01-assert.yaml | 14 + .../app-creation-request/01-install.yaml | 58 ++++ 5 files changed, 357 insertions(+), 9 deletions(-) create mode 100644 test/integration/app-creation-request/01-assert.yaml create mode 100644 test/integration/app-creation-request/01-install.yaml diff --git a/operator/controllers/lifecycle/keptnappcreationrequest/controller.go b/operator/controllers/lifecycle/keptnappcreationrequest/controller.go index c827d80666..f57f0c6d68 100644 --- a/operator/controllers/lifecycle/keptnappcreationrequest/controller.go +++ b/operator/controllers/lifecycle/keptnappcreationrequest/controller.go @@ -164,8 +164,20 @@ func (r *KeptnAppCreationRequestReconciler) SetupWithManager(mgr ctrl.Manager) e func (r *KeptnAppCreationRequestReconciler) updateKeptnApp(ctx context.Context, keptnApp *lifecycle.KeptnApp, workloads *lifecycle.KeptnWorkloadList) error { - updated := false + addOrUpdatedWorkload := r.addOrUpdateWorkloads(workloads, keptnApp) + removedWorkload := r.cleanupWorkloads(workloads, keptnApp) + + if !addOrUpdatedWorkload && !removedWorkload { + return nil + } + + keptnApp.Spec.Version = computeVersionFromWorkloads(workloads.Items) + + return r.Update(ctx, keptnApp) +} +func (r *KeptnAppCreationRequestReconciler) addOrUpdateWorkloads(workloads *lifecycle.KeptnWorkloadList, keptnApp *lifecycle.KeptnApp) bool { + updated := false for _, workload := range workloads.Items { foundWorkload := false workloadName := strings.TrimPrefix(workload.Name, fmt.Sprintf("%s-", keptnApp.Name)) @@ -190,14 +202,27 @@ func (r *KeptnAppCreationRequestReconciler) updateKeptnApp(ctx context.Context, updated = true } } + return updated +} - if !updated { - return nil - } - - keptnApp.Spec.Version = computeVersionFromWorkloads(workloads.Items) +func (r *KeptnAppCreationRequestReconciler) cleanupWorkloads(workloads *lifecycle.KeptnWorkloadList, keptnApp *lifecycle.KeptnApp) bool { + updated := false + for index, appWorkload := range keptnApp.Spec.Workloads { + foundWorkload := false + for _, workload := range workloads.Items { + workloadName := strings.TrimPrefix(workload.Name, fmt.Sprintf("%s-", keptnApp.Name)) + if appWorkload.Name == workloadName { + foundWorkload = true + break + } + } - return r.Update(ctx, keptnApp) + if !foundWorkload { + keptnApp.Spec.Workloads = append(keptnApp.Spec.Workloads[:index], keptnApp.Spec.Workloads[index+1:]...) + updated = true + } + } + return updated } func (r *KeptnAppCreationRequestReconciler) createKeptnApp(ctx context.Context, creationRequest *lifecycle.KeptnAppCreationRequest, workloads *lifecycle.KeptnWorkloadList) error { diff --git a/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go b/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go index 9db807e04c..2c84d12935 100644 --- a/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go +++ b/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go @@ -113,7 +113,7 @@ func TestKeptnAppCreationRequestReconciler_CreateAppAfterTimeout(t *testing.T) { require.True(t, errors.IsNotFound(err)) } -func TestKeptnAppCreationRequestReconciler_UpdateWorkloadsWithNewVersion(t *testing.T) { +func TestKeptnAppCreationRequestReconciler_UpdateWorkloadsWithNewWorkload(t *testing.T) { r, fakeClient, theClock := setupReconcilerAndClient(t) const namespace = "my-namespace" const appName = "my-app" @@ -246,6 +246,257 @@ func TestKeptnAppCreationRequestReconciler_UpdateWorkloadsWithNewVersion(t *test }) } +func TestKeptnAppCreationRequestReconciler_UpdateWorkloadsWithNewVersion(t *testing.T) { + r, fakeClient, theClock := setupReconcilerAndClient(t) + const namespace = "my-namespace" + const appName = "my-app" + kacr := &klcv1alpha3.KeptnAppCreationRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-kacr", + Namespace: namespace, + CreationTimestamp: metav1.Time{Time: theClock.Now()}, + }, + Spec: klcv1alpha3.KeptnAppCreationRequestSpec{ + AppName: appName, + }, + } + + err := fakeClient.Create(context.TODO(), kacr) + require.Nil(t, err) + + workload1 := &klcv1alpha3.KeptnWorkload{ + ObjectMeta: metav1.ObjectMeta{ + Name: "w1", + Namespace: namespace, + }, + Spec: klcv1alpha3.KeptnWorkloadSpec{ + AppName: appName, + Version: "1.0", + }, + } + + err = fakeClient.Create(context.TODO(), workload1) + require.Nil(t, err) + + request := controllerruntime.Request{ + NamespacedName: types.NamespacedName{ + Namespace: kacr.Namespace, + Name: kacr.Name, + }, + } + + // turn the clock forward + theClock.Add(1 * time.Minute) + + // reconcile again - now we should get a KeptnApp as a result + res, err := r.Reconcile(context.TODO(), request) + + require.Nil(t, err) + require.False(t, res.Requeue) + require.Zero(t, res.RequeueAfter) + + kApp := &klcv1alpha3.KeptnApp{} + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: kacr.Spec.AppName, Namespace: kacr.Namespace}, kApp) + + require.Nil(t, err) + require.NotEmpty(t, kApp) + + require.NotEmpty(t, kApp.Spec.Version) + require.Len(t, kApp.Spec.Workloads, 1) + require.Contains(t, kApp.Spec.Workloads, klcv1alpha3.KeptnWorkloadRef{ + Name: workload1.Name, + Version: workload1.Spec.Version, + }) + + firstVersion := kApp.Spec.Version + + // verify that the creationRequest has been deleted + cr := &klcv1alpha3.KeptnAppCreationRequest{} + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: kacr.Name, Namespace: kacr.Namespace}, cr) + + require.True(t, errors.IsNotFound(err)) + + // update the workload with a new version + + workload1.Spec.Version = "2.0" + err = fakeClient.Update(context.TODO(), workload1) + require.Nil(t, err) + + // create a new instance of a CreationRequest + newKACR := &klcv1alpha3.KeptnAppCreationRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-kacr", + Namespace: namespace, + CreationTimestamp: metav1.Time{Time: theClock.Now()}, + }, + Spec: klcv1alpha3.KeptnAppCreationRequestSpec{ + AppName: appName, + }, + } + err = fakeClient.Create(context.TODO(), newKACR) + require.Nil(t, err) + + // turn the clock forward + theClock.Add(1 * time.Minute) + + res, err = r.Reconcile(context.TODO(), request) + + require.Nil(t, err) + require.False(t, res.Requeue) + require.Zero(t, res.RequeueAfter) + + kApp = &klcv1alpha3.KeptnApp{} + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: kacr.Spec.AppName, Namespace: kacr.Namespace}, kApp) + + require.Nil(t, err) + require.NotEmpty(t, kApp) + + require.NotEmpty(t, kApp.Spec.Version) + // the version number of the app should have been changed + require.NotEqual(t, firstVersion, kApp.Spec.Version) + require.Len(t, kApp.Spec.Workloads, 1) + require.Contains(t, kApp.Spec.Workloads, klcv1alpha3.KeptnWorkloadRef{ + Name: workload1.Name, + Version: workload1.Spec.Version, + }) +} + +func TestKeptnAppCreationRequestReconciler_RemoveWorkload(t *testing.T) { + r, fakeClient, theClock := setupReconcilerAndClient(t) + const namespace = "my-namespace" + const appName = "my-app" + kacr := &klcv1alpha3.KeptnAppCreationRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-kacr", + Namespace: namespace, + CreationTimestamp: metav1.Time{Time: theClock.Now()}, + }, + Spec: klcv1alpha3.KeptnAppCreationRequestSpec{ + AppName: appName, + }, + } + + err := fakeClient.Create(context.TODO(), kacr) + require.Nil(t, err) + + workload1 := &klcv1alpha3.KeptnWorkload{ + ObjectMeta: metav1.ObjectMeta{ + Name: "w1", + Namespace: namespace, + }, + Spec: klcv1alpha3.KeptnWorkloadSpec{ + AppName: appName, + Version: "1.0", + }, + } + + err = fakeClient.Create(context.TODO(), workload1) + require.Nil(t, err) + + workload2 := &klcv1alpha3.KeptnWorkload{ + ObjectMeta: metav1.ObjectMeta{ + Name: "w2", + Namespace: namespace, + }, + Spec: klcv1alpha3.KeptnWorkloadSpec{ + AppName: appName, + Version: "1.0", + }, + } + + err = fakeClient.Create(context.TODO(), workload2) + require.Nil(t, err) + + request := controllerruntime.Request{ + NamespacedName: types.NamespacedName{ + Namespace: kacr.Namespace, + Name: kacr.Name, + }, + } + + // turn the clock forward and reconcile + theClock.Add(1 * time.Minute) + res, err := r.Reconcile(context.TODO(), request) + + require.Nil(t, err) + require.False(t, res.Requeue) + require.Zero(t, res.RequeueAfter) + + kApp := &klcv1alpha3.KeptnApp{} + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: kacr.Spec.AppName, Namespace: kacr.Namespace}, kApp) + + require.Nil(t, err) + require.NotEmpty(t, kApp) + + require.NotEmpty(t, kApp.Spec.Version) + require.Len(t, kApp.Spec.Workloads, 2) + require.Contains(t, kApp.Spec.Workloads, klcv1alpha3.KeptnWorkloadRef{ + Name: workload1.Name, + Version: workload1.Spec.Version, + }) + require.Contains(t, kApp.Spec.Workloads, klcv1alpha3.KeptnWorkloadRef{ + Name: workload2.Name, + Version: workload2.Spec.Version, + }) + + firstVersion := kApp.Spec.Version + + // verify that the creationRequest has been deleted + cr := &klcv1alpha3.KeptnAppCreationRequest{} + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: kacr.Name, Namespace: kacr.Namespace}, cr) + + require.True(t, errors.IsNotFound(err)) + + // delete one of the workloads + err = fakeClient.Delete(context.TODO(), workload1) + require.Nil(t, err) + + // create a new instance of a CreationRequest + newKACR := &klcv1alpha3.KeptnAppCreationRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-kacr", + Namespace: namespace, + CreationTimestamp: metav1.Time{Time: theClock.Now()}, + }, + Spec: klcv1alpha3.KeptnAppCreationRequestSpec{ + AppName: appName, + }, + } + err = fakeClient.Create(context.TODO(), newKACR) + require.Nil(t, err) + + // turn the clock forward + theClock.Add(1 * time.Minute) + + // reconcile again - now we should get an updated KeptnApp as a result + res, err = r.Reconcile(context.TODO(), request) + + require.Nil(t, err) + require.False(t, res.Requeue) + require.Zero(t, res.RequeueAfter) + + kApp = &klcv1alpha3.KeptnApp{} + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: kacr.Spec.AppName, Namespace: kacr.Namespace}, kApp) + + require.Nil(t, err) + require.NotEmpty(t, kApp) + + require.NotEmpty(t, kApp.Spec.Version) + require.NotEqual(t, firstVersion, kApp.Spec.Version) + // now we should see only one workload + require.Len(t, kApp.Spec.Workloads, 1) + require.Contains(t, kApp.Spec.Workloads, klcv1alpha3.KeptnWorkloadRef{ + Name: workload2.Name, + Version: workload2.Spec.Version, + }) +} + func TestKeptnAppCreationRequestReconciler_DoNotOverwriteUserDefinedApp(t *testing.T) { r, fakeClient, theClock := setupReconcilerAndClient(t) const namespace = "my-namespace" diff --git a/test/integration/app-creation-request/00-assert.yaml b/test/integration/app-creation-request/00-assert.yaml index f6ca1ff6e0..c7de12ac1b 100644 --- a/test/integration/app-creation-request/00-assert.yaml +++ b/test/integration/app-creation-request/00-assert.yaml @@ -1,4 +1,4 @@ -# Check for the KeptnApp to be created - the KeptnAppCreateionRequest will not be here anymore as it is +# Check for the KeptnApp to be created - the KeptnAppCreationRequest will not be here anymore as it is # deleted after reconciliation apiVersion: lifecycle.keptn.sh/v1alpha3 kind: KeptnApp diff --git a/test/integration/app-creation-request/01-assert.yaml b/test/integration/app-creation-request/01-assert.yaml new file mode 100644 index 0000000000..f35fe34fa5 --- /dev/null +++ b/test/integration/app-creation-request/01-assert.yaml @@ -0,0 +1,14 @@ +# Check for the KeptnApp to be created - the KeptnAppCreationRequest will not be here anymore as it is +# deleted after reconciliation +apiVersion: lifecycle.keptn.sh/v1alpha3 +kind: KeptnApp +metadata: + name: my-app + labels: + app.kubernetes.io/managed-by: "klt" +spec: + workloads: + - name: my-workload + version: 2.0.0 + - name: my-workload-2 + version: 1.0.0 diff --git a/test/integration/app-creation-request/01-install.yaml b/test/integration/app-creation-request/01-install.yaml new file mode 100644 index 0000000000..eefdf047c0 --- /dev/null +++ b/test/integration/app-creation-request/01-install.yaml @@ -0,0 +1,58 @@ +apiVersion: lifecycle.keptn.sh/v1alpha3 +kind: KeptnAppCreationRequest +metadata: + name: my-kacr +spec: + appName: my-app +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: test + name: test +spec: + replicas: 1 + selector: + matchLabels: + app: test + strategy: {} + template: + metadata: + labels: + app: test + annotations: + keptn.sh/workload: my-workload + keptn.sh/version: "2.0.0" + keptn.sh/app: "my-app" + spec: + containers: + - image: busybox + name: busybox + command: ['sh', '-c', 'echo The app is running! && sleep infinity'] +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: test2 + name: test2 +spec: + replicas: 1 + selector: + matchLabels: + app: test2 + strategy: {} + template: + metadata: + labels: + app: test + annotations: + keptn.sh/workload: my-workload-2 + keptn.sh/version: "1.0.0" + keptn.sh/app: "my-app" + spec: + containers: + - image: busybox + name: busybox + command: ['sh', '-c', 'echo The app is running! && sleep infinity'] From bf96eb986b4083ea09b9eb05d4899c2e51596c09 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Mon, 24 Apr 2023 10:15:09 +0200 Subject: [PATCH 2/4] fix label selector Signed-off-by: Florian Bacher --- test/integration/app-creation-request/01-install.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/app-creation-request/01-install.yaml b/test/integration/app-creation-request/01-install.yaml index eefdf047c0..27c9b1d7bd 100644 --- a/test/integration/app-creation-request/01-install.yaml +++ b/test/integration/app-creation-request/01-install.yaml @@ -46,7 +46,7 @@ spec: template: metadata: labels: - app: test + app: test2 annotations: keptn.sh/workload: my-workload-2 keptn.sh/version: "1.0.0" From 8a01ba6e4afb5cdfab0fb48ff16e1ff460539258 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Mon, 24 Apr 2023 11:04:10 +0200 Subject: [PATCH 3/4] pr review Signed-off-by: Florian Bacher --- .../lifecycle/v1alpha3/keptnworkload_types.go | 5 +++ .../v1alpha3/keptnworkload_types_test.go | 43 +++++++++++++++++++ .../keptnappcreationrequest/controller.go | 5 +-- 3 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go diff --git a/operator/apis/lifecycle/v1alpha3/keptnworkload_types.go b/operator/apis/lifecycle/v1alpha3/keptnworkload_types.go index fedbca3e80..c4c6937941 100644 --- a/operator/apis/lifecycle/v1alpha3/keptnworkload_types.go +++ b/operator/apis/lifecycle/v1alpha3/keptnworkload_types.go @@ -17,6 +17,7 @@ limitations under the License. package v1alpha3 import ( + "fmt" "strings" "github.com/keptn/lifecycle-toolkit/operator/apis/lifecycle/v1alpha3/common" @@ -117,3 +118,7 @@ func (w KeptnWorkload) GetEventAnnotations() map[string]string { "workloadVersion": w.Spec.Version, } } + +func (w KeptnWorkload) GetNameWithoutAppPrefix() string { + return strings.TrimPrefix(w.Name, fmt.Sprintf("%s-", w.Spec.AppName)) +} diff --git a/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go b/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go new file mode 100644 index 0000000000..4657ce34ff --- /dev/null +++ b/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go @@ -0,0 +1,43 @@ +package v1alpha3 + +import ( + "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "testing" +) + +func TestKeptnWorkload_GetNameWithoutAppPrefix(t *testing.T) { + type fields struct { + ObjectMeta v1.ObjectMeta + Spec KeptnWorkloadSpec + } + tests := []struct { + name string + fields fields + want string + }{ + { + name: "remove app prefix", + fields: fields{ + ObjectMeta: v1.ObjectMeta{ + Name: "my-app-my-workload", + }, + Spec: KeptnWorkloadSpec{ + AppName: "my-app", + }, + }, + want: "my-workload", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := KeptnWorkload{ + ObjectMeta: tt.fields.ObjectMeta, + Spec: tt.fields.Spec, + } + got := w.GetNameWithoutAppPrefix() + + require.Equal(t, tt.want, got) + }) + } +} diff --git a/operator/controllers/lifecycle/keptnappcreationrequest/controller.go b/operator/controllers/lifecycle/keptnappcreationrequest/controller.go index f57f0c6d68..410a0cfd1b 100644 --- a/operator/controllers/lifecycle/keptnappcreationrequest/controller.go +++ b/operator/controllers/lifecycle/keptnappcreationrequest/controller.go @@ -180,7 +180,7 @@ func (r *KeptnAppCreationRequestReconciler) addOrUpdateWorkloads(workloads *life updated := false for _, workload := range workloads.Items { foundWorkload := false - workloadName := strings.TrimPrefix(workload.Name, fmt.Sprintf("%s-", keptnApp.Name)) + workloadName := workload.GetNameWithoutAppPrefix() for index, appWorkload := range keptnApp.Spec.Workloads { if appWorkload.Name == workloadName { // make sure the version matches the current version of the workload @@ -210,8 +210,7 @@ func (r *KeptnAppCreationRequestReconciler) cleanupWorkloads(workloads *lifecycl for index, appWorkload := range keptnApp.Spec.Workloads { foundWorkload := false for _, workload := range workloads.Items { - workloadName := strings.TrimPrefix(workload.Name, fmt.Sprintf("%s-", keptnApp.Name)) - if appWorkload.Name == workloadName { + if appWorkload.Name == workload.GetNameWithoutAppPrefix() { foundWorkload = true break } From 2da0779c66d5fd005f6040eeaac903aba600fa17 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Mon, 24 Apr 2023 12:50:46 +0200 Subject: [PATCH 4/4] fix linting, pr review Signed-off-by: Florian Bacher --- .../lifecycle/v1alpha3/keptnworkload_types_test.go | 3 ++- .../lifecycle/keptnappcreationrequest/controller.go | 5 +++-- .../keptnappcreationrequest/controller_test.go | 10 ++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go b/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go index 4657ce34ff..a114e026ef 100644 --- a/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go +++ b/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go @@ -1,9 +1,10 @@ package v1alpha3 import ( + "testing" + "github.com/stretchr/testify/require" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "testing" ) func TestKeptnWorkload_GetNameWithoutAppPrefix(t *testing.T) { diff --git a/operator/controllers/lifecycle/keptnappcreationrequest/controller.go b/operator/controllers/lifecycle/keptnappcreationrequest/controller.go index 410a0cfd1b..b02d940976 100644 --- a/operator/controllers/lifecycle/keptnappcreationrequest/controller.go +++ b/operator/controllers/lifecycle/keptnappcreationrequest/controller.go @@ -207,20 +207,21 @@ func (r *KeptnAppCreationRequestReconciler) addOrUpdateWorkloads(workloads *life func (r *KeptnAppCreationRequestReconciler) cleanupWorkloads(workloads *lifecycle.KeptnWorkloadList, keptnApp *lifecycle.KeptnApp) bool { updated := false + updatedWorkloads := []lifecycle.KeptnWorkloadRef{} for index, appWorkload := range keptnApp.Spec.Workloads { foundWorkload := false for _, workload := range workloads.Items { if appWorkload.Name == workload.GetNameWithoutAppPrefix() { - foundWorkload = true + updatedWorkloads = append(updatedWorkloads, keptnApp.Spec.Workloads[index]) break } } if !foundWorkload { - keptnApp.Spec.Workloads = append(keptnApp.Spec.Workloads[:index], keptnApp.Spec.Workloads[index+1:]...) updated = true } } + keptnApp.Spec.Workloads = updatedWorkloads return updated } diff --git a/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go b/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go index 2c84d12935..7441939e87 100644 --- a/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go +++ b/operator/controllers/lifecycle/keptnappcreationrequest/controller_test.go @@ -2,6 +2,7 @@ package keptnappcreationrequest import ( "context" + "fmt" "testing" "time" @@ -590,3 +591,12 @@ func setupReconcilerAndClient(t *testing.T) (*KeptnAppCreationRequestReconciler, } return r, fakeClient, theClock } + +func TestKeptnAppCreationRequestReconciler_cleanupWorkloads(t *testing.T) { + mySlice := []string{"a", "b", "c", "d"} + + res := append(mySlice[:2], mySlice[2+1:]...) + + fmt.Println(cap(mySlice)) + fmt.Println(cap(res)) +}