Skip to content

Commit 36f59be

Browse files
adamwgadamwg
committed
pkg: Stop installing CRDs from Function packages
The xpkg spec says that CRDs included in Function packages will not be installed into the cluster, but until now they have been since Functions are treated the same as all other packages. Additionally, the previous commit allowed packages with arbitrary objects included to be installed, contrary to the spec (and desired behavior). Filter objects in the establisher, so that regardless of what kinds of resources are present in a package we install only the desired ones. Fixes crossplane#5294 Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
1 parent 7cc7a25 commit 36f59be

4 files changed

Lines changed: 230 additions & 8 deletions

File tree

‎internal/controller/pkg/revision/establisher.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package revision
1919
import (
2020
"context"
2121
"fmt"
22+
"slices"
2223
"strings"
2324

2425
"golang.org/x/sync/errgroup"
@@ -29,6 +30,7 @@ import (
2930
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3031
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
3132
"k8s.io/apimachinery/pkg/runtime"
33+
"k8s.io/apimachinery/pkg/runtime/schema"
3234
"k8s.io/apimachinery/pkg/types"
3335
"k8s.io/utils/ptr"
3436
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -579,3 +581,36 @@ func GetPackageOwnerReference(rev resource.Object) (metav1.OwnerReference, bool)
579581

580582
return metav1.OwnerReference{}, false
581583
}
584+
585+
// FilteringEstablisher wraps another establisher, but filters the objects that
586+
// get passed to it so that only certain kinds are allowed.
587+
type FilteringEstablisher struct {
588+
wrap Establisher
589+
gks []schema.GroupKind
590+
}
591+
592+
// NewFilteringEstablisher creates a new FilteringEstablisher.
593+
func NewFilteringEstablisher(wrap Establisher, gks ...schema.GroupKind) *FilteringEstablisher {
594+
return &FilteringEstablisher{
595+
wrap: wrap,
596+
gks: gks,
597+
}
598+
}
599+
600+
// Establish filters objects, then uses the wrapped establisher to establish
601+
// them.
602+
func (e *FilteringEstablisher) Establish(ctx context.Context, objects []runtime.Object, parent v1.PackageRevision, control bool) ([]xpv1.TypedReference, error) {
603+
filtered := make([]runtime.Object, 0, len(objects))
604+
for _, obj := range objects {
605+
if slices.Contains(e.gks, obj.GetObjectKind().GroupVersionKind().GroupKind()) {
606+
filtered = append(filtered, obj)
607+
}
608+
}
609+
610+
return e.wrap.Establish(ctx, filtered, parent, control)
611+
}
612+
613+
// ReleaseObjects uses the wrapped establisher to release objects.
614+
func (e *FilteringEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRevision) error {
615+
return e.wrap.ReleaseObjects(ctx, parent)
616+
}

‎internal/controller/pkg/revision/establisher_test.go‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1230,3 +1230,155 @@ func newAPIEstablisher(client client.Client) *APIEstablisher {
12301230
MaxConcurrentPackageEstablishers: 10, // Use the current default
12311231
}
12321232
}
1233+
1234+
func TestFilteringEstablisherEstablish(t *testing.T) {
1235+
errBoom := errors.New("boom")
1236+
1237+
crd := &extv1.CustomResourceDefinition{
1238+
TypeMeta: metav1.TypeMeta{
1239+
APIVersion: extv1.SchemeGroupVersion.String(),
1240+
Kind: "CustomResourceDefinition",
1241+
},
1242+
ObjectMeta: metav1.ObjectMeta{
1243+
Name: "test-crd",
1244+
},
1245+
}
1246+
1247+
sa := &corev1.ServiceAccount{
1248+
TypeMeta: metav1.TypeMeta{
1249+
APIVersion: corev1.SchemeGroupVersion.String(),
1250+
Kind: "ServiceAccount",
1251+
},
1252+
ObjectMeta: metav1.ObjectMeta{
1253+
Name: "test-sa",
1254+
},
1255+
}
1256+
1257+
type args struct {
1258+
wrap Establisher
1259+
gks []schema.GroupKind
1260+
objs []runtime.Object
1261+
}
1262+
1263+
type want struct {
1264+
refs []xpv1.TypedReference
1265+
err error
1266+
}
1267+
1268+
cases := map[string]struct {
1269+
reason string
1270+
args args
1271+
want want
1272+
}{
1273+
"FilterPartialMatch": {
1274+
reason: "Should only pass objects matching the filter to the wrapped establisher",
1275+
args: args{
1276+
wrap: &MockEstablisher{
1277+
MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
1278+
if diff := cmp.Diff([]runtime.Object{crd}, objects); diff != "" {
1279+
t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
1280+
return nil, errBoom
1281+
}
1282+
1283+
return []xpv1.TypedReference{{Name: "test-crd"}}, nil
1284+
},
1285+
},
1286+
gks: []schema.GroupKind{crd.GroupVersionKind().GroupKind()},
1287+
objs: []runtime.Object{crd, sa},
1288+
},
1289+
want: want{
1290+
refs: []xpv1.TypedReference{{Name: "test-crd"}},
1291+
},
1292+
},
1293+
"FilterFullMatch": {
1294+
reason: "Should pass all objects matching any of the filters to the wrapped establisher",
1295+
args: args{
1296+
wrap: &MockEstablisher{
1297+
MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
1298+
if diff := cmp.Diff([]runtime.Object{crd, sa}, objects); diff != "" {
1299+
t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
1300+
return nil, errBoom
1301+
}
1302+
1303+
return []xpv1.TypedReference{{Name: "test-crd"}, {Name: "test-sa"}}, nil
1304+
},
1305+
},
1306+
gks: []schema.GroupKind{crd.GroupVersionKind().GroupKind(), sa.GroupVersionKind().GroupKind()},
1307+
objs: []runtime.Object{crd, sa},
1308+
},
1309+
want: want{
1310+
refs: []xpv1.TypedReference{{Name: "test-crd"}, {Name: "test-sa"}},
1311+
},
1312+
},
1313+
"FilterNoMatches": {
1314+
reason: "Should pass no objects to the wrapped establisher if none match the filter",
1315+
args: args{
1316+
wrap: &MockEstablisher{
1317+
MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
1318+
if diff := cmp.Diff([]runtime.Object{}, objects); diff != "" {
1319+
t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
1320+
return nil, errBoom
1321+
}
1322+
1323+
return []xpv1.TypedReference{}, nil
1324+
},
1325+
},
1326+
gks: []schema.GroupKind{{Group: "example.com", Kind: "CustomKind"}},
1327+
objs: []runtime.Object{crd, sa},
1328+
},
1329+
want: want{
1330+
refs: []xpv1.TypedReference{},
1331+
},
1332+
},
1333+
"FilterEmpty": {
1334+
reason: "Should pass no objects to the wrapped establisher if empty filter is specified",
1335+
args: args{
1336+
wrap: &MockEstablisher{
1337+
MockEstablish: func(_ context.Context, objects []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
1338+
if diff := cmp.Diff([]runtime.Object{}, objects); diff != "" {
1339+
t.Errorf("\n%s\nMockEstablish(...): -want error, +got error:\n%s", "incorrect objects passed to wrapped establisher", diff)
1340+
return nil, errBoom
1341+
}
1342+
1343+
return []xpv1.TypedReference{}, nil
1344+
},
1345+
},
1346+
gks: []schema.GroupKind{},
1347+
objs: []runtime.Object{crd, sa},
1348+
},
1349+
want: want{
1350+
refs: []xpv1.TypedReference{},
1351+
},
1352+
},
1353+
"ErrorFromWrappedEstablisher": {
1354+
reason: "Should propagate errors from the wrapped establisher",
1355+
args: args{
1356+
wrap: &MockEstablisher{
1357+
MockEstablish: func(_ context.Context, _ []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
1358+
return nil, errBoom
1359+
},
1360+
},
1361+
gks: []schema.GroupKind{crd.GroupVersionKind().GroupKind()},
1362+
objs: []runtime.Object{crd},
1363+
},
1364+
want: want{
1365+
err: errBoom,
1366+
},
1367+
},
1368+
}
1369+
1370+
for name, tc := range cases {
1371+
t.Run(name, func(t *testing.T) {
1372+
est := NewFilteringEstablisher(tc.args.wrap, tc.args.gks...)
1373+
refs, err := est.Establish(context.Background(), tc.args.objs, &v1.ProviderRevision{}, true)
1374+
1375+
if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
1376+
t.Errorf("\n%s\nest.Establish(...): -want error, +got error:\n%s", tc.reason, diff)
1377+
}
1378+
1379+
if diff := cmp.Diff(tc.want.refs, refs); diff != "" {
1380+
t.Errorf("\n%s\nest.Establish(...): -want, +got:\n%s", tc.reason, diff)
1381+
}
1382+
})
1383+
}
1384+
}

