Proposal: optional service links #1249

kongslund · 2017-10-21T18:54:28Z

This proposal has its root in an environment conflict that I encountered while trying to get an Docker image to run on Kubernetes. @thockin was kind to provide a bit of input on the mailing list and at DockerCon EU.

I've got a proof of concept working which always disables the injection of service links. I'm new to the code base so I'd probably need a bit of assistance to make a proper PR. This is of course assuming that this proposal is accepted.

I'm looking forward to your input.

Ping Node SIG leads: @dchen1107 @derekwaynecarr

thockin

Overall seems non-controversial.

@erictune @bgrant0607 @smarterclayton
@kubernetes/api-reviewers

thockin · 2017-10-26T00:09:28Z

contributors/design-proposals/node/optional-service-links.md

+
+## Implementation
+
+`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false.


Open question: I'd prefer this as enableServiceEnvVars with default true. Objections?

Also, would this make sense as defaulting from PodPreset? @jessfraz and ???

enable is preferred over disable, yes.

@droot regarding PodPreset

No objections.

The field should be a pointer to a boolean, default to true if nil, and have a positive value (enableServiceEnvVars).

thockin · 2017-10-26T00:10:18Z

contributors/design-proposals/node/optional-service-links.md

+
+`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false.
+
+In `kubelet_pods.go`, the value of that field is passed along to the function `getServiceEnvVarMap` where it is used to decide which selector should be used for the `serviceLister`. Current behavior is `labels.Everything()`. In case `disableServiceLinks` is true then only the `kubernetes` service in the `kl.masterServiceNamespace` should be injected.


Open: Should this include the kubernetes variables? Or maybe we need a different field for that? Anyone have thoughts?

All the service variables.

DNS, PodPreset, and potentially explicit service links should replace them all. The existence of any is a problem for scalability, creation-order dependencies, and other reasons.

If this omits KUBERNETES_SERVICE_HOST then in cluster config will break. That impacts sidecars like istio.

It’s not clear to me that we should omit those variables. If we do, we may have to make this a container field, not pod field.

bgrant0607 · 2017-10-26T00:21:19Z

See also #176

bgrant0607 · 2017-10-26T00:22:58Z

Also ref kubernetes/kubernetes#1768

thockin · 2017-10-26T00:32:40Z

I called out the KUBERNETES_* ones because their existence is a check that some code (e.g. some of the client setup) use as fallbacks: staging/src/k8s.io/client-go/rest/config.go

…

On Wed, Oct 25, 2017 at 5:27 PM, Brian Grant ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In contributors/design-proposals/node/optional-service-links.md <#1249 (comment)>: > +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfer with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false. + +In `kubelet_pods.go`, the value of that field is passed along to the function `getServiceEnvVarMap` where it is used to decide which selector should be used for the `serviceLister`. Current behavior is `labels.Everything()`. In case `disableServiceLinks` is true then only the `kubernetes` service in the `kl.masterServiceNamespace` should be injected. All the service variables. DNS, PodPreset, and potentially explicit service links should replace them all. The existence of any is a problem for scalability, creation-order dependencies, and other reasons. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1249 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVGLmo6CEFb16DyVAbkZaWZmAm3nUks5sv9HUgaJpZM4QBrHD> .

bgrant0607 · 2017-10-26T20:31:55Z

cc @kubernetes/sig-api-machinery-api-reviews

bgrant0607 · 2017-10-26T20:32:41Z

We may need a special case for just the apiserver-related variables.

ncdc · 2017-10-26T20:33:45Z

contributors/design-proposals/node/optional-service-links.md

+
+### Use Cases
+
+* As a user, I want to be able to disable service link injection since the injected environment variables interfer with a Docker image that I am trying to run on Kubernetes


typo: interfere

thockin · 2017-10-30T04:20:04Z

The apiserver vars are the only one I want special-cased (and I think they are by this proposal). The question is whether we want a parallael flag for them. But for now, I'll argue not.

…

On Thu, Oct 26, 2017 at 1:32 PM, Brian Grant ***@***.***> wrote: We may need a special case for just the apiserver-related variables. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1249 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVKs63Mzad3lk3nApnt641Fkyn_nTks5swOxugaJpZM4QBrHD> .

kongslund · 2017-11-02T15:48:03Z

Thanks for comments so far. Any other remarks?

stp-ip · 2017-11-04T08:32:31Z

contributors/design-proposals/node/optional-service-links.md

+
+## Alternatives considered
+
+An alternative is to add support for explicit service links, e.g. by applying a label selector map with a default behavior of including everything.


Starting with a way to disable the current behavior and considering extending via selector typ behavior might be nice, but definitely needs a few specific examples, where that might be necessary from a k8s perspective.

thockin · 2017-11-13T19:41:57Z

@kongslund I think you're on-target. You have a week before code freeze for 1.9 :)

