[Fix] Remove the requirement for VAP #3908

varshaprasad96 · 2024-12-23T19:59:26Z

What type of PR is this?

What this PR does / why we need it:

VAP is a default admission plugin enabled while starting an API server for visibility (ref). 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.

For details:
By default, the admission server enables the default set of admission plugins while starting an API server in K8s: https://github.com/kubernetes/kubernetes/blob/e85c72d4177fba224cb1baa1b5abfb5980e6d867/pkg/kubeapiserver/options/admission.go#L59. However, all the plugins are not necessary, as the visibility server is publishing metrics though read-only APIs, rather than modifying/validating the request.

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+.

netlify · 2024-12-23T19:59:47Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`81ec4ac`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/676bf3867239050008877bf9

varshaprasad96 · 2024-12-23T20:22:37Z

pkg/visibility/server.go

@@ -77,7 +78,7 @@ func applyVisibilityServerOptions(config *genericapiserver.RecommendedConfig) er
 	o.SecureServing.BindPort = 8082
 	// The directory where TLS certs will be created
 	o.SecureServing.ServerCert.CertDirectory = "/tmp"
-
+	o.Admission.DisablePlugins = []string{validatingadmissionpolicy.PluginName}


Ideally the visibility server can be minimal and any other plugins which are not required can be disabled. I believe the only plugin we would need would be NamespaceLifecycle, ServiceAccount, ResourceQuota.

That is interesting. What are your motivations for trying to get rid of only the VAP plugin?
What if when we enable only NamespaceLifecycle, ServiceAccount, and RersourceQuota?

Additionally, why do we need ResourceQuota plugin? IIUC, the visibility server doesn't touch ResourceQuta everywhere.

What are your motivations for trying to get rid of only the VAP plugin?

There are 2 benefits to this:

Latest releases of Kueue would become compatible with previous versions of K8s - (like 1.29) where VAP is not enabled by default in the admission plugin.

Currently Kueue has CRUD perms for VAP API which is not required by the controller at all. So this would reduce unnecessary privileges being given to the controller.

Additionally, why do we need ResourceQuota plugin? IIUC, the visibility server doesn't touch ResourceQuta everywhere.

Fair. Ideally it would be good to spawn the visibility server with just what's required rather than enabling all the plugins by default. If ResourceQuota isn't required we can remove it from the list too.

Nice find! Makes sense to me to enable just the features we are using.

Fair. Ideally it would be good to spawn the visibility server with just what's required rather than enabling all the plugins by default. If ResourceQuota isn't required we can remove it from the list too.

@varshaprasad96 That makes sense. Could you open an issue for that?

kannon92 · 2024-12-23T20:42:34Z

pkg/util/cert/cert.go

@@ -38,8 +38,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
-// +kubebuilder:rbac:groups="admissionregistration.k8s.io",resources=validatingadmissionpolicies,verbs=get;list;watch


Any reason why these roles were put in cert rotations?

This was added to give Kueue controller permissions when errors were coming with k8s 1.31. Turns out the root cause for it is from the API server rather than cert controller (ref).

kannon92 · 2024-12-23T20:43:56Z

/release-note-none

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]>

dgrove-oss · 2024-12-30T22:46:38Z

This should fix #3496

tenzen-y

Thank you!

/lgtm
/approve
/kind cleanup

k8s-ci-robot · 2024-12-31T20:51:35Z

LGTM label has been added.

Git tree hash: 3093dbda981fd855113fcaaa556f831f49e7c53e

k8s-ci-robot · 2024-12-31T20:51:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y, 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:

~~OWNERS~~ [tenzen-y]

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

tenzen-y · 2024-12-31T20:53:27Z

@mimowo We might be able to consider this PR as a bug fix and cherry-pick this to the release-0.10 branch since this PR tries to remove the unnecessary permissions.

WDYT?

varshaprasad96 · 2025-01-02T11:23:19Z

@tenzen-y could we back port this to v0.10.x and v0.9.x too? Both the versions would have this bug.

tenzen-y · 2025-01-03T18:25:15Z

@tenzen-y could we back port this to v0.10.x and v0.9.x too? Both the versions would have this bug.

Before we backport this to older versions, as I mentioned #3908 (comment), let me have an agreement with Michal.

mimowo · 2025-01-07T10:41:23Z

