Exclude config vars from Heroku Addons in App config_vars attribute

[catsby]

In #17 we modified heroku_app to read all config vars into it's config_vars attribute. Unfortunately this includes config vars added by external services such as Heroku Addons, and as a result users would see a diff on terraform plan where Terraform wanted to remove config vars added by AddOns.

Here we query the Heroku API for the app's attached addons and gather a list of the config vars added by them, and then filter those out from the map that we use to set config_vars on the app resource.

Fixes https://github.com/terraform-providers/terraform-provider-heroku/issues/24 , includes regression test TestAccHerokuApp_AllConfigVars which will fail on current master branch.

cc @justincampbell

---

Test runs:

[22:06:07][Step 2/2] TestAccHerokuApp_importBasic
[22:06:11][Step 2/2] TestAccHerokuApp_importOrganization
[22:06:18][Step 2/2] TestAccHerokuAppFeature
[22:06:21][Step 2/2] TestAccHerokuApp_Basic
[22:06:24][Step 2/2] TestAccHerokuApp_Disappears
[22:06:30][Step 2/2] TestAccHerokuApp_Change
[22:06:37][Step 2/2] TestAccHerokuApp_NukeVars
[22:06:46][Step 2/2] TestAccHerokuApp_Buildpacks
[22:06:51][Step 2/2] TestAccHerokuApp_ExternallySetBuildpacks
[22:06:55][Step 2/2] TestAccHerokuApp_Organization
[22:14:31][Step 2/2] TestAccHerokuApp_Space
[22:14:34][Step 2/2] TestAccHerokuApp_EmptyConfigVars
[22:14:56][Step 2/2] TestAccHerokuApp_AllConfigVars

[radeksimko]

That's quite clever 👍

[radeksimko]

😭 but we're fortunate that the deprecation is not actually visible anywhere (b/c it's computed-only field) so this is practically no-op 😅

[justincampbell]

@catsby I've test this and it works! However there's one major change. Now, any additional config variables added outside of Terraform are removed. For instance, a Rails app with Sidekiq might change SIDEKIQ_CONCURRENCY as part of runtime tuning. But perhaps this should be stored in the TF state regardless, and was the original intention of #17.

/cc @banks

[catsby]

Hey @danp can you or @wchrisjohnson review this? I'm curious now if #17 was based on bad assumptions. Normally Terraform wants the configuration to be the source of truth but as this PR here demonstrates, that over steps some operational/features of Heroku such as adding env vars by Addons. What do you think?

[catsby]

as part of runtime tuning

@justincampbell can you educate me a bit on when this happens? The Rail app does this while running? Maybe #17 should just be reverted 🤔

[JamesBelchamber]

I have identified a few issues with this - environment variables created by addon attachments (from addons that are created on other apps) and labs features (like runtime-dyno-metadata) are not identified by the code, so Terraform will destroy them.

