Gracefully handle ignoreChanges without user needing to refresh a stack #2566

rquitales · 2023-09-13T21:42:00Z

Note: this is stacked on top of #2570

This PR updates the logic for handling fields added to ignoreChanges. Previously, to accurately ignore a field without hitting an SSA conflict error, the Pulumi user would need to run a refresh BEFORE running pulumi up or pulumi preview. This is due to the provider using the last known value of the ignored field stored in state when computing what should be set in the ignored field when the resource is sent to the k8s API. If drift has occurred on that field and is not reflected in the Pulumi state as it is out of date, then the provider sends old information to the API server resulting in a field conflict error.

As we hit the live server to obtain the resources' up to date resourceVersion, we can also use this API result to gracefully handle fields marked within ignoreChanges. This PR uses the fieldpath package from upstream k8s libraries to parse the serialized json managed fields entries and compares it to a normalized version of the Pulumi program's ignoreFields value.

Fixes: #2542

github-actions · 2023-09-13T21:47:57Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

codecov-commenter · 2023-09-13T22:50:02Z

Codecov Report

Merging #2566 (3a915cd) into master (d2a3b8c) will decrease coverage by 0.20%.
Report is 1 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2566      +/-   ##
==========================================
- Coverage   18.64%   18.45%   -0.20%     
==========================================
  Files          47       47              
  Lines        9434     9523      +89     
==========================================
- Hits         1759     1757       -2     
- Misses       7576     7667      +91     
  Partials       99       99

Files Changed	Coverage Δ
provider/pkg/await/await.go	`0.00% <0.00%> (ø)`
provider/pkg/provider/provider.go	`7.37% <0.00%> (-0.03%)`	⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

… kubectl-patch field manager

iwahbe

I don't have the context to review on the feature change, but I left some local comments.

provider/pkg/await/await.go

tests/sdk/nodejs/nodejs_test.go

provider/pkg/await/await.go

EronWright

Good job adding the CSA support!

t0yv0 · 2023-09-22T15:49:40Z

provider/pkg/await/await.go

@@ -498,13 +502,43 @@ func ssaUpdate(c *UpdateConfig, liveOldObj *unstructured.Unstructured, client dy
 	return currentOutputs, nil
 }

+// handleCSAIgnoreFields handles updating the inputs to use the last known value applied to the cluster. If the value is not present,
+// then we use what is declared in state as per the specs of Pulumi's ignoreChanges.
+func handleCSAIgnoreFields(c *UpdateConfig, liveOldObj *unstructured.Unstructured) error {


I've not a good intuition around liveOldObj *unstructured.Unstructur and *UpdateConfig but this looks kind of similar to what I've seen bridged providers have to do for ignoreChanges.. It makes sense with he comment 👍

t0yv0 · 2023-09-22T15:51:06Z

provider/pkg/await/await.go

+			return fmt.Errorf("unable to parse ignoreField path %q: %w", ignorePath, err)
+		}
+
+		pathComponents := strings.Split(ipParsed.String(), ".")


Are we not worried here, this conversion would just work? ipParsed will have strings or integers (to drill down arrays), for example:

"foo", 0, "barProperty"

Does strings.Split(ipParsed.String(), ".") give you what you expect for the numbers?

Will this ever receive '*' as a path fragment? Pulumi docs permit patterns like this one:

foo.*

But we're lacking great SDK API to make it easy to implement. I wonder if the user specifies that will it hit this code or be resolved higher up the stack.

t0yv0 · 2023-09-22T15:52:10Z

provider/pkg/await/await.go

+
+		pathComponents := strings.Split(ipParsed.String(), ".")
+
+		lastLiveVal, found, err := unstructured.NestedFieldCopy(liveOldObj.Object, pathComponents...)


Do paths expected by unstructured.NestedFieldCopy map 1:1 to pulumi property paths? No "foo_bar=>fooBar" renaming? No off-by-1 errors? (those may be specific to TF where pulumi paths are shorter than TF paths sometimes).

t0yv0 · 2023-09-22T15:54:41Z

provider/pkg/await/await.go

@@ -527,6 +561,7 @@ func handleSSAIgnoreFields(c *UpdateConfig, liveOldObj *unstructured.Unstructure
 			return fmt.Errorf("unable to parse ignoreField path %q: %w", ignorePath, err)
 		}

+		// TODO: Enhance support for ignoreField path to support nested arrays.


Ah here we go again. Might be worth lifting this comment and the split logic into one helper function so it's in one place.

t0yv0

