Merge pull request crossplane#6976 from adamwg/awg/no-more-input-crds

pkg: Stop installing function input CRDs and ignore disallowed kinds

Author: phisco

internal/controller/pkg/revision/establisher.go

@@ -19,6 +19,7 @@ package revision
 import (
 	"context"
 	"fmt"
+	"slices"
 	"strings"
 
 	"golang.org/x/sync/errgroup"
@@ -29,6 +30,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -579,3 +581,36 @@ func GetPackageOwnerReference(rev resource.Object) (metav1.OwnerReference, bool)
 
 	return metav1.OwnerReference{}, false
 }
+
+// FilteringEstablisher wraps another establisher, but filters the objects that
+// get passed to it so that only certain kinds are allowed.
+type FilteringEstablisher struct {
+	wrap Establisher
+	gks  []schema.GroupKind
+}
+
+// NewFilteringEstablisher creates a new FilteringEstablisher.
+func NewFilteringEstablisher(wrap Establisher, gks ...schema.GroupKind) *FilteringEstablisher {
+	return &FilteringEstablisher{
+		wrap: wrap,
+		gks:  gks,
+	}
+}
+
+// Establish filters objects, then uses the wrapped establisher to establish
+// them.
+func (e *FilteringEstablisher) Establish(ctx context.Context, objects []runtime.Object, parent v1.PackageRevision, control bool) ([]xpv1.TypedReference, error) {
+	filtered := make([]runtime.Object, 0, len(objects))
+	for _, obj := range objects {
+		if slices.Contains(e.gks, obj.GetObjectKind().GroupVersionKind().GroupKind()) {
+			filtered = append(filtered, obj)
+		}
+	}
+
+	return e.wrap.Establish(ctx, filtered, parent, control)
+}
+
+// ReleaseObjects uses the wrapped establisher to release objects.
+func (e *FilteringEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRevision) error {
+	return e.wrap.ReleaseObjects(ctx, parent)
+}

internal/controller/pkg/revision/establisher_test.go

@@ -1230,3 +1230,155 @@ func newAPIEstablisher(client client.Client) *APIEstablisher {
 		MaxConcurrentPackageEstablishers: 10, // Use the current default
 	}
 }
