Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helper/schema: Custom diff logic support for providers #14887

Merged
merged 33 commits into from
Nov 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
b99c615
helper/schema: New ResourceDiff object
vancluever May 23, 2017
1e8cfc5
helper/schema: Updated ResourceDiff prototype
vancluever May 25, 2017
aeb793f
helper/schema: Add Clear function to ResourceDiff
vancluever May 25, 2017
f5f4e03
helper/schema: Track updated keys in ResourceDiff
vancluever May 25, 2017
64cc408
helper/schema: ResourceDiff tests, bug fixes
vancluever May 27, 2017
6a4f7b0
helper/schema: Don't ignore diffs for certain NewComputed keys
vancluever May 27, 2017
a5fc664
helper/schema: Add schemaMap.DeepCopy
vancluever May 28, 2017
22220fd
helper/schema: Final set of ResourceDiff tests, bug fixes
vancluever May 28, 2017
196d7e6
helper/schema: Drop ClearAll from ResourceDiff
vancluever May 28, 2017
8126ee8
helper/schema: Fix supplied config to tests
vancluever May 28, 2017
8af9610
helper/schema: Hook CustomizeDiffFunc into diff logic
vancluever May 28, 2017
ee76918
helper/schema: More CustomizeDiff test cases
vancluever May 28, 2017
fa1fc2c
helper/schema: CustomizeDiff invocation test
vancluever May 28, 2017
f7e4272
helper/schema: Remove unused Destroy check for CustomizeDiff
vancluever May 28, 2017
c6647a3
helper/schema: CustomizeDiff -> Review
vancluever May 31, 2017
6f422d8
helper/schema: Remove unused log line
vancluever May 31, 2017
7d5f9ed
helper/schema: NewComputed values should be nil
vancluever May 31, 2017
8a7c9a6
helper/schema: frist -> first
vancluever May 31, 2017
ad98471
helper/schema: Correct some comments
vancluever May 31, 2017
931b0e1
helper/schema: Remove exported SetDiff method
vancluever May 31, 2017
36aa63b
helper/schema: Guard against out of range on childAddrOf
vancluever May 31, 2017
3444549
helper/schema: Move computed key reader logic to childAddrOf
vancluever May 31, 2017
f0aafe4
helper/schema: Restore new value for complex set test
vancluever May 31, 2017
9625830
helper/schema: Move computedKeys init to init function
vancluever May 31, 2017
09e2109
helper/schema: Resouce.Diff no longer ResourceProvider API compatible
vancluever May 31, 2017
529d7e6
helper/schema: Review -> CustomizeDiff
vancluever May 31, 2017
5d5a670
provider/test: Added complex-ish list testing
vancluever Jun 1, 2017
3ac0cdf
helper/schema: Better ResourceDiff schema key validation
vancluever Jun 1, 2017
f5bdbc0
helper/schema: Simplify setDiff, remove exported SetDiff mentions
vancluever Jun 1, 2017
12378b4
terraform: Add value check to diff.Same
vancluever Jul 3, 2017
5031767
terraform: Add more exception cases for value comparison
vancluever Jul 3, 2017
2c541e8
helper/schema: Add behaviour detail to various custom diff comments
vancluever Jul 5, 2017
0c0ae3c
helper/schema: CustomizeDiff allowed on writable resources only
vancluever Jul 9, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions builtin/providers/test/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ func Provider() terraform.ResourceProvider {
},
},
ResourcesMap: map[string]*schema.Resource{
"test_resource": testResource(),
"test_resource_gh12183": testResourceGH12183(),
"test_resource": testResource(),
"test_resource_gh12183": testResourceGH12183(),
"test_resource_with_custom_diff": testResourceCustomDiff(),
},
DataSourcesMap: map[string]*schema.Resource{
"test_data_source": testDataSource(),
Expand Down
154 changes: 154 additions & 0 deletions builtin/providers/test/resource_with_custom_diff.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
package test

import (
"fmt"

"github.com/hashicorp/terraform/helper/schema"
)

func testResourceCustomDiff() *schema.Resource {
return &schema.Resource{
Create: testResourceCustomDiffCreate,
Read: testResourceCustomDiffRead,
CustomizeDiff: testResourceCustomDiffCustomizeDiff,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor cosmetic nit: visually most of the existing Resource attributes are short, so they line up nicely when formatted in a struct literal like this. Perhaps we could preserve that here by just calling this Diff. I think that's consistent with the other usage here, since all of these operations are in some sense "customizing" the default behavior that helper/schema naturally does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - I kind of was wondering this myself and I guess I didn't want a developer thinking that Terraform wasn't doing any diffing behaviour if this was left out now, but I do like the more concise Diff for sure. Will adjust!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so it turns out that this can't necessarily function because it collides with the already existing resource.Diff, of course... doh!

Trying to think of something else that still keeps the spirit of what we are trying to do here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Review or Revise? That is kind of what is happening here... the diff is being reviewed, and possibly being vetoed or rejected. :)

Other names I could think of were either too close to CRUD-like things that already exist (like Patch) or just didn't really roll off the tongue like the current set of functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes, of course. I forgot that there are actually methods on this struct. 😀

I think part of why this is hard to name is that this function kinda serves two purposes:

  • Check whether a diff is valid
  • Alter a diff to provide more context

So AlterDiff seems promising if we consider the checking to be an implicit side-effect, but that's still longer than all of the other verbs here, so doesn't really make a great deal of difference.

So with all of that said, maybe let's just stick with CustomizeDiff and accept the weird visual texture it creates... too minor a detail to have sweated this much over it. 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I did go forward with changing it to Review last night - check 6cbfe22 for the commit and how it looks cosmetically. I can roll it back to CustomizeDiff if you still think it's too ambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to bike-shed it too much, but to me "reviewing" a diff sounds like something a human would do rather than something code would do, and feels a little vague.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sweat :) will roll this back. I think after this the only thing left to review will be the deposed stuff.

Update: testResourceCustomDiffUpdate,
Delete: testResourceCustomDiffDelete,
Schema: map[string]*schema.Schema{
"required": {
Type: schema.TypeString,
Required: true,
},
"computed": {
Type: schema.TypeInt,
Computed: true,
},
"index": {
Type: schema.TypeInt,
Computed: true,
},
"veto": {
Type: schema.TypeBool,
Optional: true,
},
"list": {
Type: schema.TypeList,
Computed: true,
Elem: &schema.Schema{Type: schema.TypeString},
},
},
}
}

