Fix: DiffConfig fails when provider's kubeconfig is set to file path #2771
Conversation
EronWright
changed the title
EronWright
force-pushed
the
eronwright/issue-2663
branch
from
50d5adc
to
6ec77f8
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2771 +/- ##
==========================================
+ Coverage 23.12% 23.41% +0.28%
==========================================
Files 48 48
Lines 9620 9629 +9
==========================================
+ Hits 2225 2255 +30
+ Misses 7240 7224 -16
+ Partials 155 150 -5 ☔ View full report in Codecov by Sentry. |
| oldActiveCluster, oldFound := getActiveClusterFromConfig(oldConfig, olds) | ||
| activeCluster, found := getActiveClusterFromConfig(newConfig, news) | ||
| if !oldFound || !found { | ||
| // The config is either ambient or invalid, so we can't draw any conclusions. | ||
| } else if !reflect.DeepEqual(oldActiveCluster, activeCluster) { | ||
| // one of these properties must have changed for the active cluster to change. | ||
| for _, key := range []string{"kubeconfig", "context", "cluster"} { | ||
| if slices.Contains(diffs, key) { | ||
| replaces = append(replaces, key) | ||
| } | ||
| } |
There was a problem hiding this comment.
This change makes the replacement logic be slightly less conservative, because it now won't suggest a replacement when the old or new cluster info couldn't be resolved, e.g. when the config is ambient or the context doesn't match anything. Given that this PR fixes a problem in parseKubeconfigPropertyValue, we're more likely to reach this logic than in the past.
For example, imagine that olds["kubeconfig"] was empty and news["kubeconfig"] had the value ~/.kube/config. They're actually the same configuration, but oldFound would be false and found would be true. The provider should not do replacement in that case. Prior to this PR, DiffConfig would have returned an error before reaching this block, which the engine would have ignored and not triggered replacement.
Rather than blindly using the diffs as the replaces, it makes more sense to pick out the specific field that is causing replacement. Imagine that the diffs also included a change to an unrelated property such as suppressDeprecationWarnings.
| // and force a replacement. Note that getActiveClusterFromConfig relies on all three of the below properties. | ||
| for _, key := range []resource.PropertyKey{"kubeconfig", "context", "cluster"} { | ||
| if news[key].IsComputed() { | ||
| return &pulumirpc.DiffResponse{ | ||
| Changes: pulumirpc.DiffResponse_DIFF_SOME, | ||
| Diffs: []string{string(key)}, | ||
| Replaces: []string{string(key)}, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
This fix is to avoid a panic that otherwise occurs in getActiveClusterFromConfig if either context or cluster has a computed value. I think it is safe to be more conservative during preview since it is not determinative as to whether a resource would actually be replaced.
| } else { | ||
| return nil, fmt.Errorf("unexpected kubeconfig format, type: %v", kubeconfig.TypeString()) | ||
| } | ||
| config, err := clientcmd.Load(cfg) | ||
| config, err := parseKubeconfigString(cfg) |
There was a problem hiding this comment.
This change is the essential one that teaches DiffConfig to support a file-based kubeconfig property, by reusing the logic of Configure.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@pulumi/kubernetes](https://pulumi.com) ([source](https://togithub.com/pulumi/pulumi-kubernetes)) | dependencies | minor | [`4.7.1` -> `4.8.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.7.1/4.8.0) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>pulumi/pulumi-kubernetes (@​pulumi/kubernetes)</summary> ### [`v4.8.0`](https://togithub.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#480-February-22-2024) [Compare Source](https://togithub.com/pulumi/pulumi-kubernetes/compare/v4.7.1...v4.8.0) - Fix DiffConfig issue when when provider's kubeconfig is set to file path ([pulumi/pulumi-kubernetes#2771) - Fix for replacement having incorrect status messages ([pulumi/pulumi-kubernetes#2810) - Use output properties for await logic ([pulumi/pulumi-kubernetes#2790) - Support for metadata.generateName (CSA) ([pulumi/pulumi-kubernetes#2808) - Fix unmarshalling of Helm values yaml file ([pulumi/pulumi-kubernetes#2815) - Handle unknowns in Helm Release resource ([pulumi/pulumi-kubernetes#2822) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMTAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIxMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
Proposed changes
This PR refactors the kubeconfig loading code to be more consistent between
DiffConfigandConfigure. Previously theDiffConfigmethod did not support file-based kubeconfigs, and would throw a spurious parser error.DiffConfigloads the kubeconfig to be able to detect cluster replacement, by comparing the active cluster info. UnlikeConfigure, it doesn't have support for ambient kubeconfigs. When an ambient config is in use, or the config is invalid (e.g. thecontextorclusterproperties refer to a non-existent entry), then the replacement logic is skipped.This PR also fixes a panic in
DiffConfigthat would occur ifclusterorcontextis a computed value; the provider will conservatively suggest replacement as it does whenkubeconfigis a computed value.Testing
New test cases were added for cluster change detection.
New tests were added for the parsing code that was consolidated into
utils.go. The tests now cover file-, string-, and object-based resource property values forkubeconfig.Related issues (optional)
Closes: #2663
Related: #1032