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

[sdk/providers] Fix update previews #7560

Merged
merged 7 commits into from
Aug 11, 2021
Merged

[sdk/providers] Fix update previews #7560

merged 7 commits into from
Aug 11, 2021

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Jul 16, 2021

Do not return the inputs as the state for update previews that use an
unconfigured provider. Returning the inputs as the state allows the
language SDKs to incorrectly treat unknown properties as known (because
we can't call Update on an unconfigured provider, we can't know which
properties are unknown). Users can re-enable the existing behavior by
setting the PULUMI_LEGACY_PROVIDER_PREVIEW environment variable to a
truthy value (e.g. 1, true, etc.).

Most users will be unaffected by these changes. The most common programs
that may be affected are those that combine the creation of a managed
Kubernetes cluster with the deployment of applications to that cluster. These
programs generally need to configure a k8s provider instance by constructing
a kubeconfig from the output of the managed k8s cluster. Any changes to the
cluster that cause the kubeconfig to be unknown then cause the provider to
go unconfigured at runtime. Prior to these changes, resources managed by the
k8s provider would have some known outputs in this scenario, as the engine
would treat the resource's input values as its output values. After these changes,
the resource's outputs will be treated as unknown. The most frequent affect
that this has is that applies/stack outputs that depend on the outputs of
a k8s resource managed by a provider with an unknown kubeconfig will not
run/be displayed as outputs during previews, respectively.

We might be able to improve on this by taking advantage of schema
information and filling in unknown values for properties that do not
exist in the inputs.

Fixes #7521.

@pgavlin
Copy link
Member Author

pgavlin commented Jul 16, 2021

Note that this PR is essentially a work-in-progress: we'd like to take some additional time to trial the experience here and make sure that previews aren't too badly affected by this change.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

I think the approach makes sense overall

sdk/go/common/resource/plugin/provider_plugin.go Outdated Show resolved Hide resolved
@lukehoban
Copy link
Member

A couple notes from an offline conversation on this:

  1. Let's have an env var that allows opting into the previous experience, in case this has any unforeseen negative impact on outlier scenarios.
  2. Let's validate the before/after experience for 2-3 common scenarios that are impacted by this, both positively and potentially negatively. Would be great to have these real-world before/after examples in the PR description so we can review and agree that on balance this is something that is not going to cause problems for any common scenarios.

Do not return the inputs as the state for update previews that use an
unconfigured provider. Returning the inputs as the state allows the
language SDKs to incorrectly treat unknown properties as known (because
we can't call `Update` on an unconfigured provider, we can't know which
properties are unknown). Users can re-enable the existing behavior by
setting the `PULUMI_LEGACY_PROVIDER_PREVIEW` environment variable to a
truthy value (e.g. `1`, `true`, etc.).

We might be able to improve on this by taking advantage of schema
information and filling in unknown values for properties that do not
exist in the inputs.

Fixes #7521.
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

LGTM

Let's validate the before/after experience for 2-3 common scenarios that are impacted by this, both positively and potentially negatively. Would be great to have these real-world before/after examples in the PR description so we can review and agree that on balance this is something that is not going to cause problems for any common scenarios.

I'd still love to see this in the PR description - a before/after of the most common impacted scenario.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
sdk/go.sum Show resolved Hide resolved
@pgavlin
Copy link
Member Author

pgavlin commented Aug 10, 2021

I'd still love to see this in the PR description - a before/after of the most common impacted scenario.

The general scenario is in the CHANGELOG. I'll add it to the description as well.

pgavlin and others added 2 commits August 10, 2021 13:37
Co-authored-by: Luke Hoban <luke@pulumi.com>
@pgavlin pgavlin added the p1 A bug severe enough to be the next item assigned to an engineer label Aug 10, 2021
@pgavlin pgavlin merged commit 64696b4 into master Aug 11, 2021
@pulumi-bot pulumi-bot deleted the pgavlin/gh7521 branch August 11, 2021 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 A bug severe enough to be the next item assigned to an engineer
Projects
None yet
4 participants