kongslund · 2017-11-13T20:07:49Z

Thanks for the heads up, @thockin.
So does everyone think that a boolean flag is sufficient for now?

thockin · 2017-11-13T20:57:57Z

I think that's fine. Let's get it working so we can take a concrete look at it. Deltas from there are not so terrible to contemplate.

thockin · 2018-02-28T06:07:40Z

Is this abandoned?

vadorovsky · 2018-03-06T10:58:45Z

Does that proposal need to be merged before I continue the work on my implementation? If yes, @kongslund, are you up to finishing the work on proposal?

I'm sorry that I started the implementation, but I didn't see the proposal before and I just started working on the issue...

kongslund · 2018-03-06T12:15:21Z

@mrostecki The proposal is a starting point and already has good feedback from the community. It does not need to be merged before you can provide an implementation but you should incorporate the contents of the proposal or start a discussion if you think something should be different.

k8s-ci-robot · 2018-03-06T12:38:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kongslund
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

Assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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:

contributors/design-proposals/node/OWNERS

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

bgrant0607 · 2018-03-06T14:14:52Z

cc @kubernetes/sig-architecture-proposals

bgrant0607 · 2018-03-06T14:16:43Z

contributors/design-proposals/node/optional-service-links.md

+
+## Implementation
+
+`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.


This may need to be disableServiceLinks, so that the existing behavior would be in effect if it were the zero value.
cc @erictune

Either is fine with me. However, there seems to be a preference for enable instead of disable cf. #1249 (comment)

Brian, as a pointer zero value would be interpreted as existing behavior by older clients - can you articulate a bit more what you’d be concerned about as a bool ptr?

Even if we go with enable, we can set its default value to true, so I don't see any backwards compatibility issues there.

Sorry for delay. I do want this to happen.

If we make it a pointer and omitempty, will both true and false values be explicitly encoded as long as the field is non-null? I want to ensure that podspecs without the field can be unambiguously interpreted.

Also, doing this will add a field to every Pod and every pod template rendered. It will change podspec hashes, but that should be ok. It will make all pods more verbose, but maybe more users will become aware of the field and use it.

If we could, it would be great to not apply the default in pod templates. In addition to not changing hashes, this would make it feasible to set the field via admission control.

I don't think that's the case, to what in the apply discussion are you referring?

Client-applied defaults are ~impossible to ever change in our system, even over version boundaries.

I still think that defaults explicitly manifested by the server is the right thing. It just needs to be done where the resources are actually reified, not in templates.

So, do we have some agreement there? Are we OK with the field named disableServiceLinks, pointer, omitempty, with the default false value on the server side? That seems to address @bgrant0607's doubts - it will preserve the old behavior by default and would not require to define that field.

enable is probably a better API. I think that's what Brian was saying - pointers make this possible.

Any serialized form of an object which has this value specified will retain that value. Anything that has this value omitted will get nil which will be defaulted to whatever the versioned default is.

Right?

So if you serialize it in v1, when you read it back it will deserialize and default. If you write it back as v2, that v1 default is still retained...

Am I missing something?

OK, I agree. Let's go with enable. I will change that in my implementation.

bgrant0607 · 2018-06-01T03:48:33Z

cc @kubernetes/sig-scalability-proprosals @kubernetes/sig-network-proposals

bgrant0607 · 2018-06-01T03:53:27Z

cc @kubernetes/api-approvers

m1093782566 · 2018-07-23T07:59:18Z

I meet the same issue when try to create more than 6000 services in my environment - Pod failed to start due to "too long parameters". I don't think this "one size fits all" approach works for me because if we disable the service ENV injection, users may have no way to know some service-related environment variables, e.g. SERVICE_PORT.

thockin · 2018-07-31T21:17:39Z

@m1093782566 SRV DNS exists for that, if you don't know the port number.

In theory we could add yet another downward API here, but I'd need to see a serious use case for it.

thockin · 2018-08-23T19:04:36Z

I still very much want to see this fixed, if you want to carry it forward @m1093782566

m1093782566 · 2018-08-24T03:19:30Z

@thockin

SRV DNS exists for that, if you don't know the port number.

Yeah, DNS SRV record works for me.

I still very much want to see this fixed, if you want to carry it forward @m1093782566

I have the same feeling - my production team has complained about the limitation of service number for many times.

If we reach the agreement about the proposal, I can carry it forward. I volunteered to send a PR if no one has objections...

thockin · 2018-08-24T17:37:07Z

I feel like we sort of agreed on a single bool to enable service env which defaults to true.

It looks like @mrostecki is awake on https://github.com/kubernetes/kubernetes/pull/60206/files so maybe this is almost done already?

bgrant0607 · 2018-08-24T18:50:54Z

I would still like to get this proposal merged, to capture the discussion.

thockin · 2018-08-24T18:54:40Z

Oh for sure, just that I might have false-alarmed on trying to find a new owner :)

…

On Fri, Aug 24, 2018 at 11:51 AM Brian Grant ***@***.***> wrote: I would still like to get this proposal merged, to capture the discussion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1249 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVEu_g4DbwuSK5AqNDXupwJmsSGQ3ks5uUEsYgaJpZM4QBrHD> .

bradhoekstra · 2018-09-24T19:32:08Z

This feature has been merged in k/k: kubernetes/kubernetes#68754

thockin · 2018-09-24T21:36:12Z

@bradhoekstra - could you take the contents here and update them as a KEP? We should have done that in the first place...

Then we can close this

bradhoekstra · 2018-09-25T18:46:55Z

KEP PR created here: #2709

bradhoekstra · 2018-09-27T13:10:43Z

The KEP for this feature is merged.

/close

k8s-ci-robot · 2018-09-27T13:10:50Z

@bradhoekstra: You can't close an active issue/PR unless you authored it or you are a collaborator.

In response to this:

The KEP for this feature is merged.

/close

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.

Proposal for optional service links.

71f3bd2

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 21, 2017

k8s-github-robot assigned smarterclayton and thockin Oct 21, 2017

thockin reviewed Oct 26, 2017

View reviewed changes

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Oct 26, 2017

ncdc reviewed Oct 26, 2017

View reviewed changes

k8s-github-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Oct 27, 2017

stp-ip reviewed Nov 4, 2017

View reviewed changes

k8s-github-robot added the kind/design Categorizes issue or PR as related to design. label Feb 6, 2018

clkao mentioned this pull request Feb 24, 2018

Notebook should not see service discovery env variables jupyterhub/zero-to-jupyterhub-k8s#536

Closed

This was referenced Feb 28, 2018

Environmental Variables for services causing conflict with external naming convention kubernetes/kubernetes#18219

Closed

kubelet: Make service environment variables optional kubernetes/kubernetes#60206

Closed

Incorporated feedback in the comments

f9237cf

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 6, 2018

k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Mar 6, 2018

bgrant0607 reviewed Mar 6, 2018

View reviewed changes

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jun 1, 2018

bradhoekstra mentioned this pull request Sep 25, 2018

KEP: Optional Service Environment Variables #2709

Merged

thockin closed this Sep 27, 2018

bradhoekstra mentioned this pull request Oct 16, 2018

New field in Pod spec cause issues with DaemonSet kubernetes/kubernetes#69445

Closed

shekhar-rajak mentioned this pull request Jan 28, 2021

Optional Service Environment Variables kubernetes/enhancements#2360

Closed

4 tasks


		## Implementation

		`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false.


		`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false.

		In `kubelet_pods.go`, the value of that field is passed along to the function `getServiceEnvVarMap` where it is used to decide which selector should be used for the `serviceLister`. Current behavior is `labels.Everything()`. In case `disableServiceLinks` is true then only the `kubernetes` service in the `kl.masterServiceNamespace` should be injected.


		### Use Cases

		* As a user, I want to be able to disable service link injection since the injected environment variables interfer with a Docker image that I am trying to run on Kubernetes


		## Alternatives considered

		An alternative is to add support for explicit service links, e.g. by applying a label selector map with a default behavior of including everything.

Proposal: optional service links #1249

Proposal: optional service links #1249

Conversation

kongslund commented Oct 21, 2017

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Oct 26, 2017

bgrant0607 commented Oct 26, 2017

thockin commented Oct 26, 2017 via email

bgrant0607 commented Oct 26, 2017

bgrant0607 commented Oct 26, 2017

Choose a reason for hiding this comment

thockin commented Oct 30, 2017 via email

kongslund commented Nov 2, 2017

Choose a reason for hiding this comment

thockin commented Nov 13, 2017

kongslund commented Nov 13, 2017 • edited Loading

thockin commented Nov 13, 2017

thockin commented Feb 28, 2018

vadorovsky commented Mar 6, 2018

kongslund commented Mar 6, 2018

k8s-ci-robot commented Mar 6, 2018

bgrant0607 commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Jun 1, 2018

bgrant0607 commented Jun 1, 2018

m1093782566 commented Jul 23, 2018

thockin commented Jul 31, 2018

thockin commented Aug 23, 2018

m1093782566 commented Aug 24, 2018 • edited Loading

thockin commented Aug 24, 2018

bgrant0607 commented Aug 24, 2018

thockin commented Aug 24, 2018 via email

bradhoekstra commented Sep 24, 2018

thockin commented Sep 24, 2018

bradhoekstra commented Sep 25, 2018

bradhoekstra commented Sep 27, 2018

k8s-ci-robot commented Sep 27, 2018

kongslund commented Nov 13, 2017 •

edited

Loading

m1093782566 commented Aug 24, 2018 •

edited

Loading