DisableServiceLinks admission controller #122631

jmcgrath207 · 2024-01-07T08:08:35Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds an optional admission controller to disable EnableServiceLinks for new Pods without the need for a third-party mutating webhook.

Which issue(s) this PR fixes:

Fixes # #121787

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Thios PR was reverted in #125002.

The original relnote, for posterity: Added DisableServiceLinks admission controller: An optional mutating controller that will set Spec.EnableServiceLinks: False for all pods manifests in the cluster before scheduling.

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

Agreement to solve this issue with an optional admission controller.
REF: #121787 (comment)

linux-foundation-easycla · 2024-01-07T08:08:39Z

The committers listed above are authorized under a signed CLA.

✅ login: jmcgrath207 / name: John McGrath (edb0287)

k8s-ci-robot · 2024-01-07T08:08:44Z

Welcome @jmcgrath207!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-01-07T08:08:45Z

Hi @jmcgrath207. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

carlory · 2024-01-07T10:19:17Z

/ok-to-test

aojea · 2024-01-07T10:45:45Z

plugin/pkg/admission/disableservicelinks/admission.go

+// newDisableServiceLinks creates a new instance of the DisableServiceLinks admission controller.
+func newDisableServiceLinks() *plugin {
+	return &plugin{
+		Handler: admission.NewHandler(admission.Create, admission.Update),


I think this should operate only on Create, IIUIC Updates to a Pod will not recreate the environment variables

Good Catch!

I did some testing in kind and found those fields are immutable when updating a pod. I removed admission.Update in my commit.

$ kubectl apply -f simple-pod.yaml The Pod "nginx" is invalid: spec: Forbidden: pod updates may not change fields other than `spec.containers[*].image`,`spec.initContainers[*].image`,`spec.activeDeadlineSeconds`,`spec.tolerations` (only additions to existing tolerations),`spec.terminationGracePeriodSeconds` (allow it to be set to 1 if it was previously negative) core.PodSpec{ ... // 26 identical fields RuntimeClassName: nil, Overhead: nil, - EnableServiceLinks: &true, + EnableServiceLinks: &false, TopologySpreadConstraints: nil, OS: nil, ... // 2 identical fields }

Supporting update might be a no-op today, but could be future-safe

plugin/pkg/admission/disableservicelinks/admission.go

jmcgrath207 · 2024-01-08T02:17:39Z

/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-dependencies

aojea · 2024-01-08T07:42:28Z

plugin/pkg/admission/disableservicelinks/admission.go

+		return errors.NewBadRequest(fmt.Sprintf("expected *core.Pod but got %T", attributes.GetObject()))
+	}
+
+	pod.Spec.EnableServiceLinks = ptr.To(false)


if we don't find a way to differentiate between defaulted by the apiserver or set by the user adding Always to the plugin name may be more accurate, AlwaysDisableServiceLinks

is just a thought wait for discussion of other reviewers

Do we want to make this "smarter" (dubious judgement) and respect cases where the user actually set it, or do we want it to be unilateral and ALWAYS disable it?

This is a semantic decision. If it was easy, I'd say let's respect the user's input but it's not. It's not very complicated, but it's not trivial. Is it worthwhile?

Notes:

2 ways to do it:

Look at managed fields info to see what was defaulted. ISTR there is not actually enough info to determine this?

Let plugins see it as nil.

PrepareForCreate() is called from BeforeCreate() is called from staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go Store.create().

createValidation (which includes admission plugins) is called AFTER callingBeforeCreate().

This means that we can't set the default in PrepareForCreate. We'd need a hook AFTER BeforeCreate() but before e.Storage.Create() to set defaults

aojea · 2024-01-08T07:44:02Z

one last comment #122631 (comment) and one doubt #122631 (comment) but LGTM
/assign @thockin

plugin/pkg/admission/disableservicelinks/admission.go

plugin/pkg/admission/disableservicelinks/admission_test.go

jmcgrath207 · 2024-01-12T06:14:43Z

@carlory Ah, I misunderstood. I've updated the release note description. Does this work for you?

thockin · 2024-03-04T21:06:46Z

I'll approve and hold, if you want to re-add Update or rename it, ping me. Else just clear the hold.

/approve
/hold

jmcgrath207 · 2024-03-06T08:03:41Z

Thanks @thockin

I've added the noop for admission.Update and updated the tests to reflect the admission operation. I also rebased and squashed.

If there is nothing else, I am ready to merge.

thockin · 2024-03-07T20:29:11Z

Ugh, I missed this ping in the code freeze chaos. It will have to wait for 1.31.

thockin

/lgtm
/approve

k8s-ci-robot · 2024-03-07T20:31:17Z

LGTM label has been added.

Git tree hash: 669ad6c4d821f5d6ccdc9dfe59cc94652caf86da

k8s-ci-robot · 2024-03-07T20:31:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmcgrath207, thockin

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/kubeapiserver/OWNERS~~ [thockin]
~~plugin/pkg/admission/OWNERS~~ [thockin]

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

enj · 2024-05-14T18:50:29Z

IMO this PR is the wrong approach, and we should consider reverting it before we release v1.31. kubernetes/enhancements#3962 should give users the ability to tweak the default as they desire though the Kube API, and we don't need to create one off admission plugins for every wrong default we have (especially since admission config isn't available to users on some environments).

thockin · 2024-05-14T20:30:58Z

I don't have any philosophical preference for this sort of laser-focused admission plugins over MAP, except that MAP is alpha. At the time this proposal was in flgiht I wasn't so aware of MAP. That said, the interplay between default values and admission control sequencing leaves me scratching my head still. In a MAP or mutating webhook, I can't tell if a value was specified by a user or by the defaulting logic. This PR would probably consider that if it could, but it can't, so it doesn't.

This issue is not so urgent that it can't wait for MAP, though I hope it's not TOOO long. So I guess I agree with @enj - are there any major reasons why we couldn't back this out and pivot the solution to MAP?

@jmcgrath207 ?

thockin · 2024-05-20T15:28:27Z

ping to @jmcgrath207 one more time

jmcgrath207 · 2024-05-20T17:07:16Z

Hey All,

Sorry for the late response.

I went ahead and reviewed KEP-3962, and I agree this meets my needs.

I feel it's best to revert my commit and move forward with the KEP-3962 solution to prevent a deprecation scenario. The Alpha part doesn't bother me since both solutions would require a control-plane config change.

As for urgency, users can use a policy controller like kyverno that I mentioned here here or it can just edit the manifest. I assume I am in the minority here with heavy restrictions on image approval running in their environment.

thockin · 2024-05-20T17:15:59Z

Do you want to send a revert or shall I?

jmcgrath207 · 2024-05-20T17:23:17Z

@thockin Here is the revert PR. If this isn't the proper way, I have no issues with you reverting it.

#125002

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 7, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 7, 2024

k8s-ci-robot requested review from deads2k and yue9944882 January 7, 2024 08:09

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 7, 2024

aojea reviewed Jan 7, 2024

View reviewed changes

plugin/pkg/admission/disableservicelinks/admission.go Outdated Show resolved Hide resolved

aojea mentioned this pull request Jan 7, 2024

Set enableServiceLinks to false as default #121787

Open

aojea reviewed Jan 7, 2024

View reviewed changes

plugin/pkg/admission/disableservicelinks/admission.go Outdated Show resolved Hide resolved

aojea reviewed Jan 8, 2024

View reviewed changes

k8s-ci-robot assigned thockin Jan 8, 2024

HirazawaUi reviewed Jan 8, 2024

View reviewed changes

plugin/pkg/admission/disableservicelinks/admission.go Show resolved Hide resolved

plugin/pkg/admission/disableservicelinks/admission_test.go Show resolved Hide resolved

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 12, 2024

jmcgrath207 changed the title ~~Disable EnableServiceLinks admission controller~~ DisableServiceLinks admission controller Jan 12, 2024

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 4, 2024

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2024

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2024

k8s-ci-robot requested a review from thockin March 6, 2024 06:24

DisableServiceLinks admission controller

edb0287

jmcgrath207 force-pushed the disable-service-links branch from da39708 to edb0287 Compare March 6, 2024 06:40

thockin reviewed Mar 7, 2024

View reviewed changes

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

thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2024

k8s-ci-robot merged commit 6faeecc into kubernetes:master Apr 18, 2024
14 checks passed

k8s-ci-robot added this to the v1.31 milestone Apr 18, 2024

jmcgrath207 mentioned this pull request May 20, 2024

Revert "DisableServiceLinks admission controller" #125002

Merged

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DisableServiceLinks admission controller #122631

DisableServiceLinks admission controller #122631

jmcgrath207 commented Jan 7, 2024 •

edited by thockin

Loading

linux-foundation-easycla bot commented Jan 7, 2024 •

edited

Loading

k8s-ci-robot commented Jan 7, 2024

k8s-ci-robot commented Jan 7, 2024

carlory commented Jan 7, 2024

aojea Jan 7, 2024

jmcgrath207 Jan 8, 2024

thockin Mar 4, 2024

jmcgrath207 commented Jan 8, 2024

aojea Jan 8, 2024 •

edited

Loading

thockin Mar 4, 2024

aojea commented Jan 8, 2024 •

edited

Loading

jmcgrath207 commented Jan 12, 2024

thockin commented Mar 4, 2024

jmcgrath207 commented Mar 6, 2024

thockin commented Mar 7, 2024 •

edited

Loading

thockin left a comment

k8s-ci-robot commented Mar 7, 2024

k8s-ci-robot commented Mar 7, 2024

enj commented May 14, 2024

thockin commented May 14, 2024

thockin commented May 20, 2024

jmcgrath207 commented May 20, 2024

thockin commented May 20, 2024

jmcgrath207 commented May 20, 2024 •

edited

Loading

DisableServiceLinks admission controller #122631

DisableServiceLinks admission controller #122631

Conversation

jmcgrath207 commented Jan 7, 2024 • edited by thockin Loading

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

linux-foundation-easycla bot commented Jan 7, 2024 • edited Loading

k8s-ci-robot commented Jan 7, 2024

k8s-ci-robot commented Jan 7, 2024

carlory commented Jan 7, 2024

aojea Jan 7, 2024

Choose a reason for hiding this comment

jmcgrath207 Jan 8, 2024

Choose a reason for hiding this comment

thockin Mar 4, 2024

Choose a reason for hiding this comment

jmcgrath207 commented Jan 8, 2024

aojea Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

thockin Mar 4, 2024

Choose a reason for hiding this comment

aojea commented Jan 8, 2024 • edited Loading

jmcgrath207 commented Jan 12, 2024

thockin commented Mar 4, 2024

jmcgrath207 commented Mar 6, 2024

thockin commented Mar 7, 2024 • edited Loading

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 7, 2024

k8s-ci-robot commented Mar 7, 2024

enj commented May 14, 2024

thockin commented May 14, 2024

thockin commented May 20, 2024

jmcgrath207 commented May 20, 2024

thockin commented May 20, 2024

jmcgrath207 commented May 20, 2024 • edited Loading

jmcgrath207 commented Jan 7, 2024 •

edited by thockin

Loading

linux-foundation-easycla bot commented Jan 7, 2024 •

edited

Loading

aojea Jan 8, 2024 •

edited

Loading

aojea commented Jan 8, 2024 •

edited

Loading

thockin commented Mar 7, 2024 •

edited

Loading

jmcgrath207 commented May 20, 2024 •

edited

Loading