Skip to content

Commit

Permalink
Merge pull request #81 from negz/fatalistic
Browse files Browse the repository at this point in the history 
Don't delete composed resources when we hit an error
  • Loading branch information
@negz
negz authored Feb 1, 2024
2 parents aa352e7 + a7c2fe8 commit 0d69580
Show file tree
Hide file tree
Showing 12 changed files with 1,058 additions and 1,261 deletions.
5 changes: 1 addition & 4 deletions example/composition.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,9 @@ spec:
base:
apiVersion: s3.aws.upbound.io/v1beta1
kind: Bucket
spec:
forProvider:
region: us-east-2
patches:
- type: FromCompositeFieldPath
fromFieldPath: "location"
fromFieldPath: "spec.location"
toFieldPath: "spec.forProvider.region"
transforms:
- type: map
Expand Down
72 changes: 58 additions & 14 deletions fn.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

"github.com/crossplane/crossplane-runtime/pkg/errors"
"github.com/crossplane/crossplane-runtime/pkg/fieldpath"
"github.com/crossplane/crossplane-runtime/pkg/logging"
"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"

Expand Down Expand Up @@ -114,10 +115,14 @@ func (f *Function) RunFunction(ctx context.Context, req *fnv1beta1.RunFunctionRe
}

if input.Environment != nil {
// Run all patches that are from the (observed) XR to the environment or from the environment to the (desired) XR.
if err := RenderEnvironmentPatches(env, oxr.Resource, dxr.Resource, input.Environment.Patches); err != nil {
response.Fatal(rsp, errors.Wrapf(err, "cannot render ToEnvironment patches from the composite resource"))
return rsp, nil
// Run all patches that are from the (observed) XR to the environment or
// from the environment to the (desired) XR.
for i := range input.Environment.Patches {
p := &input.Environment.Patches[i]
if err := ApplyEnvironmentPatch(p, env, oxr.Resource, dxr.Resource); err != nil {
response.Fatal(rsp, errors.Wrapf(err, "cannot apply the %q environment patch at index %d", p.GetType(), i))
return rsp, nil
}
}
}

Expand Down Expand Up @@ -155,8 +160,8 @@ func (f *Function) RunFunction(ctx context.Context, req *fnv1beta1.RunFunctionRe
}
}

