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

Cherry-pick #3908: Fix the requirement for VAP #3977

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Cherry-pick #3908: Fix the requirement for VAP #3977

merged 1 commit into from
Jan 17, 2025

Conversation

varshaprasad96
Copy link
Member

@varshaprasad96 varshaprasad96 commented Jan 15, 2025 •
edited by k8s-ci-robot
Loading

What type of PR is this?

Cherry-pick: #3908

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Disable the unnecessary Validating Admission Policy for the visibility server, and drop the associated RBAC permissions to make the server minimal. This also prevents periodic error logging on clusters above Kubernetes 1.29+.

@k8s-ci-robot k8s-ci-robot added this to the v0.9 milestone Jan 15, 2025
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 15, 2025
@varshaprasad96 varshaprasad96 changed the title Cherry-pick: Fix the requirement for VAP Cherry-pick #3908: Fix the requirement for VAP Jan 15, 2025
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 15, 2025
@netlify Netlify
Copy link

netlify bot commented Jan 15, 2025 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 756da95
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/678a8dcebafbfb0007cfc471

@varshaprasad96 varshaprasad96 mentioned this pull request Jan 15, 2025
Merged
@mimowo
Copy link
Contributor

mimowo commented Jan 15, 2025

It looks suspiciously many files updated compared to the original fix. Can you double check

@varshaprasad96
Copy link
Member Author

@mimowo This is a cherry pick of #3908 and #3923. Since there are additional plugins defined here in the disablePlugins slice, those packages are being vendored.

@varshaprasad96 varshaprasad96 changed the title Cherry-pick #3908: Fix the requirement for VAP Cherry-pick #3908 and #3923: Fix the requirement for VAP Jan 15, 2025
@kannon92
Copy link
Contributor

@mimowo k8s/apiserver was a dependency added for the kube-rbac deprecation. The original PR did not require adding that dependency since it was already in 0.10.

The vendor package was added due to the plugins requiring apiserver.

@bobsongplus
Copy link
Contributor

The two commit squares to one which makes confuse to review. Maybe cherry-picking two commits independently would be good.

@mimowo
Copy link
Contributor

mimowo commented Jan 16, 2025

I see, so the dependency was added here when removing the plugins from the visibility server: #3923.

In that case, I want to split the PR into two, mostly to have the commit history consistent in main, release-0.10 and release-0.9. Also, note that this PR proposes to cherry-pick to 0.9 code which has not yet been cherry-picked to 0.10. We need to first decide on cherry-picking #3923 to 0.10 before going to 0.9 directly.

@tenzen-y should we say that #3923 is part of #3496 and cherry-pick it to 0.10 and 0.9?

@mimowo
Copy link
Contributor

mimowo commented Jan 16, 2025 •
edited
Loading

OTOH, #3923 feels more like a drive-by improvement rather than part of the bugfix to #3496. So, IIUC I would suggest to only limit the cherry-pick to bugfix for #3496. #3923 seems like a nice follow up but it remains unclear to me if it fixes any practical problem. If this is the case I would suggest to just keep it in main.

@mimowo
Copy link
Contributor

mimowo commented Jan 16, 2025

/hold
to clarify the status for cherry-picking #3923

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 16, 2025 •
edited
Loading

@mimowo k8s/apiserver was a dependency added for the kube-rbac deprecation. The original PR did not require adding that dependency since it was already in 0.10.

I don't think this is accurate since #3923 hasn't been cherry-picked to 0.10 branch. I don't see the depencencies on the release-0.10 branch in the modules.txt

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 16, 2025
@varshaprasad96 varshaprasad96 changed the title Cherry-pick #3908 and #3923: Fix the requirement for VAP Cherry-pick #3908: Fix the requirement for VAP Jan 16, 2025
@varshaprasad96
Copy link
Member Author

@mimowo @bobsongplus Updated to be only the cherry-pick #3908

@varshaprasad96
Copy link
Member Author

Looking into the failures 👀

@tenzen-y
Copy link
Member

tenzen-y commented Jan 16, 2025 •
edited
Loading

