BundleSource cleanup (#1061)

Signed-off-by: dtfranz <[email protected]>

Author: dtfranz

internal/controllers/clusterextension_controller.go

@@ -67,7 +67,6 @@ import (
 	"github.com/operator-framework/operator-controller/internal/conditionsets"
 	"github.com/operator-framework/operator-controller/internal/labels"
 	"github.com/operator-framework/operator-controller/internal/resolve"
-	"github.com/operator-framework/operator-controller/internal/rukpak/bundledeployment"
 	"github.com/operator-framework/operator-controller/internal/rukpak/convert"
 	"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
 	rukpaksource "github.com/operator-framework/operator-controller/internal/rukpak/source"
@@ -278,12 +277,14 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	ext.Status.ResolvedBundle = bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion)
 	setResolvedStatusConditionSuccess(ext, fmt.Sprintf("resolved to %q", resolvedBundle.Image))
 
-	// Generate a BundleDeployment from the ClusterExtension to Unpack.
-	// Note: The BundleDeployment here is not a k8s API, its a simple Go struct which
-	// necessary embedded values.
-	bd := r.generateBundleDeploymentForUnpack(resolvedBundle.Image, ext)
+	bundleSource := &rukpaksource.BundleSource{
+		Type: rukpaksource.SourceTypeImage,
+		Image: &rukpaksource.ImageSource{
+			Ref: resolvedBundle.Image,
+		},
+	}
 	l.V(1).Info("unpacking resolved bundle")
-	unpackResult, err := r.Unpacker.Unpack(ctx, bd)
+	unpackResult, err := r.Unpacker.Unpack(ctx, bundleSource)
 	if err != nil {
 		setStatusUnpackFailed(ext, err.Error())
 		return ctrl.Result{}, err
@@ -505,21 +506,6 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundleName string, d
 	}
 }
 
-func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(bundlePath string, ce *ocv1alpha1.ClusterExtension) *bundledeployment.BundleDeployment {
-	return &bundledeployment.BundleDeployment{
-		Name: ce.Name,
-		Spec: bundledeployment.BundleDeploymentSpec{
-			InstallNamespace: ce.Spec.InstallNamespace,
-			Source: bundledeployment.BundleSource{
-				Type: bundledeployment.SourceTypeImage,
-				Image: &bundledeployment.ImageSource{
-					Ref: bundlePath,
-				},
-			},
-		},
-	}
-}
-
 // SetupWithManager sets up the controller with the Manager.
 func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	controller, err := ctrl.NewControllerManagedBy(mgr).

internal/controllers/clusterextension_controller_test.go

@@ -89,7 +89,7 @@ func TestClusterExtensionResolutionSucceeds(t *testing.T) {
 	cl, reconciler := newClientAndReconciler(t, nil)
 	mockUnpacker := unpacker.(*MockUnpacker)
 	// Set up the Unpack method to return a result with StateUnpacked
-	mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*bundledeployment.BundleDeployment")).Return(&source.Result{
+	mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*source.BundleSource")).Return(&source.Result{
 		State: source.StatePending,
 	}, nil)
 

internal/controllers/clusterextension_registryv1_validation_test.go

@@ -100,7 +100,7 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) {
 			})
 			mockUnpacker := unpacker.(*MockUnpacker)
 			// Set up the Unpack method to return a result with StatePending
-			mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*v1alpha2.BundleDeployment")).Return(&source.Result{
+			mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*source.BundleSource")).Return(&source.Result{
 				State: source.StatePending,
 			}, nil)
 

internal/controllers/suite_test.go

@@ -37,7 +37,6 @@ import (
 
 	ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
 	"github.com/operator-framework/operator-controller/internal/controllers"
-	bd "github.com/operator-framework/operator-controller/internal/rukpak/bundledeployment"
 	"github.com/operator-framework/operator-controller/internal/rukpak/source"
 )
 
@@ -47,12 +46,12 @@ type MockUnpacker struct {
 }
 
 // Unpack mocks the Unpack method