I like this code change otherwise, Terraform should manage all other environment variables (Rails apps can change environment variables, but it's debatable whether they should)

[JamesBelchamber]

There's not a way to make this work with attachments (type AddOnAttachment) and labs features (type AppFeature and AccountFeature) because they do not expose their environment variables (ConfigVars).

A possible workaround (I love this code and would rather have it than not) is to create a whitelist of variables that the user allows Heroku to manage. That would be better than the status quo of Terraform not "fixing" manually configured variables.

EDIT: to be clear, as it currently stands we could not use this code since we use addon attachments between applications, and we also use labs features. It doesn't fix the broken functionality introduced in #17 for us.

[justincampbell]

@catsby No, it's not the app changing the setting, it's an operator. I think these PRs are good, will just require a minor workflow change where we were treating some env vars as "runtime tuning" instead of "managed infrastructure settings". Another example would be GOMAXPROCS.

Would love to see this merged/released.

[justincampbell]

is to create a whitelist of variables that the user allows Heroku to manage.

I like this. Maybe it could look like:

config_vars {
  FOO = "123"
  BAR = "456"
}

ignored_config_vars = [
  # runtime-dyno-metadata
  "HEROKU_APP_ID",
  "HEROKU_APP_NAME",
  "HEROKU_DYNO_ID",
  "HEROKU_RELEASE_CREATED_AT",
  "HEROKU_RELEASE_VERSION",
  "HEROKU_SLUG_COMMIT",
  "HEROKU_SLUG_DESCRIPTION",
]

[JeremyLoy]

Speaking purely from ignorance since I haven't contributed here yet (just a user), I wonder if this 'ignored_config_vars' idea will be useful upstream later?

I'm sure Azure App Service/Elastic Beanstalk will auto-set DATABASE_URL etc in the future as they continue to polish their platforms.

[JamesBelchamber]

It's worth noting that I got in contact with Heroku support about this and they don't seem to recognise it as an issue, so we shouldn't expect these variables to be exposed any time soon.

@catsby, we should probably go ahead with a whitelist in addition to your current work, do you have time to implement it? If not I'm happy to fork and continue your work, I'm not much of a developer but I really would like this feature!

[catsby]

I'm currently on the fence about adding ignored_vars as suggested, or simply reverting #17 . Thoughts? @justincampbell / @danp

[justincampbell]

The ignored_vars list seems like a good way forward, but I'll think about it some more. Overall the handling of variables does seem better with this change.

[JamesBelchamber]

I did get some more out of Heroku - they pointed out that all Features create variables in the HEROKU_ namespace, so we could simply exclude all vars that start that way (and warn users that they can't use variables starting with HEROKU_).

On Addon Attachments, there is a way of getting the config_vars from the API (that's how the Heroku CLI does it) but it's not documented or supported (and could change at any time). So, if we could get that into heroku-go we could use it instead of an ignored_vars list, but at the risk of it breaking at any time.

[danp]

I think there is some confusion here. The HEROKU_ prefixed config that dynos see, such as HEROKU_DYNO_ID should not be showing up in config var info.

Haven't had a chance to read this over fully but not sure about having to list ignored config.

[catsby]

I have not yet seen HEROKU_ config vars in my testing/work here... the config_vars is meant to be a list of environment variables relevant the app, not just every env var on the dyno.

#17 made Terraform's handling of env vars relevant to the app more strict, but that caused a problem with add-ons like Heroku Postgres et. al. because they add DATABASE_URL and things. When those problems popped up, only those things were showing in the diff, not any HEROKU_ vars.

@JamesBelchamber can you expand on this some?

I have identified a few issues with this - environment variables created by addon attachments (from addons that are created on other apps) and labs features (like runtime-dyno-metadata) are not identified by the code, so Terraform will destroy them.

I see this now.. this could be handled with an ignore_vars attribute 🤔 .. but I wonder if the old functionality of config_vars and all_config_vars is just better. Maybe not though, maybe being able to be strict "only these vars, minus these exceptions" is a good.

Thoughts?

[JamesBelchamber]

Turn on an app feature like runtime-dyno-metadata and you'll see the new vars that get created :) I've tested and the current version definitely tries to delete them.

The old way isn't great because it doesn't really manage the resource, it just applies some config to it and keeps that in check. So if, say, I had the Kafka addon, and someone added the KAFKA_MAX_WAIT_TIME variable to the Heroku app (say, a developer), that will definitely change the configuration of Kafka and Terraform will never show that as being out of sync with the code. My expectation would be that Terraform try to remove that variable on the next apply, like with any other resource, unless I explicitly set it.

It would be better for Heroku to tell us all the variables being created by things other than Terraform, but I don't know what the appetite is for Terraform to rely on unpublished API features and a built-in whitelist of the variables created by app features (or generally ignoring the HEROKU_ namespace).

[catsby]

@JamesBelchamber yeah, that makes a lot of sense now. I'll see what an ignore_vars attribute looks like

[robertrossmann]

Another thing that could be interesting is to allow Terraform to ensure that certain env vars are present on an app, ie. by allowing config_vars to have some env var defined as DATABASE_URL = true.

That would ensure that if someone accidentally or intentionally deletes an environment variable which is required for the app then Terraform would catch that and warn the developer about that.

Otherwise I like the idea of ignore_vars.

[JamesBelchamber]

@catsby Any progress on this? The current release is broken so it would be good to get this out there 😅

[justincampbell]

I spoke with @catsby and I'll be picking this up. I'll update this PR or open a new one with the ignored_vars attribute mentioned above.

[catsby]

I apologize for dropping the ball here. Thank you @justincampbell for picking it up!

Reworking this

[justincampbell]

We're proposing to revert the changes instead, please see #36 and let us know what you think