‎internal/controller/pkg/revision/reconciler.go‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,11 @@ import (
2525
"strings"
2626
"time"
2727

28+
admv1 "k8s.io/api/admissionregistration/v1"
2829
corev1 "k8s.io/api/core/v1"
30+
k8sextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2931
kerrors "k8s.io/apimachinery/pkg/api/errors"
32+
"k8s.io/apimachinery/pkg/runtime/schema"
3033
"k8s.io/client-go/kubernetes"
3134
ctrl "sigs.k8s.io/controller-runtime"
3235
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -43,6 +46,10 @@ import (
4346
"github.com/crossplane/crossplane-runtime/v2/pkg/parser"
4447
"github.com/crossplane/crossplane-runtime/v2/pkg/resource"
4548

49+
extv1 "github.com/crossplane/crossplane/v2/apis/apiextensions/v1"
50+
extv1alpha1 "github.com/crossplane/crossplane/v2/apis/apiextensions/v1alpha1"
51+
extv2 "github.com/crossplane/crossplane/v2/apis/apiextensions/v2"
52+
opsv1alpha1 "github.com/crossplane/crossplane/v2/apis/ops/v1alpha1"
4653
pkgmetav1 "github.com/crossplane/crossplane/v2/apis/pkg/meta/v1"
4754
v1 "github.com/crossplane/crossplane/v2/apis/pkg/v1"
4855
"github.com/crossplane/crossplane/v2/apis/pkg/v1beta1"
@@ -299,10 +306,18 @@ func SetupProviderRevision(mgr ctrl.Manager, o controller.Options) error {
299306
Watches(&v1beta1.Lock{}, EnqueuePackageRevisionsForLock(mgr.GetClient(), &v1.ProviderRevisionList{}, log)).
300307
Watches(&v1beta1.ImageConfig{}, EnqueuePackageRevisionsForImageConfig(mgr.GetClient(), &v1.ProviderRevisionList{}, log))
301308

309+
est := NewFilteringEstablisher(
310+
NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers),
311+
extv1alpha1.ManagedResourceDefinitionGroupVersionKind.GroupKind(),
312+
schema.GroupKind{Group: k8sextv1.SchemeGroupVersion.Group, Kind: "CustomResourceDefinition"},
313+
schema.GroupKind{Group: admv1.SchemeGroupVersion.Group, Kind: "ValidatingWebhookConfiguration"},
314+
schema.GroupKind{Group: admv1.SchemeGroupVersion.Group, Kind: "MutatingWebhookConfiguration"},
315+
)
316+
302317
r := NewReconciler(mgr,
303318
WithCache(o.Cache),
304319
WithDependencyManager(NewPackageDependencyManager(mgr.GetClient(), dag.NewMapDag, v1.ProviderGroupVersionKind, log)),
305-
WithEstablisher(NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers)),
320+
WithEstablisher(est),
306321
WithNewPackageRevisionFn(nr),
307322
WithParser(parser.New(metaScheme, objScheme)),
308323
WithParserBackend(NewImageBackend(fetcher)),
@@ -345,12 +360,22 @@ func SetupConfigurationRevision(mgr ctrl.Manager, o controller.Options) error {
345360
return errors.Wrap(err, errCannotBuildFetcher)
346361
}
347362

363+
est := NewFilteringEstablisher(
364+
NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers),
365+
extv2.CompositeResourceDefinitionGroupVersionKind.GroupKind(),
366+
extv1.CompositionGroupVersionKind.GroupKind(),
367+
extv1alpha1.ManagedResourceActivationPolicyGroupVersionKind.GroupKind(),
368+
opsv1alpha1.OperationGroupVersionKind.GroupKind(),
369+
opsv1alpha1.CronOperationGroupVersionKind.GroupKind(),
370+
opsv1alpha1.WatchOperationGroupVersionKind.GroupKind(),
371+
)
372+
348373
log := o.Logger.WithValues("controller", name)
349374
r := NewReconciler(mgr,
350375
WithCache(o.Cache),
351376
WithDependencyManager(NewPackageDependencyManager(mgr.GetClient(), dag.NewMapDag, v1.ConfigurationGroupVersionKind, log)),
352377
WithNewPackageRevisionFn(nr),
353-
WithEstablisher(NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers)),
378+
WithEstablisher(est),
354379
WithParser(parser.New(metaScheme, objScheme)),
355380
WithParserBackend(NewImageBackend(f)),
356381
WithConfigStore(xpkg.NewImageConfigStore(mgr.GetClient(), o.Namespace)),
@@ -405,10 +430,18 @@ func SetupFunctionRevision(mgr ctrl.Manager, o controller.Options) error {
405430
Watches(&v1beta1.Lock{}, EnqueuePackageRevisionsForLock(mgr.GetClient(), &v1.FunctionRevisionList{}, log)).
406431
Watches(&v1beta1.ImageConfig{}, EnqueuePackageRevisionsForImageConfig(mgr.GetClient(), &v1.FunctionRevisionList{}, log))
407432

433+
// The xpkg spec allows for CRDs to be included in function packages, but
434+
// states that they will not be installed. This means we shouldn't install
435+
// any objects from function packages. Create an empty filtering establisher
436+
// to filter them all out.
437+
est := NewFilteringEstablisher(
438+
NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers),
439+
)
440+
408441
r := NewReconciler(mgr,
409442
WithCache(o.Cache),
410443
WithDependencyManager(NewPackageDependencyManager(mgr.GetClient(), dag.NewMapDag, v1.FunctionGroupVersionKind, log)),
411-
WithEstablisher(NewAPIEstablisher(mgr.GetClient(), o.Namespace, o.MaxConcurrentPackageEstablishers)),
444+
WithEstablisher(est),
412445
WithNewPackageRevisionFn(nr),
413446
WithParser(parser.New(metaScheme, objScheme)),
414447
WithParserBackend(NewImageBackend(fetcher)),

‎internal/controller/pkg/revision/reconciler_test.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func (e *ErrBackend) Init(_ context.Context, _ ...parser.BackendOption) (io.Read
6464
var _ Establisher = &MockEstablisher{}
6565

6666
type MockEstablisher struct {
67-
MockEstablish func() ([]xpv1.TypedReference, error)
67+
MockEstablish func(context.Context, []runtime.Object, v1.PackageRevision, bool) ([]xpv1.TypedReference, error)
6868
MockRelinquish func() error
6969
}
7070

@@ -75,16 +75,18 @@ func NewMockEstablisher() *MockEstablisher {
7575
}
7676
}
7777

78-
func NewMockEstablishFn(refs []xpv1.TypedReference, err error) func() ([]xpv1.TypedReference, error) {
79-
return func() ([]xpv1.TypedReference, error) { return refs, err }
78+
func NewMockEstablishFn(refs []xpv1.TypedReference, err error) func(context.Context, []runtime.Object, v1.PackageRevision, bool) ([]xpv1.TypedReference, error) {
79+
return func(_ context.Context, _ []runtime.Object, _ v1.PackageRevision, _ bool) ([]xpv1.TypedReference, error) {
80+
return refs, err
81+
}
8082
}
8183

8284
func NewMockRelinquishFn(err error) func() error {
8385
return func() error { return err }
8486
}
8587

86-
func (e *MockEstablisher) Establish(context.Context, []runtime.Object, v1.PackageRevision, bool) ([]xpv1.TypedReference, error) {
87-
return e.MockEstablish()
88+
func (e *MockEstablisher) Establish(ctx context.Context, objects []runtime.Object, parent v1.PackageRevision, control bool) ([]xpv1.TypedReference, error) {
89+
return e.MockEstablish(ctx, objects, parent, control)
8890
}
8991

9092
func (e *MockEstablisher) ReleaseObjects(context.Context, v1.PackageRevision) error {

0 commit comments

Comments
 (0)