type listDiffCases struct {
Type string
Value string
}

func testListDiffCases(index int) []listDiffCases {
switch index {
case 0:
return []listDiffCases{
{
Type: "add",
Value: "dc1",
},
}
case 1:
return []listDiffCases{
{
Type: "remove",
Value: "dc1",
},
{
Type: "add",
Value: "dc2",
},
{
Type: "add",
Value: "dc3",
},
}
}
return nil
}

func testListDiffCasesReadResult(index int) []interface{} {
switch index {
case 1:
return []interface{}{"dc1"}
default:
return []interface{}{"dc2", "dc3"}
}
}

func testResourceCustomDiffCreate(d *schema.ResourceData, meta interface{}) error {
d.SetId("testId")

// Required must make it through to Create
if _, ok := d.GetOk("required"); !ok {
return fmt.Errorf("missing attribute 'required', but it's required")
}

_, new := d.GetChange("computed")
expected := new.(int) - 1
actual := d.Get("index").(int)
if expected != actual {
return fmt.Errorf("expected computed to be 1 ahead of index, got computed: %d, index: %d", expected, actual)
}
d.Set("index", new)

return testResourceCustomDiffRead(d, meta)
}

func testResourceCustomDiffRead(d *schema.ResourceData, meta interface{}) error {
if err := d.Set("list", testListDiffCasesReadResult(d.Get("index").(int))); err != nil {
return err
}
return nil
}