Before we backport this to older versions, as I mentioned #3908 (comment), let me have an agreement with Michal.

Thank you for investigating the issue, I'm good to cherry-pick this back to 0.9 and 0.10 as a bugfix.

mimowo · 2025-01-07T10:54:30Z

/kind bug

IIUC it also fixes #3496.

@varshaprasad96 please add a release note, and add a note "Fixes #3496" to indicate that. As for the release note maybe:
`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 Kubernetes 1.29.".

kannon92 · 2025-01-07T18:32:29Z

/cherry-pick release-0.10

k8s-infra-cherrypick-robot · 2025-01-07T18:33:08Z

@kannon92: new pull request created: #3940

In response to this:

/cherry-pick release-0.10

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.

mimowo · 2025-01-08T08:20:04Z

@kannon92 any particular reason to close the cherry-pick PR?

varshaprasad96 · 2025-01-08T09:24:06Z

@mimowo Done, thank you!

tenzen-y · 2025-01-08T20:43:14Z

/cherry-pick release-0.9
/cherry-pick release-0.10

k8s-infra-cherrypick-robot · 2025-01-08T20:43:53Z

@tenzen-y: new pull request created: #3946

In response to this:

/cherry-pick release-0.9
/cherry-pick release-0.10

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-infra-cherrypick-robot · 2025-01-08T20:44:27Z

@tenzen-y: #3908 failed to apply on top of branch "release-0.9":

Applying: Remove the requirement for VAP
Using index info to reconstruct a base tree...
M	charts/kueue/templates/rbac/role.yaml
M	config/components/rbac/role.yaml
M	pkg/visibility/server.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/visibility/server.go
Auto-merging config/components/rbac/role.yaml
CONFLICT (content): Merge conflict in config/components/rbac/role.yaml
Auto-merging charts/kueue/templates/rbac/role.yaml
CONFLICT (content): Merge conflict in charts/kueue/templates/rbac/role.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Remove the requirement for VAP

In response to this:

/cherry-pick release-0.9
/cherry-pick release-0.10

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.

tenzen-y · 2025-01-08T20:46:02Z

@varshaprasad96 Could you manually create a cherry pick PR against release-0.9 branch?

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 · 2025-01-15T14:14:43Z

@varshaprasad96 @ChristianZaccaria I see some commits on your branch, are there some unforeseen complications for cherry-picking to 0.9? Asking cause I would like to release 0.10.1 along with 0.9.2 this week.

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]>

varshaprasad96 · 2025-01-15T15:42:54Z

@tenzen-y Sorry, missed your message last week. I can create a cherry-pick PR now.

@mimowo Are you referring to these? Backporting to v0.9.x shouldn't be an issue afaik. We cherry-picked this commit and back ported it to 0.10.x in downstream because of internal release deadlines and were planning to use 0.10 directly instead of 0.9.

mimowo · 2025-01-15T16:10:00Z

Thank you! Cool, yeah I was referring to the commits as they are referenced by GH, wasn't sure why they are displayed.

varshaprasad96 · 2025-01-15T17:00:38Z

#3977 -> cherry pick of this fix in 0.9

k8s-ci-robot added 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 Dec 23, 2024

k8s-ci-robot requested review from mimowo and tenzen-y December 23, 2024 19:59

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 23, 2024

varshaprasad96 commented Dec 23, 2024

View reviewed changes

kannon92 reviewed Dec 23, 2024

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 23, 2024

varshaprasad96 force-pushed the visibility-server-minimal branch from aff35fd to 26a6b55 Compare December 24, 2024 13:15

varshaprasad96 force-pushed the visibility-server-minimal branch from 26a6b55 to 81ec4ac Compare December 25, 2024 11:59

dgrove-oss mentioned this pull request Dec 30, 2024

disabling visibilityOnDemand in Kueue blocks namespace deletion project-codeflare/appwrapper#296

Merged

tenzen-y reviewed Dec 31, 2024

View reviewed changes

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 31, 2024

k8s-ci-robot assigned tenzen-y Dec 31, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 31, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 31, 2024

k8s-ci-robot merged commit 869d003 into kubernetes-sigs:main Dec 31, 2024
16 checks passed

k8s-ci-robot added this to the v0.11 milestone Dec 31, 2024

This was referenced Jan 2, 2025

Make the visibility server lightweight #3918

Closed

[Follow-up] Remove unnecessary admission plugins from visibility server #3923

Merged

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 7, 2025

k8s-infra-cherrypick-robot mentioned this pull request Jan 7, 2025

[release-0.10] [Fix] Remove the requirement for VAP #3940

Closed

dgrove-oss mentioned this pull request Jan 8, 2025

Disabling VisibilityOnDemand feature gate blocks namespace deletion #3943

Closed

k8s-infra-cherrypick-robot mentioned this pull request Jan 8, 2025

[release-0.10] [Fix] Remove the requirement for VAP #3946

Merged

dgrove-oss mentioned this pull request Jan 13, 2025

AppWrapper integration #3953

Merged

varshaprasad96 mentioned this pull request Jan 15, 2025

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Remove the requirement for VAP #3908

[Fix] Remove the requirement for VAP #3908

varshaprasad96 commented Dec 23, 2024 •

edited

Loading

netlify bot commented Dec 23, 2024 •

edited

Loading

varshaprasad96 Dec 23, 2024

tenzen-y Dec 28, 2024

varshaprasad96 Dec 30, 2024 •

edited

Loading

dgrove-oss Dec 30, 2024

tenzen-y Dec 31, 2024 •

edited

Loading

kannon92 Dec 23, 2024

varshaprasad96 Dec 24, 2024

kannon92 commented Dec 23, 2024

dgrove-oss commented Dec 30, 2024

tenzen-y left a comment

k8s-ci-robot commented Dec 31, 2024

k8s-ci-robot commented Dec 31, 2024

tenzen-y commented Dec 31, 2024

varshaprasad96 commented Jan 2, 2025

tenzen-y commented Jan 3, 2025

mimowo commented Jan 7, 2025

mimowo commented Jan 7, 2025

kannon92 commented Jan 7, 2025

k8s-infra-cherrypick-robot commented Jan 7, 2025

mimowo commented Jan 8, 2025

varshaprasad96 commented Jan 8, 2025

tenzen-y commented Jan 8, 2025

k8s-infra-cherrypick-robot commented Jan 8, 2025

k8s-infra-cherrypick-robot commented Jan 8, 2025

tenzen-y commented Jan 8, 2025

mimowo commented Jan 15, 2025

varshaprasad96 commented Jan 15, 2025

mimowo commented Jan 15, 2025

varshaprasad96 commented Jan 15, 2025

[Fix] Remove the requirement for VAP #3908

[Fix] Remove the requirement for VAP #3908

Conversation

varshaprasad96 commented Dec 23, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

netlify bot commented Dec 23, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

varshaprasad96 Dec 23, 2024

Choose a reason for hiding this comment

tenzen-y Dec 28, 2024

Choose a reason for hiding this comment

varshaprasad96 Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

dgrove-oss Dec 30, 2024

Choose a reason for hiding this comment

tenzen-y Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

kannon92 Dec 23, 2024

Choose a reason for hiding this comment

varshaprasad96 Dec 24, 2024

Choose a reason for hiding this comment

kannon92 commented Dec 23, 2024

dgrove-oss commented Dec 30, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 31, 2024

k8s-ci-robot commented Dec 31, 2024

tenzen-y commented Dec 31, 2024

varshaprasad96 commented Jan 2, 2025

tenzen-y commented Jan 3, 2025

mimowo commented Jan 7, 2025

mimowo commented Jan 7, 2025

kannon92 commented Jan 7, 2025

k8s-infra-cherrypick-robot commented Jan 7, 2025

mimowo commented Jan 8, 2025

varshaprasad96 commented Jan 8, 2025

tenzen-y commented Jan 8, 2025

k8s-infra-cherrypick-robot commented Jan 8, 2025

k8s-infra-cherrypick-robot commented Jan 8, 2025

tenzen-y commented Jan 8, 2025

mimowo commented Jan 15, 2025

varshaprasad96 commented Jan 15, 2025

mimowo commented Jan 15, 2025

varshaprasad96 commented Jan 15, 2025

varshaprasad96 commented Dec 23, 2024 •

edited

Loading

netlify bot commented Dec 23, 2024 •

edited

Loading

varshaprasad96 Dec 30, 2024 •

edited

Loading

tenzen-y Dec 31, 2024 •

edited

Loading