refactor: dedicated struct for building source data

Signed-off-by: Erik Godding Boye <egboye@gmail.com>

cmd/trust-manager/app/app.go

@@ -35,9 +35,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 	ctrlwebhook "sigs.k8s.io/controller-runtime/pkg/webhook"
 
-	"github.com/cert-manager/trust-manager/cmd/trust-manager/app/options"
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
 	"github.com/cert-manager/trust-manager/pkg/bundle"
+	"github.com/cert-manager/trust-manager/pkg/options"
 	"github.com/cert-manager/trust-manager/pkg/webhook"
 )
 

pkg/bundle/bundle.go

@@ -38,45 +38,25 @@ import (
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
 	"github.com/cert-manager/trust-manager/pkg/bundle/internal/ssa_client"
 	"github.com/cert-manager/trust-manager/pkg/bundle/internal/target"
-	"github.com/cert-manager/trust-manager/pkg/fspkg"
+	"github.com/cert-manager/trust-manager/pkg/options"
 )
 
-// Options hold options for the Bundle controller.
-type Options struct {
-	// Namespace is the trust Namespace that source data can be referenced.
-	Namespace string
-
-	// DefaultPackageLocation is the location on the filesystem from which the 'default'
-	// certificate package should be loaded. If set, a valid package must be successfully
-	// loaded in order for the controller to start. If unset, referring to the default
-	// certificate package in a `Bundle` resource will cause that Bundle to error.
-	DefaultPackageLocation string
-
-	// SecretTargetsEnabled controls if secret targets are enabled in the Bundle API.
-	SecretTargetsEnabled bool
-
-	// FilterExpiredCerts controls if expired certificates are filtered from the bundle.
-	FilterExpiredCerts bool
-}
-
 // bundle is a controller-runtime controller. Implements the actual controller
 // logic by reconciling over Bundles.
 type bundle struct {
 	// a cache-backed Kubernetes client
 	client client.Client
 
-	// defaultPackage holds the loaded 'default' certificate package, if one was specified
-	// at startup.
-	defaultPackage *fspkg.Package
-
 	// recorder is used for create Kubernetes Events for reconciled Bundles.
 	recorder record.EventRecorder
 
 	// clock returns time which can be overwritten for testing.
 	clock clock.Clock
 
 	// Options holds options for the Bundle controller.
-	Options
+	Options options.Bundle
+
+	sources *target.BundleBuilder
 
 	targetReconciler *target.Reconciler
 }
@@ -133,10 +113,10 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result
 	statusPatch = &trustapi.BundleStatus{
 		DefaultCAPackageVersion: bundle.Status.DefaultCAPackageVersion,
 	}
-	resolvedBundle, err := b.buildSourceBundle(ctx, bundle.Spec.Sources, bundle.Spec.Target.AdditionalFormats)
+	resolvedBundle, err := b.sources.BuildBundle(ctx, bundle.Spec.Sources, bundle.Spec.Target.AdditionalFormats)
 
 	// If any source is not found, update the Bundle status to an unready state.
