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

feat: allow customizable [Mutating|Validating]WebhookConfiguration names #2751

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidxia
Copy link
Contributor

@davidxia davidxia commented Aug 1, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

closes #2750

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 1, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: davidxia
Once this PR has been reviewed and has the lgtm label, please assign alculquicondor 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:

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

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 1, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @davidxia. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 1, 2024
Copy link

netlify bot commented Aug 1, 2024

Deploy Preview for kubernetes-sigs-kueue ready!

Name Link
🔨 Latest commit 8e87d8b
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66abe681a366a70008a43ddf
😎 Deploy Preview https://deploy-preview-2751--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@davidxia
Copy link
Contributor Author

davidxia commented Aug 2, 2024

@denkensk @mimowo are you open to this small feature? If so, how does this implementation look?

@mimowo
Copy link
Contributor

mimowo commented Aug 2, 2024

I'm not sure, let me ask some questions to better understand the feature and your use case.

Do you want to replace the built-in webhooks with custom ones?

Which part is really important for you - disabling the built-in ones, or the ability to register extra ones?

If so, what is the issue with the built-in ones, can we extend them?

cc @tenzen-y

@davidxia
Copy link
Contributor Author

davidxia commented Aug 2, 2024

Do you want to replace the built-in webhooks with custom ones?

No, I just want to be able to specify a custom name for the Validating and MutatingWebhookConfigurations.

@mimowo
Copy link
Contributor

mimowo commented Aug 2, 2024

No, I just want to be able to specify a custom name for the Validating and MutatingWebhookConfigurations.

Ok, but then why?

Just a side note that the names are hard-coded in the helm charts see here.

@davidxia
Copy link
Contributor Author

davidxia commented Aug 2, 2024

Ok, but then why?

I'm deploying the helm charts with ArgoCD to multiple K8s clusters. ArgoCD itself runs on one K8s cluster. Each helm chart deployment is created by an ArgoCD Application K8s custom resource. These Applications need to have unique names. So I prefix them with the target K8s cluster name as a prefix. e.g. cluster1-kueue, cluster2-kueue.

Without being able to customize the webhook name, I get K8s resource name mismatches because of this cluster prefix.

@davidxia
Copy link
Contributor Author

davidxia commented Aug 2, 2024

Just a side note that the names are hard-coded in the helm charts see here.

ArgoCD is able to update the Helm chart names somehow. So I see all the K8s resource names are prefixed as I expect. It's just the hard-coded names in Golang that aren't matching.

@mimowo
Copy link
Contributor

mimowo commented Aug 2, 2024

Thanks for explaining. I need some time to digest, as I'm not quite familiar with helm charts yet.

In the meanwhile let's see if @tenzen-y or @mwielgus has some thoughts on this

@tenzen-y
Copy link
Member

tenzen-y commented Aug 5, 2024

Maybe I couldn't understand your use cases property.
Are there any reasons why you do not prefer using the ApplicationSet instead of the Application to deploy ArgoCD Applications to the multiplier clusters?

@davidxia
Copy link
Contributor Author

davidxia commented Aug 5, 2024

Maybe I couldn't understand your use cases property. Are there any reasons why you do not prefer using the ApplicationSet instead of the Application to deploy ArgoCD Applications to the multiplier clusters?

We are locked into a legacy system that uses Applications. Does ApplicationSet support what we need?

@davidxia
Copy link
Contributor Author

davidxia commented Sep 2, 2024

just bumping this PR to see if you'd be open to this change?

@davidxia
Copy link
Contributor Author

bumping again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make MutatingWebhookConfiguration and ValidatingWebhookConfiguration names configurable
4 participants