terraform-provider-restapi icon indicating copy to clipboard operation
terraform-provider-restapi copied to clipboard

Planned support for refreshing state against the remote state?

Open davidronk opened this issue 4 years ago • 12 comments

I was wondering if there was planned support for refreshing state against the remote state? Currently the only diff the provider sees is when the local TF file changes vs the tfstate file. I wasn't sure if maybe it was left out intentionally for some reason.

I've mainly only used the AWS provider and it refreshes against the remote state. Also it seems to be a standard thing: https://www.terraform.io/docs/extend/best-practices/detecting-drift.html

I ask because I've grown accustomed to creating/importing an AWS resource then making changes in the AWS console (the api and settings are complicated and sometimes it's just easier to use the console) then running terraform plan and using the diff to see what I need to set in my terraform file to get it to do what I want.

I planned to use this provider for the product I work on but I can't because I don't want to unknowingly blow away changes in the system because someone made a change in the UI and didn't use TF.

davidronk avatar Mar 19 '20 19:03 davidronk

I really like this idea, but am not entirely sure how to implement it.

I suppose one option, now that api_response is available, is to store it in state and compare it to the most recent read. I'm concerned this may cause unneeded changes in the API if the server doesn't always return the JSON in the same order. For example, such a simple check would think { "id": 123, "path": "abc"} and {"path": "abc", "id": 123} differs.

I've tagged this as 'help wanted'... I'm keen to see this capability in the provider, but could use some input.

DRuggeri avatar Mar 23 '20 14:03 DRuggeri

Just wanted to chime in with my two cents - I came here to ask exactly the same question. Unfortunately I don't really have enough knowledge to contribute code, but I'm very happy to test out PRs against our API.

glitchcrab avatar Mar 25 '20 16:03 glitchcrab

Is it possible to sort the keys of 'api_response' and the most recent 'read' before the diff happens?

Also in my experience with the AWS provider the local api_response doesn't really factor into the diff (I think?). The diff is between the remote state (recent read) and the user specified key/values in the '.tf' file. So if the terraform file sets/tracks 'name' but some other attribute changed remotely then the diff shows nothing. I believe it still records that remote state but just doesn't show a change?

davidronk avatar Mar 31 '20 18:03 davidronk

I've thought more about it and I don't think that will work either, sadly. As JSON is decoded, golang doesn't preserve order - and sorting on keys will only sort the "top level". It would get super complicated to have to sort a structure with nested fields like this:

{
  "id": "123",
  "user": {
    "first": "Daniel",
    "last": "Ruggeri"
   }
 }

I think the only reasonable way to go about it is to utilize a library that offers support of deep comparison of nested structure. https://github.com/nsf/jsondiff looks promising, so I'll look at this when time allows.

DRuggeri avatar May 18 '20 18:05 DRuggeri

I began working on this and also realized there may be a problem with comparing the current response to previous responses. It's reasonable for APIs to add additional information to a response that isn't really related to the object. For example, imagine a user API read like so:

{
  "first": "John",
  "last": "Doe",
  "id": 123,
  "lastLogin": 1589833978
}

In this simple (but reasonable) case, the detection for changes would trigger just because the user logged in since the previous terraform apply.

Perhaps the best way to do this is to allow the user to set a list of fields (subject to GetStringAtKey capabilities) that are stored as an internal state data.

So, for the above example, we could allow an optional parameter (state_keys = [ "first", "last" ]) that narrows down which keys are tracked. I'll try working this up on a branch and see how it works...

DRuggeri avatar May 18 '20 20:05 DRuggeri

