From 959643d01a424d87d36e272e88edd614a589400e Mon Sep 17 00:00:00 2001
From: Bartosz Majsak <bartosz.majsak@gmail.com>
Date: Thu, 8 Aug 2024 15:15:05 +0200
Subject: [PATCH] fix(feature): ensures all new resources are created in one
 reconcile (#1159)

Manifests used by Feature to create cluster resources can be either
defined as a single resource per file or grouped together and separated
using the `---` delimiter. Even though the former approach is currently
preferred across the codebase, the latter is used to create Envoy filters
for the KServe setup.

When applying resources not yet present in the cluster, the original
implementation returns after creating the first resource, leaving the rest
unprocessed. They are applied in the next reconcile loop, though again
on a one-per-reconcile event basis. This is not a bug per se, as it will
eventually result in creating all resources; however, it is suboptimal.

This change ensures all resources defined in a single YAML manifest,
separated using `---`, are processed in a single go.
---
 pkg/feature/resource/operations.go              |  5 +++--
 .../features/fixtures/templates/namespace.yaml  |  4 ----
 .../features/fixtures/templates/namespaces.yaml | 10 ++++++++++
 .../integration/features/manifests_int_test.go  | 17 +++++++++++------
 4 files changed, 24 insertions(+), 12 deletions(-)
 delete mode 100644 tests/integration/features/fixtures/templates/namespace.yaml
 create mode 100644 tests/integration/features/fixtures/templates/namespaces.yaml

diff --git a/pkg/feature/resource/operations.go b/pkg/feature/resource/operations.go
index 8d7a35b5c0e..e2d56b4ad40 100644
--- a/pkg/feature/resource/operations.go
+++ b/pkg/feature/resource/operations.go
@@ -31,15 +31,16 @@ func Apply(ctx context.Context, cli client.Client, objects []*unstructured.Unstr
 			return fmt.Errorf("failed to get resource %s/%s: %w", namespace, name, errGet)
 		}
 
+		justCreated := false
 		if k8serr.IsNotFound(errGet) {
 			if errCreate := cli.Create(ctx, target); client.IgnoreAlreadyExists(errCreate) != nil {
 				return fmt.Errorf("failed to create source %s/%s: %w", namespace, name, errCreate)
 			}
 
-			return nil
+			justCreated = true
 		}
 
-		if shouldReconcile(source) {
+		if !justCreated && shouldReconcile(source) {
 			if errUpdate := patchUsingApplyStrategy(ctx, cli, source, target); errUpdate != nil {
 				return fmt.Errorf("failed to reconcile resource %s/%s: %w", namespace, name, errUpdate)
 			}
diff --git a/tests/integration/features/fixtures/templates/namespace.yaml b/tests/integration/features/fixtures/templates/namespace.yaml
deleted file mode 100644
index 6e6e5ab44cb..00000000000
--- a/tests/integration/features/fixtures/templates/namespace.yaml
+++ /dev/null
@@ -1,4 +0,0 @@
-apiVersion: v1
-kind: Namespace
-metadata:
-  name: embedded-test-ns
diff --git a/tests/integration/features/fixtures/templates/namespaces.yaml b/tests/integration/features/fixtures/templates/namespaces.yaml
new file mode 100644
index 00000000000..62a42a34a06
--- /dev/null
+++ b/tests/integration/features/fixtures/templates/namespaces.yaml
@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: embedded-test-ns-1
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: embedded-test-ns-2
+
diff --git a/tests/integration/features/manifests_int_test.go b/tests/integration/features/manifests_int_test.go
index 6811b900b8b..6f084c29663 100644
--- a/tests/integration/features/manifests_int_test.go
+++ b/tests/integration/features/manifests_int_test.go
@@ -46,11 +46,11 @@ var _ = Describe("Applying resources", func() {
 	It("should be able to process an embedded YAML file", func(ctx context.Context) {
 		// given
 		featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error {
-			errNsCreate := registry.Add(feature.Define("create-namespace").
+			errNsCreate := registry.Add(feature.Define("create-namespaces").
 				UsingConfig(envTest.Config).
 				Manifests(
 					manifest.Location(fixtures.TestEmbeddedFiles).
-						Include(path.Join(fixtures.BaseDir, "namespace.yaml")),
+						Include(path.Join(fixtures.BaseDir, "namespaces.yaml")),
 				),
 			)
 
@@ -63,10 +63,15 @@ var _ = Describe("Applying resources", func() {
 		Expect(featuresHandler.Apply(ctx)).To(Succeed())
 
 		// then
-		embeddedNs, err := fixtures.GetNamespace(ctx, envTestClient, "embedded-test-ns")
-		defer objectCleaner.DeleteAll(ctx, embeddedNs)
-		Expect(err).ToNot(HaveOccurred())
-		Expect(embeddedNs.Name).To(Equal("embedded-test-ns"))
+		embeddedNs1, errNS1 := fixtures.GetNamespace(ctx, envTestClient, "embedded-test-ns-1")
+		embeddedNs2, errNS2 := fixtures.GetNamespace(ctx, envTestClient, "embedded-test-ns-2")
+		defer objectCleaner.DeleteAll(ctx, embeddedNs1, embeddedNs2)
+
+		Expect(errNS1).ToNot(HaveOccurred())
+		Expect(errNS2).ToNot(HaveOccurred())
+
+		Expect(embeddedNs1.Name).To(Equal("embedded-test-ns-1"))
+		Expect(embeddedNs2.Name).To(Equal("embedded-test-ns-2"))
 	})
 
 	It("should be able to process an embedded template file", func(ctx context.Context) {