func testResourceCustomDiffCustomizeDiff(d *schema.ResourceDiff, meta interface{}) error {
if d.Get("veto").(bool) == true {
return fmt.Errorf("veto is true, diff vetoed")
}
// Note that this gets put into state after the update, regardless of whether
// or not anything is acted upon in the diff.
d.SetNew("computed", d.Get("computed").(int)+1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any ill-effects if the Create / Update implementations override this to be a different value before they return? If the final write "wins" then that seems fine, but I want to make sure we don't end up creating the dreaded "Diffs didn't match during apply" error here in that case.

At the very least, we'd need to document that providers shouldn't do that if so, but ideally we'd find a way to make it not arise in the first place since those errors tend to be pretty annoying to track down when the do arise in the wild.

Copy link
Contributor Author

@vancluever vancluever May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, I don't think there should be any ill effects. What would happen is any values that got set in with Set* functions in the schema.ResourceData that gets passed along would be set in the set writer layer, while the data set in the diff function would have been already properly rolled into the diff and would exist exclusively in the diff reader layer. set takes precedence, so what I'm pretty sure will happen is that after Create or Update returns any value that has been put into the set writer will be the one that "wins" when the new state is calculated.

The final step of a Resource.Apply is to merge the state using ResourceData.State(), which reconciles the state with all the data collected in the various levels of the writer, starting with the set layer (or state if in partial mode without a SetPartial for the specific key, which bypasses the diff anyway, before this change), and returns it, which ultimately becomes the new state.

The good old "diffs didn't match during apply" error was something I was worried about too, but I don't think we are at risk for that, mainly because EvalCompareDiff gets evaluated before EvalApply, which means that 1) state hasn't been written yet, and 2) Create or Update will not have fired anyway, meaning that there is no risk in either of these creating an issue. What I think we need to be more worried about is a pre-written plan drifting too far from the current real state of the resource that any custom diff behaviour is pulling to calculate the diff. However, I think that's an accurate case of what you were describing in #8769 where the user may need to re-refresh (and then re-plan, ultimately).

I think this could be properly vetoed - the drift could be detected via checking the current real world value and then comparing it to the data from GetChange, giving the opportunity to provide a friendly error to the user to request they re-plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of wondering if we can possibly build some safety in here so that this can be guarded against before it hits EvalCompareDiff and spews both of the diffs in a way that's not so friendly to the user. I want to come back to this.


// This tests a diffed list, based off of the value of index
dcs := testListDiffCases(d.Get("index").(int))
s := d.Get("list").([]interface{})
for _, dc := range dcs {
switch dc.Type {
case "add":
s = append(s, dc.Value)
case "remove":
for i := range s {
if s[i].(string) == dc.Value {
copy(s[i:], s[i+1:])
s = s[:len(s)-1]
break
}
}
}
}
d.SetNew("list", s)

return nil
}

func testResourceCustomDiffUpdate(d *schema.ResourceData, meta interface{}) error {
_, new := d.GetChange("computed")
expected := new.(int) - 1
actual := d.Get("index").(int)
if expected != actual {
return fmt.Errorf("expected computed to be 1 ahead of index, got computed: %d, index: %d", expected, actual)
}
d.Set("index", new)
return testResourceCustomDiffRead(d, meta)
}

func testResourceCustomDiffDelete(d *schema.ResourceData, meta interface{}) error {
d.SetId("")
return nil
}
53 changes: 53 additions & 0 deletions builtin/providers/test/resource_with_custom_diff_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package test

import (
"fmt"
"regexp"
"testing"

"github.com/hashicorp/terraform/helper/resource"
)

// TestResourceWithCustomDiff test custom diff behaviour.
func TestResourceWithCustomDiff(t *testing.T) {
resource.UnitTest(t, resource.TestCase{
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: resourceWithCustomDiffConfig(false),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "computed", "1"),
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "index", "1"),
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "list.#", "1"),
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "list.0", "dc1"),
),
ExpectNonEmptyPlan: true,
},
{
Config: resourceWithCustomDiffConfig(false),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "computed", "2"),
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "index", "2"),
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "list.#", "2"),
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "list.0", "dc2"),
resource.TestCheckResourceAttr("test_resource_with_custom_diff.foo", "list.1", "dc3"),
resource.TestCheckNoResourceAttr("test_resource_with_custom_diff.foo", "list.2"),
),
ExpectNonEmptyPlan: true,
},
{
Config: resourceWithCustomDiffConfig(true),
ExpectError: regexp.MustCompile("veto is true, diff vetoed"),
},
},
})
}

func resourceWithCustomDiffConfig(veto bool) string {
return fmt.Sprintf(`
resource "test_resource_with_custom_diff" "foo" {
required = "yep"
veto = %t
}
`, veto)
}
2 changes: 1 addition & 1 deletion helper/schema/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (b *Backend) Configure(c *terraform.ResourceConfig) error {

// Get a ResourceData for this configuration. To do this, we actually
// generate an intermediary "diff" although that is never exposed.
diff, err := sm.Diff(nil, c)
diff, err := sm.Diff(nil, c, nil, nil)
if err != nil {
return err
}
Expand Down
6 changes: 3 additions & 3 deletions helper/schema/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (p *Provider) Configure(c *terraform.ResourceConfig) error {

// Get a ResourceData for this configuration. To do this, we actually
// generate an intermediary "diff" although that is never exposed.
diff, err := sm.Diff(nil, c)
diff, err := sm.Diff(nil, c, nil, p.meta)
if err != nil {
return err
}
Expand Down Expand Up @@ -293,7 +293,7 @@ func (p *Provider) Diff(
return nil, fmt.Errorf("unknown resource type: %s", info.Type)
}

return r.Diff(s, c)
return r.Diff(s, c, p.meta)
}

// Refresh implementation of terraform.ResourceProvider interface.
Expand Down Expand Up @@ -410,7 +410,7 @@ func (p *Provider) ReadDataDiff(
return nil, fmt.Errorf("unknown data source: %s", info.Type)
}

return r.Diff(nil, c)
return r.Diff(nil, c, p.meta)
}

