Remove crd conversion webhook #5065

nshankar13 · 2022-09-01T18:27:10Z

Signed-off-by: nshankar13 [email protected]

Description: Resolves issue: #4967

As discussed in the issue, the crd conversion webhook currently does not act differently than how k8s would update CRDs with the conversion strategy set to None. Typically, the conversionwebhook is necessary for schema changes between CRDs on upgrades (for instance, if the field foo was renamed to the field bar). However, we can handle these scenarios separately down the road if the need arises.

Testing done: Tested upgrading from v1.0.0 to a v1.2.1-test version built from this branch and the upgrades succeeded.

OSM successfully upgraded mesh [osm] in namespace [osm-system]

Affected area:

Functional Area
New Functionality	[ ]
CI System	[ ]
CLI Tool	[ ]
Certificate Management	[ ]
Control Plane	[X]
Demo	[ ]
Documentation	[ ]
Egress	[ ]
Ingress	[ ]
Install	[ ]
Networking	[ ]
Observability	[ ]
Performance	[ ]
SMI Policy	[ ]
Security	[ ]
Sidecar Injection	[ ]
Tests	[ ]
Upgrade	[X]
Other	[ ]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project?
- Did you notify the maintainers and provide attribution?
Is this a breaking change?
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?

steeling · 2022-09-01T18:32:23Z

cmd/osm-bootstrap/osm-bootstrap.go

-	if err := crdconversion.NewConversionWebhook(ctx, crdClient, certManager, osmNamespace); err != nil {
-		events.GenericEventRecorder().FatalEvent(err, events.InitializationError, fmt.Sprintf("Error creating crd conversion webhook: %s", err))
-	}
-
 	version.SetMetric()


Can you remove the reconciler as well?

@steeling looks like we also add the reconcile label to the mutatingwebhook and validatingwebhook, should we handle the reconciler removal in a follow up PR? Or will removing the conversionwebhook while keeping the reconciler introduce bugs?

we should delete it for all. it's broken on upgrades in that an old reconciler will stomp over any changes we try to make

shashankram

Thanks for working on a fix for the upgrade issue.

I want to ensure we address a couple of things before we make a decision on completely removing the CRD conversion framework.

Does this actually fix the bug. If yes, kindly provide a repro for the issue and add a e2e test to prove this is the correct fix.
Have a proposal on how CRD schema changes are to be handled. Completely removing this will only push this problem down the road for someone else that needs to update the CRD schema to worry about. I don't believe this is the correct approach to solving this problem.
Instead of deleting the framework, add a feature flag to optionally turn it on, as it may be desirable to re-integrate this in the future. We do not know of how we can seamlessly handle CRD conversions when the API schema changes, so I am uncomfortable trading 1 bug fix without a concrete proposal to address future needs, which will arise.

Overall, I'd like to be more cautious on making such changes without a definitive way to repro and verify the fix while addressing future extensibility.

steeling · 2022-09-01T19:16:48Z

Thanks for working on a fix for the upgrade issue.

I want to ensure we address a couple of things before we make a decision on completely removing the CRD conversion framework.

Does this actually fix the bug. If yes, kindly provide a repro for the issue and add a e2e test to prove this is the correct fix.

I have a repro for this, happy to demo it, although adding a test for this would be difficult (but not impossible). I think we should move forward with the fix now, and can circle back to an E2E test in the future.

For those wondering the steps are simple:

Deploy OSM v1.0.0
Scale the bootstrap pod to 0
Upgrade to v1.2.0
The new bootstrap pod fails to initialize
Attempt to scale the old bootstrap pod back up
Both the old and new bootstrap pod cannot start, and the upgrade fails.
Rollout restart the injector and controller, and now all components are down and nothing can turn on.

Have a proposal on how CRD schema changes are to be handled. Completely removing this will only push this problem down the road for someone else that needs to update the CRD schema to worry about. I don't believe this is the correct approach to solving this problem.

There's a proposal here with the full description

Instead of deleting the framework, add a feature flag to optionally turn it on, as it may be desirable to re-integrate this in the future. We do not know of how we can seamlessly handle CRD conversions when the API schema changes, so I am uncomfortable trading 1 bug fix without a concrete proposal to address future needs, which will arise.