-func (m *MockUnpacker) Unpack(ctx context.Context, bd *bd.BundleDeployment) (*source.Result, error) {
-	args := m.Called(ctx, bd)
+func (m *MockUnpacker) Unpack(ctx context.Context, bundle *source.BundleSource) (*source.Result, error) {
+	args := m.Called(ctx, bundle)
 	return args.Get(0).(*source.Result), args.Error(1)
 }
 
-func (m *MockUnpacker) Cleanup(ctx context.Context, bundle *bd.BundleDeployment) error {
+func (m *MockUnpacker) Cleanup(ctx context.Context, bundle *source.BundleSource) error {
 	//TODO implement me
 	panic("implement me")
 }

internal/rukpak/bundledeployment/bundledeployment.go

@@ -20,10 +20,23 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	apimacherrors "k8s.io/apimachinery/pkg/util/errors"
 	"sigs.k8s.io/controller-runtime/pkg/log"
-
-	bd "github.com/operator-framework/operator-controller/internal/rukpak/bundledeployment"
 )
 
+// SourceTypeImage is the identifier for image-type bundle sources
+const SourceTypeImage SourceType = "image"
+
+type ImageSource struct {
+	// Ref contains the reference to a container image containing Bundle contents.
+	Ref string
+	// ImagePullSecretName contains the name of the image pull secret in the namespace that the provisioner is deployed.
+	ImagePullSecretName string
+	// InsecureSkipTLSVerify indicates that TLS certificate validation should be skipped.
+	// If this option is specified, the HTTPS protocol will still be used to
+	// fetch the specified image reference.
+	// This should not be used in a production environment.
+	InsecureSkipTLSVerify bool
+}
+
 // Unrecoverable represents an error that can not be recovered
 // from without user intervention. When this error is returned
 // the request should not be requeued.
@@ -43,25 +56,25 @@ type ImageRegistry struct {
 	CaCertPool    *x509.CertPool
 }
 
-func (i *ImageRegistry) Unpack(ctx context.Context, bundle *bd.BundleDeployment) (*Result, error) {
+func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Result, error) {
 	l := log.FromContext(ctx)
-	if bundle.Spec.Source.Type != bd.SourceTypeImage {
-		panic(fmt.Sprintf("programmer error: source type %q is unable to handle specified bundle source type %q", bd.SourceTypeImage, bundle.Spec.Source.Type))
+	if bundle.Type != SourceTypeImage {
+		panic(fmt.Sprintf("programmer error: source type %q is unable to handle specified bundle source type %q", SourceTypeImage, bundle.Type))
 	}
 
-	if bundle.Spec.Source.Image == nil {
+	if bundle.Image == nil {
 		return nil, NewUnrecoverable(fmt.Errorf("error parsing bundle, bundle %s has a nil image source", bundle.Name))
 	}
 
-	imgRef, err := name.ParseReference(bundle.Spec.Source.Image.Ref)
+	imgRef, err := name.ParseReference(bundle.Image.Ref)
 	if err != nil {
 		return nil, NewUnrecoverable(fmt.Errorf("error parsing image reference: %w", err))
 	}
 
 	remoteOpts := []remote.Option{}
-	if bundle.Spec.Source.Image.ImagePullSecretName != "" {
+	if bundle.Image.ImagePullSecretName != "" {
 		chainOpts := k8schain.Options{
-			ImagePullSecrets: []string{bundle.Spec.Source.Image.ImagePullSecretName},
+			ImagePullSecrets: []string{bundle.Image.ImagePullSecretName},
 			Namespace:        i.AuthNamespace,
 			// TODO: Do we want to use any secrets that are included in the rukpak service account?
 			// If so, we will need to add the permission to get service accounts and specify
@@ -83,7 +96,7 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *bd.BundleDeployment)
 			MinVersion:         tls.VersionTLS12,
 		} // nolint:gosec
 	}
-	if bundle.Spec.Source.Image.InsecureSkipTLSVerify {
+	if bundle.Image.InsecureSkipTLSVerify {
 		transport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec
 	}
 	if i.CaCertPool != nil {
@@ -146,19 +159,19 @@ func wrapUnrecoverable(err error, isUnrecoverable bool) error {
 	return err
 }
 
-func (i *ImageRegistry) Cleanup(_ context.Context, bundle *bd.BundleDeployment) error {
+func (i *ImageRegistry) Cleanup(_ context.Context, bundle *BundleSource) error {
 	return os.RemoveAll(filepath.Join(i.BaseCachePath, bundle.Name))
 }
 
-func unpackedResult(fsys fs.FS, bundle *bd.BundleDeployment, ref string) *Result {
+func unpackedResult(fsys fs.FS, bundle *BundleSource, ref string) *Result {
 	return &Result{
 		Bundle: fsys,
-		ResolvedSource: &bd.BundleSource{
-			Type: bd.SourceTypeImage,
-			Image: &bd.ImageSource{
+		ResolvedSource: &BundleSource{
+			Type: SourceTypeImage,
+			Image: &ImageSource{
 				Ref:                   ref,
-				ImagePullSecretName:   bundle.Spec.Source.Image.ImagePullSecretName,
-				InsecureSkipTLSVerify: bundle.Spec.Source.Image.InsecureSkipTLSVerify,
+				ImagePullSecretName:   bundle.Image.ImagePullSecretName,
+				InsecureSkipTLSVerify: bundle.Image.InsecureSkipTLSVerify,
 			},
 		},
 		State: StateUnpacked,

internal/rukpak/source/image_registry.go

@@ -6,8 +6,6 @@ import (
 	"io/fs"
 
 	"sigs.k8s.io/controller-runtime/pkg/manager"
-
-	bd "github.com/operator-framework/operator-controller/internal/rukpak/bundledeployment"
 )
 
 // Unpacker unpacks bundle content, either synchronously or asynchronously and
@@ -25,8 +23,8 @@ import (
 // specifications. A source should treat a bundle root directory as an opaque
 // file tree and delegate bundle format concerns to bundle parsers.
 type Unpacker interface {
-	Unpack(context.Context, *bd.BundleDeployment) (*Result, error)
-	Cleanup(context.Context, *bd.BundleDeployment) error
+	Unpack(context.Context, *BundleSource) (*Result, error)
+	Cleanup(context.Context, *BundleSource) error
 }
 
 // Result conveys progress information about unpacking bundle content.
@@ -43,7 +41,7 @@ type Result struct {
 	// For example, resolved image sources should reference a container image
 	// digest rather than an image tag, and git sources should reference a
 	// commit hash rather than a branch or tag.
-	ResolvedSource *bd.BundleSource
+	ResolvedSource *BundleSource
 
 	// State is the current state of unpacking the bundle content.
 	State State
@@ -69,28 +67,38 @@ const (
 	StateUnpacked State = "Unpacked"
 )
 
+type SourceType string
+
+type BundleSource struct {
+	Name string
+	// Type defines the kind of Bundle content being sourced.
+	Type SourceType
+	// Image is the bundle image that backs the content of this bundle.
+	Image *ImageSource
+}
+
 type unpacker struct {
-	sources map[bd.SourceType]Unpacker
+	sources map[SourceType]Unpacker
 }
 
 // NewUnpacker returns a new composite Source that unpacks bundles using the source
 // mapping provided by the configured sources.
-func NewUnpacker(sources map[bd.SourceType]Unpacker) Unpacker {
+func NewUnpacker(sources map[SourceType]Unpacker) Unpacker {
 	return &unpacker{sources: sources}
 }
 
-func (s *unpacker) Unpack(ctx context.Context, bundle *bd.BundleDeployment) (*Result, error) {
-	source, ok := s.sources[bundle.Spec.Source.Type]
+func (s *unpacker) Unpack(ctx context.Context, bundle *BundleSource) (*Result, error) {
+	source, ok := s.sources[bundle.Type]
 	if !ok {
-		return nil, fmt.Errorf("source type %q not supported", bundle.Spec.Source.Type)
+		return nil, fmt.Errorf("source type %q not supported", bundle.Type)
 	}
 	return source.Unpack(ctx, bundle)
 }
 
-func (s *unpacker) Cleanup(ctx context.Context, bundle *bd.BundleDeployment) error {
-	source, ok := s.sources[bundle.Spec.Source.Type]
+func (s *unpacker) Cleanup(ctx context.Context, bundle *BundleSource) error {
+	source, ok := s.sources[bundle.Type]
 	if !ok {
-		return fmt.Errorf("source type %q not supported", bundle.Spec.Source.Type)
+		return fmt.Errorf("source type %q not supported", bundle.Type)
 	}
 	return source.Cleanup(ctx, bundle)
 }
@@ -99,8 +107,8 @@ func (s *unpacker) Cleanup(ctx context.Context, bundle *bd.BundleDeployment) err
 // a default source mapping with built-in implementations of all of the supported
 // source types.
 func NewDefaultUnpacker(mgr manager.Manager, namespace, cacheDir string) (Unpacker, error) {
-	return NewUnpacker(map[bd.SourceType]Unpacker{
-		bd.SourceTypeImage: &ImageRegistry{
+	return NewUnpacker(map[SourceType]Unpacker{
+		SourceTypeImage: &ImageRegistry{
 			BaseCachePath: cacheDir,
 			AuthNamespace: namespace,
 		},