pkg: Separate validation and linting of packages

Crossplane uses package linters in two places: at build time (in the CLI) and at
install time (in the revision controller). In both cases, linter errors are
treated as errors and stop the operation.

At build time, this makes sense: we should be strict about the packages we
produce. At install time, this means we may reject packages that we could
successfully install. For example (as described in crossplane#6525) if we allow new
resource types in a package in a new version of Crossplane (as we've done with
Operations and MRDs), older versions should still be able to install packages
that contain them; the new resources would just be ignored. If package authors
want to limit the versions their package works with, they can set a version
constraint in the metadata.

Introduce a new Validator type, which is identical to a linter except in
name. Add validators for all package types, which are identical to their linters
except that they allow any object to be included.

In the revision controller, use validators to determine whether package
installation should be attempted, where we previously used linters. Run the
linters as well and record any lint errors as events, to help the user diagnose
any unexpected behavior.

Fixes crossplane#6525

Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>

Author: adamwg

internal/controller/pkg/revision/reconciler.go

@@ -82,6 +82,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 +109,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 +206,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 +252,7 @@ type Reconciler struct {
 	objects        Establisher
 	parser         parser.Parser
 	linter         parser.Linter
+	validator      xpkg.Validator
 	versioner      version.Operations
 	backend        parser.Backend
 	config         xpkg.ConfigStore
@@ -298,6 +308,7 @@ func SetupProviderRevision(mgr ctrl.Manager, o controller.Options) error {
 		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),
@@ -344,6 +355,7 @@ func SetupConfigurationRevision(mgr ctrl.Manager, o controller.Options) error {
 		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),
@@ -402,6 +414,7 @@ func SetupFunctionRevision(mgr ctrl.Manager, o controller.Options) error {
 		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 +797,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.

internal/controller/pkg/revision/reconciler_test.go

@@ -36,6 +36,7 @@ import (
 
 	xpv1 "github.com/crossplane/crossplane-runtime/v2/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
+	"github.com/crossplane/crossplane-runtime/v2/pkg/event"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/feature"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/logging"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/meta"
@@ -753,8 +754,8 @@ func TestReconcile(t *testing.T) {
 				err: errors.Wrap(errBoom, errParsePackage),
 			},
 		},
-		"ErrLint": {
-			reason: "We should return an error if fail to lint the package.",
+		"ErrValidate": {
+			reason: "We should return an error if validation fails.",
 			args: args{
 				mgr: &fake.Manager{},
 				rec: []ReconcilerOption{
@@ -771,7 +772,7 @@ func TestReconcile(t *testing.T) {
 								want := &v1.ProviderRevision{}
 								want.SetGroupVersionKind(v1.ProviderRevisionGroupVersionKind)
 								want.SetDesiredState(v1.PackageRevisionActive)
-								want.SetConditions(v1.RevisionUnhealthy().WithMessage("linting package contents failed: boom"))
+								want.SetConditions(v1.RevisionUnhealthy().WithMessage("validating package contents failed: boom"))
 
 								if diff := cmp.Diff(want, o); diff != "" {
 									t.Errorf("-want, +got:\n%s", diff)
@@ -785,7 +786,8 @@ func TestReconcile(t *testing.T) {
 					}}),
 					WithParser(parser.New(metaScheme, objScheme)),
 					WithParserBackend(parser.NewEchoBackend(string(providerBytes))),
-					WithLinter(&MockLinter{MockLint: NewMockLintFn(errBoom)}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(errBoom)}),
+					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithCache(&xpkgfake.MockCache{
 						MockHas: xpkgfake.NewMockCacheHasFn(false),
 						MockStore: func(_ string, rc io.ReadCloser) error {
@@ -800,7 +802,7 @@ func TestReconcile(t *testing.T) {
 				},
 			},
 			want: want{
-				err: errors.Wrap(errBoom, errLintPackage),
+				err: errors.Wrap(errBoom, errValidatePackage),
 			},
 		},
 		"ErrCrossplaneConstraints": {
@@ -853,6 +855,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{
 						MockInConstraints:    verfake.NewMockInConstraintsFn(false, errBoom),
@@ -904,6 +907,7 @@ func TestReconcile(t *testing.T) {
 						MockHas:   xpkgfake.NewMockCacheHasFn(false),
 						MockStore: xpkgfake.NewMockCacheStoreFn(nil),
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
 						MockPullSecretFor: xpkgfake.NewMockConfigStorePullSecretForFn("", "", nil),
@@ -956,6 +960,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
 						MockPullSecretFor: xpkgfake.NewMockConfigStorePullSecretForFn("", "", nil),
@@ -1023,6 +1028,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1035,6 +1041,76 @@ func TestReconcile(t *testing.T) {
 				err: errors.Wrap(errBoom, errResolveDeps),
 			},
 		},
+		"SuccessfulWithLintErrors": {
+			reason: "We should record an event but successfully install the package if linting fails.",
+			args: args{
+				mgr: &fake.Manager{},
+				rec: []ReconcilerOption{
+					WithNewPackageRevisionFn(func() v1.PackageRevision { return &v1.ProviderRevision{} }),
+					WithClientApplicator(resource.ClientApplicator{
+						Client: &test.MockClient{
+							MockGet: test.NewMockGetFn(nil, func(o client.Object) error {
+								pr := o.(*v1.ProviderRevision)
+								pr.SetGroupVersionKind(v1.ProviderRevisionGroupVersionKind)
+								pr.SetDesiredState(v1.PackageRevisionActive)
+								return nil
+							}),
+							MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil, func(o client.Object) error {
+								want := &v1.ProviderRevision{}
+								want.SetGroupVersionKind(v1.ProviderRevisionGroupVersionKind)
+								want.SetDesiredState(v1.PackageRevisionActive)
+								want.SetAnnotations(map[string]string{"author": "crossplane"})
+								want.SetConditions(v1.RevisionHealthy())
+
+								if diff := cmp.Diff(want, o); diff != "" {
+									t.Errorf("-want, +got:\n%s", diff)
+								}
+								return nil
+							}),
+							MockUpdate: test.NewMockUpdateFn(nil, func(o client.Object) error {
+								want := &v1.ProviderRevision{}
+								want.SetGroupVersionKind(v1.ProviderRevisionGroupVersionKind)
+								want.SetDesiredState(v1.PackageRevisionActive)
+								want.SetAnnotations(map[string]string{"author": "crossplane"})
+								if diff := cmp.Diff(want, o); diff != "" {
+									t.Errorf("-want, +got:\n%s", diff)
+								}
+								return nil
+							}),
+
+							MockDelete: test.NewMockDeleteFn(nil),
+						},
+					}),
+					WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error {
+						return nil
+					}}),
+					WithParser(parser.New(metaScheme, objScheme)),
+					WithEstablisher(NewMockEstablisher()),
+					WithParserBackend(parser.NewEchoBackend(string(providerBytes))),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
+					WithLinter(&MockLinter{MockLint: NewMockLintFn(errBoom)}),
+					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
+					WithCache(&xpkgfake.MockCache{
+						MockHas: xpkgfake.NewMockCacheHasFn(false),
+						MockStore: func(_ string, rc io.ReadCloser) error {
+							_, err := io.ReadAll(rc)
+							return err
+						},
+					}),
+					WithConfigStore(&xpkgfake.MockConfigStore{
+						MockPullSecretFor: xpkgfake.NewMockConfigStorePullSecretForFn("", "", nil),
+						MockRewritePath:   xpkgfake.NewMockRewritePathFn("", "", nil),
+					}),
+					WithRecorder(newTestRecorder(
+						event.Warning(reasonLint, errors.Wrap(errBoom, errLintPackage)),
+						event.Normal(reasonSync, "Successfully reconciled package revision"),
+					)),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
 		"SuccessfulActiveRevision": {
 			reason: "An active revision should establish control of all of its resources.",
 			args: args{
@@ -1088,6 +1164,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1145,6 +1222,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1226,6 +1304,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1295,6 +1374,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(false, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1361,6 +1441,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1503,6 +1584,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1631,6 +1713,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 				},
@@ -1693,6 +1776,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1855,6 +1939,7 @@ func TestReconcile(t *testing.T) {
 							return err
 						},
 					}),
+					WithValidator(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}),
 					WithVersioner(&verfake.MockVersioner{MockInConstraints: verfake.NewMockInConstraintsFn(true, nil)}),
 					WithConfigStore(&xpkgfake.MockConfigStore{
@@ -1924,6 +2009,12 @@ func TestReconcile(t *testing.T) {
 			if diff := cmp.Diff(tc.want.r, got, test.EquateErrors()); diff != "" {
 				t.Errorf("\n%s\nr.Reconcile(...): -want, +got:\n%s", tc.reason, diff)
 			}
+
+			if tr, ok := r.record.(*testRecorder); ok {
+				if diff := cmp.Diff(tr.Want, tr.Got); diff != "" {
+					t.Errorf("\n%s\nr.Reconcile(...): -want events, +got events:\n%s", tc.reason, diff)
+				}
+			}
 		})
 	}
 }
@@ -1934,3 +2025,23 @@ func signatureVerificationEnabled() *feature.Flags {
 
 	return f
 }
+
+// testRecorder allows asserting event creation.
+type testRecorder struct {
+	Want []event.Event
+	Got  []event.Event
+}
+
+func (r *testRecorder) Event(_ runtime.Object, e event.Event) {
+	r.Got = append(r.Got, e)
+}
+
+func (r *testRecorder) WithAnnotations(_ ...string) event.Recorder {
+	return r
+}
+
+func newTestRecorder(expected ...event.Event) *testRecorder {
+	return &testRecorder{
+		Want: expected,
+	}
+}