From f485127aa2680553e04916ed39f47434535232bf Mon Sep 17 00:00:00 2001 From: Erik Godding Boye Date: Sat, 21 Sep 2024 11:22:29 +0200 Subject: [PATCH] refactor: dedicated struct for building source data Signed-off-by: Erik Godding Boye --- cmd/trust-manager/app/options/options.go | 25 ++++- pkg/bundle/bundle.go | 40 ++----- pkg/bundle/bundle_test.go | 18 ++-- pkg/bundle/controller.go | 26 +++-- pkg/bundle/{ => internal/target}/source.go | 100 +++++++++++------- .../{ => internal/target}/source_test.go | 18 ++-- pkg/bundle/internal/target/target.go | 24 +++-- pkg/bundle/internal/target/target_test.go | 2 + test/env/data.go | 8 +- test/integration/bundle/suite.go | 5 +- test/smoke/suite_test.go | 6 +- 11 files changed, 149 insertions(+), 123 deletions(-) rename pkg/bundle/{ => internal/target}/source.go (63%) rename pkg/bundle/{ => internal/target}/source_test.go (98%) diff --git a/cmd/trust-manager/app/options/options.go b/cmd/trust-manager/app/options/options.go index baf6ea4b..344a0ac0 100644 --- a/cmd/trust-manager/app/options/options.go +++ b/cmd/trust-manager/app/options/options.go @@ -31,8 +31,6 @@ import ( cliflag "k8s.io/component-base/cli/flag" "k8s.io/klog/v2" - "github.com/cert-manager/trust-manager/pkg/bundle" - _ "k8s.io/client-go/plugin/pkg/client/auth" ) @@ -60,7 +58,7 @@ type Options struct { Webhook // Bundle are options specific to the Bundle controller. - Bundle bundle.Options + Bundle Bundle // log are options controlling logging log logOptions @@ -248,3 +246,24 @@ func (o *Options) addWebhookFlags(fs *pflag.FlagSet) { "Certificate and private key must be named 'tls.crt' and 'tls.key' "+ "respectively.") } + +// Bundle hold options for the Bundle controller. +type Bundle struct { + // Log is the Bundle controller logger. + Log logr.Logger + + // 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 +} diff --git a/pkg/bundle/bundle.go b/pkg/bundle/bundle.go index c503b494..20a208a1 100644 --- a/pkg/bundle/bundle.go +++ b/pkg/bundle/bundle.go @@ -22,7 +22,6 @@ import ( "fmt" "strings" - "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -35,43 +34,18 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "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/internal/ssa_client" "github.com/cert-manager/trust-manager/pkg/bundle/internal/target" - "github.com/cert-manager/trust-manager/pkg/fspkg" ) -// Options hold options for the Bundle controller. -type Options struct { - // Log is the Bundle controller logger. - Log logr.Logger - - // 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 @@ -79,7 +53,9 @@ type bundle struct { clock clock.Clock // Options holds options for the Bundle controller. - Options + Options options.Bundle + + sources *target.BundleBuilder targetReconciler *target.Reconciler } @@ -106,7 +82,7 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, } func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result ctrl.Result, statusPatch *trustapi.BundleStatus, returnedErr error) { - log := b.Log.WithValues("bundle", req.NamespacedName.Name) + log := b.Options.Log.WithValues("bundle", req.NamespacedName.Name) log.V(2).Info("syncing bundle") var bundle trustapi.Bundle @@ -135,10 +111,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, ¬FoundError{}) { + if errors.As(err, &target.SourceNotFoundError{}) { log.Error(err, "bundle source was not found") b.setBundleCondition( bundle.Status.Conditions, @@ -309,7 +285,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 } diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index 88952967..ef15a26a 100644 --- a/pkg/bundle/bundle_test.go +++ b/pkg/bundle/bundle_test.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "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/internal/ssa_client" "github.com/cert-manager/trust-manager/pkg/bundle/internal/target" @@ -1443,15 +1444,20 @@ func Test_Reconcile(t *testing.T) { ) log, ctx := ktesting.NewTestContext(t) + opts := options.Bundle{ + Log: log, + Namespace: trustNamespace, + SecretTargetsEnabled: !test.disableSecretTargets, + FilterExpiredCerts: true, + } b := &bundle{ client: fakeClient, recorder: fakeRecorder, clock: fixedclock, - Options: Options{ - Log: log, - Namespace: trustNamespace, - SecretTargetsEnabled: !test.disableSecretTargets, - FilterExpiredCerts: true, + Options: opts, + sources: &target.BundleBuilder{ + Client: fakeClient, + Options: opts, }, targetReconciler: &target.Reconciler{ Client: fakeClient, @@ -1467,7 +1473,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 { diff --git a/pkg/bundle/controller.go b/pkg/bundle/controller.go index 3ea5d114..25b82607 100644 --- a/pkg/bundle/controller.go +++ b/pkg/bundle/controller.go @@ -36,9 +36,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" + "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/internal/target" - "github.com/cert-manager/trust-manager/pkg/fspkg" ) // AddBundleController will register the Bundle controller with the @@ -49,31 +49,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(); 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 - - b.Options.Log.Info("successfully loaded default package from filesystem", "path", b.Options.DefaultPackageLocation) - } - // Only reconcile config maps that match the well known name controller := ctrl.NewControllerManagedBy(mgr). Named("bundles"). @@ -191,7 +189,7 @@ func (b *bundle) enqueueRequestsFromBundleFunc(fn func(obj client.Object, bundle func (b *bundle) mustBundleList(ctx context.Context) *trustapi.BundleList { var bundleList trustapi.BundleList if err := b.client.List(ctx, &bundleList); err != nil { - b.Log.Error(err, "failed to list all Bundles, exiting error") + b.Options.Log.Error(err, "failed to list all Bundles, exiting error") os.Exit(-1) } diff --git a/pkg/bundle/source.go b/pkg/bundle/internal/target/source.go similarity index 63% rename from pkg/bundle/source.go rename to pkg/bundle/internal/target/source.go index 7c86af68..ff36ebbe 100644 --- a/pkg/bundle/source.go +++ b/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" @@ -27,32 +27,60 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "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/internal/target" + "github.com/cert-manager/trust-manager/pkg/fspkg" "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() 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 + + b.Options.Log.Info("successfully loaded default package from filesystem", "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 - certPool := util.NewCertPool(util.WithFilteredExpiredCerts(b.FilterExpiredCerts)) +func (b *BundleBuilder) BuildBundle(ctx context.Context, sources []trustapi.BundleSource, formats *trustapi.AdditionalFormats) (BundleData, error) { + var resolvedBundle BundleData + certPool := util.NewCertPool(util.WithFilteredExpiredCerts(b.Options.FilterExpiredCerts)) for _, source := range sources { var ( @@ -75,43 +103,43 @@ 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() } } // A source selector may select no configmaps/secrets, and this is not an error. if errors.As(err, &selectsNothingError{}) { - b.Log.Info(err.Error()) + b.Options.Log.Info(err.Error()) continue } 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 @@ -119,13 +147,13 @@ func (b *bundle) configMapBundle(ctx context.Context, ref *trustapi.SourceObject // 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} @@ -134,9 +162,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())} @@ -150,7 +178,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') @@ -165,7 +193,7 @@ 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 @@ -173,13 +201,13 @@ func (b *bundle) secretBundle(ctx context.Context, ref *trustapi.SourceObjectKey // 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} @@ -188,9 +216,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())} @@ -204,7 +232,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') diff --git a/pkg/bundle/source_test.go b/pkg/bundle/internal/target/source_test.go similarity index 98% rename from pkg/bundle/source_test.go rename to pkg/bundle/internal/target/source_test.go index bdae4d13..b57a3025 100644 --- a/pkg/bundle/source_test.go +++ b/pkg/bundle/internal/target/source_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package bundle +package target import ( "bytes" @@ -39,12 +39,6 @@ import ( "github.com/cert-manager/trust-manager/test/dummy" ) -const ( - jksKey = "trust.jks" - pkcs12Key = "trust.p12" - data = dummy.TestCertificate1 -) - func Test_buildSourceBundle(t *testing.T) { tests := map[string]struct { sources []trustapi.BundleSource @@ -445,9 +439,9 @@ func Test_buildSourceBundle(t *testing.T) { WithScheme(trustapi.GlobalScheme). Build() - b := &bundle{ - client: fakeClient, - defaultPackage: &fspkg.Package{ + b := &BundleBuilder{ + Client: fakeClient, + DefaultPackage: &fspkg.Package{ Name: "testpkg", Version: "123", Bundle: dummy.TestCertificate5, @@ -471,12 +465,12 @@ func Test_buildSourceBundle(t *testing.T) { } } - resolvedBundle, err := b.buildSourceBundle(context.TODO(), test.sources, test.formats) + resolvedBundle, err := b.BuildBundle(context.TODO(), test.sources, test.formats) if (err != nil) != test.expError { t.Errorf("unexpected error, exp=%t got=%v", test.expError, err) } - if errors.As(err, ¬FoundError{}) != test.expNotFoundError { + if errors.As(err, &SourceNotFoundError{}) != test.expNotFoundError { t.Errorf("unexpected notFoundError, exp=%t got=%v", test.expNotFoundError, err) } if errors.As(err, &invalidSecretSourceError{}) != test.expInvalidSecretSourceError { diff --git a/pkg/bundle/internal/target/target.go b/pkg/bundle/internal/target/target.go index 3d81bf96..96c8cce9 100644 --- a/pkg/bundle/internal/target/target.go +++ b/pkg/bundle/internal/target/target.go @@ -122,9 +122,6 @@ func (r *Reconciler) syncConfigMap( return false, errors.New("target not defined") } - // Generated PKCS #12 is not deterministic - best we can do here is update if the pem cert has - // changed (hence not checking if PKCS #12 matches) - bundleHash := TrustBundleHash([]byte(resolvedBundle.Data), bundle.Spec.Target.AdditionalFormats) data := map[string]string{ bundleTarget.ConfigMap.Key: resolvedBundle.Data, } @@ -133,7 +130,7 @@ func (r *Reconciler) syncConfigMap( // If the resource exists, check if it is up-to-date. if !apierrors.IsNotFound(err) { // Exit early if no update is needed - if exit, err := r.needsUpdate(ctx, target.Kind, log, targetObj, bundle, bundleHash); err != nil { + if exit, err := r.needsUpdate(ctx, target.Kind, log, targetObj, bundle, resolvedBundle.Hash); err != nil { return false, err } else if !exit { return false, nil @@ -142,7 +139,7 @@ func (r *Reconciler) syncConfigMap( patch := prepareTargetPatch(coreapplyconfig.ConfigMap(target.Name, target.Namespace), *bundle). WithAnnotations(map[string]string{ - trustapi.BundleHashAnnotationKey: bundleHash, + trustapi.BundleHashAnnotationKey: resolvedBundle.Hash, }). WithData(data). WithBinaryData(binData) @@ -200,9 +197,6 @@ func (r *Reconciler) syncSecret( return false, errors.New("target not defined") } - // Generated PKCS #12 is not deterministic - best we can do here is update if the pem cert has - // changed (hence not checking if PKCS #12 matches) - bundleHash := TrustBundleHash([]byte(resolvedBundle.Data), bundle.Spec.Target.AdditionalFormats) data := map[string][]byte{ bundleTarget.Secret.Key: []byte(resolvedBundle.Data), } @@ -214,7 +208,7 @@ func (r *Reconciler) syncSecret( // If the resource exists, check if it is up-to-date. if !apierrors.IsNotFound(err) { // Exit early if no update is needed - if exit, err := r.needsUpdate(ctx, target.Kind, log, targetObj, bundle, bundleHash); err != nil { + if exit, err := r.needsUpdate(ctx, target.Kind, log, targetObj, bundle, resolvedBundle.Hash); err != nil { return false, err } else if !exit { return false, nil @@ -223,7 +217,7 @@ func (r *Reconciler) syncSecret( patch := prepareTargetPatch(coreapplyconfig.Secret(target.Name, target.Namespace), *bundle). WithAnnotations(map[string]string{ - trustapi.BundleHashAnnotationKey: bundleHash, + trustapi.BundleHashAnnotationKey: resolvedBundle.Hash, }). WithData(data) @@ -412,8 +406,13 @@ type Resource struct { } type Data struct { - Data string + Data string + BinaryData map[string][]byte + + // Hash is used to determine if target resource needs to be updated or is up-to-date. + // The PKCS#12 format is not deterministic meaning target resources cannot be updated unconditionally. + Hash string } func (b *Data) Populate(pool *util.CertPool, formats *trustapi.AdditionalFormats) error { @@ -438,6 +437,9 @@ func (b *Data) Populate(pool *util.CertPool, formats *trustapi.AdditionalFormats b.BinaryData[formats.PKCS12.Key] = encoded } } + + b.Hash = TrustBundleHash([]byte(b.Data), formats) + return nil } diff --git a/pkg/bundle/internal/target/target_test.go b/pkg/bundle/internal/target/target_test.go index c773fd5e..861f9013 100644 --- a/pkg/bundle/internal/target/target_test.go +++ b/pkg/bundle/internal/target/target_test.go @@ -599,6 +599,7 @@ func Test_syncConfigMapTarget(t *testing.T) { } resolvedBundle.BinaryData[pkcs12Key] = pkcs12Data } + resolvedBundle.Hash = TrustBundleHash([]byte(data), spec.Target.AdditionalFormats) log, ctx := ktesting.NewTestContext(t) needsUpdate, err := r.Sync(ctx, Resource{ @@ -1211,6 +1212,7 @@ func Test_syncSecretTarget(t *testing.T) { } resolvedBundle.BinaryData[pkcs12Key] = pkcs12Data } + resolvedBundle.Hash = TrustBundleHash([]byte(data), spec.Target.AdditionalFormats) log, ctx := ktesting.NewTestContext(t) needsUpdate, err := r.Sync(ctx, Resource{ diff --git a/test/env/data.go b/test/env/data.go index 6188da85..c502c6a1 100644 --- a/test/env/data.go +++ b/test/env/data.go @@ -29,8 +29,8 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/cert-manager/trust-manager/cmd/trust-manager/app/options" trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" - bundlectrl "github.com/cert-manager/trust-manager/pkg/bundle" "github.com/cert-manager/trust-manager/pkg/util" "github.com/cert-manager/trust-manager/test/dummy" @@ -78,7 +78,7 @@ func DefaultTrustData() TestData { // newTestBundle creates a new Bundle in the API using the input test data. // Returns the create Bundle object. -func newTestBundle(ctx context.Context, cl client.Client, opts bundlectrl.Options, td TestData, targetType string) *trustapi.Bundle { +func newTestBundle(ctx context.Context, cl client.Client, opts options.Bundle, td TestData, targetType string) *trustapi.Bundle { By("creating trust Bundle") configMap := corev1.ConfigMap{ @@ -148,13 +148,13 @@ func newTestBundle(ctx context.Context, cl client.Client, opts bundlectrl.Option // NewTestBundleSecretTarget creates a new Bundle in the API using the input test data. // Returns the create Bundle object. -func NewTestBundleSecretTarget(ctx context.Context, cl client.Client, opts bundlectrl.Options, td TestData) *trustapi.Bundle { +func NewTestBundleSecretTarget(ctx context.Context, cl client.Client, opts options.Bundle, td TestData) *trustapi.Bundle { return newTestBundle(ctx, cl, opts, td, "Secret") } // newTestBundleConfigMapTarget creates a new Bundle in the API using the input test data with target set to ConfigMap. // Returns the create Bundle object. -func NewTestBundleConfigMapTarget(ctx context.Context, cl client.Client, opts bundlectrl.Options, td TestData) *trustapi.Bundle { +func NewTestBundleConfigMapTarget(ctx context.Context, cl client.Client, opts options.Bundle, td TestData) *trustapi.Bundle { return newTestBundle(ctx, cl, opts, td, "ConfigMap") } diff --git a/test/integration/bundle/suite.go b/test/integration/bundle/suite.go index 00e26928..a77d2a8b 100644 --- a/test/integration/bundle/suite.go +++ b/test/integration/bundle/suite.go @@ -37,6 +37,7 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" + "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/fspkg" @@ -63,7 +64,7 @@ var _ = Describe("Integration", func() { cl client.Client mgr manager.Manager mgrStopped chan struct{} - opts bundle.Options + opts options.Bundle testBundle *trustapi.Bundle testData testenv.TestData @@ -95,7 +96,7 @@ var _ = Describe("Integration", func() { Expect(cl.Create(ctx, namespace)).NotTo(HaveOccurred()) By("Created trust Namespace: " + namespace.Name) - opts = bundle.Options{ + opts = options.Bundle{ Log: logf.Log, Namespace: namespace.Name, DefaultPackageLocation: tmpFileName, diff --git a/test/smoke/suite_test.go b/test/smoke/suite_test.go index b4e1bcd8..7c4f780b 100644 --- a/test/smoke/suite_test.go +++ b/test/smoke/suite_test.go @@ -27,8 +27,8 @@ import ( "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + "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/test/dummy" "github.com/cert-manager/trust-manager/test/env" @@ -67,7 +67,7 @@ var _ = Describe("Smoke", func() { By("Creating Bundle for test") testData := env.DefaultTrustData() - testBundle := env.NewTestBundleConfigMapTarget(ctx, cl, bundle.Options{ + testBundle := env.NewTestBundleConfigMapTarget(ctx, cl, options.Bundle{ Log: log, Namespace: cnf.TrustNamespace, }, testData) @@ -87,7 +87,7 @@ var _ = Describe("Smoke", func() { By("Creating Bundle for test") testData := env.DefaultTrustData() - testBundle := env.NewTestBundleSecretTarget(ctx, cl, bundle.Options{ + testBundle := env.NewTestBundleSecretTarget(ctx, cl, options.Bundle{ Log: log, Namespace: cnf.TrustNamespace, }, testData)