Merge pull request #142 from turkenh/sync-upstream-release-1.16

Sync upstream release 1.16 for v1.16.2

cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml

@@ -103,6 +103,10 @@ rules:
   - pkg.crossplane.io
   resources: ["*"]
   verbs: ["*"]
+- apiGroups:
+  - secrets.crossplane.io
+  resources: ["*"]
+  verbs: ["*"]
 # Crossplane administrators have access to view CRDs in order to debug XRDs.
 - apiGroups: [apiextensions.k8s.io]
   resources: [customresourcedefinitions]
@@ -139,6 +143,10 @@ rules:
   - pkg.crossplane.io
   resources: ["*"]
   verbs: ["*"]
+- apiGroups:
+  - secrets.crossplane.io
+  resources: ["*"]
+  verbs: ["*"]
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
@@ -166,6 +174,10 @@ rules:
   - pkg.crossplane.io
   resources: ["*"]
   verbs: [get, list, watch]
+- apiGroups:
+  - secrets.crossplane.io
+  resources: ["*"]
+  verbs: [get, list, watch]
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole

cmd/crank/beta/trace/trace.go

@@ -124,6 +124,19 @@ func (c *Cmd) Run(k *kong.Context, logger logging.Logger) error {
 	if err != nil {
 		return errors.Wrap(err, errKubeConfig)
 	}
+
+	// NOTE(phisco): We used to get them set as part of
+	// https://github.com/kubernetes-sigs/controller-runtime/blob/2e9781e9fc6054387cf0901c70db56f0b0a63083/pkg/client/config/config.go#L96,
+	// this new approach doesn't set them, so we need to set them here to avoid
+	// being utterly slow.
+	// TODO(phisco): make this configurable.
+	if kubeconfig.QPS == 0 {
+		kubeconfig.QPS = 20
+	}
+	if kubeconfig.Burst == 0 {
+		kubeconfig.Burst = 30
+	}
+
 	logger.Debug("Found kubeconfig")
 
 	client, err := client.New(kubeconfig, client.Options{

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/Masterminds/semver v1.5.0
 	github.com/alecthomas/kong v0.8.1
 	github.com/crossplane/crossplane-runtime v1.16.0
-	github.com/docker/docker v25.0.5+incompatible
+	github.com/docker/docker v25.0.6+incompatible
 	github.com/docker/go-connections v0.5.0
 	github.com/emicklei/dot v1.6.1
 	github.com/go-git/go-billy/v5 v5.5.0

go.sum

@@ -150,8 +150,8 @@ github.com/docker/cli v24.0.7+incompatible h1:wa/nIwYFW7BVTGa7SWPVyyXU9lgORqUb1x
 github.com/docker/cli v24.0.7+incompatible/go.mod h1:JLrzqnKDaYBop7H2jaqPtU4hHvMKP+vjCwu2uszcLI8=
 github.com/docker/distribution v2.8.3+incompatible h1:AtKxIZ36LoNK51+Z6RpzLpddBirtxJnzDrHLEKxTAYk=
 github.com/docker/distribution v2.8.3+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
-github.com/docker/docker v25.0.5+incompatible h1:UmQydMduGkrD5nQde1mecF/YnSbTOaPeFIeP5C4W+DE=
-github.com/docker/docker v25.0.5+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
+github.com/docker/docker v25.0.6+incompatible h1:5cPwbwriIcsua2REJe8HqQV+6WlWc1byg2QSXzBxBGg=
+github.com/docker/docker v25.0.6+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
 github.com/docker/docker-credential-helpers v0.7.0/go.mod h1:rETQfLdHNT3foU5kuNkFR1R1V12OJRRO5lzt2D1b5X0=
 github.com/docker/docker-credential-helpers v0.8.1 h1:j/eKUktUltBtMzKqmfLB0PAgqYyMHOp5vfsD1807oKo=
 github.com/docker/docker-credential-helpers v0.8.1/go.mod h1:P3ci7E3lwkZg6XiHdRKft1KckHiO9a2rNtyFbZ/ry9M=

internal/controller/apiextensions/composite/composition_functions.go

@@ -72,6 +72,7 @@ const (
 	errFmtUnmarshalPipelineStepInput = "cannot unmarshal input for Composition pipeline step %q"
 	errFmtGetCredentialsFromSecret   = "cannot get Composition pipeline step %q credential %q from Secret"
 	errFmtRunPipelineStep            = "cannot run Composition pipeline step %q"
+	errFmtControllerMismatch         = "refusing to delete composed resource %q that is controlled by %s %q"
 	errFmtDeleteCD                   = "cannot delete composed resource %q (a %s named %s)"
 	errFmtUnmarshalDesiredCD         = "cannot unmarshal desired composed resource %q from RunFunctionResponse"
 	errFmtCDAsStruct                 = "cannot encode composed resource %q to protocol buffer Struct well-known type"
@@ -828,9 +829,17 @@ func (d *DeletingComposedResourceGarbageCollector) GarbageCollectComposedResourc
 	}
 
 	for name, cd := range del {
-		// We want to garbage collect this resource, but we don't control it.
-		if c := metav1.GetControllerOf(cd.Resource); c == nil || c.UID != owner.GetUID() {
-			continue
+		// Don't garbage collect composed resources that someone else controls.
+		//
+		// We do garbage collect composed resources that no-one controls. If a
+		// composed resource appears in observed (i.e. appears in the XR's
+		// spec.resourceRefs) but doesn't have a controller ref, most likely we
+		// created it but its controller ref was stripped. In this situation it
+		// would be permissible for us to adopt the composed resource by setting
+		// our XR as the controller ref, then delete it. So we may as well just
+		// go straight to deleting it.
+		if c := metav1.GetControllerOf(cd.Resource); c != nil && c.UID != owner.GetUID() {
+			return errors.Errorf(errFmtControllerMismatch, name, c.Kind, c.Name)
 		}
 
 		if err := d.client.Delete(ctx, cd.Resource); resource.IgnoreNotFound(err) != nil {

internal/controller/apiextensions/composite/composition_functions_test.go

@@ -1283,11 +1283,21 @@ func TestGarbageCollectComposedResources(t *testing.T) {
 					},
 				},
 				observed: ComposedResourceStates{
-					"undesired-resource": ComposedResourceState{Resource: &fake.Composed{}},
+					"undesired-resource": ComposedResourceState{Resource: &fake.Composed{
+						ObjectMeta: metav1.ObjectMeta{
+							// This resource isn't controlled by the XR.
+							OwnerReferences: []metav1.OwnerReference{{
+								Controller: ptr.To(true),
+								UID:        "a-different-xr",
+								Kind:       "XR",
+								Name:       "different",
+							}},
+						},
+					}},
 				},
 			},
 			want: want{
-				err: nil,
+				err: errors.New(`refusing to delete composed resource "undesired-resource" that is controlled by XR "different"`),
 			},
 		},
 		"DeleteError": {

internal/controller/apiextensions/composite/composition_pt.go

@@ -509,9 +509,17 @@ func (a *GarbageCollectingAssociator) AssociateTemplates(ctx context.Context, cr
 			continue
 		}
 
-		// We want to garbage collect this resource, but we don't control it.
-		if c := metav1.GetControllerOf(cd); c == nil || c.UID != cr.GetUID() {
-			continue
+		// Don't garbage collect composed resources that someone else controls.
+		//
+		// We do garbage collect composed resources that no-one controls. If a
+		// composed resource appears in observed (i.e. appears in the XR's
+		// spec.resourceRefs) but doesn't have a controller ref, most likely we
+		// created it but its controller ref was stripped. In this situation it
+		// would be permissible for us to adopt the composed resource by setting
+		// our XR as the controller ref, then delete it. So we may as well just
+		// go straight to deleting it.
+		if c := metav1.GetControllerOf(cd); c != nil && c.UID != cr.GetUID() {
+			return nil, errors.Errorf(errFmtControllerMismatch, name, c.Kind, c.Name)
 		}
 
 		// This existing resource does not correspond to an extant template. It

internal/controller/apiextensions/composite/composition_pt_test.go

@@ -646,7 +646,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 			},
 		},
 		"ResourceControlledBySomeoneElse": {
-			reason: "We should not garbage colle_ a resource that is controlled by another resource.",
+			reason: "We should not garbage collect a resource that is controlled by another resource.",
 			c: &test.MockClient{
 				MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
 					// The template used to create this resource is no longer known to us.
@@ -658,6 +658,8 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 						Controller:         &ctrl,
 						BlockOwnerDeletion: &ctrl,
 						UID:                types.UID("who-dat"),
+						Kind:               "XR",
+						Name:               "different",
 					}})
 					return nil
 				}),