I disagree with adding a feature flag for this. Version control makes it trivial to bring it back if we desire, by simply reverting this PR. In the proposal I demonstrate how and why we won't need a conversion webhook, and that there are a lot of (unsafe) misconceptions about how the webhook works.

Overall, I'd like to be more cautious on making such changes without a definitive way to repro and verify the fix while addressing future extensibility.

shashankram · 2022-09-01T21:14:03Z

Thanks for working on a fix for the upgrade issue.
I want to ensure we address a couple of things before we make a decision on completely removing the CRD conversion framework.

Does this actually fix the bug. If yes, kindly provide a repro for the issue and add a e2e test to prove this is the correct fix.

I have a repro for this, happy to demo it, although adding a test for this would be difficult (but not impossible). I think we should move forward with the fix now, and can circle back to an E2E test in the future.

For those wondering the steps are simple:

Deploy OSM v1.0.0

Scale the bootstrap pod to 0

Upgrade to v1.2.0

The new bootstrap pod fails to initialize

Attempt to scale the old bootstrap pod back up

Both the old and new bootstrap pod cannot start, and the upgrade fails.

Rollout restart the injector and controller, and now all components are down and nothing can turn on.

Have a proposal on how CRD schema changes are to be handled. Completely removing this will only push this problem down the road for someone else that needs to update the CRD schema to worry about. I don't believe this is the correct approach to solving this problem.

There's a proposal here with the full description

Instead of deleting the framework, add a feature flag to optionally turn it on, as it may be desirable to re-integrate this in the future. We do not know of how we can seamlessly handle CRD conversions when the API schema changes, so I am uncomfortable trading 1 bug fix without a concrete proposal to address future needs, which will arise.

I disagree with adding a feature flag for this. Version control makes it trivial to bring it back if we desire, by simply reverting this PR. In the proposal I demonstrate how and why we won't need a conversion webhook, and that there are a lot of (unsafe) misconceptions about how the webhook works.

Overall, I'd like to be more cautious on making such changes without a definitive way to repro and verify the fix while addressing future extensibility.

@steeling Thanks for sharing your proposal, looking at it now.

Also please share a repro script and link it to the bug if possible.

Unblocking this based on offline discussions.

codecov-commenter · 2022-09-02T06:00:19Z

Codecov Report

Merging #5065 (8b06752) into main (ab69461) will increase coverage by 0.16%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5065      +/-   ##
==========================================
+ Coverage   68.83%   69.00%   +0.16%     
==========================================
  Files         214      204      -10     
  Lines       16025    15745     -280     
==========================================
- Hits        11031    10865     -166     
+ Misses       4934     4821     -113     
+ Partials       60       59       -1