OTOH, #3923 feels more like a drive-by improvement rather than part of the bugfix to #3496. So, IIUC I would suggest to only limit the cherry-pick to bugfix for #3496. #3923 seems like a nice follow up but it remains unclear to me if it fixes any practical problem. If this is the case I would suggest to just keep it in main.

+1 on Michal. IIUC, #3923 is performance improvements. So, I recommended doing that as a follow-up to the original PR.

tenzen-y
tenzen-y reviewed Jan 16, 2025
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

I believe that this could fix the CI issue.

pkg/visibility/server.go Outdated
@@ -19,6 +19,7 @@ package visibility
import (
"context"
"fmt"
validatingadmissionpolicy "k8s.io/apiserver/pkg/admission/plugin/policy/validating"
Copy link
Member

Choose a reason for hiding this comment

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

Move to the group with Line 27-32.

mimowo
mimowo reviewed Jan 17, 2025
View reviewed changes
pkg/util/cert/cert.go
@@ -37,9 +37,6 @@ const (

// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;update
// +kubebuilder:rbac:groups="admissionregistration.k8s.io",resources=mutatingwebhookconfigurations,verbs=get;list;watch;update
// +kubebuilder:rbac:groups="admissionregistration.k8s.io",resources=validatingwebhookconfigurations,verbs=get;list;watch;update
Copy link
Contributor

Choose a reason for hiding this comment

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

@varshaprasad96 this probably should not be removed, looking here: https://github.com/kubernetes-sigs/kueue/pull/3908/files#diff-d8800d9157feef2105fa59914df47a17ec8145a3242396fe47657894ac591241R40

mimowo
mimowo reviewed Jan 17, 2025
View reviewed changes
charts/kueue/templates/rbac/role.yaml Outdated
@@ -73,21 +73,11 @@ rules:
- admissionregistration.k8s.io
resources:
- mutatingwebhookconfigurations
- validatingwebhookconfigurations
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the PR https://github.com/kubernetes-sigs/kueue/pull/3908/files# this should not be removed.

mimowo
mimowo reviewed Jan 17, 2025
View reviewed changes
config/components/rbac/role.yaml Outdated
@@ -72,21 +72,11 @@ rules:
- admissionregistration.k8s.io
resources:
- mutatingwebhookconfigurations
- validatingwebhookconfigurations
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the PR https://github.com/kubernetes-sigs/kueue/pull/3908/files# this should not be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that was the issue for getting stuck at waitForKueueReady. Just pushed the changes, I think it should work now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one yet: https://github.com/kubernetes-sigs/kueue/pull/3977/files#r1920444008, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! my bad! Sorry!

@varshaprasad96
Cherry-pick kubernetes-sigs#3908: Fix the requirement for VAP
756da95 
VAP is a default admission plugin enabled while
starting an API server for visibility. The Kueue
controller has additional permissions to watch those
GVKs even though it is not required. Disabling the plugin
from api server helps in keeping it minimal and maintaining
compatibility with previous versions of K8s.

Signed-off-by: Varsha Prasad Narsing <[email protected]>
@mimowo
Copy link
Contributor

mimowo commented Jan 17, 2025

/release-note-edit

Disable the unnecessary Validating Admission Policy for the visibility server, and drop the associated RBAC permissions to make the server minimal. This also prevents periodic error logging on clusters above Kubernetes 1.29+.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 17, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 17, 2025

/hold cancel
it seems all clear at this point, we only go with the main fix.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 17, 2025

/lgtm
/approve
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7000c8db6eb8159037ffaf45199000193a7ca16f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, varshaprasad96

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2025
@k8s-ci-robot k8s-ci-robot merged commit 5c36a8a into kubernetes-sigs:release-0.9 Jan 17, 2025
15 checks passed
@mimowo
Copy link
Contributor

mimowo commented Jan 17, 2025

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenzen-y tenzen-y tenzen-y left review comments

@mimowo mimowo mimowo left review comments

@denkensk denkensk Awaiting requested review from denkensk

Assignees

@mimowo mimowo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v0.9
Development

Successfully merging this pull request may close these issues.
6 participants
@varshaprasad96 @mimowo @kannon92 @bobsongplus @tenzen-y @k8s-ci-robot