From b3b70109a9125ef4ea017a5f3d25d02146438a46 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Tue, 25 Apr 2023 09:29:40 +0200 Subject: [PATCH] feat(operator): consider corner cases in KACR controller (#1270) Signed-off-by: Florian Bacher --- .../lifecycle/v1alpha3/keptnworkload_types.go | 5 + .../v1alpha3/keptnworkload_types_test.go | 44 +++ .../keptnappcreationrequest/controller.go | 41 ++- .../controller_test.go | 263 +++++++++++++++++- .../app-creation-request/00-assert.yaml | 2 +- .../app-creation-request/01-assert.yaml | 14 + .../app-creation-request/01-install.yaml | 58 ++++ 7 files changed, 417 insertions(+), 10 deletions(-) create mode 100644 operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go 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/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..a114e026ef --- /dev/null +++ b/operator/apis/lifecycle/v1alpha3/keptnworkload_types_test.go @@ -0,0 +1,44 @@ +package v1alpha3 + +import ( + "testing" + + "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +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 c827d80666..b02d940976 100644 --- a/operator/controllers/lifecycle/keptnappcreationrequest/controller.go +++ b/operator/controllers/lifecycle/keptnappcreationrequest/controller.go @@ -164,11 +164,23 @@ 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)) + 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 @@ -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 + updatedWorkloads := []lifecycle.KeptnWorkloadRef{} + for index, appWorkload := range keptnApp.Spec.Workloads { + foundWorkload := false + for _, workload := range workloads.Items { + if appWorkload.Name == workload.GetNameWithoutAppPrefix() { + updatedWorkloads = append(updatedWorkloads, keptnApp.Spec.Workloads[index]) + break + } + } - return r.Update(ctx, keptnApp) + if !foundWorkload { + updated = true + } + } + keptnApp.Spec.Workloads = updatedWorkloads + 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..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" @@ -113,7 +114,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 +247,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" @@ -339,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)) +} 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..27c9b1d7bd --- /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: test2 + 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']