// RefreshData implementation of terraform.ResourceProvider interface.
Expand Down
4 changes: 2 additions & 2 deletions helper/schema/provisioner.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func (p *Provisioner) Apply(
}

sm := schemaMap(p.ConnSchema)
diff, err := sm.Diff(nil, terraform.NewResourceConfig(c))
diff, err := sm.Diff(nil, terraform.NewResourceConfig(c), nil, nil)
if err != nil {
return err
}
Expand All @@ -160,7 +160,7 @@ func (p *Provisioner) Apply(
// Build the configuration data. Doing this requires making a "diff"
// even though that's never used. We use that just to get the correct types.
configMap := schemaMap(p.Schema)
diff, err := configMap.Diff(nil, c)
diff, err := configMap.Diff(nil, c, nil, nil)
if err != nil {
return err
}
Expand Down
47 changes: 43 additions & 4 deletions helper/schema/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,37 @@ type Resource struct {
Delete DeleteFunc
Exists ExistsFunc

// CustomizeDiff is a custom function for working with the diff that
// Terraform has created for this resource - it can be used to customize the
// diff that has been created, diff values not controlled by configuration,
// or even veto the diff altogether and abort the plan. It is passed a
// *ResourceDiff, a structure similar to ResourceData but lacking most write
// functions like Set, while introducing new functions that work with the
// diff such as SetNew, SetNewComputed, and ForceNew.
//
// The phases Terraform runs this in, and the state available via functions
// like Get and GetChange, are as follows:
//
// * New resource: One run with no state
// * Existing resource: One run with state
// * Existing resource, forced new: One run with state (before ForceNew),
// then one run without state (as if new resource)
// * Tainted resource: No runs (custom diff logic is skipped)
// * Destroy: No runs (standard diff logic is skipped on destroy diffs)
//
// This function needs to be resilient to support all scenarios.
//
// If this function needs to access external API resources, remember to flag
// the RequiresRefresh attribute mentioned below to ensure that
// -refresh=false is blocked when running plan or apply, as this means that
// this resource requires refresh-like behaviour to work effectively.
//
// For the most part, only computed fields can be customized by this
// function.
//
// This function is only allowed on regular resources (not data sources).
CustomizeDiff CustomizeDiffFunc

// Importer is the ResourceImporter implementation for this resource.
// If this is nil, then this resource does not support importing. If
// this is non-nil, then it supports importing and ResourceImporter
Expand Down Expand Up @@ -126,6 +157,9 @@ type ExistsFunc func(*ResourceData, interface{}) (bool, error)
type StateMigrateFunc func(
int, *terraform.InstanceState, interface{}) (*terraform.InstanceState, error)

// See Resource documentation.
type CustomizeDiffFunc func(*ResourceDiff, interface{}) error

// Apply creates, updates, and/or deletes a resource.
func (r *Resource) Apply(
s *terraform.InstanceState,
Expand Down Expand Up @@ -202,11 +236,11 @@ func (r *Resource) Apply(
return r.recordCurrentSchemaVersion(data.State()), err
}

// Diff returns a diff of this resource and is API compatible with the
// ResourceProvider interface.
// Diff returns a diff of this resource.
func (r *Resource) Diff(
s *terraform.InstanceState,
c *terraform.ResourceConfig) (*terraform.InstanceDiff, error) {
c *terraform.ResourceConfig,
meta interface{}) (*terraform.InstanceDiff, error) {

t := &ResourceTimeout{}
err := t.ConfigDecode(r, c)
Expand All @@ -215,7 +249,7 @@ func (r *Resource) Diff(
return nil, fmt.Errorf("[ERR] Error decoding timeout: %s", err)
}

instanceDiff, err := schemaMap(r.Schema).Diff(s, c)
instanceDiff, err := schemaMap(r.Schema).Diff(s, c, r.CustomizeDiff, meta)
if err != nil {
return instanceDiff, err
}
Expand Down Expand Up @@ -346,6 +380,11 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error
if r.Create != nil || r.Update != nil || r.Delete != nil {
return fmt.Errorf("must not implement Create, Update or Delete")
}

// CustomizeDiff cannot be defined for read-only resources
if r.CustomizeDiff != nil {
return fmt.Errorf("cannot implement CustomizeDiff")
}
}

tsm := topSchemaMap
Expand Down
Loading