-	if errors.As(err, &notFoundError{}) {
+	if errors.As(err, &target.SourceNotFoundError{}) {
 		log.Error(err, "bundle source was not found")
 		b.setBundleCondition(
 			bundle.Status.Conditions,
@@ -307,7 +287,7 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result
 		}
 	}
 
-	if b.setBundleStatusDefaultCAVersion(statusPatch, resolvedBundle.defaultCAPackageStringID) {
+	if b.setBundleStatusDefaultCAVersion(statusPatch, resolvedBundle.DefaultCAPackageStringID) {
 		needsUpdate = true
 	}
 

pkg/bundle/bundle_test.go

@@ -42,6 +42,7 @@ import (
 	"github.com/cert-manager/trust-manager/pkg/bundle/internal/target"
 	"github.com/cert-manager/trust-manager/pkg/bundle/internal/truststore"
 	"github.com/cert-manager/trust-manager/pkg/fspkg"
+	"github.com/cert-manager/trust-manager/pkg/options"
 	"github.com/cert-manager/trust-manager/pkg/util"
 	"github.com/cert-manager/trust-manager/test/dummy"
 	"github.com/cert-manager/trust-manager/test/gen"
@@ -1443,14 +1444,19 @@ func Test_Reconcile(t *testing.T) {
 			)
 
 			_, ctx := ktesting.NewTestContext(t)
+			opts := options.Bundle{
+				Namespace:            trustNamespace,
+				SecretTargetsEnabled: !test.disableSecretTargets,
+				FilterExpiredCerts:   true,
+			}
 			b := &bundle{
 				client:   fakeClient,
 				recorder: fakeRecorder,
 				clock:    fixedclock,
-				Options: Options{
-					Namespace:            trustNamespace,
-					SecretTargetsEnabled: !test.disableSecretTargets,
-					FilterExpiredCerts:   true,
+				Options:  opts,
+				sources: &target.BundleBuilder{
+					Client:  fakeClient,
+					Options: opts,
 				},
 				targetReconciler: &target.Reconciler{
 					Client: fakeClient,
@@ -1466,7 +1472,7 @@ func Test_Reconcile(t *testing.T) {
 			}
 
 			if test.configureDefaultPackage {
-				b.defaultPackage = testDefaultPackage.Clone()
+				b.sources.DefaultPackage = testDefaultPackage.Clone()
 			}
 			resp, result, err := b.reconcileBundle(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: bundleName}})
 			if (err != nil) != test.expError {

pkg/bundle/controller.go

@@ -39,7 +39,7 @@ import (
 
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
 	"github.com/cert-manager/trust-manager/pkg/bundle/internal/target"
-	"github.com/cert-manager/trust-manager/pkg/fspkg"
+	"github.com/cert-manager/trust-manager/pkg/options"
 )
 
 // AddBundleController will register the Bundle controller with the
@@ -50,31 +50,29 @@ import (
 func AddBundleController(
 	ctx context.Context,
 	mgr manager.Manager,
-	opts Options,
+	opts options.Bundle,
 	targetCache cache.Cache,
 ) error {
+	sourceBuilder := &target.BundleBuilder{
+		Client:  mgr.GetClient(),
+		Options: opts,
+	}
+	if err := sourceBuilder.Init(ctx); err != nil {
+		return err
+	}
+
 	b := &bundle{
 		client:   mgr.GetClient(),
 		recorder: mgr.GetEventRecorderFor("bundles"),
 		clock:    clock.RealClock{},
 		Options:  opts,
+		sources:  sourceBuilder,
 		targetReconciler: &target.Reconciler{
 			Client: mgr.GetClient(),
 			Cache:  targetCache,
 		},
 	}
 
-	if b.Options.DefaultPackageLocation != "" {
-		pkg, err := fspkg.LoadPackageFromFile(b.Options.DefaultPackageLocation)
-		if err != nil {
-			return fmt.Errorf("must load default package successfully when default package location is set: %w", err)
-		}
-
-		b.defaultPackage = &pkg
-
-		logf.FromContext(ctx).Info("successfully loaded default package from filesystem", "id", pkg.StringID(), "path", b.Options.DefaultPackageLocation)
-	}
-
 	// Only reconcile config maps that match the well known name
 	controller := ctrl.NewControllerManagedBy(mgr).
 		Named("bundles").

pkg/bundle/source.go → pkg/bundle/internal/target/source.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package bundle
+package target
 
 import (
 	"context"
@@ -29,32 +29,60 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
-	"github.com/cert-manager/trust-manager/pkg/bundle/internal/target"
+	"github.com/cert-manager/trust-manager/pkg/fspkg"
+	"github.com/cert-manager/trust-manager/pkg/options"
 	"github.com/cert-manager/trust-manager/pkg/util"
 )
 
-type notFoundError struct{ error }
+type SourceNotFoundError struct{ error }
 
 type selectsNothingError struct{ error }
 
 type invalidSecretSourceError struct{ error }
 
-// bundleData holds the result of a call to buildSourceBundle. It contains the resulting PEM-encoded
+// BundleData holds the result of a call to BuildBundle. It contains the resulting PEM-encoded
 // certificate data from concatenating all the sources together, binary data for any additional formats and
 // any metadata from the sources which needs to be exposed on the Bundle resource's status field.
-type bundleData struct {
-	target.Data
+type BundleData struct {
+	Data
 
-	defaultCAPackageStringID string
+	DefaultCAPackageStringID string
 }
 
-// buildSourceBundle retrieves and concatenates all source bundle data for this Bundle object.
+type BundleBuilder struct {
+	// a cache-backed Kubernetes Client
+	Client client.Client
+
+	// DefaultPackage holds the loaded 'default' certificate package, if one was specified
+	// at startup.
+	DefaultPackage *fspkg.Package
+
+	// Options holds options for the Bundle controller.
+	Options options.Bundle
+}
+
+func (b *BundleBuilder) Init(ctx context.Context) error {
+	if b.Options.DefaultPackageLocation != "" {
+		pkg, err := fspkg.LoadPackageFromFile(b.Options.DefaultPackageLocation)
+		if err != nil {
+			return fmt.Errorf("must load default package successfully when default package location is set: %w", err)
+		}
+
+		b.DefaultPackage = &pkg
+
+		logf.FromContext(ctx).Info("successfully loaded default package from filesystem", "id", pkg.StringID(), "path", b.Options.DefaultPackageLocation)
+	}
+
+	return nil
+}
+
+// BuildBundle retrieves and concatenates all source bundle data for this Bundle object.
 // Each source data is validated and pruned to ensure that all certificates within are valid, and
 // is each bundle is concatenated together with a new line character.
-func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.BundleSource, formats *trustapi.AdditionalFormats) (bundleData, error) {
-	var resolvedBundle bundleData
+func (b *BundleBuilder) BuildBundle(ctx context.Context, sources []trustapi.BundleSource, formats *trustapi.AdditionalFormats) (BundleData, error) {
+	var resolvedBundle BundleData
 	certPool := util.NewCertPool(
-		util.WithFilteredExpiredCerts(b.FilterExpiredCerts),
+		util.WithFilteredExpiredCerts(b.Options.FilterExpiredCerts),
 		util.WithLogger(logf.FromContext(ctx).WithName("cert-pool")),
 	)
 
@@ -79,11 +107,11 @@ func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.Bundl
 				continue
 			}
 
-			if b.defaultPackage == nil {
-				err = notFoundError{fmt.Errorf("no default package was specified when trust-manager was started; default CAs not available")}
+			if b.DefaultPackage == nil {
+				err = SourceNotFoundError{fmt.Errorf("no default package was specified when trust-manager was started; default CAs not available")}
 			} else {
-				sourceData = b.defaultPackage.Bundle
-				resolvedBundle.defaultCAPackageStringID = b.defaultPackage.StringID()
+				sourceData = b.DefaultPackage.Bundle
+				resolvedBundle.DefaultCAPackageStringID = b.DefaultPackage.StringID()
 			}
 		}
 
@@ -94,42 +122,42 @@ func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.Bundl
 		}
 
 		if err != nil {
-			return bundleData{}, fmt.Errorf("failed to retrieve bundle from source: %w", err)
+			return BundleData{}, fmt.Errorf("failed to retrieve bundle from source: %w", err)
 		}
 
 		if err := certPool.AddCertsFromPEM([]byte(sourceData)); err != nil {
-			return bundleData{}, fmt.Errorf("invalid PEM data in source: %w", err)
+			return BundleData{}, fmt.Errorf("invalid PEM data in source: %w", err)
 		}
 	}
 
 	// NB: empty bundles are not valid so check and return an error if one somehow snuck through.
 	if certPool.Size() == 0 {
-		return bundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle")
+		return BundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle")
 	}
 
 	if err := resolvedBundle.Data.Populate(certPool, formats); err != nil {
-		return bundleData{}, err
+		return BundleData{}, err
 	}
 
 	return resolvedBundle, nil
 }
 
 // configMapBundle returns the data in the source ConfigMap within the trust Namespace.
-func (b *bundle) configMapBundle(ctx context.Context, ref *trustapi.SourceObjectKeySelector) (string, error) {
+func (b *BundleBuilder) configMapBundle(ctx context.Context, ref *trustapi.SourceObjectKeySelector) (string, error) {
 	// this slice will contain a single ConfigMap if we fetch by name
 	// or potentially multiple ConfigMaps if we fetch by label selector
 	var configMaps []corev1.ConfigMap
 
 	// if Name is set, we `Get` by name
 	if ref.Name != "" {
 		cm := corev1.ConfigMap{}
-		if err := b.client.Get(ctx, client.ObjectKey{
-			Namespace: b.Namespace,
+		if err := b.Client.Get(ctx, client.ObjectKey{
+			Namespace: b.Options.Namespace,
 			Name:      ref.Name,
 		}, &cm); apierrors.IsNotFound(err) {
-			return "", notFoundError{err}
+			return "", SourceNotFoundError{err}
 		} else if err != nil {
-			return "", fmt.Errorf("failed to get ConfigMap %s/%s: %w", b.Namespace, ref.Name, err)
+			return "", fmt.Errorf("failed to get ConfigMap %s/%s: %w", b.Options.Namespace, ref.Name, err)
 		}
 
 		configMaps = []corev1.ConfigMap{cm}
@@ -138,9 +166,9 @@ func (b *bundle) configMapBundle(ctx context.Context, ref *trustapi.SourceObject
 		cml := corev1.ConfigMapList{}
 		selector, selectorErr := metav1.LabelSelectorAsSelector(ref.Selector)
 		if selectorErr != nil {
-			return "", fmt.Errorf("failed to parse label selector as Selector for ConfigMap in namespace %s: %w", b.Namespace, selectorErr)
+			return "", fmt.Errorf("failed to parse label selector as Selector for ConfigMap in namespace %s: %w", b.Options.Namespace, selectorErr)
 		}
-		if err := b.client.List(ctx, &cml, client.MatchingLabelsSelector{Selector: selector}); err != nil {
+		if err := b.Client.List(ctx, &cml, client.MatchingLabelsSelector{Selector: selector}); err != nil {
 			return "", fmt.Errorf("failed to get ConfigMapList: %w", err)
 		} else if len(cml.Items) == 0 {
 			return "", selectsNothingError{fmt.Errorf("label selector %s for ConfigMap didn't match any resources", selector.String())}
@@ -154,7 +182,7 @@ func (b *bundle) configMapBundle(ctx context.Context, ref *trustapi.SourceObject
 		if len(ref.Key) > 0 {
 			data, ok := cm.Data[ref.Key]
 			if !ok {
-				return "", notFoundError{fmt.Errorf("no data found in ConfigMap %s/%s at key %q", cm.Namespace, cm.Name, ref.Key)}
+				return "", SourceNotFoundError{fmt.Errorf("no data found in ConfigMap %s/%s at key %q", cm.Namespace, cm.Name, ref.Key)}
 			}
 			results.WriteString(data)
 			results.WriteByte('\n')
@@ -169,21 +197,21 @@ func (b *bundle) configMapBundle(ctx context.Context, ref *trustapi.SourceObject
 }
 
 // secretBundle returns the data in the source Secret within the trust Namespace.
-func (b *bundle) secretBundle(ctx context.Context, ref *trustapi.SourceObjectKeySelector) (string, error) {
+func (b *BundleBuilder) secretBundle(ctx context.Context, ref *trustapi.SourceObjectKeySelector) (string, error) {
 	// this slice will contain a single Secret if we fetch by name
 	// or potentially multiple Secrets if we fetch by label selector
 	var secrets []corev1.Secret
 
 	// if Name is set, we `Get` by name
 	if ref.Name != "" {
 		s := corev1.Secret{}
-		if err := b.client.Get(ctx, client.ObjectKey{
-			Namespace: b.Namespace,
+		if err := b.Client.Get(ctx, client.ObjectKey{
+			Namespace: b.Options.Namespace,
 			Name:      ref.Name,
 		}, &s); apierrors.IsNotFound(err) {
-			return "", notFoundError{err}
+			return "", SourceNotFoundError{err}
 		} else if err != nil {
-			return "", fmt.Errorf("failed to get Secret %s/%s: %w", b.Namespace, ref.Name, err)
+			return "", fmt.Errorf("failed to get Secret %s/%s: %w", b.Options.Namespace, ref.Name, err)
 		}
 
 		secrets = []corev1.Secret{s}
@@ -192,9 +220,9 @@ func (b *bundle) secretBundle(ctx context.Context, ref *trustapi.SourceObjectKey
 		sl := corev1.SecretList{}
 		selector, selectorErr := metav1.LabelSelectorAsSelector(ref.Selector)
 		if selectorErr != nil {
-			return "", fmt.Errorf("failed to parse label selector as Selector for Secret in namespace %s: %w", b.Namespace, selectorErr)
+			return "", fmt.Errorf("failed to parse label selector as Selector for Secret in namespace %s: %w", b.Options.Namespace, selectorErr)
 		}
-		if err := b.client.List(ctx, &sl, client.MatchingLabelsSelector{Selector: selector}); err != nil {
+		if err := b.Client.List(ctx, &sl, client.MatchingLabelsSelector{Selector: selector}); err != nil {
 			return "", fmt.Errorf("failed to get SecretList: %w", err)
 		} else if len(sl.Items) == 0 {
 			return "", selectsNothingError{fmt.Errorf("label selector %s for Secret didn't match any resources", selector.String())}
@@ -208,7 +236,7 @@ func (b *bundle) secretBundle(ctx context.Context, ref *trustapi.SourceObjectKey
 		if len(ref.Key) > 0 {
 			data, ok := secret.Data[ref.Key]
 			if !ok {
-				return "", notFoundError{fmt.Errorf("no data found in Secret %s/%s at key %q", secret.Namespace, secret.Name, ref.Key)}
+				return "", SourceNotFoundError{fmt.Errorf("no data found in Secret %s/%s at key %q", secret.Namespace, secret.Name, ref.Key)}
 			}
 			results.Write(data)
 			results.WriteByte('\n')