Cleanup, sync and commit actions for Providers

[mongrelion]

Hi!

The Provider API could use a TeardownFunc method just like it offers a ConfigureFunc method.
Basically, some use cases need to make a final call to some API after the whole Terraform run is done, say, for example, in cases where the client used to communicate with the API does not support token-based authentication but instead user/password combination, and must make a call in the end to kill the session (some systems start to show issues if you leave too many connections open as the timeout is quite long and the developer may not have control whatsoever over that value).
There is another use case for this that I can think of at the moment and that is the Cobbler provider: by the end of creating a bunch of systems you need to make a call to their sync method, which you can call from when you create a resource from within Terraform but this has lead to odd behaviour as Cobbler is not thread safe and making a call to this sync method several times in a short period of time will make things look funky (cobbler/cobbler#1570 & hashicorp/terraform#5969)

Something like this is what I have in mind:

schema.Provider{
  Schema: ...,
  ResourcesMap: ...,
  ConfigureFunc: func (d *schema.ResourceData) ({}interface, error) {},
  TeardownFunc: func (meta {}interface) (error) {},
}

And the teardown method would receive the value previously returned by the ConfigureFunc.

What do you guys think?

[apparentlymart]

This seems like a reasonable idea to me!

In the core Terraform layer we have an interface ResourceProviderCloser which a provider can implement so that Close() will be called on it once it's no longer needed, but as far as I can tell the helper/schema layer doesn't actually implement it and so the real-world providers we have today (which all use the schema helper) don't currently support it.

However, I expect we could extend schema.Provider to implement a Close method in addition to its existing Configure method -- probably with a very similar implementation -- to get what you're suggesting here. We stash a pointer to the meta value inside the provider instance so it should be easy to support the interface you suggested.

I would suggest that the attribute name be CloseFunc rather than TeardownFunc, since the Close method is already named by the layer below and it would be consistent with the existing Configure/ConfigureFunc pairing.

We might consider also exposing the *schema.ResourceData for the config to CloseFunc, in case there are some settings in there that only apply on close. I'm ambivalent about it since the provider could in principle save the relevant settings within the arbitrary meta object, but for many providers meta is just a direct pointer to some object from the underlying client library and so doesn't have a place to stash extra book-keeping information like this.

[srinarayanant]

+1
Is the tear down method available in some format ? it will be ideal to login in configure and logout in teardown

[mavogel]

@apparentlymart are there any updates or ETA on this issue?
Is it as straightforward as I maybe imagine that due to Graph it is possible to determine the last resource which is created by a provider and then call the CloseFunc after the last call of this resource depending if it was as plan/apply/destroy/show? If yes I'd be keen to contribute to get more into terraform

[apparentlymart]

Oh my... this is an old issue! Sorry for the long silence here. Before I respond, I just want to be explicit that my comment above was from long before I was a HashiCorp employee, and was just participating in this project as a community contributor. Therefore when making that comment I didn't have the full context of the long- and short-term product goals.

---

We've actually ended up returning to this topic recently since we've been working on revamping the provider RPC protocol to work with the new capabilities we get from introducing the improved version of HCL, so while my above comment was long enough ago that I don't have a good sense of what I was thinking about at that time, I do have some context from our recent discussion this week.

A key problem we identified while discussing the possibility of teardown-type actions is how to handle failure. How best to handle failure depends on what sort of operation the provider is attempting to do:

• In the case of terminating a stateful session, producing an error that explains to the user how they might manually terminate their session seems like a reasonable outcome, since Terraform will create a new session on the next run anyway and so the current session is effectively abandoned.
• For the use-case of performing some sort of "commit" action things are more complex. For any operation that affects the persistent state of remote objects, we always try to retain an accurate picture in Terraform's state file of the outcome of an action, even if it fails partway, so that on the next run Terraform can attempt to retry or conclude any pending operations. If Terraform leaves some changes uncommitted without tracking that, then a subsequent Terraform run may inadvertently apply those changes even though they are not listed in the plan, which violates Terraform's primary promise.

So with that said, we'd concluded that we need to think some more about the design of a feature like this to ensure that it can be safe and explicit in its behavior. We did an initial brainstorm of an idea of making Terraform Core have an explicit representation of actions that need to be "committed" in some way, with these indicated in plan output so the user can see where in the process commit actions may occur. This builds on some other design work we've started around supporting batch requests (discussed in hashicorp/terraform#7388) but is different because our conception of batch requests was as a hidden optimization rather than as something explicitly shown to the user.

We're not able to do detailed research and prototyping on this problem at present because our current focus is on configuration language improvements, but we know there are several providers that would benefit from explicit modelling of a "deploy" or "commit" action (the Fastly provider being a key example, and hypothetical providers for various network equipment similarly) and so do intend to return to this issue at some point.

From reading the linked issues it seems like the Cobbler provider in particular has since been fixed with a workaround of manually syncing after each operation but using a mutex to avoid concurrent syncs. The Fastly provider currently works around this by managing all of the versioned API objects as a single, huge "service" resource. Neither of these approaches are ideal by any means, but hopefully similar patterns can be employed for other providers in the mean time, until we're able to devote more time to a solution in Terraform Core.

Sorry for the delay here and the long lack of response. I'll try to come back here and update this issue again when we're ready to do more in-depth work here; to help us find this again I'm going to change the summary to something more likely to come up in search when we go looking for issues on this topic.

[mavogel]

thank you @apparentlymart for your detailed answer. I was too optimistic thinking it could be that straightforward. For the Docker provider is also used a temporary workaround.

[shinmog]

I've mentioned this during the Palo Alto Networks panos provider validation process, but we will eventually need commit type functionality for our provider as well. When you guys circle back to this issue, we would definitely like to be involved so we can explain our use case in more depth.

[jonshern]

Yeah we are using the Pan OS Provider for Palo Alto and this functionality would be very important to be able to fully automate

[phamann]

The Dyn provider would also benefit from such functionality. The Dynect API is stateful and uses a session token which the provider currently creates during the ConfigureFunc by calling the Dyn go clients login method. However, the APIs logout method is never called once the provider has finished thus leaving open sessions in the Dyn API. This causes unforeseen side-effects, for example if you run multiple Terraform runs in CI environments.

[paultyng]

The protocol has a Close which we plan to expose in the future. That being said, this will strictly be for cleanup, session logout, etc, not for commit type operations. Terraform's state model doesn't have any concept of a "commit" or what resources are committed, and as far as I know the plans to add this are not on the roadmap. If you need to support a commit model, there are potential resource design changes you could make to make it work, and I'm happy to discuss those on specific providers, but without a significant change to Terraform upstream, I don't think this will be added anytime soon.