@@ -670,11 +672,11 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 				ct: []v1.ComposedTemplate{t0},
 			},
 			want: want{
-				tas: []TemplateAssociation{{Template: t0}},
+				err: errors.New(`refusing to delete composed resource "unknown" that is controlled by XR "different"`),
 			},
 		},
 		"ResourceNotControlled": {
-			reason: "We should not garbage colle_ a resource that has no controller reference.",
+			reason: "We should garbage collect a resource that has no controller reference.",
 			c: &test.MockClient{
 				MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
 					// The template used to create this resource is no longer known to us.
@@ -683,6 +685,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 					// This resource is not controlled by anyone.
 					return nil
 				}),
+				MockDelete: test.NewMockDeleteFn(nil),
 			},
 			args: args{
 				cr: &fake.Composite{

internal/controller/pkg/revision/dependency.go

@@ -137,6 +137,22 @@ func (m *PackageDependencyManager) Resolve(ctx context.Context, pkg runtime.Obje
 		Dependencies: sources,
 	}
 
+	// Delete packages in lock with same name and distinct source
+	// This is a corner case when source is updated but image SHA is not (i.e. relocate same image
+	// to another registry)
+	for _, lp := range lock.Packages {
+		if self.Name == lp.Name && self.Type == lp.Type && self.Source != lp.Identifier() {
+			if err := m.RemoveSelf(ctx, pr); err != nil {
+				return found, installed, invalid, err
+			}
+			// refresh the lock to be in sync with the contents
+			if err = m.client.Get(ctx, types.NamespacedName{Name: lockName}, lock); err != nil {
+				return found, installed, invalid, err
+			}
+			break
+		}
+	}
+
 	prExists := false
 	for _, lp := range lock.Packages {
 		if lp.Name == pr.GetName() {

internal/controller/pkg/revision/dependency_test.go

@@ -42,6 +42,7 @@ var _ DependencyManager = &PackageDependencyManager{}
 
 func TestResolve(t *testing.T) {
 	errBoom := errors.New("boom")
+	mockUpdateCallCount := 0
 
 	type args struct {
 		dep  *PackageDependencyManager
@@ -553,9 +554,68 @@ func TestResolve(t *testing.T) {
 				invalid:   0,
 			},
 		},
+		"SuccessfulLockPackageSourceMismatch": {
+			reason: "Should not return error if source in packages does not match provider revision package.",
+			args: args{
+				dep: &PackageDependencyManager{
+					client: &test.MockClient{
+						MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+							l := obj.(*v1beta1.Lock)
+							if mockUpdateCallCount < 1 {
+								l.Packages = []v1beta1.LockPackage{
+									{
+										Name: "config-nop-a-abc123",
+										// Source mistmatch provider revision package
+										Source: "hasheddan/config-nop-b",
+									},
+								}
+							} else {
+								l.Packages = []v1beta1.LockPackage{}
+							}
+							return nil
+						}),
+						MockUpdate: func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error {
+							mockUpdateCallCount++
+							return nil
+						},
+					},
+					newDag: func() dag.DAG {
+						return &dagfake.MockDag{
+							MockInit: func(_ []dag.Node) ([]dag.Node, error) {
+								return []dag.Node{}, nil
+							},
+							MockTraceNode: func(s string) (map[string]dag.Node, error) {
+								if s == "hasheddan/config-nop-a" {
+									return map[string]dag.Node{
+										s: &v1beta1.Dependency{},
+									}, nil
+								}
+								return nil, errors.New("missing node in tree")
+							},
+							MockAddOrUpdateNodes: func(_ ...dag.Node) {},
+						}
+					},
+				},
+				meta: &pkgmetav1.Configuration{},
+				pr: &v1.ConfigurationRevision{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "config-nop-a-abc123",
+					},
+					Spec: v1.PackageRevisionSpec{
+						Package:      "hasheddan/config-nop-a:v0.0.1",
+						DesiredState: v1.PackageRevisionActive,
+					},
+				},
+			},
+			want: want{
+				total:     1,
+				installed: 1,
+			},
+		},
 	}
 
 	for name, tc := range cases {
+		mockUpdateCallCount = 0
 		t.Run(name, func(t *testing.T) {
 			total, installed, invalid, err := tc.args.dep.Resolve(context.TODO(), tc.args.meta, tc.args.pr)