Progress

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: Per Goncalves da Silva

internal/operator-controller/applier/boxcutter.go

@@ -1,23 +1,22 @@
 package applier
 
 import (
+	"cmp"
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"github.com/davecgh/go-spew/spew"
 	"hash"
 	"io/fs"
-	"maps"
-	"slices"
-	"sort"
-
-	"github.com/davecgh/go-spew/spew"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"maps"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"slices"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
@@ -27,40 +26,37 @@ import (
 
 const (
 	revisionHashAnnotation = "olm.operatorframework.io/hash"
-	revisionHistoryLimit   = 5
+	// revisionHistoryLimit   = 5
 )
 
-type Boxcutter struct {
-	Client         client.Client
-	Scheme         *runtime.Scheme
-	BundleRenderer render.BundleRenderer
+type BundleRenderer interface {
+	Render(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error)
 }
 
-func (bc *Boxcutter) Apply(
-	ctx context.Context, contentFS fs.FS,
-	ext *ocv1.ClusterExtension,
-	objectLabels, storageLabels map[string]string,
-) ([]client.Object, string, error) {
-	objs, err := bc.apply(ctx, contentFS, ext, objectLabels, storageLabels)
-	return objs, "", err
+type RegistryV1BundleRenderer struct {
+	BundleRenderer render.BundleRenderer
 }
 
-func (bc *Boxcutter) apply(
-	ctx context.Context, contentFS fs.FS,
-	ext *ocv1.ClusterExtension,
-	objectLabels, _ map[string]string,
-) ([]client.Object, error) {
-	reg, err := source.FromFS(contentFS).GetBundle()
+func (r *RegistryV1BundleRenderer) Render(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error) {
+	reg, err := source.FromFS(bundleFS).GetBundle()
 	if err != nil {
 		return nil, err
 	}
-
 	watchNamespace, err := GetWatchNamespace(ext)
 	if err != nil {
 		return nil, err
 	}
+	return r.BundleRenderer.Render(reg, ext.Spec.Namespace, render.WithTargetNamespaces(watchNamespace))
+}
 
-	plain, err := bc.BundleRenderer.Render(reg, ext.Spec.Namespace, render.WithTargetNamespaces(watchNamespace))
+type BundleRevisionBuilder struct {
+	Scheme         *runtime.Scheme
+	BundleRenderer BundleRenderer
+}
+
+func (r *BundleRevisionBuilder) MakeClusterExtensionRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string) (*ocv1.ClusterExtensionRevision, error) {
+	// extract plain manifests
+	plain, err := r.BundleRenderer.Render(bundleFS, ext)
 	if err != nil {
 		return nil, err
 	}
@@ -72,7 +68,7 @@ func (bc *Boxcutter) apply(
 		maps.Copy(labels, objectLabels)
 		obj.SetLabels(labels)
 
-		gvk, err := apiutil.GVKForObject(obj, bc.Scheme)
+		gvk, err := apiutil.GVKForObject(obj, r.Scheme)
 		if err != nil {
 			return nil, err
 		}
@@ -89,18 +85,8 @@ func (bc *Boxcutter) apply(
 		})
 	}
 
-	// List all existing revisions
-	existingRevisionList := &ocv1.ClusterExtensionRevisionList{}
-	if err := bc.Client.List(ctx, existingRevisionList, client.MatchingLabels{
-		controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
-	}); err != nil {
-		return nil, fmt.Errorf("listing revisions: %w", err)
-	}
-	sort.Sort(revisionAscending(existingRevisionList.Items))
-	existingRevisions := existingRevisionList.Items
-
 	// Build desired revision
-	desiredRevision := &ocv1.ClusterExtensionRevision{
+	return &ocv1.ClusterExtensionRevision{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{},
 			Labels: map[string]string{
@@ -116,6 +102,31 @@ func (bc *Boxcutter) apply(
 				},
 			},
 		},
+	}, nil
+}
+
+type Boxcutter struct {
+	Client                client.Client
+	Scheme                *runtime.Scheme
+	BundleRevisionBuilder BundleRevisionBuilder
+}
+
+func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, storageLabels map[string]string) ([]client.Object, string, error) {
+	objs, err := bc.apply(ctx, contentFS, ext, objectLabels, storageLabels)
+	return objs, "", err
+}
+
+func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, _ map[string]string) ([]client.Object, error) {
+	// Generate desired revision
+	desiredRevision, err := bc.BundleRevisionBuilder.MakeClusterExtensionRevision(contentFS, ext, objectLabels)
+	if err != nil {
+		return nil, err
+	}
+
+	// List all existing revisions
+	existingRevisions, err := bc.getExistingRevisions(ctx, ext.GetName())
+	if err != nil {
+		return nil, err
 	}
 	desiredHash := computeSHA256Hash(desiredRevision.Spec.Phases)
 
@@ -126,11 +137,9 @@ func (bc *Boxcutter) apply(
 	)
 	if len(existingRevisions) > 0 {
 		maybeCurrentRevision := existingRevisions[len(existingRevisions)-1]
-
 		annotations := maybeCurrentRevision.GetAnnotations()
 		if annotations != nil {
-			if hash, ok := annotations[revisionHashAnnotation]; ok &&
-				hash == desiredHash {
+			if revisionHash, ok := annotations[revisionHashAnnotation]; ok && revisionHash == desiredHash {
 				currentRevision = &maybeCurrentRevision
 				prevRevisions = existingRevisions[0 : len(existingRevisions)-1] // previous is everything excluding current
 			}
@@ -140,8 +149,7 @@ func (bc *Boxcutter) apply(
 	if currentRevision == nil {
 		// all Revisions are outdated => create a new one.
 		prevRevisions = existingRevisions
-		revisionNumber := latestRevisionNumber(prevRevisions)
-		revisionNumber++
+		revisionNumber := latestRevisionNumber(prevRevisions) + 1
 
 		newRevision := desiredRevision
 		newRevision.Name = fmt.Sprintf("%s-%d", ext.Name, revisionNumber)
@@ -163,25 +171,46 @@ func (bc *Boxcutter) apply(
 	}
 
 	// Delete archived previous revisions over revisionHistory limit
-	numToDelete := len(prevRevisions) - revisionHistoryLimit
-	slices.Reverse(prevRevisions)
+	//numToDelete := len(prevRevisions) - revisionHistoryLimit
+	//slices.Reverse(prevRevisions)
+	//
+	//for _, prevRev := range prevRevisions {
+	//	if numToDelete <= 0 {
+	//		break
+	//	}
+	//
+	//	if err := client.IgnoreNotFound(bc.Client.Delete(ctx, &prevRev)); err != nil {
+	//		return nil, fmt.Errorf("failed to delete revision (history limit): %w", err)
+	//	}
+	//	numToDelete--
+	//}
 
-	for _, prevRev := range prevRevisions {
-		if numToDelete <= 0 {
-			break
-		}
+	// TODO: Read status from revision.
 
-		if err := client.IgnoreNotFound(bc.Client.Delete(ctx, &prevRev)); err != nil {
-			return nil, fmt.Errorf("failed to delete revision (history limit): %w", err)
+	// Collect objects
+	var plain []client.Object
+	for _, phase := range desiredRevision.Spec.Phases {
+		for _, phaseObject := range phase.Objects {
+			plain = append(plain, &phaseObject.Object)
 		}
-		numToDelete--
 	}
-
-	// TODO: Read status from revision.
-
 	return plain, nil
 }
 
+// getExistingRevisions returns the list of ClusterExtensionRevisions for a ClusterExtension with name extName in revision order (oldest to newest)
+func (bc *Boxcutter) getExistingRevisions(ctx context.Context, extName string) ([]ocv1.ClusterExtensionRevision, error) {
+	existingRevisionList := &ocv1.ClusterExtensionRevisionList{}
+	if err := bc.Client.List(ctx, existingRevisionList, client.MatchingLabels{
+		controllers.ClusterExtensionRevisionOwnerLabel: extName,
+	}); err != nil {
+		return nil, fmt.Errorf("listing revisions: %w", err)
+	}
+	slices.SortFunc(existingRevisionList.Items, func(a, b ocv1.ClusterExtensionRevision) int {
+		return cmp.Compare(a.Spec.Revision, b.Spec.Revision)
+	})
+	return existingRevisionList.Items, nil
+}
+
 // computeSHA256Hash returns a sha236 hash value calculated from object.
 func computeSHA256Hash(obj any) string {
 	hasher := sha256.New()
@@ -206,21 +235,9 @@ func deepHashObject(hasher hash.Hash, objectToWrite any) {
 	}
 }
 
-type revisionAscending []ocv1.ClusterExtensionRevision
-
-func (a revisionAscending) Len() int      { return len(a) }
-func (a revisionAscending) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
-func (a revisionAscending) Less(i, j int) bool {
-	iObj := a[i]
-	jObj := a[j]
-
-	return iObj.Spec.Revision < jObj.Spec.Revision
-}
-
 func latestRevisionNumber(prevRevisions []ocv1.ClusterExtensionRevision) int64 {
 	if len(prevRevisions) == 0 {
 		return 0
 	}
-
 	return prevRevisions[len(prevRevisions)-1].Spec.Revision
 }

internal/operator-controller/applier/boxcutter_test.go

@@ -0,0 +1,11 @@
+package applier_test
+
+import (
+	"testing"
+
+	testutils "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing"
+)
+
+func Test_RegistryV1BundleRenderer_Render_Success(t *testing.T) {
+	bundleFS := testutils.NewBundleFS()
+}

internal/operator-controller/rukpak/bundle/source/source_test.go

@@ -10,14 +10,11 @@ import (
 
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
+	. "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing"
 )
 
 const (
 	olmProperties = "olm.properties"
-
-	bundlePathAnnotations = "metadata/annotations.yaml"
-	bundlePathProperties  = "metadata/properties.yaml"
-	bundlePathCSV         = "manifests/csv.yaml"
 )
 
 func Test_FromBundle_Success(t *testing.T) {
@@ -30,7 +27,7 @@ func Test_FromBundle_Success(t *testing.T) {
 }
 
 func Test_FromFS_Success(t *testing.T) {
-	rv1, err := source.FromFS(newBundleFS()).GetBundle()
+	rv1, err := source.FromFS(NewBundleFS()).GetBundle()
 	require.NoError(t, err)
 
 	t.Log("Check package name is correctly taken from metadata/annotations.yaml")
@@ -47,16 +44,16 @@ func Test_FromFS_Fails(t *testing.T) {
 	}{
 		{
 			name: "bundle missing ClusterServiceVersion manifest",
-			FS:   removePaths(newBundleFS(), bundlePathCSV),
+			FS:   removePaths(NewBundleFS(), BundlePathCSV),
 		}, {
 			name: "bundle missing metadata/annotations.yaml",
-			FS:   removePaths(newBundleFS(), bundlePathAnnotations),
+			FS:   removePaths(NewBundleFS(), BundlePathAnnotations),
 		}, {
 			name: "bundle missing metadata/ directory",
-			FS:   removePaths(newBundleFS(), "metadata/"),
+			FS:   removePaths(NewBundleFS(), "metadata/"),
 		}, {
 			name: "bundle missing manifests/ directory",
-			FS:   removePaths(newBundleFS(), "manifests/"),
+			FS:   removePaths(NewBundleFS(), "manifests/"),
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -66,39 +63,6 @@ func Test_FromFS_Fails(t *testing.T) {
 	}
 }
 
-func newBundleFS() fstest.MapFS {
-	annotationsYml := `
-annotations:
-  operators.operatorframework.io.bundle.mediatype.v1: registry+v1
-  operators.operatorframework.io.bundle.package.v1: test
-`
-
-	propertiesYml := `
-properties:
-  - type: "from-file-key"
-    value: "from-file-value"
-`
-
-	csvYml := `
-apiVersion: operators.operatorframework.io/v1alpha1
-kind: ClusterServiceVersion
-metadata:
-  name: test.v1.0.0
-  annotations:
-    olm.properties: '[{"type":"from-csv-annotations-key", "value":"from-csv-annotations-value"}]'
-spec:
-  installModes:
-    - type: AllNamespaces
-      supported: true
-`
-
-	return fstest.MapFS{
-		bundlePathAnnotations: &fstest.MapFile{Data: []byte(strings.Trim(annotationsYml, "\n"))},
-		bundlePathProperties:  &fstest.MapFile{Data: []byte(strings.Trim(propertiesYml, "\n"))},
-		bundlePathCSV:         &fstest.MapFile{Data: []byte(strings.Trim(csvYml, "\n"))},
-	}
-}
-
 func removePaths(mapFs fstest.MapFS, paths ...string) fstest.MapFS {
 	for k := range mapFs {
 		for _, path := range paths {