rename webhooks to be clearly identifiable as from karmada

grosser commented 2 months ago

What type of PR is this?

/kind cleanup /kind deprecation

What this PR does / why we need it:

Which issue(s) this PR fixes: Fixes https://github.com/karmada-io/karmada/issues/5231

Special notes for your reviewer:

I used app: karmada-webhook since that is where the requests are sent to, made more sense to me then declaring 2 apps that only have the hooks

Does this PR introduce a user-facing change?:

- after deploying delete the mutating-config MutatingWebhookConfiguration and validating-config ValidatingWebhookConfiguration (they were renamed)

TODO:

[ ] merge label change and rebase
[ ] add helm clenup job

karmada-bot commented 2 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign kevin-wangzefeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/karmada-io/karmada/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

codecov-commenter commented 2 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 28.26%. Comparing base (721495d) to head (a0b5364).

Files	Patch %	Lines
pkg/karmadactl/cmdinit/karmada/deploy.go	0.00%	2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #5240 +/- ## ======================================= Coverage 28.26% 28.26% ======================================= Files 632 632 Lines 43732 43732 ======================================= + Hits 12359 12361 +2 + Misses 30472 30471 -1 + Partials 901 900 -1 ``` | [Flag](https://app.codecov.io/gh/karmada-io/karmada/pull/5240/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/karmada-io/karmada/pull/5240/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | `28.26% <66.66%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

grosser commented 2 months ago

afaik helm does not delete/rename so users have to delete the old hook to avoid the hook getting called twice

XiShanYongYe-Chang commented 2 months ago

Is it possible to make the user insensible?

And are other installations affected?

grosser commented 2 months ago

we could leave webhooks with the old name that do nothing, so if a user does not see the update instructions there would be no issue
we could add a helm pre-upgrade hook, idk if that would work for all users + would be lots of effort to validate + need to figure out rbac

XiShanYongYe-Chang commented 2 months ago

Ask more guys to help take a look. cc @chaunceyjiang @whitewindmills @chaosi-zju

whitewindmills commented 2 months ago

we need to delete MutatingWebhookConfiguration/ValidatingWebhookConfiguration with the old name after the upgrade. and this cleanup is temporary and will be removed in the release v1.12+.

XiShanYongYe-Chang commented 2 months ago

we need to delete MutatingWebhookConfiguration/ValidatingWebhookConfiguration with the old name after the upgrade.

Is the delete operation performed by the user?

whitewindmills commented 2 months ago

Is the delete operation performed by the user?

no, it should be executed by karmada-operator/helm/karmadactl.

XiShanYongYe-Chang commented 2 months ago

Hi @grosser Can you help us evaluate the feasibility and follow-up plan?

grosser commented 2 months ago

I'm not deep enough in this projects deploy workflow to know how this would work. Sounds like there is a desire to fix this via helm, but I think this would not work for us since we just render helm via kustomize and then apply. My initial idea was that users would have to kubectl delete after upgrading, but I'm not sure what happens if 2 webhooks are running at the same time, I assume they would not step over each other and just work.

whitewindmills commented 2 months ago

but I'm not sure what happens if 2 webhooks are running at the same time, I assume they would not step over each other and just work.

we cannot guarantee that all webhooks are idempotent. this may cause some unexpected errors. so we need to first figure out how to delete MutatingWebhookConfiguration/ValidatingWebhookConfiguration with the old name through each installation method(karmada-operator/helm/karmadactl).

chaunceyjiang commented 2 months ago

we cannot guarantee that all webhooks are idempotent. this may cause some unexpected errors. so we need to first figure out how to delete MutatingWebhookConfiguration/ValidatingWebhookConfiguration with the old name through each installation method(karmada-operator/helm/karmadactl).

+1

grosser commented 2 months ago

So maybe best to add noop webhooks with the old names to make this safe, then the helm upgrade process is not that important and even if it goes wrong or someone does not use helm they can safely upgrade.

whitewindmills commented 2 months ago

So maybe best to add noop webhooks with the old names to make this safe

this may not be a good approach, we will eventually delete them anyway.

grosser commented 2 months ago

yeah it's not great, but it makes the transition easier, otherwise you have a a time when there are either no webhooks or 2 webhooks if we leave a noop copy then users have time to verify manually after upgrading (we can still do the whole try to delete automatically thing)

whitewindmills commented 2 months ago

otherwise you have a a time when there are either no webhooks or 2 webhooks

during the upgrade process, this is no big deal.

for helm installation, we can add a job to complete this cleanup work.
for karmada-operator/karmadactl, we can delete MutatingWebhookConfiguration/ValidatingWebhookConfiguration with the old name in the code.

this cleanup is temporary and will be removed in the release v1.12+. @grosser can you break it down into multiple tasks to track?

grosser commented 2 months ago

added todo list to PR if that is what you meant

XiShanYongYe-Chang commented 2 months ago

For users who use binary or image upgrade, is it only possible to handle it manually? Is there any specific operation guide?

Aslo invite @RainbowMango to evaluate the upgrade impact. /cc @RainbowMango

karmada-io / karmada

rename webhooks to be clearly identifiable as from karmada #5240

Codecov Report