+
+func TestFilteringEstablisherEstablish(t *testing.T) {
+	errBoom := errors.New("boom")
+
+	crd := &extv1.CustomResourceDefinition{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: extv1.SchemeGroupVersion.String(),
+			Kind:       "CustomResourceDefinition",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-crd",
+		},
+	}
+
+	sa := &corev1.ServiceAccount{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: corev1.SchemeGroupVersion.String(),
+			Kind:       "ServiceAccount",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-sa",
+		},
+	}
+
+	type args struct {
+		wrap Establisher
+		gks  []schema.GroupKind
+		objs []runtime.Object
+	}
+
+	type want struct {
+		refs []xpv1.TypedReference
+		err  error
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"FilterPartialMatch": {
+			reason: "Should only pass objects matching the filter to the wrapped establisher",
+			args: args{
+				wrap: &MockEstablisher{
+					MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
+						if diff := cmp.Diff([]runtime.Object{crd}, objects); diff != "" {
+							t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
+							return nil, errBoom
+						}
+
+						return []xpv1.TypedReference{{Name: "test-crd"}}, nil
+					},
+				},
+				gks:  []schema.GroupKind{crd.GroupVersionKind().GroupKind()},
+				objs: []runtime.Object{crd, sa},
+			},
+			want: want{
+				refs: []xpv1.TypedReference{{Name: "test-crd"}},
+			},
+		},
+		"FilterFullMatch": {
+			reason: "Should pass all objects matching any of the filters to the wrapped establisher",
+			args: args{
+				wrap: &MockEstablisher{
+					MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
+						if diff := cmp.Diff([]runtime.Object{crd, sa}, objects); diff != "" {
+							t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
+							return nil, errBoom
+						}
+
+						return []xpv1.TypedReference{{Name: "test-crd"}, {Name: "test-sa"}}, nil
+					},
+				},
+				gks:  []schema.GroupKind{crd.GroupVersionKind().GroupKind(), sa.GroupVersionKind().GroupKind()},
+				objs: []runtime.Object{crd, sa},
+			},
+			want: want{
+				refs: []xpv1.TypedReference{{Name: "test-crd"}, {Name: "test-sa"}},
+			},
+		},
+		"FilterNoMatches": {
+			reason: "Should pass no objects to the wrapped establisher if none match the filter",
+			args: args{
+				wrap: &MockEstablisher{
+					MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
+						if diff := cmp.Diff([]runtime.Object{}, objects); diff != "" {
+							t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
+							return nil, errBoom
+						}
+
+						return []xpv1.TypedReference{}, nil
+					},
+				},
+				gks:  []schema.GroupKind{{Group: "example.com", Kind: "CustomKind"}},
+				objs: []runtime.Object{crd, sa},
+			},
+			want: want{
+				refs: []xpv1.TypedReference{},
+			},
+		},
+		"FilterEmpty": {
+			reason: "Should pass no objects to the wrapped establisher if empty filter is specified",
+			args: args{
+				wrap: &MockEstablisher{
+					MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
+						if diff := cmp.Diff([]runtime.Object{}, objects); diff != "" {
+							t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
+							return nil, errBoom
+						}
+
+						return []xpv1.TypedReference{}, nil
+					},
+				},
+				gks:  []schema.GroupKind{},
+				objs: []runtime.Object{crd, sa},
+			},
+			want: want{
+				refs: []xpv1.TypedReference{},
+			},
+		},
+		"ErrorFromWrappedEstablisher": {
+			reason: "Should propagate errors from the wrapped establisher",
+			args: args{
+				wrap: &MockEstablisher{
+					MockEstablish: func(_ context.Context, _ []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
+						return nil, errBoom
+					},
+				},
+				gks:  []schema.GroupKind{crd.GroupVersionKind().GroupKind()},
+				objs: []runtime.Object{crd},
+			},
+			want: want{
+				err: errBoom,
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			est := NewFilteringEstablisher(tc.args.wrap, tc.args.gks...)
+			refs, err := est.Establish(context.Background(), tc.args.objs, &v1.ProviderRevision{}, true)
+
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nest.Establish(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+
+			if diff := cmp.Diff(tc.want.refs, refs); diff != "" {
+				t.Errorf("\n%s\nest.Establish(...): -want, +got:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}

internal/controller/pkg/revision/reconciler.go

@@ -25,8 +25,11 @@ import (
 	"strings"
 	"time"
 
+	admv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
+	k8sextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -43,6 +46,10 @@ import (
 	"github.com/crossplane/crossplane-runtime/v2/pkg/parser"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/resource"
 
+	extv1 "github.com/crossplane/crossplane/v2/apis/apiextensions/v1"
+	extv1alpha1 "github.com/crossplane/crossplane/v2/apis/apiextensions/v1alpha1"
+	extv2 "github.com/crossplane/crossplane/v2/apis/apiextensions/v2"
+	opsv1alpha1 "github.com/crossplane/crossplane/v2/apis/ops/v1alpha1"
 	pkgmetav1 "github.com/crossplane/crossplane/v2/apis/pkg/meta/v1"
 	v1 "github.com/crossplane/crossplane/v2/apis/pkg/v1"
 	"github.com/crossplane/crossplane/v2/apis/pkg/v1beta1"
@@ -82,6 +89,7 @@ const (
 	errInitParserBackend = "cannot initialize parser backend"
 	errParsePackage      = "cannot parse package contents"
 	errLintPackage       = "linting package contents failed"
+	errValidatePackage   = "validating package contents failed"
 	errNotOneMeta        = "cannot install package with multiple meta types"
 	errIncompatible      = "incompatible Crossplane version"
 
@@ -108,6 +116,7 @@ const (
 	reasonImageConfig  event.Reason = "ImageConfigSelection"
 	reasonParse        event.Reason = "ParsePackage"
 	reasonLint         event.Reason = "LintPackage"
+	reasonValidate     event.Reason = "ValidatePackage"
 	reasonDependencies event.Reason = "ResolveDependencies"
 	reasonConvertCRD   event.Reason = "ConvertCRDToMRD"
 	reasonSync         event.Reason = "SyncPackage"
@@ -204,6 +213,13 @@ func WithLinter(l parser.Linter) ReconcilerOption {
 	}
 }
 
+// WithValidator specifies how the Reconciler should validate a package.
+func WithValidator(v xpkg.Validator) ReconcilerOption {
+	return func(r *Reconciler) {
+		r.validator = v
+	}
+}
+
 // WithVersioner specifies how the Reconciler should fetch the current
 // Crossplane version.
 func WithVersioner(v version.Operations) ReconcilerOption {
@@ -243,6 +259,7 @@ type Reconciler struct {
 	objects        Establisher
 	parser         parser.Parser
 	linter         parser.Linter
+	validator      xpkg.Validator
 	versioner      version.Operations
 	backend        parser.Backend
 	config         xpkg.ConfigStore
@@ -289,15 +306,24 @@ func SetupProviderRevision(mgr ctrl.Manager, o controller.Options) error {
 		Watches(&v1beta1.Lock{}, EnqueuePackageRevisionsForLock(mgr.GetClient(), &v1.ProviderRevisionList{}, log)).
 		Watches(&v1beta1.ImageConfig{}, EnqueuePackageRevisionsForImageConfig(mgr.GetClient(), &v1.ProviderRevisionList{}, log))
 
+	est := NewFilteringEstablisher(
+		NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers),
+		extv1alpha1.ManagedResourceDefinitionGroupVersionKind.GroupKind(),
+		schema.GroupKind{Group: k8sextv1.SchemeGroupVersion.Group, Kind: "CustomResourceDefinition"},
+		schema.GroupKind{Group: admv1.SchemeGroupVersion.Group, Kind: "ValidatingWebhookConfiguration"},
+		schema.GroupKind{Group: admv1.SchemeGroupVersion.Group, Kind: "MutatingWebhookConfiguration"},
+	)
+
 	r := NewReconciler(mgr,
 		WithCache(o.Cache),
 		WithDependencyManager(NewPackageDependencyManager(mgr.GetClient(), dag.NewMapDag, v1.ProviderGroupVersionKind, log)),
-		WithEstablisher(NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers)),
+		WithEstablisher(est),
 		WithNewPackageRevisionFn(nr),
 		WithParser(parser.New(metaScheme, objScheme)),
 		WithParserBackend(NewImageBackend(fetcher)),
 		WithConfigStore(xpkg.NewImageConfigStore(mgr.GetClient(), o.Namespace)),
 		WithLinter(xpkg.NewProviderLinter()),
+		WithValidator(xpkg.NewProviderValidator()),
 		WithLogger(log),
 		WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name), o.EventFilterFunctions...)),
 		WithNamespace(o.Namespace),
@@ -334,16 +360,27 @@ func SetupConfigurationRevision(mgr ctrl.Manager, o controller.Options) error {
 		return errors.Wrap(err, errCannotBuildFetcher)
 	}
 
+	est := NewFilteringEstablisher(
+		NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers),
+		extv2.CompositeResourceDefinitionGroupVersionKind.GroupKind(),
+		extv1.CompositionGroupVersionKind.GroupKind(),
+		extv1alpha1.ManagedResourceActivationPolicyGroupVersionKind.GroupKind(),
+		opsv1alpha1.OperationGroupVersionKind.GroupKind(),
+		opsv1alpha1.CronOperationGroupVersionKind.GroupKind(),
+		opsv1alpha1.WatchOperationGroupVersionKind.GroupKind(),
+	)
+
 	log := o.Logger.WithValues("controller", name)
 	r := NewReconciler(mgr,
 		WithCache(o.Cache),
 		WithDependencyManager(NewPackageDependencyManager(mgr.GetClient(), dag.NewMapDag, v1.ConfigurationGroupVersionKind, log)),
 		WithNewPackageRevisionFn(nr),
-		WithEstablisher(NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers)),
+		WithEstablisher(est),
 		WithParser(parser.New(metaScheme, objScheme)),
 		WithParserBackend(NewImageBackend(f)),
 		WithConfigStore(xpkg.NewImageConfigStore(mgr.GetClient(), o.Namespace)),
 		WithLinter(xpkg.NewConfigurationLinter()),
+		WithValidator(xpkg.NewConfigurationValidator()),
 		WithLogger(log),
 		WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name), o.EventFilterFunctions...)),
 		WithNamespace(o.Namespace),
@@ -393,15 +430,24 @@ func SetupFunctionRevision(mgr ctrl.Manager, o controller.Options) error {
 		Watches(&v1beta1.Lock{}, EnqueuePackageRevisionsForLock(mgr.GetClient(), &v1.FunctionRevisionList{}, log)).
 		Watches(&v1beta1.ImageConfig{}, EnqueuePackageRevisionsForImageConfig(mgr.GetClient(), &v1.FunctionRevisionList{}, log))
 
+	// The xpkg spec allows for CRDs to be included in function packages, but
+	// states that they will not be installed. This means we shouldn't install
+	// any objects from function packages. Create an empty filtering establisher
+	// to filter them all out.
+	est := NewFilteringEstablisher(
+		NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers),
+	)
+
 	r := NewReconciler(mgr,
 		WithCache(o.Cache),
 		WithDependencyManager(NewPackageDependencyManager(mgr.GetClient(), dag.NewMapDag, v1.FunctionGroupVersionKind, log)),
-		WithEstablisher(NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers)),
+		WithEstablisher(est),
 		WithNewPackageRevisionFn(nr),
 		WithParser(parser.New(metaScheme, objScheme)),
 		WithParserBackend(NewImageBackend(fetcher)),
 		WithConfigStore(xpkg.NewImageConfigStore(mgr.GetClient(), o.Namespace)),
 		WithLinter(xpkg.NewFunctionLinter()),
+		WithValidator(xpkg.NewFunctionValidator()),
 		WithLogger(log),
 		WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name), o.EventFilterFunctions...)),
 		WithNamespace(o.Namespace),