@DRuggeri we could just use a string input where the user can copy the json layout they want to have in the state (just let them remove the things they don't want) create a dynamic struct https://github.com/Ompluscator/dynamic-struct by iterating over the json object and save this to the state, on plan compare by hash

SvenHamers avatar Nov 12 '20 11:11 SvenHamers

Thanks, @SvenHamers - I've been unable to get back to this for some time, but the dynamic struct sounds interesting. Do you have an illustration of what that might look like for the user? I'm having trouble wrapping my brain around how we can let the user say something like, "I want these keys (and all their sub objects) to be compared on run"

DRuggeri avatar Dec 28 '20 21:12 DRuggeri

Have there been any updates on this?

ryanskandal avatar Aug 31 '22 16:08 ryanskandal

@DRuggeri while using the provider I came across the same issue. Terraform's ability to detect configuration drift against the real infrastructure is a core feature for us. Are there any plans on fixing this?

bogi0704 avatar Jan 05 '23 10:01 bogi0704

Totally agreed that it's an important feature, but just not one that time is dedicated to implementing (this provider is intended to be a stopgap solution for cases where a "real" provider is missing).

We're definitely interested in any patches or code proposals on how to implement this. There are a number of challenges that need to be though through with performing such a diff against an unknown API structure.

DRuggeri avatar Jan 05 '23 14:01 DRuggeri

Hi @DRuggeri I've put together some code that resolves configuration drift for the base case of this. Like you pointed out though, if the server adds in fields to the response you end up with a perpetual diff. Looking for some feedback on possible solutions to this.

Side note: I haven't run into issues with json ordering - the api response object and the state object are already held in memory as map[string]interface{} types so they can be deeply compared without ordering issues.

To resolve the server adding fields problem, I think the best solution would be to add a parameter, say ignore_server_added_fields = [...], that takes a list of fields. Any fields added by the server that match the list are ignored, while other fields added by the server will attempt to be corrected. To support nested objects, it could support syntax like [foo.bar, bar.baz.quid, bar.baz.zad]. IMO it seems likely that most servers will only add a field or two, so having an "ignore list" will be shorter than a "track list"

Does this seem like a reasonable approach? Can you think of other edges cases that might need to be solved?

michaelPotter avatar Mar 14 '23 05:03 michaelPotter

Hi @DRuggeri, sorry for the long post.

I've come up with several scenarios, and I'd like your (or anybody's) feedback on them before I get too deep in the code if possible. These are scenarios that I think would need to be addressed in the fix for this, and what my proposed solution looks like for each scenario. I intend to use them as test cases for my WiP patch.

There are a couple scenarios involving lists that I don't intend to support (5 and 6). I'm also interested in your feedback specifically for scenario 9 - what if you want to ignore all changes? Seems like it might be good to add another var to support the current ("legacy"?) behavior.

For context, the default behavior for all scenarios would be to revert any changes made outside of terraform and returned by the server. However you will be able to pass in a list of fields you'd like to ignore changes to. For each scenario, "in" refers to the payload that is sent to the server (i.e. the "data" object in state) while "out" refers to the response returned by the server ("api_response"). I'm not sure yet how "create_response" state field or the "update_data" parameter fit into this yet. Advice welcome.

Scenario 1 - server changes a field:

in: { foo: some_default_value } out: { foo: modified_value }

Supported behavior: ignoring - ignore_list = [ "foo" ]

Scenario 2 - server adds a field:

in: {foo: bar} out: {foo: bar, created: 1234, modified: 5678}

Supported behavior: ignoring - ignore_list = [ "created", "modified" ] caveat: you have to know which fields the server is going to add.

Scenario 3 - server removes a field:

e.g. you might POST the id in the body, but the api doesn't return it

in: { id: foo, value: 4 } out: { value: 4 }

Supported behavior: ignoring - ignore_list = [ "id" ]

Scenario 4 - server add/changes/removes a deep field:

in {outside: {in: bar, in2: bar}} out: {outside: {in: bar, in2: bar2, in3: bar3}}

Default behavior: revert Supported behavior: ignoring - ignore_list = [ "outside.in2", "outside.in3" ] (note: this ignore list would still revert changes to outside.in, or other new fields. You could also ignore "outside" to ignore all deep changes)

caveat: you have to know which fields the server is going to add/change

Scenario 5 - server changes list items:

This one is a little complicated... some of these details might change depending on the implementation.

in: { foo: bar, list: [ alpha, bravo ] } out: { foo: bar, list: [ alpha, bravo, charlie ] }

Default behavior: revert additions and removals from list. Revert changes to ordering. Supported behavior: ignoring changes to the list as a whole: ignore_list = [ "list" ] NON-supported behavior: - ignoring changes to specific indexes in the list: e.g. you might want to ignore the addition of extra fields but ensure that list[0] does not change - ignoring changes to ordering - we will always attempt to revert changes to ordering

Scenario 6 - changes to sub-value of a list:

This scenario is out of scope, i.e. NOT supported in this change, but could be supported in the future.

in: { tags: [ { key: permanent, value: fixed }, { key: transient, value: changes } ] } out: { tags: [ { key: permanent, value: fixed }, { key: transient, value: something_else } ] }

This is a scenario I could see happening with e.g. AWS resource tags if you have certain tags that are attached or modified by an outside process. This is NOT something we want to support.

Default behavior is to revert the change. We will support ignoring the "tags" field as a whole, but will NOT support ignoring subvalues of a list. For example:

  1. we will NOT support ignoring changes in a specific list index, e.g. the second list item
  2. we will NOT support a syntax to describe that you'd like to ignore changes to the object with field .key == "transient"
  3. we will NOT (yet) support ignoring changes to a specific field of a list sub-object. E.g. for all "tag" objects, you might want to revert changes to the "key" field, but ignore changes to the "value" field. In the future however, this could potentially be supported with the syntax tags[].value, similar to how jq (and perhaps jmespath) expresses it.

Scenario 7 - server changes the type of a value:

(This is similar to other cases, but mostly here to ensure I test it)

in: { "string": "foo", "list": ["foo", "bar"], "object": {"a":"b"}, "deep_object": {"inner_object": {"c":"d"}}} out: { "string": 4, "list": "foo", "object": [1,2,3], "deep_object": {"inner_object": 100}}

Default behavior: revert Supported behavior: ignore changes - ignore_list = [ "string", "list", "object", "deep_object.inner_object" ]

Scenario 8 - Primitives as a root object:

(assuming this is even possible; I think it might not be)

Changes should be reverted - no support for ignoring

numbers: in: 5 out: 6

strings: in: "foobar" out: "foobar"

booleans: in: true out: false

Scenario 9 - ignore all changes:

Perhaps somebody has a use-case for the current behavior of ignoring all remote changes. Maybe as a workaround for unsupported cases. Could either have a second parameter, ignore_all_server_changes, or support a special case: ignore_list = [ "*" ] where including the star in the list means that all changes are ignored. I think a second param would be better.

Anyways, let me know what you think. I'm hoping to open a PR in the next day or two

michaelPotter avatar Mar 15 '23 00:03 michaelPotter