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

Fix previews in SSA mode when there are replacements due to immutable fields #3053

Merged
merged 7 commits into from
Jun 26, 2024

Conversation

rquitales
Copy link
Member

Proposed changes

This PR adds special handling for previews with SSA when we have a replacement (delete before create) operation. There is no easy way to tell the API server that the dry-run to be run is an object replacement. As such, a workaround that we can do is to send a different object name so the API server thinks we are creating a new object, so a already exists type error doesn't occur.

Note, because of how SSA creates work currently with upserts, we'll never face the AlreadyExists error, so we do not have to account for that in this PR.

As an aside, kubectl also supports delete before create replacements with the replace sub-command. A user would need to run kubectl replace --force -f . to recreate any resources that update an immutable field. kubectl does not support -dry-run=server with the --force flag and the follow error is shown --dry-run can not be used when --force is set since the API server does not support previewing this type of operation.

Related issues (optional)

Fixes: #2575

Copy link

Does the PR have any schema changes?

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

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 36.60%. Comparing base (deae4dc) to head (d0486ed).

Files Patch % Lines
provider/pkg/await/await.go 10.00% 8 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3053      +/-   ##
==========================================
- Coverage   36.65%   36.60%   -0.06%     
==========================================
  Files          71       71              
  Lines        9249     9259      +10     
==========================================
- Hits         3390     3389       -1     
- Misses       5522     5531       +9     
- Partials      337      339       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rquitales rquitales requested review from blampe and EronWright June 10, 2024 22:06
@rquitales rquitales force-pushed the rquitales/fix-replace-ssa-previews branch from 2ababb9 to 10468a9 Compare June 17, 2024 21:25
Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Reasonable! It's too bad we can't know upfront whether we're about to replace or update. Another consequence of the engine not expecting upserts.

provider/pkg/await/await.go Outdated Show resolved Hide resolved
@rquitales rquitales force-pushed the rquitales/fix-replace-ssa-previews branch from 10468a9 to 23e3361 Compare June 25, 2024 18:21
@rquitales rquitales force-pushed the rquitales/fix-replace-ssa-previews branch from 23e3361 to 90da69e Compare June 25, 2024 18:25
Copy link
Contributor

@EronWright EronWright left a comment

Choose a reason for hiding this comment

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

I like this because it is targeted. Down the road I would advocate for simply using client-side validation during previews, to avoid the problems inherent to server-side dry-run such as missing CRDs, missing namespaces, missing admission webhooks, inability to preview a replacement, etc.

provider/pkg/await/await.go Outdated Show resolved Hide resolved
@rquitales rquitales force-pushed the rquitales/fix-replace-ssa-previews branch from 09bdf7a to 24c4a0e Compare June 25, 2024 21:41
@rquitales rquitales enabled auto-merge (squash) June 25, 2024 21:42
@rquitales rquitales disabled auto-merge June 26, 2024 21:01
@rquitales rquitales enabled auto-merge (squash) June 26, 2024 21:03
@rquitales rquitales merged commit d418c9a into master Jun 26, 2024
19 checks passed
@rquitales rquitales deleted the rquitales/fix-replace-ssa-previews branch June 26, 2024 22:07
lumiere-bot bot referenced this pull request in coolguy1771/home-ops Jun 29, 2024
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.13.1` ->
`4.14.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.13.1/4.14.0)
|

---

### Release Notes

<details>
<summary>pulumi/pulumi-kubernetes (@&#8203;pulumi/kubernetes)</summary>

###
[`v4.14.0`](https://togithub.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#4140-June-28-2024)

[Compare
Source](https://togithub.com/pulumi/pulumi-kubernetes/compare/v4.13.1...v4.14.0)

##### Added

- `TypedDict` input types for the Python SDK
([https://github.com/pulumi/pulumi-kubernetes/pull/3070](https://togithub.com/pulumi/pulumi-kubernetes/pull/3070))

##### Changed

- The `Release` resource no longer ignores empty lists when merging
values.
([https://github.com/pulumi/pulumi-kubernetes/pull/2995](https://togithub.com/pulumi/pulumi-kubernetes/pull/2995))

##### Fixed

- `Chart` v4 now handles an array of assets.
([https://github.com/pulumi/pulumi-kubernetes/pull/3061](https://togithub.com/pulumi/pulumi-kubernetes/pull/3061))
- Fix previews always failing when a resource is to be replaced
([https://github.com/pulumi/pulumi-kubernetes/pull/3053](https://togithub.com/pulumi/pulumi-kubernetes/pull/3053))

</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:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjAuMSIsInVwZGF0ZWRJblZlciI6IjM3LjQyMC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJ0eXBlL21pbm9yIl19-->

Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v4.14.0.

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.

replaceOnChange doesn't work for immutable Fields
4 participants