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.

Sign up for GitHub

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

Jump to bottom

Select webhooks to be registered #3396

Closed
wants to merge 1 commit into from
Closed

Select webhooks to be registered #3396

wants to merge 1 commit into from

Conversation

xujingjing-cmss
Copy link
Contributor

Signed-off-by: xujingjing xujingjing_yewu@cmss.chinamobile.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
Support to select webhooks which to be registered

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 29, 2021
@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign chendave after the PR has been reviewed.
You can assign the PR to them by writing /assign @chendave in a comment when ready.

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

@kubeedge-bot kubeedge-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 29, 2021
@gy95
Copy link
Member

gy95 commented Nov 29, 2021

Isn't there only one webhook right now? :)
why need to introduce this feature?

@xujingjing-cmss
Copy link
Contributor Author

Isn't there only one webhook right now? :) why need to introduce this feature?

There is two webhooks now. One is validating webhook, another is mutating webhook.

gy95
gy95 reviewed Nov 29, 2021
View reviewed changes
cloud/pkg/admissioncontroller/admission.go
@@ -224,6 +234,7 @@ func (ac *AdmissionController) registerWebhooks(opt *options.AdmissionOptions, c
},
},
}
validatingWebhookConfigurations[ValidateDeviceModelConfigName] = deviceModelCRDWebhook
Copy link
Member

Choose a reason for hiding this comment

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

yes, but for validating webhook, we only have one hook deviceModelCRDWebhook, and can we not register it?

Copy link
Member

Choose a reason for hiding this comment

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

For example, Is this command OK?
admission --webhook-configuration-names="mutate-offlinemigration"
we don't even register the deviceModelCRDWebhook webhook at all. Will this work well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ok to do that. We can only register offlineMigrationWebhook. It will work well.

gy95
gy95 reviewed Dec 1, 2021
View reviewed changes
cloud/cmd/admission/app/options/options.go Outdated
@@ -50,5 +51,6 @@ func (o *AdmissionOptions) Flags() (fss cliflag.NamedFlagSets) {
fs.Int32Var(&o.Port, "port", 443, "the port used by admission-controller-server.")
fs.StringVar(&o.AdmissionServiceNamespace, "webhook-namespace", "kubeedge", "The namespace of this webhook")
fs.StringVar(&o.AdmissionServiceName, "webhook-service-name", "kubeedge-admission-service", "The name of this admission service")
fs.StringVar(&o.WebhookConfigurationNames, "webhook-configuration-names", "all", "The name of webhook configurations selected to be used, default all,comma separated")
Copy link
Member

Choose a reason for hiding this comment

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

better give an all choice example, eg: --webhook-configuration-names="validate-devicemodel,mutate-offlinemigration"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@xujingjing-cmss xujingjing-cmss mentioned this pull request Dec 1, 2021
Closed
7 tasks
gy95
gy95 reviewed Dec 1, 2021
View reviewed changes
cloud/cmd/admission/app/options/options.go Outdated
@@ -50,5 +51,6 @@ func (o *AdmissionOptions) Flags() (fss cliflag.NamedFlagSets) {
fs.Int32Var(&o.Port, "port", 443, "the port used by admission-controller-server.")
fs.StringVar(&o.AdmissionServiceNamespace, "webhook-namespace", "kubeedge", "The namespace of this webhook")
fs.StringVar(&o.AdmissionServiceName, "webhook-service-name", "kubeedge-admission-service", "The name of this admission service")
fs.StringVar(&o.WebhookConfigurationNames, "webhook-configuration-names", "all", "Names of webhook configuration selected to be used. (default all) (eg: --webhook-configuration-names= validate-devicemodel,mutate-offlinemigration)")
Copy link
Member

Choose a reason for hiding this comment

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

--webhook-configuration-names= validate-devicemodel,mutate-offlinemigration
blank space may not need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@xujingjing-cmss
Select webhooks to be registered
6f8075f 
Signed-off-by: xujingjing <xujingjing_yw@cmss.chinamobile.com>
@gy95
Copy link
Member

gy95 commented Dec 1, 2021

/lgtm

@kubeedge-bot
Copy link
Collaborator

@gy95: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

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/test-infra repository.

@kubeedge-bot kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2021
@kubeedge-bot
Copy link
Collaborator

@xujingjing-cmss: PR needs rebase.

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/test-infra repository.

@stale
Copy link

stale bot commented Feb 16, 2022

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot closed this Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chendave chendave Awaiting requested review from chendave

@daixiang0 daixiang0 Awaiting requested review from daixiang0

@gy95 gy95 Awaiting requested review from gy95

Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@xujingjing-cmss @kubeedge-bot @gy95