Diff against the old live state to enable drift correction #2403
Conversation
rquitales
changed the title
There was a problem hiding this comment.
I'm wary of this change since it affects old / well-tested code, and alters the core diffing functionality. I haven't had a chance to review in-depth, but some particular things to watch out for:
- CSA and SSA diffing work differently; CSA diff used the
lastAppliedConfigurationannotation, while SSA diffing is based on apiserver responses, with a fallback to CSA during preview if the cluster is unavailable. - Live state is not always available during previews; any changes need to account for that
That said, this change may indeed fix a longstanding bug, but we need to be extra careful here.
|
|
||
| import * as k8s from "@pulumi/kubernetes"; | ||
|
|
||
| // This test creates a Provider with `enableServerSideApply` enabled. The following scenarios are tested: |
There was a problem hiding this comment.
| // This test creates a Provider with `enableServerSideApply` enabled. The following scenarios are tested: | |
| // This test creates a Provider with `enableServerSideApply` disabled. The following scenarios are tested: |
| // 3. Rerun the pulumi program and verify that the labels are restored. | ||
|
|
||
| // Create provider with SSA enabled. | ||
| const provider = new k8s.Provider("k8s"); |
There was a problem hiding this comment.
We should explicitly disable SSA here since the default will change in v4.
| assert.Contains(t, string(out), "bar") // ConfigMap should have been created with data foo: bar. | ||
|
|
||
| // Update the ConfigMap and change the data foo: bar to foo: baz. | ||
| out, err = exec.Command("kubectl", "patch", "configmap", "-n", ns, cmName, "-p", `{"data":{"foo":"baz"}}`).CombinedOutput() |
There was a problem hiding this comment.
I just checked, and kubectl patch does not update the lastAppliedConfiguration annotation, which is used for CSA diffing. See #694 for a related issue.
There was a problem hiding this comment.
Only kubectl apply attaches that annotation iirc. This bug would also be triggered by out of band resource modifications by other controllers/tools that use client-go, or other libraries.
Updates the workflow files to HEAD of ci-mgmt/$GITHUB_REF_NAME (commit dca7b52d162b32ebe8cbf2652c3822981ca7725e)
This reverts commit 9467a1e.
|
Converting this to a draft PR for now. This will require slightly more work than what is in this first PR attempt. |
The helm/v2 SDK is deprecated, and is now being removed. The helm/v3 SDK provides equivalent support, but uses the helm client library rather than shelling out to the helm CLI. The v2:Chart and v3:Chart resources are already aliased, so users can update the SDK in code without a disruptive update.
# Conflicts: # CHANGELOG.md
# Conflicts: # provider/pkg/provider/provider.go
As part of upgrading the client-go dependency, we need to drop support for Kubernetes versions older than v1.13.0 (released December 2018, EOL as of October 2019). client-go has dropped support [1] for the dry-run verifier because all cluster versions v1.13 and greater include this support. As a result, we drop the dry run check from the provider, and enforce a minimum version of v1.13. Additionally, one of the options for kustomize support moved [2], so update the calling code to match. [1] kubernetes/kubernetes#114294 [2] kubernetes-sigs/kustomize#4945
# Conflicts: # CHANGELOG.md
… fails (#2419) Server-side apply previews currently require "patch" permission to run. For cases where the user doesn't have permission to perform a "patch" operation, attempt a graceful fallback to Client-side preview. The Client-side preview may not be 100% accurate, but is preferable to failing with a permission error.
Previously, the resource properties were not represented consistently across all Pulumi SDKs. In particular, the .NET and NodeJS SDKs represented all resource output properties as required, while Go, Python, and Java represented them as optional. This inconsistency causes problems for multi-language features and documentation. This change unifies all SDKs to mark every top-level resource output as a required property.
lblackstone
force-pushed
the
rquitales/fix-drift
branch
from
16a94ce to
628a755
Compare
Does the PR have any schema changes?Found 2 breaking changes: |
lblackstone
force-pushed
the
rquitales/fix-drift
branch
from
7a4a8e9 to
ba2fc35
Compare
|
Closing in favor of #2445 |
Previously, external changes to Kubernetes resources managed by Pulumi would not be drift correctly by this provider when running
pulumi up --refresh. This is because we were diff'ing the new inputs against the old inputs. In this scenario, Pulumi would not detect any changes. This PR addresses this by diffing against the old live state when we runpulumi up.Fixes: #2404