From 40400b664ba6a724c2e417c832735e2cdd8384cb Mon Sep 17 00:00:00 2001 From: Fraser Waters Date: Tue, 16 Nov 2021 14:29:04 +0000 Subject: [PATCH] Better constraint diffs --- pkg/resource/deploy/plan.go | 80 ++++++++++++++++++++--- pkg/resource/deploy/step_generator.go | 22 +++---- sdk/go/common/resource/properties_diff.go | 11 +--- 3 files changed, 84 insertions(+), 29 deletions(-) diff --git a/pkg/resource/deploy/plan.go b/pkg/resource/deploy/plan.go index 7b74e4bc2..29d9614d7 100644 --- a/pkg/resource/deploy/plan.go +++ b/pkg/resource/deploy/plan.go @@ -7,6 +7,7 @@ import ( "github.com/pulumi/pulumi/pkg/v3/resource/deploy/providers" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/common/tokens" "github.com/pulumi/pulumi/sdk/v3/go/common/util/contract" ) @@ -18,10 +19,32 @@ import ( // exactly. type Plan map[resource.URN]*ResourcePlan +// Goal is a desired state for a resource object. Normally it represents a subset of the resource's state expressed by +// a program, however if Output is true, it represents a more complete, post-deployment view of the state. +type GoalPlan struct { + Type tokens.Type // the type of resource. + Name tokens.QName // the name for the resource's URN. + Custom bool // true if this resource is custom, managed by a plugin. + Adds resource.PropertyMap // the resource's properties we expect to add. + Deletes []resource.PropertyKey // the resource's properties we expect to delete. + Updates resource.PropertyMap // the resource's properties we expect to update. + Parent resource.URN // an optional parent URN for this resource. + Protect bool // true to protect this resource from deletion. + Dependencies []resource.URN // dependencies of this resource object. + Provider string // the provider to use for this resource. + PropertyDependencies map[resource.PropertyKey][]resource.URN // the set of dependencies that affect each property. + DeleteBeforeReplace *bool // true if this resource should be deleted prior to replacement. + IgnoreChanges []string // a list of property names to ignore during changes. + AdditionalSecretOutputs []resource.PropertyKey // outputs that should always be treated as secrets. + Aliases []resource.URN // additional URNs that should be aliased to this resource. + ID resource.ID // the expected ID of the resource, if any. + CustomTimeouts resource.CustomTimeouts // an optional config object for resource options +} + // A ResourcePlan represents the planned goal state and resource operations for a single resource. The operations are // ordered. type ResourcePlan struct { - Goal *resource.Goal + Goal *GoalPlan Ops []StepOp Outputs resource.PropertyMap } @@ -96,8 +119,9 @@ func (rp *ResourcePlan) diffPropertyDependencies(a, b map[resource.PropertyKey][ return nil } -func (rp *ResourcePlan) checkGoal(programGoal *resource.Goal) error { +func (rp *ResourcePlan) checkGoal(oldOutputs resource.PropertyMap, newInputs resource.PropertyMap, programGoal *resource.Goal) error { contract.Assert(programGoal != nil) + contract.Assert(newInputs != nil) // rp.Goal may be nil, but if it isn't Type and Name should match contract.Assert(rp.Goal == nil || rp.Goal.Type == programGoal.Type) contract.Assert(rp.Goal == nil || rp.Goal.Name == programGoal.Name) @@ -196,20 +220,56 @@ func (rp *ResourcePlan) checkGoal(programGoal *resource.Goal) error { return fmt.Errorf("dependencies changed: %v", message) } - // Check that the properties meet the constraints set in the plan. - if diff, constrained := programGoal.Properties.ConstrainedTo(rp.Goal.Properties); !constrained { - // TODO(pdg-plan): message! - var paths []string + // Check that the property diffs meet the constraints set in the plan. + changes := []string{} + if diff, hasDiff := oldOutputs.DiffIncludeUnknowns(newInputs); hasDiff { + // Check that any adds are in the goal for adds for k := range diff.Adds { - paths = append(paths, "+"+string(k)) + if expected, has := rp.Goal.Adds[k]; has { + actual := diff.Adds[k] + if !expected.IsComputed() && !expected.DeepEquals(actual) { + // diff wants to add this with value X but constraint wants to add with value Y + changes = append(changes, "+"+string(k)) + } + } else { + // diff wants to add this, but not listed as an add in the constraints + changes = append(changes, "+"+string(k)) + } } + + // Check that any removes are in the goal for removes for k := range diff.Deletes { - paths = append(paths, "-"+string(k)) + found := false + for i := range rp.Goal.Deletes { + if rp.Goal.Deletes[i] == k { + found = true + break + } + } + + if !found { + // diff wants to delete this, but not listed as a delete in the constraints + changes = append(changes, "-"+string(k)) + } } + + // Check that any changes are in the goal for changes for k := range diff.Updates { - paths = append(paths, "~"+string(k)) + if expected, has := rp.Goal.Updates[k]; has { + actual := diff.Adds[k] + if !expected.IsComputed() && !expected.DeepEquals(actual) { + // diff wants to change this with value X but constraint wants to change with value Y + changes = append(changes, "~"+string(k)) + } + } else { + // diff wants to update this, but not listed as an update in the constraints + changes = append(changes, "~"+string(k)) + } } - return fmt.Errorf("properties changed: %v", strings.Join(paths, ", ")) + } + + if len(changes) > 0 { + return fmt.Errorf("properties changed: %v", strings.Join(changes, ", ")) } // Check that the property dependencies match. Note that because it is legal for a property that is unknown in the diff --git a/pkg/resource/deploy/step_generator.go b/pkg/resource/deploy/step_generator.go index 2e4a23edf..4d1208cae 100644 --- a/pkg/resource/deploy/step_generator.go +++ b/pkg/resource/deploy/step_generator.go @@ -239,17 +239,6 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res } sg.urns[urn] = true - // If there is a plan for this resource, validate that the program goal conforms to the plan. - if sg.deployment.plan != nil { - resourcePlan, ok := sg.deployment.plan[urn] - if !ok { - return nil, result.Errorf("resource not found in plan") - } - if err := resourcePlan.checkGoal(goal); err != nil { - return nil, result.FromError(fmt.Errorf("resource violates plan: %w", err)) - } - } - // Check for an old resource so that we can figure out if this is a create, delete, etc., and/or // to diff. We look up first by URN and then by any provided aliases. If it is found using an // alias, record that alias so that we do not delete the aliased resource later. @@ -287,6 +276,17 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res inputs = processedInputs } + // If there is a plan for this resource, validate that the program goal conforms to the plan. + if sg.deployment.plan != nil { + resourcePlan, ok := sg.deployment.plan[urn] + if !ok { + return nil, result.Errorf("resource not found in plan") + } + if err := resourcePlan.checkGoal(oldOutputs, inputs, goal); err != nil { + return nil, result.FromError(fmt.Errorf("resource violates plan: %w", err)) + } + } + // Produce a new state object that we'll build up as operations are performed. Ultimately, this is what will // get serialized into the checkpoint file. new := resource.NewState(goal.Type, urn, goal.Custom, false, "", inputs, nil, goal.Parent, goal.Protect, false, diff --git a/sdk/go/common/resource/properties_diff.go b/sdk/go/common/resource/properties_diff.go index 5dfed1e3e..daa7fcb5e 100644 --- a/sdk/go/common/resource/properties_diff.go +++ b/sdk/go/common/resource/properties_diff.go @@ -304,7 +304,7 @@ func (props PropertyMap) DeepEquals(other PropertyMap) bool { return true } -// DeepEquals returns true if this property map is deeply equal to the other property map; and false otherwise. +// DeepEquals returns true if this property value is deeply equal to the other property value; and false otherwise. func (v PropertyValue) DeepEquals(other PropertyValue) bool { // Computed values are always equal. if v.IsComputed() && other.IsComputed() { @@ -414,12 +414,7 @@ func (v PropertyValue) DeepEquals(other PropertyValue) bool { return v.V == other.V } -func (props PropertyMap) ConstrainedTo(constraints PropertyMap) (*ObjectDiff, bool) { +func (props PropertyMap) DiffIncludeUnknowns(constraints PropertyMap) (*ObjectDiff, bool) { diff := constraints.diff(props, true, nil) - return diff, diff == nil -} - -func (v PropertyValue) ConstrainedTo(constraint PropertyValue) (*ValueDiff, bool) { - diff := constraint.diff(v, true, nil) - return diff, diff == nil + return diff, diff != nil }