Deny pod admission for static pods referencing API objects #131837

sreeram-venkitesh · 2025-05-18T15:24:02Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Currently, mirror pod reconciliation for static pods which reference API objects will fail. However the pod itself is not denied admission and the node would be silently running the static pod's container. As discussed in #103587 (comment), we want to tighten the validation here so that pod would not be running at all in the node. Currently the static pod would be running in the node while the mirror pod would not be created (which is enforced in the API as per #103587 (comment)). This PR updates the validation logic so that the pod is denied admission for static pods referencing API objects and make sure that the container is not created in the node.

Which issue(s) this PR fixes:

Fixes #103587

Special notes for your reviewer:

Notes:

I've added a new StaticPodAdmitHandler so that this validation is done from canAdmitPod. This is based on @gjkim42's comment Fail to create static pod referencing ServiceAccount #105368 (comment).
The structure of the handler and the tests were inspired from the PredicateAdmitHandler in pkg/kubelet/lifecycle/predicate.go
Only hostPath and emptyDir volume mounts are made valid for static pods based on @enj's comment: Kubelet should enforce that static pods do not reference other API objects #103587 (comment)
I've added a StaticPodStrictValidation feature gate with which we can toggle the changes to the validation.

Question:

I wanted to add an e2e_node test. I was going through test/e2e_node/mirror_pod_test.go since it has helpers for static pod creation etc, but I'm still figuring out the failure case where we need to assert that the pod was denied admission from the kubelet logs?

Does this PR introduce a user-facing change?

Static pods that reference API objects are now denied admission by the kubelet so that static pods would not be silently running even after the mirror pod creation fails.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

(The feature gate should be documented. I will update the docs PR here once opened.)

sreeram-venkitesh · 2025-05-18T15:26:08Z

CC @haircommander @liggitt Tagging from the discussion in the original issue. I'd love to hear what you folks think! Thanks!

sreeram-venkitesh · 2025-05-18T16:12:53Z

/retest

sreeram-venkitesh · 2025-05-18T17:57:44Z

/retest

lmktfy

Thanks for the PR.

Should we also deny static Pods that specify .spec.resources.claims and / or .spec.resourceClaims?