ocd, ok := observed[resource.Name(t.Name)]
if ok {
ocd, exists := observed[resource.Name(t.Name)]
if exists {
existing++
log.Debug("Resource template corresponds to existing composed resource", "metadata-name", ocd.Resource.GetName())

Expand Down Expand Up @@ -192,17 +197,56 @@ func (f *Function) RunFunction(ctx context.Context, req *fnv1beta1.RunFunctionRe
"name", ocd.Resource.GetName())
}

errs, store := RenderComposedPatches(ocd.Resource, dcd.Resource, oxr.Resource, dxr.Resource, env, t.Patches)
for _, err := range errs {
response.Warning(rsp, errors.Wrapf(err, "cannot render patches for composed resource %q", t.Name))
log.Info("Cannot render patches for composed resource", "warning", err)
warnings++
// Run all patches that are to a desired composed resource, or from an
// observed composed resource.
skip := false
for i := range t.Patches {
p := &t.Patches[i]
if err := ApplyComposedPatch(p, ocd.Resource, dcd.Resource, oxr.Resource, dxr.Resource, env); err != nil {
if fieldpath.IsNotFound(err) {
// This is a patch from a required field path that does not
// exist. The point of FromFieldPathPolicyRequired is to
// block creation of the new 'to' resource until the 'from'
// field path exists.
//
// The only kind of resource we could be patching to that
// might not exist at this point is a composed resource. So
// if we're patching to a composed resource that doesn't
// exist we want to avoid creating it. Otherwise, we just
// treat the patch from a required field path the same way
// we'd treat a patch from an optional field path and skip
// it.
if p.GetPolicy().GetFromFieldPathPolicy() == v1beta1.FromFieldPathPolicyRequired {
if ToComposedResource(p) && !exists {
response.Warning(rsp, errors.Wrapf(err, "not adding new composed resource %q to desired state because %q patch at index %d has 'policy.fromFieldPath: Required'", t.Name, p.GetType(), i))

// There's no point processing further patches.
// They'll either be from an observed composed
// resource that doesn't exist yet, or to a desired
// composed resource that we'll discard.
skip = true
break
}
response.Warning(rsp, errors.Wrapf(err, "cannot render composed resource %q %q patch at index %d: ignoring 'policy.fromFieldPath: Required' because 'to' resource already exists", t.Name, p.GetType(), i))
}

// If any optional field path isn't found we just skip this
// patch and move on. The path may be populated by a
// subsequent patch.
continue
}
response.Fatal(rsp, errors.Wrapf(err, "cannot render composed resource %q %q patch at index %d", t.Name, p.GetType(), i))
return rsp, nil
}
}

if store {
// Add or replace our desired resource.
desired[resource.Name(t.Name)] = dcd
// Skip adding this resource to the desired state because it doesn't
// exist yet, and a required FromFieldPath was not (yet) found.
if skip {
continue
}

desired[resource.Name(t.Name)] = dcd
}

if err := response.SetDesiredCompositeResource(rsp, dxr); err != nil {
Expand Down
179 changes: 156 additions & 23 deletions fn_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,17 +384,15 @@ func TestRunFunction(t *testing.T) {
},
},
},
"FailedPatchNotSaved": {
reason: "If we fail to patch a desired resource produced by a previous Function in the pipeline we should return a warning result, and leave the original desired resource untouched.",
"OptionalFieldPathNotFound": {
reason: "If we fail to patch a desired resource because an optional field path was not found we should skip the patch.",
args: args{
req: &fnv1beta1.RunFunctionRequest{
Input: resource.MustStructObject(&v1beta1.Resources{
Resources: []v1beta1.ComposedTemplate{
{
// This template base no base, so we try to
// patch the resource named "cool-resource" in
// the desired resources array.
Name: "cool-resource",
Base: &runtime.RawExtension{Raw: []byte(`{"apiVersion":"example.org/v1","kind":"CD","spec":{}}`)},
Patches: []v1beta1.ComposedPatch{
{
// This patch should work.
Expand All @@ -405,19 +403,11 @@ func TestRunFunction(t *testing.T) {
},
},
{
// This patch should return an error,
// because the required path does not
// exist.
// This patch should be skipped, because
// the path is not found
Type: v1beta1.PatchTypeFromCompositeFieldPath,
Patch: v1beta1.Patch{
FromFieldPath: ptr.To[string]("spec.doesNotExist"),
ToFieldPath: ptr.To[string]("spec.explode"),
Policy: &v1beta1.PatchPolicy{
FromFieldPath: func() *v1beta1.FromFieldPathPolicy {
r := v1beta1.FromFieldPathPolicyRequired
return &r
}(),
},
},
},
},
Expand All @@ -429,16 +419,94 @@ func TestRunFunction(t *testing.T) {
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
},
},
},
},
want: want{
rsp: &fnv1beta1.RunFunctionResponse{
Meta: &fnv1beta1.ResponseMeta{Ttl: durationpb.New(response.DefaultTTL)},
Desired: &fnv1beta1.State{
Composite: &fnv1beta1.Resource{
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR"}`),
},
Resources: map[string]*fnv1beta1.Resource{
"cool-resource": {
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"CD","spec":{"watchers":42}}`),
// Watchers becomes "10" because our first patch
// worked. We only skipped the second patch.
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"CD","spec":{"watchers":"10"}}`),
},
},
},
Context: &structpb.Struct{Fields: map[string]*structpb.Value{fncontext.KeyEnvironment: structpb.NewStructValue(nil)}},
},
},
},
"RequiredFieldPathNotFound": {
reason: "If we fail to patch a desired resource because a required field path was not found, and the resource doesn't exist, we should not add it to desired state (i.e. create it).",
args: args{
req: &fnv1beta1.RunFunctionRequest{
Input: resource.MustStructObject(&v1beta1.Resources{
Resources: []v1beta1.ComposedTemplate{
{
Name: "new-resource",
Base: &runtime.RawExtension{Raw: []byte(`{"apiVersion":"example.org/v1","kind":"CD","spec":{}}`)},
Patches: []v1beta1.ComposedPatch{
{
// This patch will fail because the path
// is not found.
Type: v1beta1.PatchTypeFromCompositeFieldPath,
Patch: v1beta1.Patch{
FromFieldPath: ptr.To[string]("spec.doesNotExist"),
Policy: &v1beta1.PatchPolicy{
FromFieldPath: ptr.To[v1beta1.FromFieldPathPolicy](v1beta1.FromFieldPathPolicyRequired),
},
},
},
},
},
{
Name: "existing-resource",
Base: &runtime.RawExtension{Raw: []byte(`{"apiVersion":"example.org/v1","kind":"CD","spec":{}}`)},
Patches: []v1beta1.ComposedPatch{
{
// This patch should work.
Type: v1beta1.PatchTypeFromCompositeFieldPath,
Patch: v1beta1.Patch{
FromFieldPath: ptr.To[string]("spec.widgets"),
ToFieldPath: ptr.To[string]("spec.watchers"),
},
},
{
// This patch will fail because the path
// is not found.
Type: v1beta1.PatchTypeFromCompositeFieldPath,
Patch: v1beta1.Patch{
FromFieldPath: ptr.To[string]("spec.doesNotExist"),
Policy: &v1beta1.PatchPolicy{
FromFieldPath: ptr.To[v1beta1.FromFieldPathPolicy](v1beta1.FromFieldPathPolicyRequired),
},
},
},
},
},
},
}),
Observed: &fnv1beta1.State{
Composite: &fnv1beta1.Resource{
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
},
Resources: map[string]*fnv1beta1.Resource{
// "existing-resource" exists.
"existing-resource": {},

// Note "new-resource" doesn't appear in the
// observed resources. It doesn't yet exist.
},
},
Desired: &fnv1beta1.State{
Composite: &fnv1beta1.Resource{
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
},
},
},
},
want: want{
Expand All @@ -449,20 +517,85 @@ func TestRunFunction(t *testing.T) {
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
},
Resources: map[string]*fnv1beta1.Resource{
"cool-resource": {
// spec.watchers would be "10" if we didn't
// discard the patch that worked.
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"CD","spec":{"watchers":42}}`),
// Note that the first patch did work. We only
// skipped the patch from the required field path.
"existing-resource": {
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"CD","spec":{"watchers":"10"}}`),
},

// Note "new-resource" doesn't appear here.
},
},
Context: &structpb.Struct{Fields: map[string]*structpb.Value{fncontext.KeyEnvironment: structpb.NewStructValue(nil)}},
Results: []*fnv1beta1.Result{
{
Severity: fnv1beta1.Severity_SEVERITY_WARNING,
Message: fmt.Sprintf("cannot render patches for composed resource %q: cannot apply the %q patch at index 1: spec.doesNotExist: no such field", "cool-resource", "FromCompositeFieldPath"),
Message: `not adding new composed resource "new-resource" to desired state because "FromCompositeFieldPath" patch at index 0 has 'policy.fromFieldPath: Required': spec.doesNotExist: no such field`,
},
{
Severity: fnv1beta1.Severity_SEVERITY_WARNING,
Message: `cannot render composed resource "existing-resource" "FromCompositeFieldPath" patch at index 1: ignoring 'policy.fromFieldPath: Required' because 'to' resource already exists: spec.doesNotExist: no such field`,
},
},
},
},
},
"PatchErrorIsFatal": {
reason: "If we fail to patch a desired resource we should return a fatal result.",
args: args{
req: &fnv1beta1.RunFunctionRequest{
Input: resource.MustStructObject(&v1beta1.Resources{
Resources: []v1beta1.ComposedTemplate{
{
Name: "cool-resource",
Base: &runtime.RawExtension{Raw: []byte(`{"apiVersion":"example.org/v1","kind":"CD","spec":{}}`)},
Patches: []v1beta1.ComposedPatch{
{
// This patch should work.
Type: v1beta1.PatchTypeFromCompositeFieldPath,
Patch: v1beta1.Patch{
FromFieldPath: ptr.To[string]("spec.widgets"),
ToFieldPath: ptr.To[string]("spec.watchers"),
},
},
{
// This patch should return an error,
// because the path is not an array.
Type: v1beta1.PatchTypeFromCompositeFieldPath,
Patch: v1beta1.Patch{
FromFieldPath: ptr.To[string]("spec.widgets[0]"),
},
},
},
},
},
}),
Observed: &fnv1beta1.State{
Composite: &fnv1beta1.Resource{
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
},
},
Desired: &fnv1beta1.State{
Composite: &fnv1beta1.Resource{
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
},
},
},
},
want: want{
rsp: &fnv1beta1.RunFunctionResponse{
Meta: &fnv1beta1.ResponseMeta{Ttl: durationpb.New(response.DefaultTTL)},
Desired: &fnv1beta1.State{
Composite: &fnv1beta1.Resource{
Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
},
},
Results: []*fnv1beta1.Result{
{
Severity: fnv1beta1.Severity_SEVERITY_FATAL,
Message: fmt.Sprintf("cannot render composed resource %q %q patch at index 1: spec.widgets: not an array", "cool-resource", "FromCompositeFieldPath"),
},
},
Context: &structpb.Struct{Fields: map[string]*structpb.Value{fncontext.KeyEnvironment: structpb.NewStructValue(nil)}},
},
},
},
Expand Down
1 change: 0 additions & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ require (
github.com/spf13/afero v1.10.0 // indirect
github.com/spf13/cobra v1.8.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/stretchr/testify v1.8.4 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.26.0 // indirect
golang.org/x/exp v0.0.0-20231006140011-7918f672742d // indirect
Expand Down
Loading

0 comments on commit 0d69580

Please sign in to comment.