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: prevent node movement by label modification #1444

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

y-ykcir
Copy link
Member

@y-ykcir y-ykcir commented May 10, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1382

Special notes for your reviewer:

add a validation webhook for node:
user can not change apps.openyurt.io/desired-nodepool if node has been added in noodpool
user can not change apps.openyurt.io/nodepool directly, only yurt-manger can change this label

Does this PR introduce a user-facing change?


other Note

@openyurt-bot
Copy link
Collaborator

@y-ykcir: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1382

Special notes for your reviewer:

add a validation webhook for node:
user can not change apps.openyurt.io/desired-nodepool if node has been added in noodpool
user can not change apps.openyurt.io/nodepool directly, only yurt-manger can change this label

Does this PR introduce a user-facing change?


other Note

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.

@openyurt-bot openyurt-bot added the kind/feature kind/feature label May 10, 2023
@openyurt-bot openyurt-bot added the size/L size/L: 100-499 label May 10, 2023
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #1444 (e97f990) into master (7f41a78) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
+ Coverage   51.42%   51.43%   +0.01%     
==========================================
  Files         134      134              
  Lines       15947    15947              
==========================================
+ Hits         8200     8202       +2     
+ Misses       7001     7000       -1     
+ Partials      746      745       -1     
Flag Coverage Δ
unittests 51.43% <ø> (+0.01%) ⬆️

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

see 1 file with indirect coverage changes

@y-ykcir y-ykcir force-pushed the feat-nodepool-label branch from 4402c99 to 9cb1ae6 Compare May 10, 2023 08:20
@rambohe-ch
Copy link
Member

/hold

@openyurt-bot openyurt-bot added the do-not-merge/hold do-not-merge/hold label May 11, 2023
@rambohe-ch
Copy link
Member

/unhold

@openyurt-bot openyurt-bot removed the do-not-merge/hold do-not-merge/hold label May 22, 2023
@rambohe-ch
Copy link
Member

@y-ykcir please fix github action's errors.

@rambohe-ch rambohe-ch added this to the controlplane-v1.4 milestone May 22, 2023
@y-ykcir
Copy link
Member Author

y-ykcir commented May 22, 2023

@rambohe-ch It seems that in e2e testing, the nodepool, yurtappdaemon, and yurtappset tests will remove nodes from the nodepool by deleting the NodePoolLabel at the beginning and end of each test. Should we modify such behavior in e2e to use join command and is there any obstacle to do this?

@rambohe-ch
Copy link
Member

@rambohe-ch It seems that in e2e testing, the nodepool, yurtappdaemon, and yurtappset tests will remove nodes from the nodepool by deleting the NodePoolLabel at the beginning and end of each test. Should we modify such behavior in e2e to use join command and is there any obstacle to do this?

@y-ykcir would you like to have to try to improve e2e tests? but i think that yurtadm join command can not be used in kind cluster.

@rambohe-ch
Copy link
Member

@y-ykcir I think we only need to make sure apps.openyurt.io/desired-nodepool label of node can not be changed, no matter which component changes this label.

at the same time, we don't need to verify apps.openyurt.io/nodepool label, because NodePool controller will make sure apps.openyurt.io/nodepool label is equal to apps.openyurt.io/desired-nodepool label.

@y-ykcir y-ykcir force-pushed the feat-nodepool-label branch 2 times, most recently from 738a3df to 1576da0 Compare May 23, 2023 07:58
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
3.3% 3.3% Duplication

@y-ykcir
Copy link
Member Author

y-ykcir commented May 23, 2023

@rambohe-ch The code logic has been simplified. Is it possible to add reset and join way in the Kind cluster? If it's not feasible, I am currently thinking of the following handling methods:

  1. Modify to run all tests in an unchanged nodepool distribution (nodepool tests may have to change nodepool)
  2. Temporarily disable this webhook in e2e testing

@y-ykcir y-ykcir force-pushed the feat-nodepool-label branch from 1576da0 to d30e94f Compare July 12, 2023 10:54
@openyurt-bot openyurt-bot added size/XXL and removed size/L size/L: 100-499 labels Jul 12, 2023
@y-ykcir y-ykcir force-pushed the feat-nodepool-label branch from d30e94f to e97f990 Compare July 12, 2023 11:03
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.2% 1.2% Duplication

@openyurt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rambohe-ch, y-ykcir

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

The pull request process is described 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

@openyurt-bot openyurt-bot added the approved approved label Jul 13, 2023
@rambohe-ch
Copy link
Member

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request]can not move node to another nodepool by label modification
3 participants