@@ -784,22 +830,29 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 		return reconcile.Result{}, err
 	}
 
-	// Lint package using package-specific linter.
-	if err := r.linter.Lint(pkg); err != nil {
-		err = errors.Wrap(err, errLintPackage)
+	// Validate the package using a package-specific validator. If validation
+	// fails, we won't try to install the package.
+	if err := r.validator.Lint(pkg); err != nil {
+		err = errors.Wrap(err, errValidatePackage)
 		status.MarkConditions(v1.RevisionUnhealthy().WithMessage(err.Error()))
 
 		_ = r.client.Status().Update(ctx, pr)
 
-		r.record.Event(pr, event.Warning(reasonLint, err))
+		r.record.Event(pr, event.Warning(reasonValidate, err))
 
-		// NOTE(hasheddan): a failed lint typically will require manual
-		// intervention, but on the off chance that we read pod logs
-		// early, which caused a linting failure, we will requeue by
-		// returning an error.
 		return reconcile.Result{}, err
 	}
 
+	// Lint package using package-specific linter. We can proceed with
+	// installation even there are lint errors; we just record them in an event
+	// for the user's information since they may cause unexpected behavior.
+	if err := r.linter.Lint(pkg); err != nil {
+		err = errors.Wrap(err, errLintPackage)
+		r.record.Event(pr, event.Warning(reasonLint, err))
+		// TODO(adamwg): Should we also record lint errors in the status for
+		// posterity? Events are ephemeral.
+	}
+
 	// NOTE(hasheddan): the linter should check this property already, but
 	// if a consumer forgets to pass an option to guarantee one meta object,
 	// we check here to avoid a potential panic on 0 index below.