Flag	Coverage Δ
unittests	`69.00% <0.00%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/osm-bootstrap/osm-bootstrap.go	`45.36% <0.00%> (-0.89%)`	⬇️
pkg/metricsstore/metricsstore.go	`92.67% <ø> (-0.23%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dockerfiles/Dockerfile.osm-crds

charts/osm/templates/osm-deployment.yaml

cmd/osm-bootstrap/crds/patch_crds.sh

cmd/osm-bootstrap/osm-bootstrap.go

Signed-off-by: nshankar13 <[email protected]>

Remove crd conversion webhook (openservicemesh#5065) Signed-off-by: nshankar13 <[email protected]>

Remove crd conversion webhook (openservicemesh#5065) Signed-off-by: nshankar13 <[email protected]> Add health handler Signed-off-by: nshankar13 <[email protected]>

* [backport] cherry-pick 68e99eb to release-v1.1 Remove crd conversion webhook (#5065) Signed-off-by: nshankar13 <[email protected]> Add health handler Signed-off-by: nshankar13 <[email protected]> * health handler Signed-off-by: nshankar13 <[email protected]> * pin linter to v1.46.0 Signed-off-by: nshankar13 <[email protected]> Signed-off-by: nshankar13 <[email protected]>

Signed-off-by: James Sturtevant <[email protected]>

nshankar13 requested review from shashankram, snehachhabria, nojnhuh, draychev, jaellio, trstringer, keithmattix and steeling as code owners September 1, 2022 18:27

steeling reviewed Sep 1, 2022

View reviewed changes

shashankram added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2022

shashankram previously requested changes Sep 1, 2022

View reviewed changes

shashankram self-requested a review September 1, 2022 21:20

nshankar13 force-pushed the remove_conversion_webhook branch 5 times, most recently from 9f4c9ea to 9e4ab55 Compare September 2, 2022 05:43

nshankar13 force-pushed the remove_conversion_webhook branch from 9e4ab55 to 278514c Compare September 2, 2022 06:07

jsturtevant reviewed Sep 2, 2022

View reviewed changes

dockerfiles/Dockerfile.osm-crds Outdated Show resolved Hide resolved

dockerfiles/Dockerfile.osm-crds Outdated Show resolved Hide resolved

nshankar13 force-pushed the remove_conversion_webhook branch from 278514c to 37c6c3a Compare September 2, 2022 17:35

steeling reviewed Sep 2, 2022

View reviewed changes

nshankar13 force-pushed the remove_conversion_webhook branch 2 times, most recently from 30c9774 to 8b06752 Compare September 2, 2022 18:11

Remove crd conversion webhook

c5ea9ed

Signed-off-by: nshankar13 <[email protected]>

nshankar13 force-pushed the remove_conversion_webhook branch from 8b06752 to c5ea9ed Compare September 2, 2022 18:31

keithmattix approved these changes Sep 2, 2022

View reviewed changes

steeling approved these changes Sep 2, 2022

View reviewed changes

steeling merged commit 68e99eb into openservicemesh:main Sep 2, 2022

shalier mentioned this pull request Sep 2, 2022

[backport] cherry-pick 68e99ebb to release-v1.2 #5069

Merged

nshankar13 added a commit to nshankar13/osm that referenced this pull request Sep 2, 2022

[backport] cherry-pick 68e99eb to release-v1.1

1d77740

Remove crd conversion webhook (openservicemesh#5065) Signed-off-by: nshankar13 <[email protected]>

nshankar13 mentioned this pull request Sep 2, 2022

[backport] cherry-pick 68e99eb to release-v1.1 #5071

Merged

keithmattix mentioned this pull request Sep 7, 2022

Error when performing upgrades on v1.0.0 #4967

Closed

jsturtevant added a commit to jsturtevant/osm that referenced this pull request Sep 7, 2022

fix crds after openservicemesh#5065

04751c5

Signed-off-by: James Sturtevant <[email protected]>

keithmattix pushed a commit that referenced this pull request Sep 8, 2022

fix crds after #5065 (#5100)

926573b

Signed-off-by: James Sturtevant <[email protected]>

whitneygriffith pushed a commit to whitneygriffith/osm that referenced this pull request Sep 12, 2022

fix crds after openservicemesh#5065 (openservicemesh#5100)

f12dc24

Signed-off-by: James Sturtevant <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove crd conversion webhook #5065

Remove crd conversion webhook #5065

nshankar13 commented Sep 1, 2022 •

edited

Loading

steeling Sep 1, 2022

nshankar13 Sep 1, 2022

steeling Sep 1, 2022

shashankram left a comment

steeling commented Sep 1, 2022 •

edited

Loading

shashankram commented Sep 1, 2022

codecov-commenter commented Sep 2, 2022 •

edited

Loading

Remove crd conversion webhook #5065

Remove crd conversion webhook #5065

Conversation

nshankar13 commented Sep 1, 2022 • edited Loading

steeling Sep 1, 2022

Choose a reason for hiding this comment

nshankar13 Sep 1, 2022

Choose a reason for hiding this comment

steeling Sep 1, 2022

Choose a reason for hiding this comment

shashankram left a comment

Choose a reason for hiding this comment

steeling commented Sep 1, 2022 • edited Loading

shashankram commented Sep 1, 2022

codecov-commenter commented Sep 2, 2022 • edited Loading

Codecov Report

nshankar13 commented Sep 1, 2022 •

edited

Loading

steeling commented Sep 1, 2022 •

edited

Loading

codecov-commenter commented Sep 2, 2022 •

edited

Loading