diff --git a/apis/apiextensions/v1/composition_transforms.go b/apis/apiextensions/v1/composition_transforms.go index 822d88da5..0ae045d3b 100644 --- a/apis/apiextensions/v1/composition_transforms.go +++ b/apis/apiextensions/v1/composition_transforms.go @@ -377,8 +377,9 @@ type StringTransform struct { // `ToJson` converts any input value into its raw JSON representation. // `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input // converted to JSON. + // `ToAdler32` generate a addler32 hash based on the input string. // +optional - // +kubebuilder:validation:Enum=ToUpper;ToLower;ToBase64;FromBase64;ToJson;ToSha1;ToSha256;ToSha512 + // +kubebuilder:validation:Enum=ToUpper;ToLower;ToBase64;FromBase64;ToJson;ToSha1;ToSha256;ToSha512;ToAdler32 Convert *StringConversionType `json:"convert,omitempty"` // Trim the prefix or suffix from the input diff --git a/apis/apiextensions/v1beta1/zz_generated.composition_transforms.go b/apis/apiextensions/v1beta1/zz_generated.composition_transforms.go index d96bf6a48..e0a3f325a 100644 --- a/apis/apiextensions/v1beta1/zz_generated.composition_transforms.go +++ b/apis/apiextensions/v1beta1/zz_generated.composition_transforms.go @@ -379,8 +379,9 @@ type StringTransform struct { // `ToJson` converts any input value into its raw JSON representation. // `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input // converted to JSON. + // `ToAdler32` generate a addler32 hash based on the input string. // +optional - // +kubebuilder:validation:Enum=ToUpper;ToLower;ToBase64;FromBase64;ToJson;ToSha1;ToSha256;ToSha512 + // +kubebuilder:validation:Enum=ToUpper;ToLower;ToBase64;FromBase64;ToJson;ToSha1;ToSha256;ToSha512;ToAdler32 Convert *StringConversionType `json:"convert,omitempty"` // Trim the prefix or suffix from the input diff --git a/cluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml b/cluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml index 9ef7b4a7a..019cb7bf1 100644 --- a/cluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml +++ b/cluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml @@ -427,7 +427,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted to - JSON. + JSON. `ToAdler32` generate a addler32 hash based + on the input string. enum: - ToUpper - ToLower @@ -437,6 +438,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format @@ -810,7 +812,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted - to JSON. + to JSON. `ToAdler32` generate a addler32 hash + based on the input string. enum: - ToUpper - ToLower @@ -820,6 +823,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format @@ -1269,7 +1273,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted - to JSON. + to JSON. `ToAdler32` generate a addler32 hash + based on the input string. enum: - ToUpper - ToLower @@ -1279,6 +1284,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format @@ -1886,7 +1892,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted to - JSON. + JSON. `ToAdler32` generate a addler32 hash based + on the input string. enum: - ToUpper - ToLower @@ -1896,6 +1903,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format @@ -2269,7 +2277,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted - to JSON. + to JSON. `ToAdler32` generate a addler32 hash + based on the input string. enum: - ToUpper - ToLower @@ -2279,6 +2288,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format @@ -2728,7 +2738,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted - to JSON. + to JSON. `ToAdler32` generate a addler32 hash + based on the input string. enum: - ToUpper - ToLower @@ -2738,6 +2749,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format diff --git a/cluster/crds/apiextensions.crossplane.io_compositions.yaml b/cluster/crds/apiextensions.crossplane.io_compositions.yaml index cbcddad82..10882fb87 100644 --- a/cluster/crds/apiextensions.crossplane.io_compositions.yaml +++ b/cluster/crds/apiextensions.crossplane.io_compositions.yaml @@ -422,7 +422,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted to - JSON. + JSON. `ToAdler32` generate a addler32 hash based + on the input string. enum: - ToUpper - ToLower @@ -432,6 +433,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format @@ -805,7 +807,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted - to JSON. + to JSON. `ToAdler32` generate a addler32 hash + based on the input string. enum: - ToUpper - ToLower @@ -815,6 +818,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format @@ -1264,7 +1268,8 @@ spec: any input value into its raw JSON representation. `ToSha1`, `ToSha256` and `ToSha512` generate a hash value based on the input converted - to JSON. + to JSON. `ToAdler32` generate a addler32 hash + based on the input string. enum: - ToUpper - ToLower @@ -1274,6 +1279,7 @@ spec: - ToSha1 - ToSha256 - ToSha512 + - ToAdler32 type: string fmt: description: Format the input using a Go format diff --git a/go.mod b/go.mod index f1721b7a0..ff259d743 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/alecthomas/kong v0.8.1 github.com/bufbuild/buf v1.27.1 github.com/crossplane/crossplane-runtime v1.14.2 - github.com/docker/docker v24.0.7+incompatible + github.com/docker/docker v24.0.9+incompatible github.com/docker/go-connections v0.4.0 github.com/emicklei/dot v1.6.0 github.com/go-git/go-billy/v5 v5.5.0 diff --git a/go.sum b/go.sum index c1d728b9a..e7fb5a394 100644 --- a/go.sum +++ b/go.sum @@ -182,8 +182,8 @@ github.com/docker/cli v24.0.6+incompatible h1:fF+XCQCgJjjQNIMjzaSmiKJSCcfcXb3TWT github.com/docker/cli v24.0.6+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 v24.0.7+incompatible h1:Wo6l37AuwP3JaMnZa226lzVXGA3F9Ig1seQen0cKYlM= -github.com/docker/docker v24.0.7+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= +github.com/docker/docker v24.0.9+incompatible h1:HPGzNmwfLZWdxHqK9/II92pyi1EpYKsAqcl4G0Of9v0= +github.com/docker/docker v24.0.9+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.0 h1:YQFtbBQb4VrpoPxhFuzEBPQ9E16qz5SpHLS+uswaCp8= github.com/docker/docker-credential-helpers v0.8.0/go.mod h1:UGFXcuoQ5TxPiB54nHOZ32AWRqQdECoh/Mg0AlEYb40= diff --git a/internal/controller/apiextensions/composite/composed.go b/internal/controller/apiextensions/composite/composed.go index 14b293a5b..8d89a5c75 100644 --- a/internal/controller/apiextensions/composite/composed.go +++ b/internal/controller/apiextensions/composite/composed.go @@ -35,8 +35,14 @@ type ComposedResource struct { ResourceName ResourceName // Ready indicates whether this composed resource is ready - i.e. whether - // all of its readiness checks passed. + // all of its readiness checks passed. Setting it to false will cause the + // XR to be marked as not ready. Ready bool + + // Synced indicates whether the composition process was able to sync the + // composed resource with its desired state. Setting it to false will cause + // the XR to be marked as not synced. + Synced bool } // ComposedResourceState represents a composed resource (either desired or diff --git a/internal/controller/apiextensions/composite/composition_functions.go b/internal/controller/apiextensions/composite/composition_functions.go index a414a3a39..0b7ff59ae 100644 --- a/internal/controller/apiextensions/composite/composition_functions.go +++ b/internal/controller/apiextensions/composite/composition_functions.go @@ -17,6 +17,7 @@ package composite import ( "context" + "crypto/sha256" "fmt" "sort" @@ -79,9 +80,9 @@ const ( // resources (XR). FieldOwnerXR = "apiextensions.crossplane.io/composite" - // FieldOwnerComposed owns the fields this controller mutates on composed + // FieldOwnerComposedPrefix owns the fields this controller mutates on composed // resources. - FieldOwnerComposed = "apiextensions.crossplane.io/composed" + FieldOwnerComposedPrefix = "apiextensions.crossplane.io/composed" ) const ( @@ -372,6 +373,46 @@ func (c *FunctionComposer) Compose(ctx context.Context, xr *composite.Unstructur return CompositionResult{}, errors.Wrap(err, errApplyXRRefs) } + // Produce our array of resources to return to the Reconciler. The + // Reconciler uses this array to determine whether the XR is ready. + resources := make([]ComposedResource, 0, len(desired)) + + // We apply all of our desired resources before we observe them in the loop + // below. This ensures that issues observing and processing one composed + // resource won't block the application of another. + for name, cd := range desired { + // We don't need any crossplane-runtime resource.Applicator style apply + // options here because server-side apply takes care of everything. + // Specifically it will merge rather than replace owner references (e.g. + // for Usages), and will fail if we try to add a controller reference to + // a resource that already has a different one. + // NOTE(phisco): We need to set a field owner unique for each XR here, + // this prevents multiple XRs composing the same resource to be + // continuously alternated as controllers. + if err := c.client.Patch(ctx, cd.Resource, client.Apply, client.ForceOwnership, client.FieldOwner(ComposedFieldOwnerName(xr))); err != nil { + if kerrors.IsInvalid(err) { + // We tried applying an invalid resource, we can't tell whether + // this means the resource will never be valid or it will if we + // run again the composition after some other resource is + // created or updated successfully. So, we emit a warning event + // and move on. + // We mark the resource as not synced, so that once we get to + // decide the XR's Synced condition, we can set it to false if + // any of the resources didn't sync successfully. + events = append(events, event.Warning(reasonCompose, errors.Wrapf(err, errFmtApplyCD, name))) + // NOTE(phisco): here we behave differently w.r.t. the native + // p&t composer, as we respect the readiness reported by + // functions, while there we defaulted to also set ready false + // in case of apply errors. + resources = append(resources, ComposedResource{ResourceName: name, Ready: cd.Ready, Synced: false}) + continue + } + return CompositionResult{}, errors.Wrapf(err, errFmtApplyCD, name) + } + + resources = append(resources, ComposedResource{ResourceName: name, Ready: cd.Ready, Synced: true}) + } + // Our goal here is to patch our XR's status using server-side apply. We // want the resulting, patched object loaded into uxr. We need to pass in // only our "fully specified intent" - i.e. only the fields that we actually @@ -390,33 +431,41 @@ func (c *FunctionComposer) Compose(ctx context.Context, xr *composite.Unstructur xr.SetName(n) xr.SetUID(u) + // NOTE(phisco): Here we are fine using a hardcoded field owner as there is + // no risk of conflict between different XRs. if err := c.client.Status().Patch(ctx, xr, client.Apply, client.ForceOwnership, client.FieldOwner(FieldOwnerXR)); err != nil { + // Note(phisco): here we are fine with this error being terminal, as + // there is no other resource to apply that might eventually resolve + // this issue. return CompositionResult{}, errors.Wrap(err, errApplyXRStatus) } - // Produce our array of resources to return to the Reconciler. The - // Reconciler uses this array to determine whether the XR is ready. - resources := make([]ComposedResource, 0, len(desired)) - - // We apply all of our desired resources before we observe them in the loop - // below. This ensures that issues observing and processing one composed - // resource won't block the application of another. - for name, cd := range desired { - // We don't need any crossplane-runtime resource.Applicator style apply - // options here because server-side apply takes care of everything. - // Specifically it will merge rather than replace owner references (e.g. - // for Usages), and will fail if we try to add a controller reference to - // a resource that already has a different one. - if err := c.client.Patch(ctx, cd.Resource, client.Apply, client.ForceOwnership, client.FieldOwner(FieldOwnerComposed)); err != nil { - return CompositionResult{}, errors.Wrapf(err, errFmtApplyCD, name) - } - - resources = append(resources, ComposedResource{ResourceName: name, Ready: cd.Ready}) - } - return CompositionResult{ConnectionDetails: d.GetComposite().GetConnectionDetails(), Composed: resources, Events: events}, nil } +// ComposedFieldOwnerName generates a unique field owner name +// for a given Crossplane composite resource (XR). This uniqueness is crucial to +// prevent multiple XRs, which compose the same resource, from continuously +// alternating as controllers. +// +// The function generates a deterministic hash based on the XR's name and +// GroupKind (GK), ensuring consistency even during system restores. The hash +// does not include the XR's UID (as it's not deterministic), namespace (XRs +// don't have one), or version (to allow version changes without needing to +// update the field owner name). +// +// We decided to include the GK in the hash to prevent transferring ownership of +// composed resources across XRs with whole new GK, as that should not be +// supported without manual intervention. +// +// Given that field owner names are limited to 128 characters, the function +// truncates the hash to 32 characters. A longer hash was deemed unnecessary. +func ComposedFieldOwnerName(xr *composite.Unstructured) string { + h := sha256.New() + _, _ = h.Write([]byte(xr.GetName() + xr.GroupVersionKind().GroupKind().String())) + return fmt.Sprintf("%s/%x", FieldOwnerComposedPrefix, h.Sum(nil)) +} + // An ExistingComposedResourceObserver uses an XR's resource references to load // any existing composed resources from the API server. It also loads their // connection details. diff --git a/internal/controller/apiextensions/composite/composition_functions_test.go b/internal/controller/apiextensions/composite/composition_functions_test.go index bc0d81305..0894303c9 100644 --- a/internal/controller/apiextensions/composite/composition_functions_test.go +++ b/internal/controller/apiextensions/composite/composition_functions_test.go @@ -614,8 +614,8 @@ func TestFunctionCompose(t *testing.T) { want: want{ res: CompositionResult{ Composed: []ComposedResource{ - {ResourceName: "desired-resource-a"}, - {ResourceName: "observed-resource-a", Ready: true}, + {ResourceName: "desired-resource-a", Synced: true}, + {ResourceName: "observed-resource-a", Ready: true, Synced: true}, }, ConnectionDetails: managed.ConnectionDetails{ "from": []byte("function-pipeline"), diff --git a/internal/controller/apiextensions/composite/composition_pt.go b/internal/controller/apiextensions/composite/composition_pt.go index efb82ede5..682422cf5 100644 --- a/internal/controller/apiextensions/composite/composition_pt.go +++ b/internal/controller/apiextensions/composite/composition_pt.go @@ -272,6 +272,21 @@ func (c *PTComposer) Compose(ctx context.Context, xr *composite.Unstructured, re o := []resource.ApplyOption{resource.MustBeControllableBy(xr.GetUID()), usage.RespectOwnerRefs()} o = append(o, mergeOptions(filterPatches(t.Patches, patchTypesFromXR()...))...) if err := c.client.Apply(ctx, cd, o...); err != nil { + if kerrors.IsInvalid(err) { + // We tried applying an invalid resource, we can't tell whether + // this means the resource will never be valid or it will if we + // run again the composition after some other resource is + // created or updated successfully. So, we emit a warning event + // and move on. + events = append(events, event.Warning(reasonCompose, errors.Wrap(err, errApplyComposed))) + // We unset the cd here so that we don't try to observe it + // later. This will also mean we report it as not ready and not + // synced. Resulting in the XR being reported as not ready nor + // synced too. + cds[i] = nil + continue + } + // TODO(negz): Include the template name (if any) in this error. // Including the rendered resource's kind may help too (e.g. if the // template is anonymous). @@ -297,7 +312,7 @@ func (c *PTComposer) Compose(ctx context.Context, xr *composite.Unstructured, re // to observe it. We still want to return it to the Reconciler so that // it knows that this desired composed resource is not ready. if cd == nil { - resources[i] = ComposedResource{ResourceName: name, Ready: false} + resources[i] = ComposedResource{ResourceName: name, Synced: false, Ready: false} continue } @@ -327,7 +342,7 @@ func (c *PTComposer) Compose(ctx context.Context, xr *composite.Unstructured, re return CompositionResult{}, errors.Wrapf(err, errFmtCheckReadiness, name) } - resources[i] = ComposedResource{ResourceName: name, Ready: ready} + resources[i] = ComposedResource{ResourceName: name, Ready: ready, Synced: true} } // Call Apply so that we do not just replace fields on existing XR but diff --git a/internal/controller/apiextensions/composite/composition_pt_test.go b/internal/controller/apiextensions/composite/composition_pt_test.go index 1b9bacc94..31f01de0c 100644 --- a/internal/controller/apiextensions/composite/composition_pt_test.go +++ b/internal/controller/apiextensions/composite/composition_pt_test.go @@ -391,6 +391,7 @@ func TestPTCompose(t *testing.T) { Composed: []ComposedResource{{ ResourceName: "cool-resource", Ready: true, + Synced: true, }}, ConnectionDetails: details, }, @@ -456,10 +457,12 @@ func TestPTCompose(t *testing.T) { { ResourceName: "cool-resource", Ready: true, + Synced: true, }, { ResourceName: "uncool-resource", Ready: false, + Synced: false, }, }, ConnectionDetails: details, diff --git a/internal/controller/apiextensions/composite/reconciler.go b/internal/controller/apiextensions/composite/reconciler.go index a7dcc0b8f..52afd7092 100644 --- a/internal/controller/apiextensions/composite/reconciler.go +++ b/internal/controller/apiextensions/composite/reconciler.go @@ -71,7 +71,9 @@ const ( errFetchEnvironment = "cannot fetch environment" errSelectEnvironment = "cannot select environment" errCompose = "cannot compose resources" + errInvalidResources = "some resources were invalid, check events" errRenderCD = "cannot render composed resource" + errSyncResources = "cannot sync composed resources" reconcilePausedMsg = "Reconciliation (including deletion) is paused via the pause annotation" ) @@ -624,6 +626,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } err = errors.Wrap(err, errCompose) r.record.Event(xr, event.Warning(reasonCompose, err)) + if kerrors.IsInvalid(err) { + // API Server's invalid errors may be unstable due to pointers in + // the string representation of invalid structs (%v), among other + // reasons. Setting these errors in conditions could cause the + // resource version to increment continuously, leading to endless + // reconciliation of the resource. To avoid this, we only log these + // errors and emit an event. The conditions' message will then just + // point to the event. + err = errors.Wrap(errors.New(errInvalidResources), errCompose) + } xr.SetConditions(xpv1.ReconcileError(err)) return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus) } @@ -671,6 +683,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } var unready []ComposedResource + var unsynced []ComposedResource for i, cd := range res.Composed { // Specifying a name for P&T templates is optional but encouraged. // If there was no name, fall back to using the index. @@ -679,38 +692,62 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco id = strconv.Itoa(i) } + if !cd.Synced { + log.Debug("Composed resource is not yet valid", "id", id) + unsynced = append(unsynced, cd) + r.record.Event(xr, event.Normal(reasonCompose, fmt.Sprintf("Composed resource %q is not yet valid", id))) + } + if !cd.Ready { log.Debug("Composed resource is not yet ready", "id", id) unready = append(unready, cd) r.record.Event(xr, event.Normal(reasonCompose, fmt.Sprintf("Composed resource %q is not yet ready", id))) - continue } } - xr.SetConditions(xpv1.ReconcileSuccess()) - - // TODO(muvaf): If a resource becomes Unavailable at some point, should we - // still report it as Creating? - if len(unready) > 0 { - // We want to requeue to wait for our composed resources to - // become ready, since we can't watch them. - names := make([]string, len(unready)) - for i, cd := range unready { - names[i] = string(cd.ResourceName) - } - // sort for stable condition messages. With functions, we don't have a - // stable order otherwise. - xr.SetConditions(xpv1.Creating().WithMessage(fmt.Sprintf("Unready resources: %s", resource.StableNAndSomeMore(resource.DefaultFirstN, names)))) + if updateXRConditions(xr, unsynced, unready) { + // This requeue is subject to rate limiting. Requeues will exponentially + // backoff from 1 to 30 seconds. See the 'definition' (XRD) reconciler + // that sets up the ratelimiter. return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus) } // We requeue after our poll interval because we can't watch composed // resources - we can't know what type of resources we might compose // when this controller is started. - xr.SetConditions(xpv1.Available()) return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus) } +// updateXRConditions updates the conditions of the supplied composite resource +// based on the supplied composed resources. It returns true if the XR should be +// requeued immediately. +func updateXRConditions(xr *composite.Unstructured, unsynced, unready []ComposedResource) (requeueImmediately bool) { + readyCond := xpv1.Available() + syncedCond := xpv1.ReconcileSuccess() + if len(unsynced) > 0 { + // We want to requeue to wait for our composed resources to + // become ready, since we can't watch them. + syncedCond = xpv1.ReconcileError(errors.New(errSyncResources)).WithMessage(fmt.Sprintf("Invalid resources: %s", resource.StableNAndSomeMore(resource.DefaultFirstN, getComposerResourcesNames(unsynced)))) + requeueImmediately = true + } + if len(unready) > 0 { + // We want to requeue to wait for our composed resources to + // become ready, since we can't watch them. + readyCond = xpv1.Creating().WithMessage(fmt.Sprintf("Unready resources: %s", resource.StableNAndSomeMore(resource.DefaultFirstN, getComposerResourcesNames(unready)))) + requeueImmediately = true + } + xr.SetConditions(syncedCond, readyCond) + return requeueImmediately +} + +func getComposerResourcesNames(cds []ComposedResource) []string { + names := make([]string, len(cds)) + for i, cd := range cds { + names[i] = string(cd.ResourceName) + } + return names +} + // EnqueueForCompositionRevisionFunc returns a function that enqueues (the // related) XRs when a new CompositionRevision is created. This speeds up // reconciliation of XRs on changes to the Composition by not having to wait for diff --git a/internal/controller/apiextensions/composite/reconciler_test.go b/internal/controller/apiextensions/composite/reconciler_test.go index a364a79a4..fd4a0169c 100644 --- a/internal/controller/apiextensions/composite/reconciler_test.go +++ b/internal/controller/apiextensions/composite/reconciler_test.go @@ -547,21 +547,27 @@ func TestReconcile(t *testing.T) { Composed: []ComposedResource{{ ResourceName: "elephant", Ready: false, + Synced: true, }, { ResourceName: "cow", Ready: false, + Synced: true, }, { ResourceName: "pig", Ready: true, + Synced: true, }, { ResourceName: "cat", Ready: false, + Synced: true, }, { ResourceName: "dog", Ready: true, + Synced: true, }, { ResourceName: "snake", Ready: false, + Synced: true, }}, }, nil })), diff --git a/internal/controller/pkg/revision/runtime_function.go b/internal/controller/pkg/revision/runtime_function.go index cf4f92673..f6d6dc7ca 100644 --- a/internal/controller/pkg/revision/runtime_function.go +++ b/internal/controller/pkg/revision/runtime_function.go @@ -123,7 +123,7 @@ func (h *FunctionHooks) Post(ctx context.Context, pkg runtime.Object, pr v1.Pack // `deploymentTemplate.spec.template.spec.serviceAccountName` in the // DeploymentRuntimeConfig. if sa.Name == d.Spec.Template.Spec.ServiceAccountName { - if err := h.client.Apply(ctx, sa); err != nil { + if err := applySA(ctx, h.client, sa); err != nil { return errors.Wrap(err, errApplyFunctionSA) } } diff --git a/internal/controller/pkg/revision/runtime_function_test.go b/internal/controller/pkg/revision/runtime_function_test.go index 53439df72..051525444 100644 --- a/internal/controller/pkg/revision/runtime_function_test.go +++ b/internal/controller/pkg/revision/runtime_function_test.go @@ -387,6 +387,56 @@ func TestFunctionPostHook(t *testing.T) { }, }, }, + "SuccessWithExtraSecret": { + reason: "Should not return error if successfully applied service account with additional secret.", + args: args{ + pkg: &pkgmetav1beta1.Function{}, + rev: &v1beta1.FunctionRevision{ + Spec: v1beta1.FunctionRevisionSpec{ + PackageRevisionSpec: v1.PackageRevisionSpec{ + Package: functionImage, + DesiredState: v1.PackageRevisionActive, + }, + }, + }, + manifests: &MockManifestBuilder{ + ServiceAccountFn: func(_ ...ServiceAccountOverride) *corev1.ServiceAccount { + return &corev1.ServiceAccount{} + }, + DeploymentFn: func(_ string, _ ...DeploymentOverride) *appsv1.Deployment { + return &appsv1.Deployment{} + }, + }, + client: &test.MockClient{ + MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + if sa, ok := obj.(*corev1.ServiceAccount); ok { + sa.ImagePullSecrets = []corev1.LocalObjectReference{{Name: "test_secret"}} + } + return nil + }, + MockPatch: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error { + if d, ok := obj.(*appsv1.Deployment); ok { + d.Status.Conditions = []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentAvailable, + Status: corev1.ConditionTrue, + }} + return nil + } + return nil + }, + }, + }, + want: want{ + rev: &v1beta1.FunctionRevision{ + Spec: v1beta1.FunctionRevisionSpec{ + PackageRevisionSpec: v1.PackageRevisionSpec{ + Package: functionImage, + DesiredState: v1.PackageRevisionActive, + }, + }, + }, + }, + }, "SuccessfulWithExternallyManagedSA": { reason: "Should be successful without creating an SA, when the SA is managed externally", args: args{ diff --git a/internal/controller/pkg/revision/runtime_override_options.go b/internal/controller/pkg/revision/runtime_override_options.go index b5b50b9ee..9302bf1be 100644 --- a/internal/controller/pkg/revision/runtime_override_options.go +++ b/internal/controller/pkg/revision/runtime_override_options.go @@ -92,6 +92,21 @@ func DeploymentWithNamespace(namespace string) DeploymentOverride { } } +// DeploymentWithOptionalPodScrapeAnnotations adds Prometheus scrape annotations +// to a Deployment pod template if they are not already set. +func DeploymentWithOptionalPodScrapeAnnotations() DeploymentOverride { + return func(d *appsv1.Deployment) { + if d.Spec.Template.Annotations == nil { + d.Spec.Template.Annotations = map[string]string{} + } + if _, ok := d.Spec.Template.Annotations["prometheus.io/scrape"]; !ok { + d.Spec.Template.Annotations["prometheus.io/scrape"] = "true" + d.Spec.Template.Annotations["prometheus.io/port"] = "8080" + d.Spec.Template.Annotations["prometheus.io/path"] = "/metrics" + } + } +} + // DeploymentWithOwnerReferences overrides the owner references of a Deployment. func DeploymentWithOwnerReferences(owners []metav1.OwnerReference) DeploymentOverride { return func(d *appsv1.Deployment) { diff --git a/internal/controller/pkg/revision/runtime_provider.go b/internal/controller/pkg/revision/runtime_provider.go index 02633da1c..cb2687b59 100644 --- a/internal/controller/pkg/revision/runtime_provider.go +++ b/internal/controller/pkg/revision/runtime_provider.go @@ -23,6 +23,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crossplane/crossplane-runtime/pkg/errors" @@ -143,7 +144,7 @@ func (h *ProviderHooks) Post(ctx context.Context, pkg runtime.Object, pr v1.Pack // `deploymentTemplate.spec.template.spec.serviceAccountName` in the // DeploymentRuntimeConfig. if sa.Name == d.Spec.Template.Spec.ServiceAccountName { - if err := h.client.Apply(ctx, sa); err != nil { + if err := applySA(ctx, h.client, sa); err != nil { return errors.Wrap(err, errApplyProviderSA) } } @@ -218,6 +219,11 @@ func providerDeploymentOverrides(providerMeta *pkgmetav1.Provider, pr v1.Package // and plan to remove this after implementing a migration in a future // release. DeploymentWithSelectors(providerSelectors(providerMeta, pr)), + + // Add optional scrape annotations to the deployment. It is possible to + // disable the scraping by setting the annotation "prometheus.io/scrape" + // as "false" in the DeploymentRuntimeConfig. + DeploymentWithOptionalPodScrapeAnnotations(), } image := pr.GetSource() @@ -264,3 +270,23 @@ func providerSelectors(providerMeta *pkgmetav1.Provider, pr v1.PackageRevisionWi "pkg.crossplane.io/provider": providerMeta.GetName(), } } + +// applySA creates/updates a ServiceAccount and includes any image pull secrets +// that have been added by external controllers. +func applySA(ctx context.Context, cl resource.ClientApplicator, sa *corev1.ServiceAccount) error { + oldSa := &corev1.ServiceAccount{} + if err := cl.Get(ctx, types.NamespacedName{Name: sa.Name, Namespace: sa.Namespace}, oldSa); err == nil { + // Add pull secrets created by other controllers + existingSecrets := make(map[string]bool) + for _, secret := range sa.ImagePullSecrets { + existingSecrets[secret.Name] = true + } + + for _, secret := range oldSa.ImagePullSecrets { + if !existingSecrets[secret.Name] { + sa.ImagePullSecrets = append(sa.ImagePullSecrets, secret) + } + } + } + return cl.Apply(ctx, sa) +} diff --git a/internal/controller/pkg/revision/runtime_provider_test.go b/internal/controller/pkg/revision/runtime_provider_test.go index 2e4205f13..629df6aa2 100644 --- a/internal/controller/pkg/revision/runtime_provider_test.go +++ b/internal/controller/pkg/revision/runtime_provider_test.go @@ -459,6 +459,56 @@ func TestProviderPostHook(t *testing.T) { }, }, }, + "SuccessWithExtraSecret": { + reason: "Should not return error if successfully applied service account with additional secret.", + args: args{ + pkg: &pkgmetav1.Provider{}, + rev: &v1.ProviderRevision{ + Spec: v1.ProviderRevisionSpec{ + PackageRevisionSpec: v1.PackageRevisionSpec{ + Package: providerImage, + DesiredState: v1.PackageRevisionActive, + }, + }, + }, + manifests: &MockManifestBuilder{ + ServiceAccountFn: func(_ ...ServiceAccountOverride) *corev1.ServiceAccount { + return &corev1.ServiceAccount{} + }, + DeploymentFn: func(_ string, _ ...DeploymentOverride) *appsv1.Deployment { + return &appsv1.Deployment{} + }, + }, + client: &test.MockClient{ + MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + if sa, ok := obj.(*corev1.ServiceAccount); ok { + sa.ImagePullSecrets = []corev1.LocalObjectReference{{Name: "test_secret"}} + } + return nil + }, + MockPatch: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error { + if d, ok := obj.(*appsv1.Deployment); ok { + d.Status.Conditions = []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentAvailable, + Status: corev1.ConditionTrue, + }} + return nil + } + return nil + }, + }, + }, + want: want{ + rev: &v1.ProviderRevision{ + Spec: v1.ProviderRevisionSpec{ + PackageRevisionSpec: v1.PackageRevisionSpec{ + Package: providerImage, + DesiredState: v1.PackageRevisionActive, + }, + }, + }, + }, + }, "SuccessfulWithExternallyManagedSA": { reason: "Should be successful without creating an SA, when the SA is managed externally", args: args{ diff --git a/internal/controller/pkg/revision/runtime_test.go b/internal/controller/pkg/revision/runtime_test.go index fa9f85b7e..adae34c77 100644 --- a/internal/controller/pkg/revision/runtime_test.go +++ b/internal/controller/pkg/revision/runtime_test.go @@ -184,6 +184,7 @@ func TestRuntimeManifestBuilderDeployment(t *testing.T) { "pkg.crossplane.io/revision": providerRevisionName, }), func(deployment *appsv1.Deployment) { deployment.Spec.Replicas = ptr.To[int32](3) + deployment.Spec.Template.Annotations = nil deployment.Spec.Template.Labels["k"] = "v" deployment.Spec.Template.Spec.Containers[0].Image = "crossplane/provider-foo:v1.2.4" deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, corev1.Volume{Name: "vol-a"}, corev1.Volume{Name: "vol-b"}) @@ -246,6 +247,43 @@ func TestRuntimeManifestBuilderDeployment(t *testing.T) { }), }, }, + "ProviderDeploymentNoScrapeAnnotation": { + reason: "It should be possible to disable default scrape annotations", + args: args{ + builder: &RuntimeManifestBuilder{ + revision: providerRevision, + namespace: namespace, + runtimeConfig: &v1beta1.DeploymentRuntimeConfig{ + Spec: v1beta1.DeploymentRuntimeConfigSpec{ + DeploymentTemplate: &v1beta1.DeploymentTemplate{ + Spec: &appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "prometheus.io/scrape": "false", + }, + }, + Spec: corev1.PodSpec{}, + }, + }, + }, + }, + }, + }, + serviceAccountName: providerRevisionName, + overrides: providerDeploymentOverrides(&pkgmetav1.Provider{ObjectMeta: metav1.ObjectMeta{Name: providerMetaName}}, providerRevision), + }, + want: want{ + want: deploymentProvider(providerName, providerRevisionName, providerImage, DeploymentWithSelectors(map[string]string{ + "pkg.crossplane.io/provider": providerMetaName, + "pkg.crossplane.io/revision": providerRevisionName, + }), func(deployment *appsv1.Deployment) { + deployment.Spec.Template.Annotations = map[string]string{ + "prometheus.io/scrape": "false", + } + }), + }, + }, "ProviderDeploymentWithAdvancedRuntimeConfig": { reason: "Baseline provided by the runtime config should be applied to the deployment for advanced use cases", args: args{ @@ -434,6 +472,11 @@ func deploymentProvider(provider string, revision string, image string, override }, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "prometheus.io/scrape": "true", + "prometheus.io/port": "8080", + "prometheus.io/path": "/metrics", + }, Labels: map[string]string{ "pkg.crossplane.io/revision": revision, "pkg.crossplane.io/provider": provider, diff --git a/pkg/validation/apiextensions/v1/composition/patches_test.go b/pkg/validation/apiextensions/v1/composition/patches_test.go index d4a7c67f1..d985ea73c 100644 --- a/pkg/validation/apiextensions/v1/composition/patches_test.go +++ b/pkg/validation/apiextensions/v1/composition/patches_test.go @@ -461,6 +461,14 @@ func TestValidateFieldPath(t *testing.T) { schema: &apiextensions.JSONSchemaProps{Properties: map[string]apiextensions.JSONSchemaProps{"metadata": {Type: "object"}}}, }, }, + "AcceptMetadataGenerateName": { + reason: "Should accept metadata.generateName", + want: want{err: nil, fieldType: "string"}, + args: args{ + fieldPath: "metadata.generateName", + schema: &apiextensions.JSONSchemaProps{Properties: map[string]apiextensions.JSONSchemaProps{"metadata": {Type: "object"}}}, + }, + }, "AcceptXPreserveUnknownFieldsInAdditionalProperties": { reason: "Should properly handle x-preserve-unknown-fields even if defined in a nested schema", want: want{err: nil, fieldType: ""}, diff --git a/pkg/validation/apiextensions/v1/composition/schema.go b/pkg/validation/apiextensions/v1/composition/schema.go index 061284f66..585bea3e6 100644 --- a/pkg/validation/apiextensions/v1/composition/schema.go +++ b/pkg/validation/apiextensions/v1/composition/schema.go @@ -31,6 +31,7 @@ func defaultMetadataOnly(metadata *apiextensions.JSONSchemaProps) *apiextensions setDefaultProperty(metadata, "name", string(schema.KnownJSONTypeString)) setDefaultProperty(metadata, "namespace", string(schema.KnownJSONTypeString)) setDefaultProperty(metadata, "uid", string(schema.KnownJSONTypeString)) + setDefaultProperty(metadata, "generateName", string(schema.KnownJSONTypeString)) setDefaultLabels(metadata) setDefaultAnnotations(metadata) return metadata diff --git a/test/e2e/apiextensions_test.go b/test/e2e/apiextensions_test.go index a3284d3b9..10f4fe90e 100644 --- a/test/e2e/apiextensions_test.go +++ b/test/e2e/apiextensions_test.go @@ -87,6 +87,50 @@ func TestCompositionMinimal(t *testing.T) { ) } +// TestCompositionInvalidComposed tests Crossplane's Composition functionality, +// checking that although a composed resource is invalid, i.e. it didn't apply +// successfully. +func TestCompositionInvalidComposed(t *testing.T) { + manifests := "test/e2e/manifests/apiextensions/composition/invalid-composed" + + xrList := composed.NewList(composed.FromReferenceToList(corev1.ObjectReference{ + APIVersion: "example.org/v1alpha1", + Kind: "XParent", + }), composed.FromReferenceToList(corev1.ObjectReference{ + APIVersion: "example.org/v1alpha1", + Kind: "XChild", + })) + + environment.Test(t, + features.New(t.Name()). + WithLabel(LabelArea, LabelAreaAPIExtensions). + WithLabel(LabelSize, LabelSizeSmall). + WithLabel(config.LabelTestSuite, config.TestSuiteDefault). + WithSetup("PrerequisitesAreCreated", funcs.AllOf( + funcs.ApplyResources(FieldManager, manifests, "setup/*.yaml"), + funcs.ResourcesCreatedWithin(30*time.Second, manifests, "setup/*.yaml"), + funcs.ResourcesHaveConditionWithin(1*time.Minute, manifests, "setup/definition.yaml", apiextensionsv1.WatchingComposite()), + funcs.ResourcesHaveConditionWithin(2*time.Minute, manifests, "setup/provider.yaml", pkgv1.Healthy(), pkgv1.Active()), + )). + Assess("CreateXR", funcs.AllOf( + funcs.ApplyResources(FieldManager, manifests, "xr.yaml"), + funcs.InBackground(funcs.LogResources(xrList)), + funcs.InBackground(funcs.LogResources(nopList)), + funcs.ResourcesCreatedWithin(30*time.Second, manifests, "xr.yaml"), + )). + Assess("XRStillAnnotated", funcs.AllOf( + // Check the XR it has metadata.annotations set + funcs.ResourcesHaveFieldValueWithin(1*time.Minute, manifests, "xr.yaml", "metadata.annotations[exampleVal]", "foo"), + )). + WithTeardown("DeleteXR", funcs.AllOf( + funcs.DeleteResources(manifests, "xr.yaml"), + funcs.ResourcesDeletedWithin(2*time.Minute, manifests, "xr.yaml"), + )). + WithTeardown("DeletePrerequisites", funcs.ResourcesDeletedAfterListedAreGone(3*time.Minute, manifests, "setup/*.yaml", nopList)). + Feature(), + ) +} + // TestCompositionPatchAndTransform tests Crossplane's Composition functionality, // checking that a claim using patch-and-transform Composition will become // available when its composed resources do, and have a field derived from diff --git a/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/composition.yaml b/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/composition.yaml new file mode 100644 index 000000000..6c9c38716 --- /dev/null +++ b/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/composition.yaml @@ -0,0 +1,75 @@ +--- +apiVersion: apiextensions.crossplane.io/v1 +kind: Composition +metadata: + name: parent +spec: + compositeTypeRef: + apiVersion: example.org/v1alpha1 + kind: XParent + resources: + - name: child + base: + apiVersion: example.org/v1alpha1 + kind: XChild + spec: {} + patches: + - type: FromCompositeFieldPath + # this is going to be 1 + fromFieldPath: spec.someField + # this will fail because it's supposed to be > 1 + toFieldPath: spec.someField + - name: nop-resource-1 + base: + apiVersion: nop.crossplane.io/v1alpha1 + kind: NopResource + metadata: + annotations: + exampleVal: "foo" + spec: + forProvider: + conditionAfter: + - conditionType: Ready + conditionStatus: "False" + time: 0s + - conditionType: Ready + conditionStatus: "True" + time: 1s + patches: + - type: FromCompositeFieldPath + fromFieldPath: metadata.name + # we should still see this in the child + toFieldPath: metadata.annotations[something] + - type: ToCompositeFieldPath + fromFieldPath: metadata.annotations[exampleVal] + # we should still see this in the composite + toFieldPath: metadata.annotations[exampleVal] +--- +apiVersion: apiextensions.crossplane.io/v1 +kind: Composition +metadata: + name: child +spec: + compositeTypeRef: + apiVersion: example.org/v1alpha1 + kind: XChild + resources: + # we don't really care about what happens here, it's not going to work + # because the composite resource will be invalid + - name: nop-resource-1 + base: + apiVersion: nop.crossplane.io/v1alpha1 + kind: NopResource + spec: + forProvider: + conditionAfter: + - conditionType: Ready + conditionStatus: "False" + time: 0s + - conditionType: Ready + conditionStatus: "True" + time: 1s + patches: + - type: FromCompositeFieldPath + fromFieldPath: metadata.name + toFieldPath: metadata.annotations[something] diff --git a/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/definition.yaml b/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/definition.yaml new file mode 100644 index 000000000..192c76708 --- /dev/null +++ b/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/definition.yaml @@ -0,0 +1,51 @@ +apiVersion: apiextensions.crossplane.io/v1 +kind: CompositeResourceDefinition +metadata: + name: xparents.example.org +spec: + defaultCompositionRef: + name: parent + group: example.org + names: + kind: XParent + plural: xparents + versions: + - name: v1alpha1 + served: true + referenceable: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + someField: + # no limits on its value + type: integer +--- +apiVersion: apiextensions.crossplane.io/v1 +kind: CompositeResourceDefinition +metadata: + name: xchildren.example.org +spec: + defaultCompositionRef: + name: child + group: example.org + names: + kind: XChild + plural: xchildren + versions: + - name: v1alpha1 + served: true + referenceable: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + someField: + minimum: 2 + type: integer diff --git a/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/provider.yaml b/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/provider.yaml new file mode 100644 index 000000000..b82f2c560 --- /dev/null +++ b/test/e2e/manifests/apiextensions/composition/invalid-composed/setup/provider.yaml @@ -0,0 +1,7 @@ +apiVersion: pkg.crossplane.io/v1 +kind: Provider +metadata: + name: provider-nop +spec: + package: xpkg.upbound.io/crossplane-contrib/provider-nop:v0.2.1 + ignoreCrossplaneConstraints: true diff --git a/test/e2e/manifests/apiextensions/composition/invalid-composed/xr.yaml b/test/e2e/manifests/apiextensions/composition/invalid-composed/xr.yaml new file mode 100644 index 000000000..a233b2916 --- /dev/null +++ b/test/e2e/manifests/apiextensions/composition/invalid-composed/xr.yaml @@ -0,0 +1,11 @@ +--- +apiVersion: example.org/v1alpha1 +kind: XParent +metadata: + name: test +# Expected: +# annotations: +# exampleVal: "foo" +spec: + # this should be > 1 in the XChild composed resource, so it will fail applying it + someField: 1