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

webhook(hypernode): validate memberSelectorType #3887

Open
wants to merge 2 commits into
base: network-topology
Choose a base branch
from
Open

webhook(hypernode): validate memberSelectorType #3887

wants to merge 2 commits into from

Conversation

Xu-Wentao
Copy link

@Xu-Wentao Xu-Wentao commented Dec 13, 2024 •
edited
Loading

Realted to: #3883

  • Add admission webhook to validate the MemberSelector fileds.
  • validate HyperNode label volcano.sh/hypernodes

@volcano-sh-bot volcano-sh-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2024
@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2024
@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch 2 times, most recently from 3f9cb67 to b619821 Compare December 13, 2024 09:02
@Xu-Wentao
Copy link
Author

This PR is based on volcano-sh/apis#144

@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch 4 times, most recently from 2a0a247 to 107dd3d Compare December 16, 2024 08:47
JesseStutler
JesseStutler reviewed Dec 16, 2024
View reviewed changes
pkg/webhooks/admission/hypernodes/validate/admit_hypernode_test.go Outdated
{
Selector: hypernodev1alpha1.MemberSelector{
Type: hypernodev1alpha1.ExactMatchMemberSelectorType,
ExactMatch: &hypernodev1alpha1.ExactMatch{Name: ""},
Copy link
Member

Choose a reason for hiding this comment

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

This case should verify that ExactMatch is nil and then return an error

Copy link
Author

Choose a reason for hiding this comment

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

yes, i'll fix it.

JesseStutler
JesseStutler reviewed Dec 16, 2024
View reviewed changes
pkg/webhooks/admission/hypernodes/validate/admit_hypernode_test.go
{
Selector: hypernodev1alpha1.MemberSelector{
Type: hypernodev1alpha1.RegexMatchMemberSelectorType,
RegexMatch: &hypernodev1alpha1.RegexMatch{
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, RegexMatch should be nil

Copy link
Author

Choose a reason for hiding this comment

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

updated, please check again. thx

@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from 107dd3d to 52f8399 Compare December 16, 2024 12:23
@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from 52f8399 to 7885fa5 Compare December 18, 2024 09:27
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign k82cn
You can assign the PR to them by writing /assign @k82cn 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

@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from 7885fa5 to 8c87ee3 Compare December 18, 2024 09:35
@Xu-Wentao Xu-Wentao changed the title [WIP] webhook(hypernode): validate memberSelectorType webhook(hypernode): validate memberSelectorType Dec 19, 2024
@volcano-sh-bot volcano-sh-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2024
@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from 8c87ee3 to 0556fec Compare December 19, 2024 09:28
@volcano-sh-bot volcano-sh-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 19, 2024
@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from 0556fec to 2449df7 Compare December 19, 2024 13:07
@volcano-sh-bot volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 19, 2024
JesseStutler
JesseStutler reviewed Dec 20, 2024
View reviewed changes
pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go Outdated
)

const (
HyperNodeLabel = "volcano.sh/hypernodes"
Copy link
Member

Choose a reason for hiding this comment

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

Also remove this code

Copy link
Author

Choose a reason for hiding this comment

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

removed

pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go Outdated
}

for _, member := range hypernode.Spec.Members {
if member.Selector == (hypernodev1alpha1.MemberSelector{}) {
Copy link
Member

Choose a reason for hiding this comment

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

We must specify one of exactMatch or regexMatch. Better to change to

if member.Selector.ExactMatch == nil && member.Selector.RegexMatch == nil {
            return fmt.Errorf("either exactMatch or regexMatch must be specified")
        }

here

Copy link
Author

Choose a reason for hiding this comment

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

updated!

@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from 2449df7 to e0670d8 Compare December 20, 2024 10:33
@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from e0670d8 to fe61a82 Compare December 20, 2024 10:35
@JesseStutler
Copy link
Member

JesseStutler commented Dec 23, 2024 •
edited
Loading

/ok-to-test

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Dec 23, 2024
@JesseStutler
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2024
Monokaix
Monokaix reviewed Dec 23, 2024
View reviewed changes
go.mod Outdated
@@ -46,7 +46,7 @@ require (
sigs.k8s.io/controller-runtime v0.13.0
sigs.k8s.io/yaml v1.4.0
stathat.com/c/consistent v1.0.0
volcano.sh/apis v1.10.0-alpha.0.0.20241016111016-bb93758bd51f
volcano.sh/apis v1.10.0-alpha.0.0.20241218081838-e5d361b6bfbe
Copy link
Member

Choose a reason for hiding this comment

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

There is a new patch in api repo, please user commit id 6ca7b0187107448a5477d08ade59dc31ac3a13a9 to update go mod.

Copy link
Author

Choose a reason for hiding this comment

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

done!

@Xu-Wentao
webhook(hypernode): validate memberSelectorType
aad5dee 
Signed-off-by: xuwentao <cutenear1993@yahoo.com>
@Xu-Wentao Xu-Wentao force-pushed the network-topology-dev branch from fe61a82 to aad5dee Compare December 25, 2024 01:38
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 25, 2024
@volcano-sh-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@Xu-Wentao Xu-Wentao requested a review from Monokaix December 25, 2024 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weapons97 weapons97 weapons97 left review comments

@huone1 huone1 Awaiting requested review from huone1

@wpeng102 wpeng102 Awaiting requested review from wpeng102

@JesseStutler JesseStutler Awaiting requested review from JesseStutler

@Monokaix Monokaix Awaiting requested review from Monokaix

Assignees

@JesseStutler JesseStutler

Labels
ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Xu-Wentao @volcano-sh-bot @JesseStutler @Monokaix @weapons97