Add PSA to block host field in probe/lifecycle handlers

[tssurya]

• One-line PR description: Add PSA to block host field in probe/lifecycle handlers

• Issue link: Add PSA to block setting .host field from ProbeHandler and LifecycleHandler #4940

• Other comments:

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[aojea]

not familiar with the mechanics of this admission plugin, the trickiest part seems to handle the roll out of the feature, as some pods will be impacted by the new policy on upgrade. Things like a rolling update on a deployment comes to mind, where valid pods will created and will be impacted, despite the existing ones running will not.

From the network perspective +1 from me, most people should not use the host field, although there are case that is used for polling external endpoints and now it can not be removed without breaking existing workloads, but seems correct to warn these users about the risk they are running by doing that.

cc: @tallclair @liggitt

[tssurya]

I have 3 more sections to update (unit, e2e, integration based on test grid links which since I was traveling was hard to load and access, will update those sections asap).. opening this up for reviews meanwhile

@deads2k PTAL if this is doable for 1.33 ?

[ibihim]

@tssurya, @deads2k, we are passed enhancement freeze v1.33. What is your plan for this?

[tallclair]

not familiar with the mechanics of this admission plugin, the trickiest part seems to handle the roll out of the feature, as some pods will be impacted by the new policy on upgrade. Things like a rolling update on a deployment comes to mind, where valid pods will created and will be impacted, despite the existing ones running will not.

pod security admission has a couple features to help with this. policies are versioned, so an admin can decouple policy upgrade from cluster upgrade. You can also dry-run policy changes against workload resources. https://kubernetes.io/docs/tasks/configure-pod-container/enforce-standards-namespace-labels/ has some of the details, but we should probably add task documentation specifically for the upgrade scenario.

[tssurya]

@tssurya, @deads2k, we are passed enhancement freeze v1.33. What is your plan for this?

I had spoken with @deads2k .. IIUC I should send an email in early part of the release cycle first and get some reviews which I had missed... So we are targeting this for next release and as soon as the window opens I plan to send an email to mailing list for reviews once I update this KEP to a recent state.

[tssurya]

@tssurya, @deads2k, we are passed enhancement freeze v1.33. What is your plan for this?

I had spoken with @deads2k .. IIUC I should send an email in early part of the release cycle first and get some reviews which I had missed... So we are targeting this for next release and as soon as the window opens I plan to send an email to mailing list for reviews once I update this KEP to a recent state.

Has the 1.34 KEP window opened? https://kubernetes.slack.com/archives/C0EN96KUY/p1745693668074409?thread_ts=1745431405.777719&cid=C0EN96KUY

[BenTheElder]

For deprecating the .Host field we should probably be reaching out to SIG projects that use it, there is at least one which appears to be used in production clusters (kops):

https://cs.k8s.io/?q=HTTPGetAction&i=fosho&literal=nope&files=&excludeFiles=%5Evendor&repos=kubernetes/kops

https://github.com/kubernetes/kops/blob/5dd2f468b46fda43f3a63ba1e6dc7c55c21919eb/nodeup/pkg/model/kube_apiserver.go#L603

[danwinship]

So they're deploying the apiserver as a host-network pod and specifying to use host: 127.0.0.1 for the probes, presumably because there may be a firewall blocking access to the healthz port on the public node IP. That seems reasonable. (Ah, actually, there are examples of this in k/k/cluster/ too.)

We could have the semantics of the deprecation/PSA be "you aren't allowed to specify host except in the case of specifying 127.0.0.1 or ::1 for a hostNetwork: true pod...

[tssurya]

For deprecating the .Host field we should probably be reaching out to SIG projects that use it, 

that's a good idea..

We could have the semantics of the deprecation/PSA be "you aren't allowed to specify host except in the case of specifying 127.0.0.1 or ::1 for a hostNetwork: true pod...

hmm we could but its still a bit mucky, i.e we either deprecate the field or we don't right? Is it OK to deprecate with exceptions :D

[BenTheElder]

I'm honestly not sure what the most reasonable option is here. cc @aojea @thockin @liggitt

[deads2k]

Deprecation doesn't mean broken and doesn't mean removed. My understanding of this change is

1. the meaning of the .Host field is remaining the same
2. the resource validation on the .Host field is remaining the same
3. PodSecurityAdmission with both version locked and intent driven evaluation modes will enforce a more accurate restriction based on the actual impact of the field. Version locked namespaces have no change. Intent-based namespaces (latest) will get the most accurate enforcement

I don't see documenting the field as deprecated as critical to this change. I'm fine with this KEP without any documentation changes to the field. Marking it deprecated seems more appropriate to do in the KEP that is adding its replacement once it reaches stable.

I suggest making this part optional in the KEP and removing from goals. The documentation should still be updated to explain that usage is privileged.

[tssurya]

ack I think for now, as mentioned by @deads2k I'm removing the deprecation from the enhancement scope.

[tssurya]

thanks for the reviews (just got back from PTO this week, will address these comments asap.

[tssurya]

I've resolved most of the comments from @BenTheElder and @liggitt : Diff for the new pushes can be seen here: https://github.com/kubernetes/enhancements/compare/8d4b17b02bb6725ebf8620657f208216c62e7627..02514fb6fb222958dbd7481cb44a5768dc74d1fe so that its easier to see what changed.

Only remaining one is - I think #4942 (comment) is a good point, however I'm not sure if deprecating with exception like that is common practice?

[deads2k]

Please fill this out with the impact to a workload resource (deployment) trying to create new pods with a PSA level set to latest after an upgrade.

[tssurya]

I've updated this to include this info.. on second thoughts I have mentioned this in L246 which probably now needs a tweak? cause on upgrades all pods get recreated so if encountered with the PSA set they would fail? -> updated that as well PTAL. cause existing pods if drained and brought up again during upgrades if PSA is enabled then they won't come up.. so my current sentence was a bit mis-leading

[tssurya]

@deads2k I've addressed the comments, thanks for taking the time to review
See the diff here: https://github.com/kubernetes/enhancements/compare/02514fb6fb222958dbd7481cb44a5768dc74d1fe..c5130fb0298befbda391e0369ebd9884be9f04bb for easiness to see what changed with latest.

[deads2k]

Remove the bit about deprecation and do it in the KEP adding the replacement. Keep the doc updates. This lgtm for PRR and sig.

[deads2k]

Deprecation doesn't mean broken and doesn't mean removed. My understanding of this change is

1. the meaning of the .Host field is remaining the same
2. the resource validation on the .Host field is remaining the same
3. PodSecurityAdmission with both version locked and intent driven evaluation modes will enforce a more accurate restriction based on the actual impact of the field. Version locked namespaces have no change. Intent-based namespaces (latest) will get the most accurate enforcement

I don't see documenting the field as deprecated as critical to this change. I'm fine with this KEP without any documentation changes to the field. Marking it deprecated seems more appropriate to do in the KEP that is adding its replacement once it reaches stable.

I suggest making this part optional in the KEP and removing from goals. The documentation should still be updated to explain that usage is privileged.

[tssurya]

Repushed removing references to deprecation: https://github.com/kubernetes/enhancements/compare/c5130fb0298befbda391e0369ebd9884be9f04bb..b287b60cc2361a7548bc70befe18ad664f6a3f61

[deads2k]

lgtm for both the sig and PRR. I see Jordan's lgtm above too

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, deads2k, tssurya

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:

• keps/sig-auth/OWNERS [deads2k]

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

[deads2k]

/lgtm

[tssurya]

oops I forgot to actually write the localhost part that @BenTheElder and @danwinship pointed out here: #4942 (comment)
will do a small FUP PR to fix that up