diff --git a/cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml b/cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml index 2ddd200c7..c8ad21be5 100644 --- a/cluster/charts/crossplane/templates/rbac-manager-managed-clusterroles.yaml +++ b/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 diff --git a/cmd/crank/beta/trace/trace.go b/cmd/crank/beta/trace/trace.go index 943f6d732..98704c9c0 100644 --- a/cmd/crank/beta/trace/trace.go +++ b/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{ diff --git a/go.mod b/go.mod index 5306a6a27..ea5fcaf34 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index d5161b509..df4c090cf 100644 --- a/go.sum +++ b/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= diff --git a/internal/controller/apiextensions/composite/composition_functions.go b/internal/controller/apiextensions/composite/composition_functions.go index a126dac18..4554ca262 100644 --- a/internal/controller/apiextensions/composite/composition_functions.go +++ b/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 { diff --git a/internal/controller/apiextensions/composite/composition_functions_test.go b/internal/controller/apiextensions/composite/composition_functions_test.go index 0c5d524d0..519e5712b 100644 --- a/internal/controller/apiextensions/composite/composition_functions_test.go +++ b/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": { diff --git a/internal/controller/apiextensions/composite/composition_pt.go b/internal/controller/apiextensions/composite/composition_pt.go index 023afa2fa..29b9fc512 100644 --- a/internal/controller/apiextensions/composite/composition_pt.go +++ b/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 diff --git a/internal/controller/apiextensions/composite/composition_pt_test.go b/internal/controller/apiextensions/composite/composition_pt_test.go index 52558ab59..6eb3b5f52 100644 --- a/internal/controller/apiextensions/composite/composition_pt_test.go +++ b/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{ diff --git a/internal/controller/pkg/revision/dependency.go b/internal/controller/pkg/revision/dependency.go index 9da209088..af7b76f0b 100644 --- a/internal/controller/pkg/revision/dependency.go +++ b/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() { diff --git a/internal/controller/pkg/revision/dependency_test.go b/internal/controller/pkg/revision/dependency_test.go index 9b3dd0ee2..f8e79d009 100644 --- a/internal/controller/pkg/revision/dependency_test.go +++ b/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) diff --git a/internal/initializer/tls.go b/internal/initializer/tls.go index 04330c01f..481b58a3a 100644 --- a/internal/initializer/tls.go +++ b/internal/initializer/tls.go @@ -26,7 +26,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - controllerruntime "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crossplane/crossplane-runtime/pkg/errors" @@ -128,7 +127,9 @@ func (e *TLSCertificateGenerator) loadOrGenerateCA(ctx context.Context, kube cli return nil, errors.Wrapf(err, errFmtGetTLSSecret, nn.Name) } + create := true if err == nil { + create = false kd := caSecret.Data[corev1.TLSPrivateKeyKey] cd := caSecret.Data[corev1.TLSCertKey] if len(kd) != 0 && len(cd) != 0 { @@ -157,13 +158,15 @@ func (e *TLSCertificateGenerator) loadOrGenerateCA(ctx context.Context, kube cli caSecret.Name = nn.Name caSecret.Namespace = nn.Namespace - _, err = controllerruntime.CreateOrUpdate(ctx, kube, caSecret, func() error { - caSecret.Data = map[string][]byte{ - corev1.TLSCertKey: caCrtByte, - corev1.TLSPrivateKeyKey: caKeyByte, - } - return nil - }) + caSecret.Data = map[string][]byte{ + corev1.TLSCertKey: caCrtByte, + corev1.TLSPrivateKeyKey: caKeyByte, + } + if create { + err = kube.Create(ctx, caSecret) + } else { + err = kube.Update(ctx, caSecret) + } if err != nil { return nil, errors.Wrapf(err, errFmtCannotCreateOrUpdate, nn.Name) } @@ -171,7 +174,7 @@ func (e *TLSCertificateGenerator) loadOrGenerateCA(ctx context.Context, kube cli return parseCertificateSigner(caKeyByte, caCrtByte) } -func (e *TLSCertificateGenerator) ensureClientCertificate(ctx context.Context, kube client.Client, nn types.NamespacedName, signer *CertificateSigner) error { +func (e *TLSCertificateGenerator) ensureClientCertificate(ctx context.Context, kube client.Client, nn types.NamespacedName, signer *CertificateSigner) error { //nolint:gocyclo // slightly over the limit, 11 vs 10 sec := &corev1.Secret{} err := kube.Get(ctx, nn, sec) @@ -179,7 +182,9 @@ func (e *TLSCertificateGenerator) ensureClientCertificate(ctx context.Context, k return errors.Wrapf(err, errFmtGetTLSSecret, nn.Name) } + create := true if err == nil { + create = false if len(sec.Data[corev1.TLSPrivateKeyKey]) != 0 || len(sec.Data[corev1.TLSCertKey]) != 0 || len(sec.Data[SecretKeyCACert]) != 0 { e.log.Info("TLS secret contains client certificate.", "secret", nn.Name) return nil @@ -212,21 +217,22 @@ func (e *TLSCertificateGenerator) ensureClientCertificate(ctx context.Context, k if e.owner != nil { sec.OwnerReferences = e.owner } - _, err = controllerruntime.CreateOrUpdate(ctx, kube, sec, func() error { - if sec.Data == nil { - sec.Data = make(map[string][]byte) - } - sec.Data[corev1.TLSCertKey] = certData - sec.Data[corev1.TLSPrivateKeyKey] = keyData - sec.Data[SecretKeyCACert] = signer.certificatePEM - - return nil - }) + if sec.Data == nil { + sec.Data = make(map[string][]byte) + } + sec.Data[corev1.TLSCertKey] = certData + sec.Data[corev1.TLSPrivateKeyKey] = keyData + sec.Data[SecretKeyCACert] = signer.certificatePEM + if create { + err = kube.Create(ctx, sec) + } else { + err = kube.Update(ctx, sec) + } return errors.Wrapf(err, errFmtCannotCreateOrUpdate, nn.Name) } -func (e *TLSCertificateGenerator) ensureServerCertificate(ctx context.Context, kube client.Client, nn types.NamespacedName, signer *CertificateSigner) error { +func (e *TLSCertificateGenerator) ensureServerCertificate(ctx context.Context, kube client.Client, nn types.NamespacedName, signer *CertificateSigner) error { //nolint:gocyclo // slightly over the limit, 11 vs 10 sec := &corev1.Secret{} err := kube.Get(ctx, nn, sec) @@ -234,7 +240,9 @@ func (e *TLSCertificateGenerator) ensureServerCertificate(ctx context.Context, k return errors.Wrapf(err, errFmtGetTLSSecret, nn.Name) } + create := true if err == nil { + create = false if len(sec.Data[corev1.TLSCertKey]) != 0 || len(sec.Data[corev1.TLSPrivateKeyKey]) != 0 || len(sec.Data[SecretKeyCACert]) != 0 { e.log.Info("TLS secret contains server certificate.", "secret", nn.Name) return nil @@ -268,17 +276,18 @@ func (e *TLSCertificateGenerator) ensureServerCertificate(ctx context.Context, k if e.owner != nil { sec.OwnerReferences = e.owner } - _, err = controllerruntime.CreateOrUpdate(ctx, kube, sec, func() error { - if sec.Data == nil { - sec.Data = make(map[string][]byte) - } - sec.Data[corev1.TLSCertKey] = certData - sec.Data[corev1.TLSPrivateKeyKey] = keyData - sec.Data[SecretKeyCACert] = signer.certificatePEM - - return nil - }) + if sec.Data == nil { + sec.Data = make(map[string][]byte) + } + sec.Data[corev1.TLSCertKey] = certData + sec.Data[corev1.TLSPrivateKeyKey] = keyData + sec.Data[SecretKeyCACert] = signer.certificatePEM + if create { + err = kube.Create(ctx, sec) + } else { + err = kube.Update(ctx, sec) + } return errors.Wrapf(err, errFmtCannotCreateOrUpdate, nn.Name) }