(We perhaps shouldn't assume that these claims will fail anyway).

pkg/kubelet/lifecycle/handlers.go

sreeram-venkitesh · 2025-05-19T15:54:27Z

(We perhaps shouldn't assume that these claims will fail anyway).

True, but if they don't make sense for static pods I'm okay with denying pods with .spec.resources.claims and / or .spec.resourceClaims too. I wonder if we'd want to let users create static pods with resource claims.

sreeram-venkitesh · 2025-05-19T15:54:59Z

/retest

liggitt · 2025-05-20T14:58:23Z

pkg/features/kube_features.go

+	// owner: @sreeram-venkitesh
+	//
+	// Denies pod admission if static pods reference other API objects.
+	StaticPodStrictValidation featuregate.Feature = "StaticPodStrictValidation"


"strict" is pretty ambiguous ... let's target this specifically at static pods referencing API objects, so call this "PreventStaticPodAPIReferences" or something similar.

That will also let us make use of the same gate in the kubelet and in NodeRestriction admission

liggitt · 2025-05-20T14:58:55Z

pkg/features/kube_features.go

@@ -1783,6 +1788,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate
 		{Version: version.MustParse("1.31"), Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.31, remove in 1.33
 	},

+	StaticPodStrictValidation: {
+		{Version: version.MustParse("1.34"), Default: false, PreRelease: featuregate.Alpha},


I would consider starting this as beta and on-by-default ... this is a bugfix with an escape hatch mitigation for a few releases

Understood. Fixed this in 8f86d9d.

liggitt · 2025-05-20T15:00:11Z

test/e2e/feature/feature.go

@@ -445,6 +445,10 @@ var (
 	// TODO: document the feature (owning SIG, when to use this feature for a test)
 	StatefulSet = framework.WithFeature(framework.ValidFeatures.Add("StatefulSet"))

+	// Owner: sig-node
+	// Denies pod admission if static pods reference other API objects.
+	StaticPodStrictValidation = framework.WithFeature(framework.ValidFeatures.Add("StaticPodStrictValidation"))


we don't need an e2e feature for this, there's no other configuration associated with it, if you write an e2e test, use framework.WithFeatureGate(features.PreventStaticPodAPIReferences), etc

liggitt · 2025-05-20T15:07:07Z

pkg/kubelet/lifecycle/handlers.go

+	if !kubetypes.IsStaticPod(pod) {
+		return PodAdmitResult{Admit: true}
+	}
+


I think a single method that takes a pod and returns an error if the pod references API objects will be easier to ensure is called correctly. It might make sense to extract the checks done at

kubernetes/plugin/pkg/admission/noderestriction/admission.go

Lines 307 to 340 in 889cd83

// don't allow a node to create a pod that references any other API objects

if pod.Spec.ServiceAccountName != "" {

return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference a service account", nodeName))

}

hasSecrets := false

podutil.VisitPodSecretNames(pod, func(name string) (shouldContinue bool) { hasSecrets = true; return false }, podutil.AllContainers)

if hasSecrets {

return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference secrets", nodeName))

}

hasConfigMaps := false

podutil.VisitPodConfigmapNames(pod, func(name string) (shouldContinue bool) { hasConfigMaps = true; return false }, podutil.AllContainers)

if hasConfigMaps {

return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference configmaps", nodeName))

}

for _, vol := range pod.Spec.Volumes {

if vol.VolumeSource.Projected != nil {

for _, src := range vol.VolumeSource.Projected.Sources {

if src.ClusterTrustBundle != nil {

return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference clustertrustbundles", nodeName))

}

}

}

}

for _, v := range pod.Spec.Volumes {

if v.PersistentVolumeClaim != nil {

return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference persistentvolumeclaims", nodeName))

}

}

if len(pod.Spec.ResourceClaims) > 0 {

return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference resourceclaims", nodeName))

}

to a method in pkg/api/pod that deals with an internal Pod.

Then the NodeRestriction admission plugin could call that, and the kubelet could call it from tryDecodeSinglePod / tryDecodePodList where it has an internal pod, and only hits that code path when loading static pods.

@liggitt Please check 4809d0f. I've extracted the logic to a method in pkg/api/pod/util.go. This is then called from both NodeRestriction admission's admitPodCreate and tryDecodeSinglePod for static pods.

I have a question about tryDecodeSinglePod. Does this only process static pods? I see that this is called from pkg/kubelet/config/file.go and pkg/kubelet/config/http.go. The file one makes sense - reading static pod manifests, but I was not sure about the tryDecodeSinglePod call here in extractFromURL in pkg/kubelet/config/http.go. generatePodName also tells me this is static pod specific.

One more question: Compared to my earlier approach of adding a new AdmitHandler, this logic would now be executed only when parsing static pods. The AdmitHandler approach is redundant because,it would process all pods and then inside the handler check if the pod is a static pod. But we can avoid checking all the pods. Am I correct?

The kubelet can load static pods from file or from a non-API server URL. Both of those should have this enforcement.

doing the enforcement there would be instead of a kubelet admission handler

Ah yes, I forgot that we can specify --manifest-url too. Thanks!

liggitt · 2025-05-21T13:31:03Z

pkg/api/pod/util.go

+	ErrStaticPodResourceClaims      = errors.New("can not create static pods that reference resourceclaims")
+)
+
+func HasAPIObjectReferences(pod *api.Pod) (bool, error) {


document what this does, something like "returns true if the pod spec references any API objects, along with the plural resource of the first reference encountered"

I would make this return a string resource name, rather than do the error wrapping here and exposing error variables

// HasAPIObjectReference returns true if a reference to an API object is found in the pod spec, // along with the plural resource of the referenced API type, or an error if an unknown field is encountered. func HasAPIObjectReference(pod *api.Pod) (bool, string, error)

liggitt · 2025-06-16T13:50:27Z

/retest

pkg/api/pod/util.go

pkg/kubelet/config/common.go

pkg/api/pod/util.go

sreeram-venkitesh · 2025-06-25T18:16:37Z

@liggitt Can I squash and clean up the commits here?

liggitt · 2025-06-25T18:21:47Z

yup, looks good to squash, thanks!

… referencing API objects

sreeram-venkitesh · 2025-06-26T01:17:30Z

/retest

liggitt · 2025-06-26T12:36:59Z

/lgtm
/approve

k8s-ci-robot · 2025-06-26T12:37:08Z

LGTM label has been added.

Git tree hash: 92ca3dde1f4a5d2a04d90c55b28d3e32557f904f

k8s-ci-robot · 2025-06-26T12:37:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, sreeram-venkitesh

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:

~~pkg/api/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [liggitt]
~~pkg/kubelet/OWNERS~~ [liggitt]
~~plugin/pkg/admission/noderestriction/OWNERS~~ [liggitt]
~~test/compatibility_lifecycle/reference/OWNERS~~ [liggitt]

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

k8s-triage-robot · 2025-06-26T16:03:49Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

sreeram-venkitesh · 2025-06-26T18:11:14Z

/retest

github-project-automation bot added this to SIG Node CI/Test Board May 18, 2025

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label May 18, 2025

github-project-automation bot moved this to Triage in SIG Node CI/Test Board May 18, 2025

k8s-ci-robot requested review from feiskyer and jsturtevant May 18, 2025 15:24

lmktfy reviewed May 18, 2025

View reviewed changes

pkg/kubelet/lifecycle/handlers.go Outdated Show resolved Hide resolved

liggitt reviewed May 20, 2025

View reviewed changes

sreeram-venkitesh force-pushed the static-pod-strict-validation-for-api-object-reference branch from 8f86d9d to 4809d0f Compare May 21, 2025 07:24

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label May 21, 2025

github-project-automation bot added this to SIG Auth May 21, 2025

sreeram-venkitesh force-pushed the static-pod-strict-validation-for-api-object-reference branch from dfad17a to aab9a2f Compare May 21, 2025 09:20

liggitt reviewed May 21, 2025

View reviewed changes

github-project-automation bot added this to SIG Node: code and documentation PRs Jun 6, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Jun 6, 2025

bart0sh moved this from Triage to not-only-sig-node in SIG Node: code and documentation PRs Jun 10, 2025

sreeram-venkitesh force-pushed the static-pod-strict-validation-for-api-object-reference branch from 0c512ae to c05007e Compare June 11, 2025 17:25

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 11, 2025

sreeram-venkitesh force-pushed the static-pod-strict-validation-for-api-object-reference branch from c05007e to bedcb38 Compare June 12, 2025 17:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2025

SergeyKanzhelev moved this from not-only-sig-node to Waiting on Author in SIG Node: code and documentation PRs Jun 13, 2025

liggitt reviewed Jun 16, 2025

View reviewed changes

pkg/api/pod/util.go Outdated Show resolved Hide resolved

pkg/kubelet/config/common.go Show resolved Hide resolved

pkg/api/pod/util.go Outdated Show resolved Hide resolved

sreeram-venkitesh added 2 commits June 25, 2025 23:59

Added podutil.HasAPIObjectReference to deny admission for static pods…

5390f75

… referencing API objects

Added unit tests

f9a5aec

sreeram-venkitesh force-pushed the static-pod-strict-validation-for-api-object-reference branch from c92f30a to f9a5aec Compare June 25, 2025 18:52

k8s-ci-robot assigned liggitt Jun 26, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2025

k8s-ci-robot merged commit b7c9333 into kubernetes:master Jun 26, 2025
16 checks passed

k8s-ci-robot added this to the v1.34 milestone Jun 26, 2025

github-project-automation bot moved this from Archive-it to Done in SIG Node CI/Test Board Jun 26, 2025

github-project-automation bot moved this from In Review to Closed / Done in SIG Auth Jun 26, 2025

github-project-automation bot moved this from Waiting on Author to Done in SIG Node: code and documentation PRs Jun 26, 2025

	// don't allow a node to create a pod that references any other API objects
	if pod.Spec.ServiceAccountName != "" {
	return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference a service account", nodeName))
	}
	hasSecrets := false
	podutil.VisitPodSecretNames(pod, func(name string) (shouldContinue bool) { hasSecrets = true; return false }, podutil.AllContainers)
	if hasSecrets {
	return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference secrets", nodeName))
	}
	hasConfigMaps := false
	podutil.VisitPodConfigmapNames(pod, func(name string) (shouldContinue bool) { hasConfigMaps = true; return false }, podutil.AllContainers)
	if hasConfigMaps {
	return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference configmaps", nodeName))
	}

	for _, vol := range pod.Spec.Volumes {
	if vol.VolumeSource.Projected != nil {
	for _, src := range vol.VolumeSource.Projected.Sources {
	if src.ClusterTrustBundle != nil {
	return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference clustertrustbundles", nodeName))
	}
	}
	}
	}

	for _, v := range pod.Spec.Volumes {
	if v.PersistentVolumeClaim != nil {
	return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference persistentvolumeclaims", nodeName))
	}
	}

	if len(pod.Spec.ResourceClaims) > 0 {
	return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference resourceclaims", nodeName))
	}

Deny pod admission for static pods referencing API objects #131837

Deny pod admission for static pods referencing API objects #131837

Conversation

sreeram-venkitesh commented May 18, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

sreeram-venkitesh commented May 18, 2025

Uh oh!

sreeram-venkitesh commented May 18, 2025

Uh oh!

sreeram-venkitesh commented May 18, 2025

Uh oh!

lmktfy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sreeram-venkitesh commented May 19, 2025

Uh oh!

sreeram-venkitesh commented May 19, 2025

Uh oh!

liggitt May 20, 2025

Choose a reason for hiding this comment

Uh oh!

liggitt May 20, 2025

Choose a reason for hiding this comment

Uh oh!

sreeram-venkitesh May 20, 2025

Choose a reason for hiding this comment

Uh oh!

liggitt May 20, 2025

Choose a reason for hiding this comment

Uh oh!

liggitt May 20, 2025

Choose a reason for hiding this comment

Uh oh!

sreeram-venkitesh May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt May 21, 2025

Choose a reason for hiding this comment

Uh oh!

sreeram-venkitesh May 21, 2025

Choose a reason for hiding this comment

Uh oh!

liggitt May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jun 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sreeram-venkitesh commented Jun 25, 2025

Uh oh!

liggitt commented Jun 25, 2025

Uh oh!

sreeram-venkitesh commented Jun 26, 2025

Uh oh!

liggitt commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

k8s-triage-robot commented Jun 26, 2025

Uh oh!

sreeram-venkitesh commented Jun 26, 2025

Uh oh!

Uh oh!

Uh oh!

sreeram-venkitesh May 21, 2025 •

edited

Loading

liggitt May 21, 2025 •

edited

Loading