rename webhooks to be clearly identifiable as from karmada

[grosser]

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]

[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

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[codecov-commenter]

:warning: Please install the 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

@@           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	Coverage Δ
unittests	28.26% <66.66%> (+<0.01%)	:arrow_up:

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

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

[grosser]

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

[XiShanYongYe-Chang]

Is it possible to make the user insensible?

And are other installations affected?

[grosser]

• 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]

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

[whitewindmills]

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]

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

Is the delete operation performed by the user?

[whitewindmills]

Is the delete operation performed by the user?

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

[XiShanYongYe-Chang]

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

[grosser]

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]

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]

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]

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]

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]

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]

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]

added todo list to PR if that is what you meant

[XiShanYongYe-Chang]

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

[RainbowMango]

First, let's revisit the origin issue, according to #5231, what we want to solve is the configuration file naming problem, not a functional issue. We have already mitigated its impact in #5246. The next step is to handle the upgrade concerns properly.

For that, we need a clear plan to move forward, including the following:

• upgrade guidance
• consider the difference between all kind of installation tools(karmadactl init/operator/helm) in the guidance

Any thoughts? And anyone willing to take on this task?

[grosser]

I think it's not worth the effort 😞