This LGTM, I left some comments per Pulumi side but I lack a good intuition yet around this provider.

rquitales force-pushed the rquitales/fix-ignore-changes branch from 95350fa to d53eb7c Compare September 19, 2023 21:37

rquitales marked this pull request as ready for review September 19, 2023 21:37

rquitales changed the title ~~Test ignore_changes conflict~~ Gracefully handle ignoreChanges without user needing to refresh a stack Sep 19, 2023

rquitales force-pushed the rquitales/fix-ignore-changes branch from d53eb7c to 62fe5d5 Compare September 19, 2023 21:52

rquitales requested review from EronWright and a team September 19, 2023 21:52

rquitales added 7 commits September 19, 2023 15:16

Add testdata to test ignore_changes conflict

bc4a758

Add test logic to test ignore_changes conflict

cc5040f

Use non default value of spec.replicas=2 for ignoreChanges test

5e58aaf

Add another step to ignoreChanges test to validate proper handling of…

aed8398

… kubectl-patch field manager

Fix handling of ignoreChanges fields for SSA

36a23d7

Add error handling

3690c69

Add CHANGELOG entry

0b77bd9

rquitales force-pushed the rquitales/fix-ignore-changes branch from 62fe5d5 to 0b77bd9 Compare September 19, 2023 22:17

iwahbe reviewed Sep 20, 2023

View reviewed changes

provider/pkg/await/await.go Outdated Show resolved Hide resolved

provider/pkg/await/await.go Outdated Show resolved Hide resolved

provider/pkg/await/await.go Outdated Show resolved Hide resolved

EronWright reviewed Sep 20, 2023

View reviewed changes

Address PR comments

fb4b755

rquitales force-pushed the rquitales/fix-ignore-changes branch from fccfebe to fb4b755 Compare September 21, 2023 19:26

rquitales requested a review from EronWright September 21, 2023 19:26

EronWright reviewed Sep 21, 2023

View reviewed changes

provider/pkg/await/await.go Outdated Show resolved Hide resolved

provider/pkg/await/await.go Outdated Show resolved Hide resolved

rquitales added 2 commits September 22, 2023 10:49

Add CSA logic to use live value as well

3724b04

Add csa ignoreChanges test

3a915cd

rquitales requested a review from EronWright September 22, 2023 14:55

EronWright approved these changes Sep 22, 2023

View reviewed changes

t0yv0 reviewed Sep 22, 2023

View reviewed changes

t0yv0 self-requested a review September 22, 2023 15:55

t0yv0 approved these changes Sep 22, 2023

View reviewed changes

rquitales merged commit 8f21952 into master Sep 22, 2023
18 checks passed

rquitales deleted the rquitales/fix-ignore-changes branch September 22, 2023 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gracefully handle ignoreChanges without user needing to refresh a stack #2566

Gracefully handle ignoreChanges without user needing to refresh a stack #2566

rquitales commented Sep 13, 2023 •

edited

Loading

github-actions bot commented Sep 13, 2023

codecov-commenter commented Sep 13, 2023 •

edited

Loading

iwahbe left a comment

EronWright left a comment

t0yv0 Sep 22, 2023

t0yv0 Sep 22, 2023

t0yv0 Sep 22, 2023

t0yv0 Sep 22, 2023

t0yv0 Sep 22, 2023

t0yv0 left a comment


		pathComponents := strings.Split(ipParsed.String(), ".")

		lastLiveVal, found, err := unstructured.NestedFieldCopy(liveOldObj.Object, pathComponents...)

Gracefully handle ignoreChanges without user needing to refresh a stack #2566

Gracefully handle ignoreChanges without user needing to refresh a stack #2566

Conversation

rquitales commented Sep 13, 2023 • edited Loading

github-actions bot commented Sep 13, 2023

Does the PR have any schema changes?

codecov-commenter commented Sep 13, 2023 • edited Loading

Codecov Report

iwahbe left a comment

Choose a reason for hiding this comment

EronWright left a comment

Choose a reason for hiding this comment

t0yv0 Sep 22, 2023

Choose a reason for hiding this comment

t0yv0 Sep 22, 2023

Choose a reason for hiding this comment

t0yv0 Sep 22, 2023

Choose a reason for hiding this comment

t0yv0 Sep 22, 2023

Choose a reason for hiding this comment

t0yv0 Sep 22, 2023

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

rquitales commented Sep 13, 2023 •

edited

Loading

codecov-commenter commented Sep 13, 2023 •

edited

Loading