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

Use PatchForUpdate in Diff. #604

Merged
merged 3 commits into from
Jun 20, 2019
Merged

Use PatchForUpdate in Diff. #604

merged 3 commits into from
Jun 20, 2019

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Jun 20, 2019

The Kubernetes provider does not currently use a consistent method to
compute the diffs and compute patches for update. The former only
compares old inputs vs. new inputs using a simple JSON diff; the latter
uses either a three-way strategic merge or a three-way JSON merge as
necessary. These changes unify these approaches so that diffs are
computed using the same three-way merge approach used during an update.

In addition to being consistent, this approach also produces more
correct results in the face of drift: if a Kubernetes resource is
modified out-of-band such that a property value is changed or has been
deleted with respect to the desired state, the new approach will detect
that change where it would not have before.

The Kubernetes provider does not currently use a consistent method to
compute the diffs and compute patches for update. The former only
compares old inputs vs. new inputs using a simple JSON diff; the latter
uses either a three-way strategic merge or a three-way JSON merge as
necessary. These changes unify these approaches so that diffs are
computed using the same three-way merge approach used during an update.

In addition to being consistent, this approach also produces more
correct results in the face of drift: if a Kubernetes resource is
modified out-of-band such that a property value is changed or has been
deleted with respect to the desired state, the new approach will detect
that change where it would not have before.
pkg/provider/provider.go Show resolved Hide resolved
@@ -538,18 +538,35 @@ func (k *kubeProvider) Diff(
oldInputs.SetNamespace("")
newInputs.SetNamespace("")
}
if oldInputs.GroupVersionKind().Empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we have an empty old input set, the old input set will not have a GVK. openapi.PatchForResourceUpdate looks at the old input set to determine the GVK it will use to lookup patch meta. This ensures that the old inputs have a GVK that can be used by openapi.PatchForResourceUpdate.

@pgavlin pgavlin requested a review from lblackstone June 20, 2019 22:51
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.

Changes look reasonable to me. Curious to see if this causes any test failures, and what (if any) changes in behavior are expected.

@pgavlin
Copy link
Member Author

pgavlin commented Jun 20, 2019

Curious to see if this causes any test failures

I hope these don't cause any failures--I've run these (or some variant of them with the changes from #477) through the tests locally without issue, but CI may make a liar of me.

@pgavlin
Copy link
Member Author

pgavlin commented Jun 20, 2019

what (if any) changes in behavior are expected.

As of today, the only scenario that should change is the case where a resource has had a property updated or deleted out-of-band, and those changes have been pulled in via a refresh, and some unrelated change has been made to the resource's inputs as part of a successive up. In that case, the out-of-band change will be detected in Diff where it would not have been before (because the live state is now taken into account).

Once the changes in pulumi/pulumi#2849 land, this scenario will be simpler: because all diffs will be done in resource providers, the k8s provider will detect the out-of-band change without requiring that some other change has been made to resource inputs.

This also affects the code in #477, as it gives us more flexibility with what to do in the case that a refreshed/imported resource is not annotated with its last applied configuration.

@pgavlin pgavlin merged commit c52daa9 into master Jun 20, 2019
@pulumi-bot pulumi-bot deleted the pgavlin/providerDiff branch June 20, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants