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

Prevent unknowns in provider config #24164

Closed
wants to merge 2 commits into from
Closed

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Feb 19, 2020

Providers cannot be configured with unknown value, but we lost the check preventing these from being applied. Most providers will ignores these values and apply defaults, which leads to resources possibly be applied through an incorrectly configured provider.

Since we do allow referencing resources from provider configurations, either directly or indirectly through module inputs, we can't statically check for unknowns in the config. Instead, we need to catch them immediately before ConfigureProvider.

Fixes #24131

A provider cannot depend on anything not during during plan, in order
for it to be properly configured. Check that the provider configuration
is wholly known before calling ConfigureProvider, indicating that the
config depended on unknown values and prevent the use of a
mis-configured provider.
@jbardin jbardin requested a review from a team February 19, 2020 15:57
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Feb 19, 2020
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

As far as I remember, we've never had an explicit check for this and it's always just been a weird situation where the provider may or may not misbehave depending on how it reacts to the unknowns.

I like the idea of adding a specific check, but I worry that it will break existing workarounds made in specific providers in order to support configuration being unknown during the planning step. Specifically, some providers just stash away the configuration arguments during Configure but intentionally don't make use of them until a call to Create, Update, Delete, or Read on a particular resource, so that they can complete planning (which for the providers in question doesn't require a configured provider anyway) and then get the finalized configuration during apply to make things appear to work.

The real example I have at the front of mind is the MySQL provider, which calls connectToMySQL at the start of every apply action and whose Configure function intentionally tolerates the hostname being empty. This is used in real-world configurations in situations like the following:

resource "aws_db_instance" "example" {
  engine = "mysql"
  # ...
}

provider "mysql" {
  endpoint = aws_db_instance.example.address
  username = aws_db_instance.example.username
  password = aws_db_instance.example.password
}

resource "mysql_database" "example" {
  name = "awesomeapp"
}

Even though the provider's endpoint argument is unknown during planning, the provider code intentionally avoids making use of it until it's finally applying that mysql_database.example resource and so it all works out anyway.

The point where this stops working is if aws_db_instance.example needs to be recreated for any reason, such as if we were to taint it. Then the provider would fail trying to refresh mysql_database.example. Although that's annoying, I think in practice people don't tend to rebuild their database servers frequently and so they either never encounter it at all or work around it using -target as an exception workflow.

I think in order to move forward with this we'd need to make a plan for what to do with the situation above and circulate it among provider developers for feedback, to get a sense of what other real-world workflows we might be affecting and document a migration path for anyone depending on the current situation.

@jbardin
Copy link
Member Author

jbardin commented Feb 19, 2020

Ah yes, now it's coming back to me. I think we had this check temporarily, but this mysql example is precisely why we pulled it to consider later.

I would love to somehow validate this, as providers themselves do not handle this correctly in the majority of cases. Maybe this is something to hoist up into the future SDK, where providers must opt-in to allowing an unknown value. It wouldn't even need to be added to the schema, as the SDK could handle that validation on its own.

@jbardin
Copy link
Member Author

jbardin commented Feb 20, 2020

Having thought about this some more, I think it's acceptable to allow this pattern, but we need to make provisions for it in the SDK.

@ghost
Copy link

ghost commented Mar 22, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Providers can be configured with unknown values
2 participants