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 CRD upgrades #1819

Merged
merged 5 commits into from
Dec 8, 2021
Merged

Fix CRD upgrades #1819

merged 5 commits into from
Dec 8, 2021

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented Dec 4, 2021

Proposed changes

Previously, the provider attempted to update CRDs with the
normal PATCH workflow (equivalent to kubectl apply). This
process did not work for some CRDs.

Rather than patching the existing CRD, the provider will now
replace the old definition using a PUT operation (equivalent to
kubectl replace). This fixes the cases where the PATCH
operation failed, while still providing accurate previews and
replacement semantics.

Related issues (optional)

Fix #1293

@github-actions
Copy link

github-actions bot commented Dec 4, 2021

Does the PR have any schema changes?

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

@lblackstone lblackstone changed the title WIP: CRD replacement Fix CRD upgrades Dec 7, 2021
@lblackstone lblackstone marked this pull request as ready for review December 7, 2021 00:35
@github-actions
Copy link

github-actions bot commented Dec 7, 2021

Does the PR have any schema changes?

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

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 7, 2021

Does the PR have any schema changes?

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

Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

👍 nice getting the integration test in too

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

Does the PR have any schema changes?

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

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

Does the PR have any schema changes?

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

Previously, the provider attempted to update CRDs with the
normal PATCH workflow (equivalent to `kubectl apply`). This
process did not work for some CRDs.

Rather than patching the existing CRD, the provider will now
replace the old definition using a PUT operation (equivalent to
`kubectl replace`). This fixes the cases where the PATCH
operation failed, while still providing accurate previews and
replacement semantics.
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

Does the PR have any schema changes?

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

@@ -95,6 +101,9 @@ type ProviderArgs struct {
// BETA FEATURE - If present and set to true, enable server-side diff calculations.
// This feature is in developer preview, and is disabled by default.
EnableDryRun pulumi.BoolPtrInput
// BETA FEATURE - If present and set to true, replace CRDs on update rather than patching.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thought is to perhaps signal the replacement behavior through an annotation instead? The advantage there would be that it might be more surgical while the provider level configuration is global and will apply to all CRD resources. As discussed offline, we can consider as a subsequent feature based on user feedback.

import * as k8s from "@pulumi/kubernetes";

const provider = new k8s.Provider("k8s", {
enableReplaceCRD: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps drop this in the initial step? Would be good to have exercised the scenario of updating the provider configs as part of the update.

func TestCRDUpgrade(t *testing.T) {
test := baseOptions.With(integration.ProgramTestOptions{
Dir: filepath.Join("crd-upgrade", "step1"),
Quick: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Quick: false,
Quick: false,

Remove this since its the default anyway?

test := baseOptions.With(integration.ProgramTestOptions{
Dir: filepath.Join("crd-upgrade", "step1"),
Quick: false,
ExpectRefreshChanges: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to document what we expect to change when we set expectRefreshChanges.

@lblackstone lblackstone merged commit 054b36e into master Dec 8, 2021
@pulumi-bot pulumi-bot deleted the lblackstone/crd-replace branch December 8, 2021 23:54
@lblackstone
Copy link
Member Author

lblackstone commented Dec 8, 2021

Oh, sorry. I missed the feedback when I refreshed the page. I'll follow up on any action items in another PR.

Edit: #1827

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.

Cannot upgrade cert-manager due to errors updating the CRDs
3 participants